Issue #3547382 by richardgaunt, joshua1234511: Remove deprecated layouts - Contained and edge to Edge [PART 2] #1442
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 tasks
Contributor
WalkthroughThis pull request removes two deprecated one-column layout definitions from the configuration, deletes their associated Twig template files, and introduces a duplicate function declaration in the post-update hooks file. The changes consolidate layout support around the three-column layout option. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
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 |
joshua-salsadigital
added
the
State: Needs review
joshua-salsadigital
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/themes/contrib/civictheme/civictheme.layouts.yml(0 hunks)web/themes/contrib/civictheme/civictheme.post_update.php(1 hunks)web/themes/contrib/civictheme/templates/layout/layout--single-column-contained.html.twig(0 hunks)web/themes/contrib/civictheme/templates/layout/layout--single-column.html.twig(0 hunks)
💤 Files with no reviewable changes (3)
- web/themes/contrib/civictheme/civictheme.layouts.yml
- web/themes/contrib/civictheme/templates/layout/layout--single-column-contained.html.twig
- web/themes/contrib/civictheme/templates/layout/layout--single-column.html.twig
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T08:55:24.182Z
Learnt from: richardgaunt
Repo: civictheme/monorepo-drupal PR: 1405
File: web/themes/contrib/civictheme/civictheme.post_update.php:873-917
Timestamp: 2025-08-14T08:55:24.182Z
Learning: For civictheme post-update functions, rely on Drupal's built-in error handling rather than adding explicit try-catch blocks around configuration operations, as the update system will naturally break and show errors if operations fail.
Applied to files:
web/themes/contrib/civictheme/civictheme.post_update.php
⏰ 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). (1)
- GitHub Check: commit
Comment on lines
+1097
to
+1165
| /** | ||
| * Migrate deprecated layouts to civictheme_three_columns. | ||
| * | ||
| * @SuppressWarnings(PHPMD.UnusedFormalParameter) | ||
| * @SuppressWarnings(PHPMD.CyclomaticComplexity) | ||
| * @SuppressWarnings(PHPMD.StaticAccess) | ||
| */ | ||
| function civictheme_post_update_migrate_one_column_layouts(): string { | ||
| $outdated_layouts = [ | ||
| 'civictheme_one_column', | ||
| 'civictheme_one_column_contained', | ||
| ]; | ||
|
|
||
| $messages = []; | ||
| $entity_displays = LayoutBuilderEntityViewDisplay::loadMultiple(); | ||
| $updated_entity_displays = []; | ||
| foreach ($entity_displays as $entity_display) { | ||
| if (!$entity_display->isLayoutBuilderEnabled()) { | ||
| continue; | ||
| } | ||
| // Update allowed layouts if the deprecated ones are present. | ||
| $entity_view_mode_restriction = $entity_display->getThirdPartySetting('layout_builder_restrictions', 'entity_view_mode_restriction'); | ||
| if (!empty($entity_view_mode_restriction['allowed_layouts'])) { | ||
| $allowed_layouts = $entity_view_mode_restriction['allowed_layouts']; | ||
| $replaced = FALSE; | ||
| foreach ($allowed_layouts as $idx => $layout_name) { | ||
| if (in_array($layout_name, $outdated_layouts)) { | ||
| unset($allowed_layouts[$idx]); | ||
| $replaced = TRUE; | ||
| } | ||
| } | ||
| if ($replaced) { | ||
| $allowed_layouts[] = 'civictheme_three_columns'; | ||
| $entity_view_mode_restriction['allowed_layouts'] = array_values($allowed_layouts); | ||
| $entity_display->setThirdPartySetting('layout_builder_restrictions', 'entity_view_mode_restriction', $entity_view_mode_restriction); | ||
| $updated_entity_displays[$entity_display->id()] = $entity_display->id(); | ||
| } | ||
| } | ||
| // Replace layouts in layout builder sections. | ||
| $layout_builder_sections = $entity_display->getThirdPartySetting('layout_builder', 'sections'); | ||
| if (!empty($layout_builder_sections)) { | ||
| foreach ($layout_builder_sections as $index => $section) { | ||
| $layout_name = $section->getLayoutId(); | ||
| if (in_array($layout_name, $outdated_layouts)) { | ||
| $section_as_array = $section->toArray(); | ||
| $section_as_array['layout_id'] = 'civictheme_three_columns'; | ||
| $section_as_array['layout_settings']['label'] = 'CivicTheme Three Columns'; | ||
| $section_as_array['layout_settings']['is_contained'] = ($layout_name === 'civictheme_one_column_contained'); | ||
| // Move all components to 'main'. | ||
| foreach ($section_as_array['components'] as &$component) { | ||
| if ($component['region'] === 'content') { | ||
| $component['region'] = 'main'; | ||
| } | ||
| } | ||
| $layout_builder_sections[$index] = Section::fromArray($section_as_array); | ||
| $updated_entity_displays[$entity_display->id()] = $entity_display->id(); | ||
| } | ||
| } | ||
| $entity_display->setThirdPartySetting('layout_builder', 'sections', $layout_builder_sections); | ||
| } | ||
| if (in_array($entity_display->id(), $updated_entity_displays)) { | ||
| $entity_display->save(); | ||
| $messages[] = (string) (new TranslatableMarkup('Updated @display_id display, replaced deprecated CivicTheme single-column layouts with civictheme_three_columns.', [ | ||
| '@display_id' => $entity_display->id(), | ||
| ])); | ||
| } | ||
| } | ||
| return implode("\n", $messages); | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Remove duplicate function declaration.
This function civictheme_post_update_migrate_one_column_layouts() is already declared at lines 1034-1095. PHP does not allow function redeclaration and will throw a fatal error: "Cannot redeclare function". This prevents the entire file from loading.
🔎 Remove the duplicate function declaration
Delete lines 1097-1165 entirely, as the function is already defined earlier in the file.
-
-/**
- * Migrate deprecated layouts to civictheme_three_columns.
- *
- * @SuppressWarnings(PHPMD.UnusedFormalParameter)
- * @SuppressWarnings(PHPMD.CyclomaticComplexity)
- * @SuppressWarnings(PHPMD.StaticAccess)
- */
-function civictheme_post_update_migrate_one_column_layouts(): string {
- $outdated_layouts = [
- 'civictheme_one_column',
- 'civictheme_one_column_contained',
- ];
-
- $messages = [];
- $entity_displays = LayoutBuilderEntityViewDisplay::loadMultiple();
- $updated_entity_displays = [];
- foreach ($entity_displays as $entity_display) {
- if (!$entity_display->isLayoutBuilderEnabled()) {
- continue;
- }
- // Update allowed layouts if the deprecated ones are present.
- $entity_view_mode_restriction = $entity_display->getThirdPartySetting('layout_builder_restrictions', 'entity_view_mode_restriction');
- if (!empty($entity_view_mode_restriction['allowed_layouts'])) {
- $allowed_layouts = $entity_view_mode_restriction['allowed_layouts'];
- $replaced = FALSE;
- foreach ($allowed_layouts as $idx => $layout_name) {
- if (in_array($layout_name, $outdated_layouts)) {
- unset($allowed_layouts[$idx]);
- $replaced = TRUE;
- }
- }
- if ($replaced) {
- $allowed_layouts[] = 'civictheme_three_columns';
- $entity_view_mode_restriction['allowed_layouts'] = array_values($allowed_layouts);
- $entity_display->setThirdPartySetting('layout_builder_restrictions', 'entity_view_mode_restriction', $entity_view_mode_restriction);
- $updated_entity_displays[$entity_display->id()] = $entity_display->id();
- }
- }
- // Replace layouts in layout builder sections.
- $layout_builder_sections = $entity_display->getThirdPartySetting('layout_builder', 'sections');
- if (!empty($layout_builder_sections)) {
- foreach ($layout_builder_sections as $index => $section) {
- $layout_name = $section->getLayoutId();
- if (in_array($layout_name, $outdated_layouts)) {
- $section_as_array = $section->toArray();
- $section_as_array['layout_id'] = 'civictheme_three_columns';
- $section_as_array['layout_settings']['label'] = 'CivicTheme Three Columns';
- $section_as_array['layout_settings']['is_contained'] = ($layout_name === 'civictheme_one_column_contained');
- // Move all components to 'main'.
- foreach ($section_as_array['components'] as &$component) {
- if ($component['region'] === 'content') {
- $component['region'] = 'main';
- }
- }
- $layout_builder_sections[$index] = Section::fromArray($section_as_array);
- $updated_entity_displays[$entity_display->id()] = $entity_display->id();
- }
- }
- $entity_display->setThirdPartySetting('layout_builder', 'sections', $layout_builder_sections);
- }
- if (in_array($entity_display->id(), $updated_entity_displays)) {
- $entity_display->save();
- $messages[] = (string) (new TranslatableMarkup('Updated @display_id display, replaced deprecated CivicTheme single-column layouts with civictheme_three_columns.', [
- '@display_id' => $entity_display->id(),
- ]));
- }
- }
- return implode("\n", $messages);
-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Migrate deprecated layouts to civictheme_three_columns. | |
| * | |
| * @SuppressWarnings(PHPMD.UnusedFormalParameter) | |
| * @SuppressWarnings(PHPMD.CyclomaticComplexity) | |
| * @SuppressWarnings(PHPMD.StaticAccess) | |
| */ | |
| function civictheme_post_update_migrate_one_column_layouts(): string { | |
| $outdated_layouts = [ | |
| 'civictheme_one_column', | |
| 'civictheme_one_column_contained', | |
| ]; | |
| $messages = []; | |
| $entity_displays = LayoutBuilderEntityViewDisplay::loadMultiple(); | |
| $updated_entity_displays = []; | |
| foreach ($entity_displays as $entity_display) { | |
| if (!$entity_display->isLayoutBuilderEnabled()) { | |
| continue; | |
| } | |
| // Update allowed layouts if the deprecated ones are present. | |
| $entity_view_mode_restriction = $entity_display->getThirdPartySetting('layout_builder_restrictions', 'entity_view_mode_restriction'); | |
| if (!empty($entity_view_mode_restriction['allowed_layouts'])) { | |
| $allowed_layouts = $entity_view_mode_restriction['allowed_layouts']; | |
| $replaced = FALSE; | |
| foreach ($allowed_layouts as $idx => $layout_name) { | |
| if (in_array($layout_name, $outdated_layouts)) { | |
| unset($allowed_layouts[$idx]); | |
| $replaced = TRUE; | |
| } | |
| } | |
| if ($replaced) { | |
| $allowed_layouts[] = 'civictheme_three_columns'; | |
| $entity_view_mode_restriction['allowed_layouts'] = array_values($allowed_layouts); | |
| $entity_display->setThirdPartySetting('layout_builder_restrictions', 'entity_view_mode_restriction', $entity_view_mode_restriction); | |
| $updated_entity_displays[$entity_display->id()] = $entity_display->id(); | |
| } | |
| } | |
| // Replace layouts in layout builder sections. | |
| $layout_builder_sections = $entity_display->getThirdPartySetting('layout_builder', 'sections'); | |
| if (!empty($layout_builder_sections)) { | |
| foreach ($layout_builder_sections as $index => $section) { | |
| $layout_name = $section->getLayoutId(); | |
| if (in_array($layout_name, $outdated_layouts)) { | |
| $section_as_array = $section->toArray(); | |
| $section_as_array['layout_id'] = 'civictheme_three_columns'; | |
| $section_as_array['layout_settings']['label'] = 'CivicTheme Three Columns'; | |
| $section_as_array['layout_settings']['is_contained'] = ($layout_name === 'civictheme_one_column_contained'); | |
| // Move all components to 'main'. | |
| foreach ($section_as_array['components'] as &$component) { | |
| if ($component['region'] === 'content') { | |
| $component['region'] = 'main'; | |
| } | |
| } | |
| $layout_builder_sections[$index] = Section::fromArray($section_as_array); | |
| $updated_entity_displays[$entity_display->id()] = $entity_display->id(); | |
| } | |
| } | |
| $entity_display->setThirdPartySetting('layout_builder', 'sections', $layout_builder_sections); | |
| } | |
| if (in_array($entity_display->id(), $updated_entity_displays)) { | |
| $entity_display->save(); | |
| $messages[] = (string) (new TranslatableMarkup('Updated @display_id display, replaced deprecated CivicTheme single-column layouts with civictheme_three_columns.', [ | |
| '@display_id' => $entity_display->id(), | |
| ])); | |
| } | |
| } | |
| return implode("\n", $messages); | |
| } |
🤖 Prompt for AI Agents
In web/themes/contrib/civictheme/civictheme.post_update.php around lines 1097 to
1165 there is a duplicate declaration of
civictheme_post_update_migrate_one_column_layouts() (the same function exists at
lines ~1034-1095) which will cause a PHP fatal "Cannot redeclare" error; remove
the entire duplicate block (lines 1097-1165) so only the original function
definition remains, then run PHP linting to confirm no remaining
duplicate/syntax issues.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 implementationNOTE
In release after this update script we remove the layouts.
#1441
Changed
Screenshots
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.