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

fix: create locale column when migrating from v4 #20261

Open
wants to merge 4 commits into
base: v5/main
Choose a base branch
from

Conversation

Marc-Roig
Copy link
Contributor

What does it do?

Migrating from v4 content types with i18n disabled resulted in the following error:
image

The cause was the locale column not being in the database for those content types. It is assumed , in v5, that the locale column will be present even if i18n is disabled on a content type.

This fix proposes adding a locale column in the discard-draft migration, similarly to how we add the document_id in another migration.

@Marc-Roig Marc-Roig added pr: fix This PR is fixing a bug source: core:core Source is core/core labels May 3, 2024
@Marc-Roig Marc-Roig self-assigned this May 3, 2024
Copy link

vercel bot commented May 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
contributor-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 7, 2024 10:24am

Copy link
Contributor

@innerdvations innerdvations left a comment

Choose a reason for hiding this comment

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

I don't like it because in theory we should be letting the db sync handle this, but the whole point is we have to do these migrations before the db sync, so... I don't see any alternative except to manually add it like this. Thanks! :)

I didn't QA it by the way, but the idea works for me and code looks fine.

@Marc-Roig
Copy link
Contributor Author

same.. we did something similar for the document id, but at this point if we want to use the document service we need to include the locale column somehow.

I also was not fully convinced this migration was the right place to do this 🤔 but as it's the only place it's necessary I put it there.

Will ask Marion to include this in a blitz session too, I think it's important we validate it works.

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

Looks good, but should we even migrate if the column doesn't exist yet ? (can't we just skip if there is no locale in the meta ? )

@Marc-Roig
Copy link
Contributor Author

@alexandrebodin In this case we need to migrate if the content type has D&P enabled, regardless of its i18n configuration. And by using the discardDraft of the document service we are tied to have content types with an available locale column

@alexandrebodin
Copy link
Member

@alexandrebodin In this case we need to migrate if the content type has D&P enabled, regardless of its i18n configuration. And by using the discardDraft of the document service we are tied to have content types with an available locale column

Ho yes ok , Should we just have a locale migration to add that column everywhere and order it earlier in the list ?

@Marc-Roig
Copy link
Contributor Author

Marc-Roig commented May 7, 2024

Ho yes ok , Should we just have a locale migration to add that column everywhere and order it earlier in the list ?

I thought something similar and yes.. makes sense, will add one in the database after the document id one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix This PR is fixing a bug source: core:core Source is core/core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants