Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix resolving DSN from dynamic environment variables #533

Closed
wants to merge 2 commits into from

Conversation

mdeboer
Copy link

@mdeboer mdeboer commented Sep 11, 2018

There was a hack in the DefaultTransportFactory class named resolveDSN which allowed for basic environment variable resolving but not for dynamic resolving.

E.g. this would work:

ENQUEUE_DSN=file:///tmp

enqueue:
  transport:
    default: '%env(ENQUEUE_DSN)%'
  client: ~

But this wouldn't:

ENQUEUE_DSN=file:///%kernel.cache_dir%/enqueue

enqueue:
  transport:
    default: '%env(resolve:ENQUEUE_DSN)%'
  client: ~

With this fix this now works like it should.

I fixed it by moving the logic of creating connection factories, contexts and drivers to a compiler pass which allows for dynamic resolving of environment placeholders.

Because this was normally done in the extension itself, some tests had to be altered to run the compiler pass after registering the extension.

The producer test was referencing the container which was already
cleaned up by the parent tearDown
@makasim
Copy link
Member

makasim commented Sep 11, 2018

The problem is properly fixed in 0.9 see #510 (current master).

The idea of resolving env vars at compile time is not right IMO.

@mdeboer
Copy link
Author

mdeboer commented Sep 11, 2018

@makasim as I need the functionality, when will 0.9 be released?

Imho this has it's limitations but is better than the hack imho.

@makasim
Copy link
Member

makasim commented Sep 11, 2018

The dev branch is pretty much stable, though some BC breaks have been done and expected to be done. I still have a lot on my plat, do not expect a release soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants