Add "dbname_suffix" config option to vary the dbname per env

[nicolas-grekas]

This should provide better flexibility than #1290 when needing to vary the dbname per env (think tests of course)

[nicolas-grekas]

See corresponding recipe update at symfony/recipes#939

[weaverryan]

This is great! A more useful and pragmatic solution to the original problem that we were trying to tackle.

[ostrolucky]

How is this more flexible than #1290? When someone will ask for suffix instead of prefix we need to bake in then yet another option? What problem is this PR solving?

[nicolas-grekas]

#1290 swaps the responsibility of configuring the database from the env to the config file. This is wrong: the env should always be in control, because it knows itself.
For testing, the app should vary the dbname, and nothing else (unlike #1290 which potentially overrides all params.) It should do so while preserving the dbname from the env. Not by ignoring it.
suffix is enough to cover all needs.
Can be turned into a sprintf template if you think that's worth it (I don't.)

[ostrolucky]

#1290 swaps the responsibility of configuring the database from the env to the config file. This is wrong: the env should always be in control, because it knows itself.

No it doesn't. Users are free to continue using DATABASE_URL as source of truth, but then they need to remove other options from config file, because they didn't take any effect. Having dead code around is very bad code smell, because it's misleading people that it's doing something.

the env should always be in control, because it knows itself.

And yet, you introduced dbname_suffix option, which makes database name specified in DATABASE_URL not being true anymore. DATABASE_URL should either be true, so should override any other option and hence defining those options doesn't make sense, or it doesn't necessarily need to be true and then it's fine if options in yaml config override parts of it.

I don't agree with removing deprecation. Either it's ok to use combination of DATABASE_URL + options, or defining options doesn't make sense (because they are overridden). This is something we want users to realize and refactor their configs so it can't happen that they have options defined by mistake which are not actually in effect. If they want to configure everything via DATABASE_URL that's fine, but then remove those options.

[nicolas-grekas]

All DSNs in Symfony work with this precedence (Cache, Lock, etc): the DSN wins over specific options. I would recommend against DoctrineBundle following with a different precedence, at least for consistency.

When one does define specific options, it can have a purpose: defining default values. If ppl use both options and DSN, they might have their reason (the default recipe doesn't encourage using these options.)

For the matter of the PR, ignoring the database name from the env is a lot less flexible. The DSN might say that the dbname is foo or bar, and suffix strategy adapts to it by creating a dbname of foo_test or foo_bar. This is where this approach is superior (and simpler).

[nicolas-grekas]

Note that I appreciate that you're asking these questions @ostrolucky. They had to be answered!

[ostrolucky]

All DSNs in Symfony work with this precedence (Cache, Lock, etc): the DSN wins over specific options

This is misleading. Cache doesn't allow anything else than DSN. And Lock uses same config option key for either of them so it's not possible to define both env string and config options. These are very different cases from doctrine-bundle, where we allow both at the same time.

I would recommend against Doctrine following with a different precedence, at least for consistency.

Please watch more closely what happens in doctrine-bundle if you want to have a say what happens here. It's a tad too late complaining about this after release, we could have agreed on something better for both sides before we released the feature.

I still don't know what problem is this PR solving though. Before #1290, users had to have

in doctrine_test.yaml

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

and in .env.test

DATABASE_URL=mysql://root:password@database:3306/main_test?serverVersion=mariadb-10.5.8

After #1290, they can keep doing same thing, they just need to add override_url: true to solve deprecation.

Functionality added in this PR is independent from #1290, so I would like to understand your motivation for doing this. Are you attempting to discourage usage of new override_url, or how is it related to #1290? Even if we accept that dbname_suffix is useful, deprecation removal needs to be extracted to different PR, because they are unrelated.

This functionality is very shortsighted IMHO. If you check original PR #1290, it's motivation was for overriding database name - without any mention of suffix. Database name can be completely different, but other parts of DATABASE_URL might be always same and you had to define whole string 10 times for 10 databases. I don't know why do you think database name in test environment is a problem here and coming with solution for that will solve all other use cases where user has one option different from DATABASE_URL so he has to have 10 different DATABASE_URL versions. But if you want to have solution which is still .env-first, I would suggest to redirect the focus and effort to symfony/symfony#40717 If that issue was solved, we wouldn't have to introduce this thing here, it would solve it for every library and any part of env string. For example, in use case presented here, you would do

in .env

DATABASE_NAME=main
DATABASE_URL=mysql://root:password@database:3306/${DATABASE_NAME}?serverVersion=mariadb-10.5.8

in .env.test

DATABASE_NAME=${DATABASE_NAME}_test

That's the way to go, IMHO. Super flexible and we could stay with env being a source of truth. I guess it would not solve the problem with 10 different databases example, that's where override_options would still be useful I guess, though 🤔

[nicolas-grekas]

Cache doesn't allow anything else than DSN

Using configuration no, but using the constructor yes. The same logic applies when thinking about the desired precedence.

we could have agreed on something better for both sides before we released the feature

Sorry about that, I missed that PR and it's quite unfortunate it's been merged and released yes. The PR is adding a mandatory deprecation to all projects, asking them all to add a new config option, which breaks consistency and creates risks for projects. That's a very high cost pushed to the community. I wish we can make a new release quickly to save this cost.

in doctrine_test.yaml [...] and in .env.test

The point is: this doesn't work when using eg docker-compose + the symfony binary. In this setup, .env files are ignored and the DB is fully configured by real env vars. That's really nice for DX but most importantly, it means this has nothing to do with how .env files behave. symfony/symfony#40717 is totally unrelated.

This PR is related to #1290 because the only use case of it is adding a suffix to the DB (see how the PR has been introduced, and see also the corresponding recipe).

But #1290 actually creates a risk, because when override_url is set, any param is overriding the DSN, not only the dbname. That's going to create hours of WTFs to many I'd bet.

I'm fine splitting this PR in two.

[nicolas-grekas]

PR updated, split into #1331

[nicolas-grekas]

Tests need #1331 to be green.

[stof]

also, DBAL itself still makes the URL win over options. Only this bundle changes the precedence order when using the new override_url

[ostrolucky]

Using configuration no, but using the constructor yes. The same logic applies when thinking about the desired precedence.

It's still similar case I mentioned: It's not possible to instantiate adapter specifying both DSN and specific options at the same time, while DBAL allows that. Hence there is no precedence in Symfony.

this doesn't work when using eg. docker-compose + the symfony binary. In this setup, .env files are ignored

I can simply export SYMFONY_DOTENV_VARS=DATABASE_URL to solve that. But let's not pretend #1290 had something to do with this problem. Values in .env being ignored when real environment variables are set is a problem of a much, much broader scope than just focusing on dbname in test environment like here. How do I solve this problem for other, non-DATABASE_URL env vars? Well, just do very same thing here.

also, DBAL itself still makes the URL win over options. Only this bundle changes the precedence order when using the new override_url

That ship has sailed thanks to our hacky ConnectionFactory, but is still conveniently used by people however they see fit. See: from an other side of argument - when introducing stuff like UTF-8 default charset. Now doctrine-bundle maintainers are stuck with bugs such as this one #1325. But I see it same way, it should have been added to DBAL instead. Hence I will most likely use this case as a precedence and start deprecating all of that crap + enforce such changes not land in bundle anymore - it should be in DBAL, not here.

[nicolas-grekas]

It's not possible to instantiate adapter specifying both DSN and specific options at the same time

Actually yes, all DSN-based factories allow that, see eg https://github.com/symfony/symfony/blob/8361713367e2735fd9b63cd2b6661411a0339811/src/Symfony/Component/Cache/Adapter/AbstractAdapter.php#L127

export SYMFONY_DOTENV_VARS=DATABASE_URL to solve that

not really: in tests, I do want to use that DSN provided as real env by the symfony binary, because eg I don't know on which port Docker made the db container listen.

How do I solve this problem for other, non-DATABASE_URL env vars?

that's a good question. I don't think there is a generic solution to that, because each network service has it's own concepts. Eg for a DB, we can change the name, but we could also prefix tables. For cache pools, Symfony already implements env-dependent namespaces, which means that there, we prefix cache keys, not dbnames. Etc, it's case by case.

[ostrolucky]

can you rebase in order to see test failures are gone?

[nicolas-grekas]

Can you please merge 2.3 into 2.4 before?

[ostrolucky]

done

[ostrolucky]

@wouterj our 2.4 branch is failing with latest symfony deps. Can you check that, since you contributed with #1297?

[wouterj]

I'll have a look!

[nicolas-grekas]

Thanks for merging!