add ability to override url with dsn params

[jrushlow]

Use case:

If you define a connection url in Symfony using a real DATABASE_URL env var for development, and you use the same server, but different database for tests - you have to override the entire DATABASE_URL in your config/packages/test/doctrine.yaml file.

This PR would allow you to override part of the url using any combination of dbname, host, port, user, and/or password. To preserve BC, we would add a doctrine.dbal.override_url bool configuration param.

Example:

// config/packages/doctrine.yaml

doctrine:
    dbal:
        url: '%env(DATABASE_URL)%'

// config/packages/test/doctrine.yaml

doctrine:
    dbal:
        override_url: true
        dbname: main_test

Assuming there is a DATABASE_DSN env var set to mysql://user:pass@host/main_database, then in the test environment it would still use mysql://user:pass@host but with the main_test database.

[weaverryan]

Hi!

👍 from me for the use-case.

This could also be used to powerParaTest: you could set the dbname to main_test_%env(TEST_TOKEN)% in config/packages/test/doctrine.yaml. That would allow you to use your normal database connection params, but toggle the database name for parallel tests.

Cheers!

[ostrolucky]

I'm also fine with idea, but I think we should push for this to be a default. Perhaps we should make it deprecated to not set this option to true and in doctrine bundle 4 remove it and just make this behavior default. I don't see a use case currently where it makes sense to define both ways to define database connection options. But that changes with this pr, that's why we should introduce such deprecation together with introducing this.

[stof]

or maybe this should be changed in DBAL so that specifying both an option explicitly and the URL makes the option win instead of the URL, removing the need to do such hack in the bundle.

[ostrolucky]

yep

[weaverryan]

but I think we should push for this to be a default. Perhaps we should make it deprecated to not set this option to true and in doctrine bundle 4 remove it and just make this behavior default

I was wondering about this too. Let's add those deprecations

or maybe this should be changed in DBAL so that specifying both an option explicitly and the URL makes the option win instead of the URL, removing the need to do such hack in the bundle

That would also be awesome. We could do this PR, then see how this could be moved upstream so that - someday - the hackery in the ConnectionFactory isn't needed.

[stof]

I think the test failures are happening for some cases where the url option is not used at all

[jrushlow]

the remaining test failures do not appear to be caused by this PR as I see them in PR #1295 as well

[ostrolucky]

• Adding note in UPGRADE-2.3.md required
• Adding docs in configuration.rst required
• Removal of deprecation-contracts required
• Fix for override_url: true in combination with URL and unspecified options that don't default to NULL required

[jrushlow]

this should be ready for another review

[weaverryan]

Very minor comments!

[weaverryan]

This looks ready to me 👍

[ostrolucky]

I don't understand why is this done. If I remove this block and revert removal of default values in Configuration.php, all tests pass. It should be clear from code why was this done this way.

[jrushlow]

Follow up to our slack conversation and to bring everyone else up to speed:

If we set default values in the Configuration - we have no way to determine if a value is an override or a default. https://github.com/doctrine/DoctrineBundle/pull/1290/files#diff-59ba6fdc91d16d6c28504e469b134f4bbf84e6b39d42c83d91652afe6015e1e7 asserts this behavior. If we set the defaults in the Configuration, this test will now fail(host would be localhost rather than the overridden value).

[ostrolucky]

@jrushlow Hey thanks for starting to fix this, but if you would like, I could fix all of the comments for you. Just please let me know in my comments if I am wrong in some of them. Especially removal of default values is questionable for me, but maybe you had a reason for that - in that case I would like to know about it.

[jrushlow]

Thanks @ostrolucky - im going through the rest of this to try and get it wrapped up for today, but ill let you know if i run out of time (kids have a soccer game in a couple hours) / get stuck on something

Especially removal of default values is questionable for me, but maybe you had a reason for that - in that case I would like to know about it.

We had to remove the defaults from the config otherwise we have no way of determining if a value for say host is the override or the default. The tests passed when you reverted because in the dbal_allow_url_override case - I was overriding everything. Which hide the fact that the url's host value was database, and if i don't override the host, we are overriding the host with the default value (localhost)... Sounds a bit confusing but basically we dont want to override a legitimate value with a default value..

Anywho, im looking at it now to see if there is a way we can do this w/o removing the defaults from the Configuration but no luck so far.

[ostrolucky]

I was testing exactly this case locally actually. I modified the test case by setting host to localhost and checked that output host was localhost. This still worked with my changes (adding defaults back and removing the mentioned block)

[ostrolucky]

I've done some cleanup for you, let me know if you notice something suspicious in my changes.

[jrushlow]

I've done some cleanup for you, let me know if you notice something suspicious in my changes.

Everything looks good to me. Thanks @ostrolucky for the help on this!

[nicolas-grekas]

I'm proposing another approach in #1328

[nicolas-grekas]

Note that I don't agree with the deprecation and that I'm proposing to revert it in #1328

[ostrolucky]

@jrushlow What do you think about that?