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

BUGFIX: Stabilize doctrine migrations #3519

Draft
wants to merge 2 commits into
base: 7.3
Choose a base branch
from

Conversation

bwaidelich
Copy link
Member

@bwaidelich bwaidelich commented Nov 23, 2021

Related: #3118

@jonnitto
Copy link
Member

How can I test this one?

@sorenmalling
Copy link
Contributor

@bwaidelich Is similar required for pgsql?

@markusguenther
Copy link
Member

@bwaidelich Is similar required for pgsql?

For Postgres, we have own migrations

Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

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

TBH: I am too lazy to try this out, so let me just ask: Not all migrations have up and down adjusted, is that correct? Does it work in both directions?

@bwaidelich
Copy link
Member Author

TBH: I am too lazy to try this out

Haha, thanks for the honest reply :D

I'll test down migration and take care of postgres and steps to reproduce asap

Copy link
Contributor

@fcool fcool left a comment

Choose a reason for hiding this comment

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

At least the up migration part for MySQL works as expected, if this patch is applied.

Tested it in several cases (empty database, already existing flow project, where neos is added right now) and everytime everything was the way it should be. :)

Comment on lines +28 to +30
if (in_array('typo3_flow_resource_resource', $tableNames, true) && in_array('typo3_typo3_domain_model_media_image', $tableNames, true)) {
$this->addSql("ALTER TABLE typo3_typo3_domain_model_media_image ADD CONSTRAINT typo3_typo3_domain_model_media_image_ibfk_1 FOREIGN KEY (flow3_resource_resource) REFERENCES typo3_flow_resource_resource(flow3_persistence_identifier)");
}
Copy link
Member

Choose a reason for hiding this comment

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

Other similar checks have an elseif part, is that not needed here?

Copy link
Contributor

Choose a reason for hiding this comment

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

At least I can confirm that the difference to the "current" schema after a doctrine:migrate has been the same in all cases.

Are we aware that even a fully migrated Neos CMS 7.3 creates new Migrations for two tables? (neos_contentrepository_domain_model_nodedata and neos_media_domain_model_tag)

Copy link
Contributor

Choose a reason for hiding this comment

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

(but that's not the business of this PR ;) )

@bwaidelich
Copy link
Member Author

This targets an outdated branch, we can pick this one up again if issues pop up

@bwaidelich bwaidelich closed this Nov 1, 2023
@fcool
Copy link
Contributor

fcool commented Nov 1, 2023

How about just retargeting it? The issue is valid even for 8.3 - which is no wonder, as the migrations usally do never change ;)

@kdambekalns kdambekalns self-assigned this Nov 4, 2023
@kdambekalns
Copy link
Member

I think something needs to be done, so I'll reopen and assign to myself. I'll check the needed things for 7.3 and up the coming two weeks If I don't, please nag me.

@kdambekalns kdambekalns reopened this Nov 4, 2023
@mhsdesign
Copy link
Member

@kdambekalns did your fix over here #4236 makes this obsolete?

@mhsdesign mhsdesign marked this pull request as draft February 8, 2024 19:08
@mhsdesign mhsdesign changed the base branch from 7.2 to 7.3 February 8, 2024 19:10
@github-actions github-actions bot added the 7.3 label Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants