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

Schema filters for setups with multiple connections are only applied to the connection referred to by default_connection #1861

Open
NanoSector opened this issue Jan 27, 2025 · 4 comments

Comments

@NanoSector
Copy link
Contributor

Bug Report

Q A
Version 2.13.2
Previous Version if the bug is a regression N/A

Summary

Schema filters are only registered for the default connection on multi-connection setups.

For example, combining this with the migrations bundle, the doctrine_migration_versions table is only filtered out of e.g. doctrine:schema:drop for the default_connection in a setup with multiple entity managers and multiple migration configurations. This causes implicit (and depending on the filters, potentially destructive) behaviour.

Current behavior

The \Doctrine\Bundle\DoctrineBundle\DependencyInjection\Compiler\DbalSchemaFilterPass registers schema filters (like \Doctrine\Bundle\MigrationsBundle\EventListener\SchemaFilterListener) only for the connection referred to by default_connection.

Expected behavior

Schema filters should be applied to every connection when tagged appropriately in the container.

How to reproduce

Configuring Symfony's doctrine.yaml as follows (snippet from our configuration with modified naming):

doctrine:
  dbal:
    connections:
      database1:
        dbname: '%env(DATABASE_SCHEMA_DATABASE1)%'
        # Common parameters
        host: '%env(string:default:database.fallback_host:DATABASE_RDS_HOST)%'
        user: '%env(DATABASE_USERNAME)%'
        password: '%env(DATABASE_PASSWORD)%'
        port: '%env(DATABASE_PORT)%'
        driver: 'pdo_mysql'
        server_version: '%env(DATABASE_SERVER_VERSION)%'
        charset: 'utf8mb4'
        profiling_collect_backtrace: '%kernel.debug%'
        use_savepoints: true
        default_table_options:
          charset: 'utf8mb4'
          collation: 'utf8mb4_general_ci'
          engine: 'InnoDB'

      database2:
        dbname: '%env(DATABASE_SCHEMA_DATABASE2)%'
        # Common parameters
        host: '%env(string:default:database.fallback_host:DATABASE_RDS_HOST)%'
        user: '%env(DATABASE_USERNAME)%'
        password: '%env(DATABASE_PASSWORD)%'
        port: '%env(DATABASE_PORT)%'
        driver: 'pdo_mysql'
        server_version: '%env(DATABASE_SERVER_VERSION)%'
        charset: 'utf8mb4'
        profiling_collect_backtrace: '%kernel.debug%'
        use_savepoints: true
        default_table_options:
          charset: 'utf8mb4'
          collation: 'utf8mb4_general_ci'
          engine: 'InnoDB'


  orm:
    auto_generate_proxy_classes: true
    enable_lazy_ghost_objects: true

    controller_resolver:
      enabled: false
      auto_mapping: false

    entity_managers:
      database1:
        connection: database1
        report_fields_where_declared: true
        validate_xml_mapping: true
        naming_strategy: doctrine.orm.naming_strategy.underscore_number_aware
        auto_mapping: false
        mappings:
          App:
            type: attribute
            is_bundle: false
            dir: '%kernel.project_dir%/src/Database1/Domain/Entity'
            prefix: 'App\Database1\Domain\Entity'
            alias: AppDatabase1

      database2:
        connection: database2
        report_fields_where_declared: true
        validate_xml_mapping: true
        naming_strategy: doctrine.orm.naming_strategy.underscore_number_aware
        auto_mapping: false
        mappings:
          Domain:
            type: attribute
            is_bundle: false
            dir: '%kernel.project_dir%/src/Database2/Domain/Entity'
            prefix: 'App\Database2\Domain\Entity'
            alias: AppDatabase2

Results in the following output for the different databases.

Assuming:

  • database1 has tables abc and doctrine_migration_versions
  • database2 has tables def and doctrine_migration_versions

Running bin/console doctrine:schema:drop --dump-sql --full-database --em=database1 yields:

DROP TABLE abc;

Running bin/console doctrine:schema:drop --dump-sql --full-database --em=database2 yields:

DROP TABLE def;
DROP TABLE doctrine_migration_versions;

Note the absence of doctrine_migration_versions in the case of database1, because it is automatically selected as default_connection by the Doctrine bundle and thus the filter is applied.

@dmaicher
Copy link
Contributor

dmaicher commented Jan 29, 2025

The DbalSchemaFilterPass supports configuring a connection on the doctrine.dbal.schema_filter tag.

On DoctrineMigrationsBundle this is however only registered for the default connection.

Or you think if no connection name is passed it should apply to all connections? (that would break BC though)

Also related: doctrine/migrations#1406 (comment) and doctrine/DoctrineMigrationsBundle#590

@NanoSector
Copy link
Contributor Author

@dmaicher If not explicitly configured otherwise, we’d expect all connections to behave the same.

The current behaviour is extremely implicit and (while I haven’t checked) I assume this is not documented in the configuration. The service definition you link to also doesn’t explicitly mention the default connection; this is a fallback which is handled in code in the filter pass.

Or you think if no connection name is passed it should apply to all connections? (that would break BC though)

I feel like this is the best outcome, but as you mention this brings a BC break. Assuming this would be fixed in the next major Symfony release I see several interim/alternative solutions:

  1. The documentation for the bundle is updated to mention this filter pass behaviour and how to apply filters to other connections. However this still causes implicit behaviour as bundles like the migrations bundle can still invisibly attach filters to just the default connection.
  2. The default connection option is given more prominence by being required in the next Symfony release (and triggering deprecation warnings), so users are actively made aware that one connection might have different properties. This at least solves the part where the first connection is implicitly chosen as default.
  3. The Doctrine bundle checks if multiple connections are present and if so, triggers warnings (in the profiler?) for mismatched filter configuration if filters have not been explicitly defined. I’m not sure how this would work on a technical level but this can be given thought later.
  4. Filters can no longer be registered without specifying a connection, similarly to how entity managers can have a connection specified. This would be the most breaking option but in my opinion the most explicit as it makes users aware of the functionality and its implications. This would also allow users to remove filters from the default connection. The DX portion of configuring filters for bundles could be solved with a Flex recipe which auto-registers the filter for the default doctrine connection recipe.

There could be better options, I hope this sparks some discussion.

@greg0ire
Copy link
Member

greg0ire commented Feb 1, 2025

Maybe another solution would be to add a bundle configuration option to make the behavior change for tags that don't have a connection specified, and deprecate not setting it to true. Code that would like to stick with the previous behavior could specify the default connection as a tag parameter to avoid having the filter applied to all connections. In the next major, the option would become a no-op, and then in the minor following the next major, setting it would be deprecated.

@ostrolucky
Copy link
Member

This seems intended #935 (comment) The way I see it, this is a bug in doctrine migrations bundle, as SchemaFilterListener is there and it doesn't list all connections

However, the way to facilitate both OP and linked comment thread would be to default to all connections if not otherwise specified, but still allow to specify connection for the tag. I don't think this is a big enough behavioral change to introduce a config option for it nor delay for next major either.

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

No branches or pull requests

4 participants