-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this 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.
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. |
There was a problem hiding this 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 ? )
@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 ? |
I thought something similar and yes.. makes sense, will add one in the database after the document id one |
@markkaylor needs this change asp, @alexandrebodin let me know if you would like to add additional changes and I can create a new pr! |
What does it do?
Migrating from v4 content types with i18n disabled resulted in the following error:
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.