-
-
Notifications
You must be signed in to change notification settings - Fork 390
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
DiffGenerator: fix applying regex pattern filter to mappings schema #1490
base: 3.8.x
Are you sure you want to change the base?
Conversation
src/Generator/DiffGenerator.php
Outdated
|
||
$schemaAssetsFilter = $this->dbalConfiguration->getSchemaAssetsFilter(); | ||
if ($previousFilter === null) { | ||
return $this->schemaManager->introspectSchema(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kept for compatibility with DBAL v3. In DBAL v4, it can no longer be null
. Might trigger a codecov warning
3936acf
to
cd81404
Compare
@@ -53,16 +51,16 @@ static function ($assetName) use ($filterExpression) { | |||
$assetName = $assetName->getName(); | |||
} | |||
|
|||
return preg_match($filterExpression, $assetName); | |||
return (bool) preg_match($filterExpression, $assetName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casting to bool
for docs and DoctrineBundle alignment
0ae9dc5
to
92b5f6c
Compare
92b5f6c
to
7a187c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a failing test?
@greg0ire My use case is the DiffCommand being called without an explicit Failing test and pipeline There are differences in the way Migrations and DBAL apply the filter: DBAL applies it during schema introspection on what it thinks it's the most accurate table name using "portableTableDefinition" which has platform-specific checks like schemas support or current / default search-path schemas (for example The Migrations library should at least apply the regex filtering on the exact table name provided by the user in the mappings, it should not assume that |
Then there is the broader issue (addressed in this PR): if the DiffGenerator should manipulate/filter the mapped The metadata schema does not have the advanced platform-specific checks that the introspected schema has, and any apparent differences are addressed by the Comparator during diff. For example, a mapped table with name |
I don't know why there is a filter at the migrations level. @goetas , maybe you know? |
Summary
DiffGenerator applies regex pattern filtering to the metadata-based schema (
$toSchema
) using DBAL'sschemaAssetsFilter
(and DoctrineBundle'sdoctrine.dbal.schema_filter
, if applicable), which was meant for filtering the introspected schema ($fromSchema
) to prevent dropping existing tables from the database (DBAL and DoctrineBundle already cover this). This is also explained in the docs : "custom tables which are not managed by Doctrine" = not in the Doctrine mappings, so if an asset is in the metadata ("managed by Doctrine"), it should be included in both$toSchema
and$fromSchema
(if it exists), regardless ofdoctrine.dbal.schema_filter
config.This extra filtering also generates differences in the diff created by Migrations vs the ones created by ORM's SchemaTool, as explained in #1487 .
The logic not to exclude tables in the introspected schema that exist in the mappings was ported from doctrine/orm#7875 and prevents adding unwanted "CREATE TABLE" queries.