DiffGenerator: fix removing schema from table name before applying regex filtering

[cristi-contiu]

Q	A
Type	bug
Fixed issues	#1487

Summary

Failing test and suggested fix for #1487, requested in #1490 (review)

There are differences in the way Migrations and DBAL apply the SchemaAssetsFilter, usually regex: DBAL applies it on the table name (which can be qualified or unqualified, depending on platform schema support and if the table is in the default search-path schema) while Migrations always removes the schema and only applies the filter on the unqualified table name (my_table).

This fix applies the SchemaAssetsFilter on the table name exactly as provided by the DBAL for platforms that support schema.

[greg0ire]

After your change, I don't think the phpdoc comment is accurate anymore. Also, if the platform does not support schémas, surely the dot in a name should be interpreted literally? So... this method is no longer needed maybe?

[ddegasperi]

You might be right that the method might not be needed anymore.

Still, this change improves behavior for platforms that support schemas, without changing how things work for platforms that don’t — so it seems like a safe and useful step forward regardless.

[ddegasperi]

Just wondering if fixing the PHPDoc alignment is the only thing holding this back.

I'm also interested in this fix and would really like to see it included in the main version.

[greg0ire]

Just wondering if fixing the PHPDoc alignment is the only thing holding this back.

No, it's not just PHPDoc, I'd like a definitive answer to my other comment. Please investigate whether removing this method fixes your issue without breaking anything else.

[ddegasperi]

Thanks for the clarification. I’ve just tested the diff command without the resolveTableName method in my Symfony application. It uses multiple connections, each managing its own schema on the same PostgreSQL database relying on the schema_filter.

I didn’t encounter any issues — on the contrary, it now correctly excludes tables with the same name (e.g. messenger_messages) from other schemas, which is exactly what I would expect.

Let me know if you'd like me to test anything else.

[greg0ire]

I guess what would need to be tested would be a table with a dot in its name with a SQLite database, but maybe you could try opening a PR to see if removing that method passes the test suite.

[cristi-contiu]

I am all good with removing the resolveTableName method altogether - was keeping that if for minimal impact / maximum BC for platforms that don't support schemas, but you are right that it could be a bug for both.

I can update this PR with the discussed changes. And will get back to you on the SQLite test.

[greg0ire]

Congrats, green tests 🎉

Now can you please squash your 2 commits into one and improve your commit message according to the contributing guide.

Make it clear why it's OK to remove the method.

[ddegasperi]

I also ran a quick test with SQLite:

1. Installed a fresh Symfony application
2. Created an entity with #[ORM\Table(name: 'product', schema: 'shop')]
3. Generated the migration:

// ...
$this->addSql('CREATE TABLE shop.product (id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, name VARCHAR(255) NOT NULL)');
// ...

Note: Without quoting shop.product, the migration fails on SQLite.

4. Added the schema filter: schema_filter: ~^(?!shop)~
5. Renamed the table to #[ORM\Table(name: 'products', schema: 'shop')]
6. Run the "diff" command -> no migration was generated, as expected.

Do you think further tests are needed?

By the way, I also noticed that with this change, schema_filter now works even without using a lookahead e.g. ~^(shop|doctrine_migration_versions) when schemas are used in the DB. That feels like a nice improvement, in my opinion.

[greg0ire]

Thanks for testing! I think that's enough tests.

Let's wait for a better commit message and merge this.

[ddegasperi]

@cristi-contiu Just checking in — since the change seems to be accepted and only the squashing and commit message update are missing, is there anything I can do to help get this over the finish line and merged into main?

I'd really love to see this included. Thanks a lot!

[greg0ire]

I guess you could provide a good commit message as a Github comment, that would be helpful.

[cristi-contiu]

@greg0ire Let me know what you think.

[greg0ire]

It looks great, but still lacks an explanation of why it's OK to remove this method. I was the one to originally suggest that in #1492 (comment), but I was unsure, and I think it warrants a thorough investigation and a rock solid explanation. Finding the commit where it should have been removed would most certainly help.

[ddegasperi]

@greg0ire Would appending the following explanation to the existing commit message of @cristi-contiu be clear enough to address the reason for the removal?

The resolveTableName() method has been removed entirely because it is no
longer needed and was causing inconsistent behavior. On platforms that
support schemas (e.g., PostgreSQL, SQL Server), DBAL provides qualified
table names (schema.table) which should be preserved for proper filtering.
On platforms that don't support schemas (e.g., MySQL, SQLite), DBAL already
provides unqualified table names, making the schema stripping unnecessary.

The original purpose of resolveTableName() was to handle cases where table
names might contain dots on platforms without schema support, but this
approach was flawed because it incorrectly assumed any dot represented a
schema separator. This could break legitimate table names containing dots
on platforms like SQLite. By removing this method and letting DBAL handle
the platform-specific table name formatting, we achieve consistent behavior
across all platforms while fixing the schema filtering issue.

Testing confirms that removing this method works correctly: on schema-aware
platforms, qualified names are properly filtered, and on non-schema
platforms, table names are already unqualified by DBAL.

[greg0ire]

Yes 👍

[cristi-contiu]

Thanks @ddegasperi , I replaced the commit message description with the one you provided.

[greg0ire]

Would appending the following explanation

@cristi-contiu please don't replace and merge both messages in one big message

[cristi-contiu]

@greg0ire I restored the first part of the description.

[greg0ire]

Thanks @cristi-contiu !