Issue #3547382 by richardgaunt, joshua1234511: Remove deprecated layouts - Contained and edge to Edge [PART 1] #1441
Conversation
WalkthroughA new post-update migration routine is added that upgrades deprecated single-column layouts to three-column layouts across all LayoutBuilderEntityViewDisplay configurations. The migration updates allowed layout registrations and revises layout section identifiers and region mappings accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant System as System/Drush
participant PostUpdate as Post-update Hook
participant Storage as Entity Storage
participant Display as LayoutBuilderEntityViewDisplay
participant Config as Updated Config
System->>PostUpdate: Execute post-update
activate PostUpdate
PostUpdate->>Storage: Load all LayoutBuilderEntityViewDisplay instances
activate Storage
Storage-->>PostUpdate: Return all display configs
deactivate Storage
loop For each display
PostUpdate->>Display: Check if layout builder enabled
alt Layout builder enabled
PostUpdate->>Display: Check for deprecated layouts
alt Contains deprecated layouts
PostUpdate->>Display: Remove civictheme_one_column/-contained<br/>from allowed_layouts
PostUpdate->>Display: Append civictheme_three_columns
PostUpdate->>Display: Replace layout section IDs
PostUpdate->>Display: Update component regions<br/>(content → main)
PostUpdate->>Config: Save updated display
activate Config
Config-->>PostUpdate: Confirmed
deactivate Config
note over PostUpdate: Log update message
else No deprecated layouts
note over PostUpdate: Skip display
end
else Layout builder not enabled
note over PostUpdate: Skip display
end
end
PostUpdate-->>System: Return all update messages
deactivate PostUpdate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Areas requiring extra attention:
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/themes/contrib/civictheme/civictheme.post_update.php (1)
997-1058: Consider extracting common logic for future updates.This function shares significant logic with
civictheme_post_update_enable_three_column_layout()(lines 685-762). While post-update hooks typically remain immutable once deployed, and this duplication appears intentional to provide a corrected migration path, consider extracting the common layout transformation logic into a reusable helper method (e.g., inCivicthemeUpdateHelper) if similar updates are anticipated in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/themes/contrib/civictheme/civictheme.post_update.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-push
- GitHub Check: build-and-push
- GitHub Check: commit
🔇 Additional comments (3)
web/themes/contrib/civictheme/civictheme.post_update.php (3)
997-1006: LGTM: Function setup is clean.The function correctly initializes the outdated layouts list, loads all LayoutBuilderEntityViewDisplay entities, and sets up tracking variables.
1028-1049: LGTM: Section replacement logic is sound.The transformation correctly:
- Replaces outdated layout IDs with
'civictheme_three_columns'- Preserves the
is_containedsetting based on the original layout- Maps components from the
'content'region to'main'region (required for three-column layouts)
1050-1058: LGTM: Conditional save and message handling.The function correctly saves only modified entity displays and returns descriptive messages. The empty return when nothing is updated is acceptable.
| } | ||
| } | ||
| if ($replaced) { | ||
| $allowed_layouts[] = 'civictheme_three_columns'; |
There was a problem hiding this comment.
Prevent potential duplicate entry in allowed_layouts.
Before appending 'civictheme_three_columns', verify it doesn't already exist in $allowed_layouts to maintain idempotency and prevent duplicates.
Apply this diff:
if ($replaced) {
- $allowed_layouts[] = 'civictheme_three_columns';
+ if (!in_array('civictheme_three_columns', $allowed_layouts, TRUE)) {
+ $allowed_layouts[] = 'civictheme_three_columns';
+ }
$entity_view_mode_restriction['allowed_layouts'] = array_values($allowed_layouts);🤖 Prompt for AI Agents
In web/themes/contrib/civictheme/civictheme.post_update.php around line 1022,
the code unconditionally appends 'civictheme_three_columns' to $allowed_layouts
which can create duplicate entries; update the code to check if
'civictheme_three_columns' is not already present in $allowed_layouts (e.g.,
using in_array or array_search) before appending so the operation is idempotent
and avoids duplicates.
joshua-salsadigital
added
the
State: Needs review
joshua-salsadigital
changed the title
https://www.drupal.org/project/civictheme/issues/3547382
Checklist before requesting a review
Issue #123456 by drupal_org_username: Issue titleChangedsection about WHY something was done if this was not a normal implementationChanged
Screenshots
Summary by CodeRabbit