-
Notifications
You must be signed in to change notification settings - Fork 33
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
Clean up after the homepage builder work #6676
Clean up after the homepage builder work #6676
Conversation
|
…s-and-homepage-settings
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.
All looks well tidied to me. Just a couple of comments
UPDATE content_builder_layouts | ||
SET content_buildable_id = NULL, content_buildable_type = NULL | ||
WHERE content_buildable_type = 'HomePage'; |
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.
Not entirely clear what's happening here, but given it is setting columns to null, just want to check there is no need for that data any more.
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.
This is because the homepage was deleted, so the layouts linked to homepages can no longer reference them.
when 'HomePage' | ||
HomePage.first | ||
nil |
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.
Would this be better as an if/else or ternary as surely any other parameter of content_buildable will return nil too?
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'm considering it tech debt, but the report builders should also be supported through this endpoint. I don't have a very strong preference, but I think it can be good to explicitly list all the expected content_buildable types (for example, here you can see that the report builders are (probably) not yet supported because they are not a part of the case statement).
This is a cleanup PR which we'll release in January 2024 (to have some time to be able to revert to the old homepage in case a customer is very unhappy with their new homepage). Review is therefore not urgent.
craftjs_jsonmultiloc
attribute from the layouts.currently_working_on_text
is no longer used.