Non-nullabe result of comparing tables #5770
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
morozov
changed the title
morozov
force-pushed
the
deprecate-comparator-diff-table
branch
from
a8d12d7 to
acc2c8a
Compare
greg0ire
approved these changes
Oct 18, 2022
derrabus
approved these changes
Oct 18, 2022
|
The I got it, that this should be deprecated and so on, but however this is now a change in behaviour, and thus "breaking". This has not been communicated that way. Before this change, renamed table stayed as changed table in the SchemaDiff. Personally I think, renamed tables are changes regarding a "SchemaDiff" - or was this not intended ? Breakes our application and popped up in nightlies automatically checkonf for new ^3.x releases of doctrine/dbal. Breaking out database compare and migrate logic. |
reviewtypo3org
pushed a commit
to TYPO3/typo3
that referenced
this pull request
Oct 23, 2022
With doctrine/dbal v3.5.0 some changes has been introduced which shows invalid test setup. This change use proper test setup to be okay again, if doctrine/dbal v3.5.0 is used. See: doctrine/dbal#5770 Used command(s): > Build/Scripts/runTests.sh -s phpstanGenerateBaseline Resolves: #98704 Releases: main Change-Id: I936e5cd291086020e60be418176b0f294fb43a45 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/76231 Tested-by: core-ci <[email protected]> Tested-by: Christian Kuhn <[email protected]> Tested-by: Andreas Fernandez <[email protected]> Tested-by: Stefan Bürk <[email protected]> Reviewed-by: Christian Kuhn <[email protected]> Reviewed-by: Andreas Fernandez <[email protected]> Reviewed-by: Stefan Bürk <[email protected]>
TYPO3IncTeam
pushed a commit
to TYPO3-CMS/core
that referenced
this pull request
Oct 23, 2022
With doctrine/dbal v3.5.0 some changes has been introduced which shows invalid test setup. This change use proper test setup to be okay again, if doctrine/dbal v3.5.0 is used. See: doctrine/dbal#5770 Used command(s): > Build/Scripts/runTests.sh -s phpstanGenerateBaseline Resolves: #98704 Releases: main Change-Id: I936e5cd291086020e60be418176b0f294fb43a45 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/76231 Tested-by: core-ci <[email protected]> Tested-by: Christian Kuhn <[email protected]> Tested-by: Andreas Fernandez <[email protected]> Tested-by: Stefan Bürk <[email protected]> Reviewed-by: Christian Kuhn <[email protected]> Reviewed-by: Andreas Fernandez <[email protected]> Reviewed-by: Stefan Bürk <[email protected]>
|
@sbuerk please file a new issue and provide the steps to reproduce it via a small snippet of code. |
reviewtypo3org
pushed a commit
to TYPO3/typo3
that referenced
this pull request
Oct 24, 2022
doctrine/dbal is working on deprecating and changing code and behaviour in 3.x as preparation for removal in 4.x. Starting with doctrine/dbal v3.5.0, the table rename detection in the TYPO3 database compare no longer works as expected. TableDiff got an `isEmpty()` check, which is used in SchemaDiff and thus filtering out "renamed" tables without further changes, leading to a change of behavior. doctrine/dbal favors renaming table no longer by setting a new name in the TableDiff, thus not having the changed name in the `isEmpty()` check. See: doctrine/dbal#5770 doctrine/dbal deprecations and changes require systematical adoption to cover all the changes. This takes time and proper testing, which is out of the scope for this change. This change overrides the `TableDiff->isEmpty()` method in the corresponding core fascade class, adding checks for $newName and $tableOptions checks as a intermediate workaround. Thus keep compatibillity between doctrine/dbal <3.5 and 3.5 for now. Resolves: #98707 Releases: main Change-Id: If305e4d809be29585e0b6a72c3215f5cff68762d Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/76232 Tested-by: core-ci <[email protected]> Tested-by: Christian Kuhn <[email protected]> Tested-by: Benni Mack <[email protected]> Tested-by: Stefan Bürk <[email protected]> Reviewed-by: Christian Kuhn <[email protected]> Reviewed-by: Benni Mack <[email protected]> Reviewed-by: Stefan Bürk <[email protected]>
TYPO3IncTeam
pushed a commit
to TYPO3-CMS/core
that referenced
this pull request
Oct 24, 2022
doctrine/dbal is working on deprecating and changing code and behaviour in 3.x as preparation for removal in 4.x. Starting with doctrine/dbal v3.5.0, the table rename detection in the TYPO3 database compare no longer works as expected. TableDiff got an `isEmpty()` check, which is used in SchemaDiff and thus filtering out "renamed" tables without further changes, leading to a change of behavior. doctrine/dbal favors renaming table no longer by setting a new name in the TableDiff, thus not having the changed name in the `isEmpty()` check. See: doctrine/dbal#5770 doctrine/dbal deprecations and changes require systematical adoption to cover all the changes. This takes time and proper testing, which is out of the scope for this change. This change overrides the `TableDiff->isEmpty()` method in the corresponding core fascade class, adding checks for $newName and $tableOptions checks as a intermediate workaround. Thus keep compatibillity between doctrine/dbal <3.5 and 3.5 for now. Resolves: #98707 Releases: main Change-Id: If305e4d809be29585e0b6a72c3215f5cff68762d Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/76232 Tested-by: core-ci <[email protected]> Tested-by: Christian Kuhn <[email protected]> Tested-by: Benni Mack <[email protected]> Tested-by: Stefan Bürk <[email protected]> Reviewed-by: Christian Kuhn <[email protected]> Reviewed-by: Benni Mack <[email protected]> Reviewed-by: Stefan Bürk <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Null references are often referred to as "The Billion Dollar Mistake". They require checking the type of the variable at runtime before passing it down the line or, otherwise, its usage may result in a type error.
The number of the assertions like the following in our test suite illustrates that well enough:
The need for such assertions could be eliminated if table comparison always resulted in a non-null (currently, non-false) value (this is already the case for
Comparator::compareSchemas()).An empty diff can be safely passed down the line and will result in a no-op (see the newly added tests). If the business logic requires checking whether the diff is empty, the newly introduced
isEmpty()methods could be used for that.