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.

Sign up for GitHub

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

Jump to bottom

Create postGenerateComparisonSchema event #11374

Open
wants to merge 1 commit into
base: 3.2.x
Choose a base branch
from
Open

Create postGenerateComparisonSchema event #11374

wants to merge 1 commit into from

Conversation

wmouwen
Copy link

@wmouwen wmouwen commented Mar 17, 2024 •
edited

Schemas generated from metadata cause the event ToolEvents::postGenerateSchema to be triggered in the method SchemaTool::getSchemaFromMetadata. It would be helpful to do the same for schemas generated for comparison in SchemaTool::createSchemaForComparison. This PR introduces the new event ToolEvents::postGenerateComparisonSchema.

PR relates to an attempt to fix doctrine/migrations#1406. It is a requirement of doctrine/migrations#1418 and doctrine/DoctrineMigrationsBundle#529.

This was referenced Mar 17, 2024
Open
Open
Open
@wmouwen wmouwen changed the base branch from 3.1.x to 3.0.x March 17, 2024 16:23
@greg0ire
Copy link
Member

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

@wmouwen wmouwen force-pushed the event-listener-migration-schema branch from 30cdf61 to c946ff3 Compare March 18, 2024 18:06
@greg0ire greg0ire changed the base branch from 3.0.x to 3.1.x March 18, 2024 20:12
@greg0ire
Copy link
Member

Retargeting to 3.1.x since 3.0.x is not maintained anymore.

@greg0ire
Copy link
Member

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

  1. git rebase -i origin/3.0.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

greg0ire
greg0ire reviewed Mar 18, 2024
View reviewed changes
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs/en/reference/events.rst needs to document the new event

@wmouwen wmouwen force-pushed the event-listener-migration-schema branch from 7fc5f68 to 71d6576 Compare March 18, 2024 20:33
greg0ire
greg0ire previously approved these changes Mar 18, 2024
View reviewed changes
SenseException
SenseException previously approved these changes Mar 20, 2024
View reviewed changes
@greg0ire
Copy link
Member

Since this is not a patch, I think we should target 3.2.x 🤔

@derrabus derrabus changed the base branch from 3.1.x to 3.2.x March 21, 2024 09:28
@derrabus derrabus dismissed stale reviews from SenseException and greg0ire March 21, 2024 09:28

The base branch was changed.

@derrabus derrabus changed the base branch from 3.2.x to 3.1.x March 21, 2024 09:28
@derrabus
Copy link
Member

Since this is not a patch, I think we should target 3.2.x 🤔

Yes. 3.2.x is a bit behind. I'll retarget after a merge up.

@derrabus derrabus changed the base branch from 3.1.x to 3.2.x March 21, 2024 14:17
derrabus
derrabus requested changes Mar 21, 2024
View reviewed changes
tests/Tests/ORM/Tools/SchemaToolTest.php Outdated
);
$schemaTool = new SchemaTool($em);

$schema = $schemaTool->getUpdateSchemaSql([]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable $schema.

tests/Tests/ORM/Tools/SchemaToolTest.php Outdated
Comment on lines 188 to 189
self::assertTrue($listener->schemaCalled);
self::assertTrue($listener->comparisonSchemaCalled);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test does not really show me that I was able to influence the generated schema or SQL.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to improve the tests. This should show you that you can access the schema which the comparator uses.

@wmouwen wmouwen force-pushed the event-listener-migration-schema branch from 71d6576 to 5ba0c02 Compare March 21, 2024 18:56
@wmouwen wmouwen requested a review from derrabus March 21, 2024 18:56
@wmouwen wmouwen force-pushed the event-listener-migration-schema branch from 5086074 to 1b938ab Compare March 22, 2024 08:14
@greg0ire greg0ire mentioned this pull request Mar 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg0ire greg0ire greg0ire left review comments

@SenseException SenseException SenseException left review comments

@beberlei beberlei Awaiting requested review from beberlei

@derrabus derrabus Awaiting requested review from derrabus

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

doctrine:schema:validate returns DROP TABLE doctrine_migration_versions;
4 participants
@wmouwen @greg0ire @derrabus @SenseException