Fix: Resolve env preprocessors for LateBoundDsnParameter

[RikudouSage]

Description

Processes an env variable using available preprocessors

Fixes #73

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

• Unit tests
• Manual Tests

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[dkarlovi]

Looking at this implementation and the tests required, it feels to me this is the wrong direction to take because it's duplicating what Symfony is already doing internally? Otherwise every single project allowing env processors in their DSN (like the linked Elastica bundle) would need to implement the same type of processing, but they don't.

Symfony already supports env var placeholders so this should be doable without implementing it yourself.

[RikudouSage]

I've found no documentation on how to do that. Of course we welcome a better implementation, I'm fine with scraping this code if something better comes along.

[dkarlovi]

There's \Symfony\Component\DependencyInjection\ContainerBuilder::resolveEnvPlaceholders, but there's also \Symfony\Component\DependencyInjection\Compiler\ResolveEnvPlaceholdersPass which, from my understanding, should do it for you.

There's also \Symfony\Component\DependencyInjection\Compiler\MergeExtensionConfigurationContainerBuilder::resolveEnvPlaceholders which I'm not sure is for. 🤔

I've asked on Symfony Slack, maybe somebody will have a better idea how exactly this works.

[dkarlovi]

@RikudouSage here's a comment by @stof who is a Symfony core team member on the topic:

to accept env placeholders in the semantic config of the bundle and have them work, the requirements are:

• don't write beforeNormalization steps in the Configuration that rely on the values of settings (the logic in DoctrineBundle supporting a shortcut config by moving keys around is fine as it does not care about the values themselves)
• same for validate steps in the Configuration
• don't write logic in the DI extension that relies on inspecting the values of those settings before injecting them in DI parameters or service definition arguments

[dkarlovi]

@RikudouSage further discussion

@dkarlovi:

if I understood you correctly, if you delay using the value while building the container and just wire it up into a parameter which gets injected, it should just work?

@stof:

Yes. The env variable will be read when needing it to instantiate the service, not during the container building

So, if the DSN is injected into the client as is and then the client interprets then instead of the cotainer extension, it should just work without any further intervention.

[RikudouSage]

Nice, thanks for the info. I'll have to think how to incorporate the new informations into the existing code, so it may turn out the current code will be merged either way, but either way I'll keep that in mind for when the code will be rewritten.

[dkarlovi]

@RikudouSage I think the solution is to not parse the DSN into individual components while building the container but instead pass the configuration as is into a client factory.

Again @stof:

If the client does not accept a DSN directly, the solution is to use a factory service doing that parsing. This is what Doctrine bundle does with its ConnectionFactory

https://github.com/doctrine/DoctrineBundle/blob/2.12.x/src/ConnectionFactory.php

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.