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

Merge 2.13.x up into 3.0.x #9764

Merged
merged 7 commits into from
May 16, 2022
Merged

Merge 2.13.x up into 3.0.x #9764

merged 7 commits into from
May 16, 2022

Conversation

greg0ire
Copy link
Member

No description provided.

greg0ire and others added 6 commits May 14, 2022 16:56
When computing a foreign key column name, the referenced column name
may be null in the case of a self referencing entity with join columns
defined in the mapping.
These tests were using the fact that some arguments of some methods of
the naming strategy interface are optional or nullable for now to avoid
providing some. In practice, these arguments are always provided, and
that should also be the case in tests.
…-platform

Do not call AbstractSchemaManager::getDatabasePlatform()
Fix phpdoc and tests for NamingStrategy
@greg0ire
Copy link
Member Author

greg0ire commented May 16, 2022

@morozov the tests fails in tests/Doctrine/Tests/ORM/Functional/SequenceEmulatedIdentityStrategyTest.php, in tearDown, where a sequence is being removed manually.

// drop sequence manually due to dependency
$connection->executeStatement(
$platform->getDropSequenceSQL(
$platform->getIdentitySequenceName('seq_identity', 'id')
)
);

The comment refers to a dependency, and so does the error message:

PDOException: SQLSTATE[2BP01]: Dependent objects still exist: 7 ERROR:  cannot drop sequence seq_identity_id_seq because column id of table seq_identity requires it
HINT:  You can drop column id of table seq_identity instead.

I'm going to drop tearDown entirely and let OrmFunctionalTestCase::createSchemaForModels() do its thing instead. I don't really understand how this relates to #9762 though.

@morozov
Copy link
Member

morozov commented May 16, 2022

I don't really understand how this relates to #9762 though.

I don't think it's related to #9762. It's more likely related to doctrine/dbal#5396.

@morozov
Copy link
Member

morozov commented May 16, 2022

I'm going to drop tearDown entirely and let OrmFunctionalTestCase::createSchemaForModels() do its thing instead.

The underlying sequence is now managed by Postgres, so there shouldn't be any additional handling in the ORM necessary.

This allows to get rid of tearDown(), which contained a special handling
that is no longer necessary since we switched away from explicitely
managed sequences, and caused the test suite to fail.
@greg0ire
Copy link
Member Author

I updated my commit message 👍

@morozov
Copy link
Member

morozov commented May 16, 2022

I updated my commit message 👍

Should this change have been part of the merge commit? If done after it, it leaves a knowingly broken state behind and it couldn't have been done prior to the merge either.

@greg0ire
Copy link
Member Author

it couldn't have been done prior to the merge either.

Why not? I think it could be cherry-picked on 3.0.x prior to the merge, couldn't it? The issue does not trigger because of the merge up, right?

@morozov
Copy link
Member

morozov commented May 16, 2022

I think it could be cherry-picked on 3.0.x prior to the merge, couldn't it?

You're actually right. Given that the dependency of dev ORM on dev DBAL isn't locked, this code could have been updated prior to the merge. If it was locked, it should have been part of the update of the lock.

@greg0ire greg0ire requested review from morozov and derrabus May 16, 2022 20:52
@greg0ire greg0ire merged commit 72810e9 into doctrine:3.0.x May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants