Either explicitly define DBAL connections error with version 2.4.1

[ph-il]

Got "Either explicitly define DBAL connections in all doctrine-bundle configuration files, or in none of them." But looking at the changelog for 2.4.x I don't see anything that need to be change.

doctrine:
  dbal:
    default_connection:    default
    connections:
      default:
        url: '%env(resolve:DATABASE_URL)%'
        charset: utf8mb4
        server_version: '5.7'
        default_table_options:
          collate: utf8mb4_unicode_ci

[ostrolucky]

Look in all files. Have you checked prod/doctrine.yaml?

[ph-il]

I was in dev, no other file. And Prod :

doctrine:
  orm:
    auto_generate_proxy_classes: false
    metadata_cache_driver: pool
    #            type: service
    #            id: doctrine.system_cache_provider
    query_cache_driver: pool
    #            type: service
    #            id: doctrine.system_cache_provider
    result_cache_driver: pool
#            type: service
#            id: doctrine.result_cache_provider

[ostrolucky]

In prod you need to use doctrine.dbal.connections too. Search for other such files, maybe you have more such files in dev too.

[ph-il]

It not suppose to be cascading?

[ostrolucky]

It was and this was always buggy with multiple connections, that's why we are fixing this by disallowing it. See #1356 for more info

[ph-il]

I copy the doctrine.yaml everywhere, plus add the prod difference to prod. But still have the error.

[ph-il]

I think broken thousands of application to fixe a few that don't understand the standard way of using symfony configuration is a bad idea. If there's a limitation (more a bug) in symfony config, there's we're it should be fixe.
Now, for the moment I have to tell all our projects (more than an hundred) to be sure not to update.

[Webonaute]

I have the same issue and was related to some dependency injection like this (which respect the config structure and should be valid configuration.):

$container->prependExtensionConfig(
            'doctrine',
            [
                'orm' => [
                    'entity_managers' => [
                        'default' => [
                            'mappings' => [
                                'SonataMediaBundle' => null,
                            ],
                        ],
                    ],
                ],
            ]
        );

[Webonaute]

I think broken thousands of application to fixe a few that don't understand the standard way of using symfony configuration is a bad idea. If there's a limitation (more a bug) in symfony config, there's we're it should be fixe.
Now, for the moment I have to tell all our projects (more than an hundred) to be sure not to update.

I agree. this is a big BC who can be avoided. A better fix (to my point of view) would have been to drop simple configuration support and add deprecation message for who use it to force multiple configuration usage to avoid config confusion. This fix is braking more stuffs than it fix.

[greg0ire]

@Webonaute I don't understand why the prepend extension config you are using triggers the exception in the title, IMO the structure is valid as you mentioned. It should work, and if it does not, that's what should be fixed, right? Can you please debug further?

[greg0ire]

A reproducer project for any of these issues would be appreciated.

[adrienfr]

b\c Breaks anything using https://github.com/snc/SncRedisBundle with error:

 The service alias "doctrine.orm.default_metadata_cache" does not exist.

I'm using SncRedisBundle for result_cache_driver (through doctrine.result_cache_pool) and don't have any issue with DoctrineBundle 2.4.1. In Doctrine config, I don't define connection(s) nor entity_manager(s), then don't have a mix configuration issue (https://github.com/doctrine/DoctrineBundle/blob/2.4.x/UPGRADE-2.4.md#configuration)

[adrienfr]

My snc_redis config is below. I'll try and reproduce later today for you

 $containerConfigurator->extension('snc_redis',
        [
            'clients' => [
                'default' => [
                    'type'    => 'predis',
                    'alias'   => 'default',
                    'dsn'     => '%env(REDIS_URL)%',
                    'logging' => false,
                ],
                'cache' => [
                    'type'    => 'predis',
                    'alias'   => 'cache',
                    'dsn'     => '%env(REDIS_URL)%',
                    'logging' => false,
                ],
            ],
            'session' => [
                'client' => 'default',
                'prefix' => 'base_session_',
            ],
            'doctrine' => [
                'metadata_cache' => [
                    'client'           => 'cache',
                    'entity_manager'   => 'default',
                    'document_manager' => 'default',
                ],
                'result_cache' => [
                    'client'         => 'cache',
                    'entity_manager' => [
                        'default',
                        'read',
                    ],
                    'document_manager' => [
                        'default',
                        'slave1',
                        'slave2',
                    ],
                    'namespace' => 'dcrc:', ],
                'query_cache' => [
                    'client'         => 'cache',
                    'entity_manager' => 'default',
                ],
                'second_level_cache' => [
                    'client'         => 'cache',
                    'entity_manager' => 'default',
                ],
            ],

            'monolog' => [
                'client' => 'cache',
                'key'    => 'monolog',
            ],
            'swiftmailer' => [
                'client' => 'default',
                'key'    => 'swiftmailer',
            ],
        ]
    );

It might be related to doctrine configuration made through SncRedisBundle (then defining entity_manager: default), see snc/SncRedisBundle#555 (comment)

[ostrolucky]

Please don't mix issues in here. snc_redis issue should go elsewhere, preferably to their bug tracker.
This issue is only about Either explicitly define DBAL connections in all doctrine-bundle configuration files, or in none of them or Either explicitly define entity managers in all doctrine-bundle configuration files, or in none of them errors. And so far it wasn't confirmed because nobody provided reproducer.

This issue can happen only if configuration is processed twice. That can happen only if you have multiple doctrine configurations that have to be processed in same environment. Additionally, it can happen only if you have multiple connections/entity managers.

[adrienfr]

Please don't mix issues in here. snc_redis issue should go elsewhere, preferably to their bug tracker.
This issue is only about Either explicitly define DBAL connections in all doctrine-bundle configuration files, or in none of them or Either explicitly define entity managers in all doctrine-bundle configuration files, or in none of them errors. And so far it wasn't confirmed because nobody provided reproducer.

This issue can happen only if configuration is processed twice. That can happen only if you have multiple doctrine configurations that have to be processed in same environment. Additionally, it can happen only if you have multiple connections/entity managers.

@ostrolucky it's somehow related because @PhilETaylor is defining entity_manager: default through snc_redis configuration, which previously worked well but doesn't anymore due to the "Either explicitly define DBAL connections/entity_manager" check. But I agree with you, this is more a snc_redis configuration issue from now on.

[ostrolucky]

Again, here let's talk only about errors I have quoted before, please. You may put feedback about snc_redis somewhere else, but it sounds like snc_redis issue. But again, it's most likely unrelated to changes that triggered the quoted errors.

[PhilETaylor]

ok its clear you are not interested in how your changes have broken production sites and the changes interaction with other projects. I have deleted my comments and will unsubscribe. Have fun.

[nicolas-grekas]

Please be sure we're all interested in feedbacks!

[NoodlesNZ]

I've setup a simple project to replicate the issue: https://github.com/NoodlesNZ/doctrine_config

clone the repo and run composer install then ./bin/console cache:clear and it should show the error

[stof]

I think the issue is that #1356 introduced an error rather than a deprecation

[NoodlesNZ]

It certainly had the desired effect of getting developer's attention. Given the project in my previous comment, how would we fix the config/packages/prod/doctrine.yaml to fix the underlying issue?

[webhdx]

Having similar issue. This happens when you inject custom entity managers via ContainerBuilder::prependExtensionConfig. I can't really see why it would be prohibited but with your fix this becomes impossible. We intentionally inject EMs via extension to avoid modifying default config files coming from your flex recipe.

For me you should deprecate using simplified syntax or log a warning that it may cause issues. Alternatively just redo your flex recipe to come with all config keys so they can be overrode from 3rd party bundles.

If you have any solution I'd like to hear because we planned new release for today and now we are blocked by this issue.

[ostrolucky]

Recipe comes with simplified/shortened configuration syntax for both files, doctrine.yaml and prod/doctrine.yaml. Changing it from shortened version in doctrine.yaml, but forgetting to do the same in prod/doctrine.yaml, or using different format somewhere else is exactly why we are throwing the error. It would be fine if it was without side effects, but that's not the case unfortunately and I am afraid lot of people don't realize this. And I don't see why it becomes impossible to define this via prependExtensionConfig anymore. It should be possible, you just need to use same format in prependExtensionConfig as you used in yaml files.

And error was intentional as well. Deprecation is too weak and would mask legitimate issues that result from this combined approach. After all, this is also documented at https://symfony.com/doc/current/reference/configuration/doctrine.html#shortened-configuration-syntax :

Keep in mind that you can’t use both syntaxes at the same time.

So from my POV I'm just making sure documentation is correct. My choice was either error when using mixed approach with multiple ems/connections, or deprecation of whole shortened configuration. Deprecating whole shortened configuration would mean pretty much every application would have to go through this issue in doctrine-bundle 3.0 though, instead of only projects with multiple managers and mixed configurations, so we decided not to go with it.

[webhdx]

I'm afraid it's not possible because the doc states

When you are only using one entity manager, all config options available can be placed directly under doctrine.orm config level.

So by providing shortened syntax in your recipe you pretty much killed all cases where 3rd party code would inject its own entity managers:

        $container->prependExtensionConfig('doctrine', [
            'orm' => [
                'entity_managers' => [],
            ],
        ]);

This raises an error here: https://github.com/doctrine/DoctrineBundle/blob/2.4.x/DependencyInjection/Configuration.php#L416

Injecting entity managers is only possible if you remove or modify default config files but this is simply not possible for some projects. Ibexa DXP can be installed over existing Symfony applications. We no longer ship it as a "project" because we don't want to duplicate and maintain all the stuff coming from 3rd party bundles.

[stof]

And I don't see why it becomes impossible to define this via prependExtensionConfig anymore. It should be possible, you just need to use same format in prependExtensionConfig as you used in yaml files.

Well, that's the issue. Bundles using prependExtensionConfig have no way to know what syntax is used in projects.

And error was intentional as well. Deprecation is too weak and would mask legitimate issues that result from this combined approach.

then release it as a major version, not as a minor one

After all, this is also documented at https://symfony.com/doc/current/reference/configuration/doctrine.html#shortened-configuration-syntax

You cannot use them at the same time in a single file. but different files could make different choices before.

So from my POV I'm just making sure documentation is correct. My choice was either error when using mixed approach with multiple ems/connections, or deprecation of whole shortened configuration. Deprecating whole shortened configuration would mean pretty much every application would have to go through this issue in doctrine-bundle 3.0 though, instead of only projects with multiple managers and mixed configurations, so we decided not to go with it.

Projects using mixed approach could still have a working configuration, and you broke BC for them.

I think the all issues with mixed configuration actually come from the fact that the shortened configuration kept the name of the entity manager configurable, by rewriting the configuration based on the separate default_entity_manager config. But it was rewriting that based on the local value of that config, not based on the final value (as the final value was not known yet at that point) and so things would go wrong for projects trying to use a custom name without redeclaring it in all files using the shortened configuration.
A solution for that could have been to deprecate using default_entity_manager alongside the shortened config (and in the next major version of the bundle, the shortened config would always configure an entity manager named default)

[stof]

Btw, I don't even think #1356 actually fixes the issue when you use the shortened configuration with customized default_entity_manager in some places only. So it breaks working configs without solving the original issue.

[ph-il]

a BC issus like this should have had at least DEPRECATED status in this version, then change in a major version.

And it seem that now we do have to put all doctrine configuration in one file, and take everything that come with others bundles (especially extensions one) into one file. That is breaking symfony way of doing stuff.

[ph-il]

Again, here let's talk only about errors I have quoted before, please. You may put feedback about snc_redis somewhere else, but it sounds like snc_redis issue. But again, it's most likely unrelated to changes that triggered the quoted errors.

No it's not. Now we CAN'T have any configuration coming from extensions bundles in separate config files.

[nicolas-grekas]

I think we should revert #1356 as a bugfix and reconsider the issue in light of the feedback we have here.
While you're all here, we would greatly appreciate any ideas to fix #1337 and doctrine/orm#8687 after the revert!

[Webonaute]

Trace debug:

[Webonaute]

@nicolas-grekas if you want my opinion, you should deprecate single configuration in this version and remove it in a major version. having a simple structure would help preventing merging wrong configuration structure.

[ostrolucky]

Well, that's the issue. Bundles using prependExtensionConfig have no way to know what syntax is used in projects.

1. Users who use the bundle can find out what format prependExtensionConfig of bundle they require uses and use the same format. They can do this now.
2. Bundle can use $container->getExtensionConfig('doctrine') and find out what format is used by user.

Blindly injecting doctrine configuration without considering what connections and entity managers did users configure is also the type of problem doctrine-bundle 2.4 solves. Unless those bundles define their own entity managers, they were broken already, IMHO. It just so happened that they were working for most users, because most users don't use multiple connections. I also wonder if these bundles consider the possibility users might need multiple bundles that use this prependExtensionConfig for configuring doctrine too...

then release it as a major version, not as a minor one

We didn't do an API break. We simply started to reject invalid configuration. Adding a validation is not a BC break included under BC promise anywhere. Users relying on this is unfortunate, but that happens.

You cannot use them at the same time in a single file. but different files could make different choices before.

Unfortunately, documentation doesn't say so. It doesn't specify single file and that implies you cannot use them at the same time no matter 1 or multiple files. You and me know documentation probably really meant single file only, but average user really wouldn't deduct so after reading that. After all, given documentation link was sent to me by someone else.

Projects using mixed approach could still have a working configuration, and you broke BC for them.

Working doesn't mean correct. What's such example? In cases I see, it either doesn't make sense to use long/short syntax, or bundle was designed incorrectly because it never assumed user might have multiple entity managers.

A solution for that could have been to deprecate using default_entity_manager alongside the shortened config (and in the next major version of the bundle, the shortened config would always configure an entity manager named default)

We can't just document this mindfuck as a feature in doctrine-bundle 3. If there is no entity manager named default, we simply cannot be applying the change to new entity manager named default. For shortened configuration to work, default entity manager must be known. Otherwise this shortened configuration will be applied for wrong entity manager.

Btw, I don't even think #1356 actually fixes the issue when you use the shortened configuration with customized default_entity_manager in some places only. So it breaks working configs without solving the original issue.

That might be true, but it's very unlikely to change name of default_entity_manager if you only use simplified config everywhere. That doesn't make sense. Additionally, users would notice such issue really quick, because such configuration from user's POV would not go in effect - they would notice since they have one entity manager only. Unlike when you have multiple entity managers and change will get applied to first one so you might think all others are fine too.

And it seem that now we do have to put all doctrine configuration in one file, and take everything that come with others bundles (especially extensions one) into one file. That is breaking symfony way of doing stuff.

No it's not. Now we CAN'T have any configuration coming from extensions bundles in separate config files.

These are false, please don't spread FUD. Config files are still merged correctly, you just need to use same format everywhere. It actually makes expectations when configs are merged more clear and without side effects.

I think we should revert #1356 as a bugfix and reconsider the issue in light of the feedback we have here.
While you're all here, we would greatly appreciate any ideas to fix #1337 and doctrine/orm#8687 after the revert!

Reverting is always a last resort. If we are to revert this, I would like a plan before doing that. But only plan I see is to deprecate whole simplified config instead. Is that fine/preferred with you?

[stof]

That might be true, but it's very unlikely to change name of default_entity_manager if you only use simplified config everywhere. That doesn't make sense. Additionally, users would notice such issue really quick, because such configuration from user's POV would not go in effect - they would notice since they have one entity manager only. Unlike when you have multiple entity managers and change will get applied to first one so you might think all others are fine too.

Well, the fact that people don't actually change the name of the default entity manager when using the short config is wrong (based on the support I provided on IRC and then Slack over the years).
If we make the shortcut syntax always configure an entity manager named default (well, deprecating the case of passing a custom name alongside the shortened syntax for now, for BC), we will make the short syntax behave in a consistent way (#1337 won't be magically solved by making all entity managers use prod-like settings in the prod env without listing them, but it is not solved either in the current state).
And if we want to warn against #1337, we could trigger a deprecation warning when some of your configs were using the shorten syntax and you have multiple entity managers (mixing the long and short syntaxes with a single entity manager would not cause the issue, solving things for bundles injecting config using the long syntax all the time). Maybe this warning should be triggered only if default_entity_manager is not default though, to avoid triggering it when the project config uses the short syntax to defines its default entity manager and a bundle injects the config for its own separate entity manager

[ostrolucky]

If we make the shortcut syntax always configure an entity manager named default

But that was the issue in #1337. Doctrine-bundle is overriding user specified default_entity_manager with default when short syntax is used. We would go back to original problem. I know you are trying to argue problem is not actually fixed, but it's not a realistic issue to happen when you use short syntax in ALL files. Nobody needs to rename default entity manager when you have one entity manager only and you don't specify this entity manager explicitly.

we could trigger a deprecation warning when some of your configs were using the shorten syntax and you have multiple entity managers (mixing the long and short syntaxes with a single entity manager would not cause the issue, solving things for bundles injecting config using the long syntax all the time). Maybe this warning should be triggered only if default_entity_manager is not default though, to avoid triggering it when the project config uses the short syntax to defines its default entity manager and a bundle injects the config for its own separate entity manager

Ok, I could live with this. So basically current check + count($entity_managers) > 1? Although bundles using prependExtension would still trigger the error if users use multiple entity managers...

[ostrolucky]

See #1368

[netbull]

@ostrolucky sorry, but it's looks like this limitation for mixed simple config is quite strange decision.. If we have configuration for tests, prod and dev we should have 3 configuration files with few lines difference... in most cases the configurations are complex, also there are other third party bundles which overrides parts of the configuration /add something/.. to disallow such override does not look as a legit solution.

Don't get me wrong.. I understand the issues which this causes, but now we loose the configuration overriding option /which is quite flexible/.

[ostrolucky]

@netbull That's again FUD like the 2 other comments I quoted...

Overrides stil work. When defining entity_managers: in one file, you just need to have it in all other files. Content of the key itself will be merged from other files. Start by ensuring empty entity_managers key is in each file, afterwards you will see what I mean. Bundle will force you to move the config related to entity managers under this key, but only within this file. So you don't have to copy configuration from other locations...

[manuel-garcia-dkt]

@netbull That's again FUD like the 2 other comments I quoted...

Overrides stil work. When defining entity_managers: in one file, you just need to have it in all other files. Content of the key itself will be merged from other files. Start by ensuring empty entity_managers key is in each file, afterwards you will see what I mean. Bundle will force you to move the config related to entity managers under this key, but only within this file. So you don't have to copy configuration from other locations...

In my case, this happens having a wrong config key under prod/doctrine.yaml, without wrong key, no duplication is neccesary

[netbull]

@ostrolucky I would not say that's FUD, but is reaction to the unclear error message out of nowhere after updating and some time debugging what the *** broke on the prod CI/CD pipeline..

However setting a dummy entity_managers works just fine. Just to mention that it should not resolve to NULL, because the check uses ISSET which returns false. The proper way /I guess/ is to set it as empty array entity_managers: []

[webhdx]

@ostrolucky Can we then add an empty entity_managers key to the default config file (coming with the recipe)? If not then we can't really extend it from 3rd party bundles without requiring adding that by the user before installing bundle.

[ostrolucky]

@webhdx have you tried

Bundle can use $container->getExtensionConfig('doctrine') and find out what format is used by user.

? Your bundle probably never worked for users with multiple connections/entity managers correctly, did it? This issue at least alerted you to it. And yes I wish we used long format everywhere, so it's possible I'll deprecate short one in next minor instead. But recipe itself is not under my control. Although anyone can try to create a pull request for recipe proposing this. If nothing else, at least default production file would stop misleading people that configuration set there is applied to all entity managers.

[greg0ire]

@NoodlesNZ I have cloned your project, and it shows a config/packages/prod/doctrine.yaml in simple config, and a doctrine/packages/doctrine.yaml in complex config, so the error message is legitimate.

I would fix the first file like so:

--- a/config/packages/prod/doctrine.yaml
+++ b/config/packages/prod/doctrine.yaml
@@ -1,15 +1,17 @@
 doctrine:
     orm:
         auto_generate_proxy_classes: false
-        metadata_cache_driver:
-            type: pool
-            pool: doctrine.system_cache_pool
-        query_cache_driver:
-            type: pool
-            pool: doctrine.system_cache_pool
-        result_cache_driver:
-            type: pool
-            pool: doctrine.result_cache_pool
+        entity_managers:
+            default:
+                metadata_cache_driver:
+                    type: pool
+                    pool: doctrine.system_cache_pool
+                query_cache_driver:
+                    type: pool
+                    pool: doctrine.system_cache_pool
+                result_cache_driver:
+                    type: pool
+                    pool: doctrine.result_cache_pool

If I check with bin/console debug:config, the result looks good 👍

@Webonaute the screenshot you posted show that the SonataMediaBundle extension you provided is considered explicit… and it is, isn't it? Do you have other files where you do not explicitely define entity managers? That's what the message is about.

[ostrolucky]

This will be reverted in 2.4.2 that will be released shortly. I've reopened #1337 and we are waiting for ideas for fixing this there. IMO best would be to deprecate whole short syntax.