[GOVCMSCT2-121] CT PORT - Fast fact card #1402
Conversation
…added test to verify the BE fields.
joshua-salsadigital
added
State: DO NOT MERGE
WalkthroughThis PR introduces a new "Fast fact card" paragraph type to the CivicTheme Drupal theme, including field definitions, form and view display configurations, preprocessing logic, rendering template, and automated setup via a post-update hook. Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin/Installer
participant PostUpdate as Post-Update Hook
participant ConfigSync as Configuration Sync
participant ManualList as Manual List Field
participant Preprocess as Preprocessing
participant Template as Twig Template
Admin->>PostUpdate: Run Drupal updates
PostUpdate->>ConfigSync: Create paragraph type config
ConfigSync->>ConfigSync: Create field storage & instances
PostUpdate->>ManualList: Update handler_settings<br/>with civictheme_fast_fact_card
ManualList->>ManualList: Register target bundle<br/>weight: -54
Note over Admin,Template: Rendering Flow
Admin->>ManualList: Add fast fact card<br/>to manual list
ManualList->>Preprocess: Invoke preprocess function
Preprocess->>Preprocess: Process theme, title,<br/>summary, link fields
Preprocess->>Preprocess: Retrieve icon media
Preprocess->>Template: Pass prepared variables
Template->>Template: Render fast-fact-card<br/>component via include
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
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
🔭 Outside diff range comments (1)
tests/behat/features/paragraph.civictheme_infographic_card.fields.feature (1)
26-27: Complete the theme field assertionsThe test scenario appears incomplete. The theme field assertion on line 26 should include checks for its required/disabled status similar to other fields.
Add the missing assertions for the theme field:
And I should see an "[name='field_c_n_components[0][subform][field_c_p_list_items][0][subform][field_c_p_theme]']" element +And I should not see an "[name='field_c_n_components[0][subform][field_c_p_list_items][0][subform][field_c_p_theme]'].required" element +And I should not see an "[name='field_c_n_components[0][subform][field_c_p_list_items][0][subform][field_c_p_theme]'][disabled]" element
🧹 Nitpick comments (1)
web/themes/contrib/civictheme/config/install/paragraphs.paragraphs_type.civictheme_infographic_card.yml (1)
8-8: Consider adding a description for better admin UX.While not required, adding a description would help content editors understand the purpose of the "Fast fact card" paragraph type.
-description: '' +description: 'A card component for displaying key facts or statistics with an icon, title, summary, and optional link.'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
tests/behat/features/paragraph.civictheme_infographic_card.fields.feature(1 hunks)web/themes/contrib/civictheme/civictheme.info.yml(6 hunks)web/themes/contrib/civictheme/config/install/core.entity_form_display.paragraph.civictheme_infographic_card.default.yml(1 hunks)web/themes/contrib/civictheme/config/install/core.entity_view_display.paragraph.civictheme_infographic_card.default.yml(1 hunks)web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_infographic_card.field_c_p_icon.yml(1 hunks)web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_infographic_card.field_c_p_link.yml(1 hunks)web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_infographic_card.field_c_p_summary.yml(1 hunks)web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_infographic_card.field_c_p_theme.yml(1 hunks)web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_infographic_card.field_c_p_title.yml(1 hunks)web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_manual_list.field_c_p_list_items.yml(3 hunks)web/themes/contrib/civictheme/config/install/paragraphs.paragraphs_type.civictheme_infographic_card.yml(1 hunks)web/themes/contrib/civictheme/includes/cards.inc(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). (1)
- GitHub Check: commit
🔇 Additional comments (19)
web/themes/contrib/civictheme/config/install/paragraphs.paragraphs_type.civictheme_infographic_card.yml (1)
1-10: LGTM! Configuration follows Drupal standards.The paragraph type configuration is correctly structured and follows Drupal's configuration standards. All required fields are properly set.
web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_infographic_card.field_c_p_title.yml (1)
1-19: LGTM! Field configuration is properly structured.The title field configuration correctly follows Drupal standards with appropriate dependencies, field type, and settings for a required string field.
web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_infographic_card.field_c_p_theme.yml (1)
1-23: LGTM! Theme field configuration is correctly implemented.The theme field configuration properly uses
list_stringtype with appropriate dependencies, including the requiredoptionsmodule. The default value of "light" provides a sensible fallback.web/themes/contrib/civictheme/includes/cards.inc (1)
205-217: LGTM! Preprocessing function follows established patterns.The preprocessing function correctly reuses existing helper functions for common fields and appropriately handles the icon media field. The implementation is consistent with other card preprocessing functions in the file and follows Drupal standards.
web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_infographic_card.field_c_p_link.yml (1)
1-23: LGTM! Link field configuration is properly set up.The link field configuration correctly uses the
linkfield type with appropriate dependencies. Thelink_type: 17setting allows both internal and external links (1 + 16), which is suitable for a flexible card component. Making the field optional (required: false) provides good UX flexibility.web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_infographic_card.field_c_p_icon.yml (1)
1-29: LGTM! Well-structured field configurationThe icon field configuration follows Drupal best practices with proper dependencies, appropriate entity reference settings, and sensible field properties for an infographic card's icon requirement.
web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_infographic_card.field_c_p_summary.yml (1)
1-19: LGTM! Appropriate summary field configurationThe summary field configuration is well-structured with helpful description text about character limits. The optional nature of the field is appropriate for infographic cards.
tests/behat/features/paragraph.civictheme_infographic_card.fields.feature (1)
1-27: Well-structured Behat test with comprehensive field coverageThe test properly verifies the presence and attributes of all infographic card fields, following good practices for testing form field states and accessibility.
web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_manual_list.field_c_p_list_items.yml (3)
8-8: LGTM! Proper dependency additionThe new civictheme_infographic_card paragraph type is correctly added to the dependencies, ensuring proper configuration loading order.
38-38: LGTM! Target bundle integrationThe infographic card is properly added to the target bundles, enabling its use within manual lists.
81-83: LGTM! Well-positioned drag-drop configurationThe infographic card is appropriately positioned in the UI with weight -54 and enabled for selection. The weight positioning maintains logical ordering with other card types.
web/themes/contrib/civictheme/config/install/core.entity_view_display.paragraph.civictheme_infographic_card.default.yml (3)
19-26: LGTM! Appropriate icon field display configurationThe icon field uses entity_reference_label formatter with linking enabled, which provides good accessibility and user experience for the infographic card's icon.
27-38: LGTM! Well-configured link field displayThe link formatter with 80-character trimming prevents UI issues with long URLs while maintaining usability. The settings are appropriate for link display in cards.
1-62: LGTM! Comprehensive and well-structured view displayThe entity view display configuration provides logical field ordering and appropriate formatters for each field type. The weight distribution and formatter choices create a coherent infographic card layout.
web/themes/contrib/civictheme/config/install/core.entity_form_display.paragraph.civictheme_infographic_card.default.yml (2)
3-12: Declare media-related module dependencies explicitlyThe
field_c_p_iconwidget references media entities, yet only thelinkmodule is declared underdependencies.module.
If this configuration is imported on an installation profile wheremedia/media_libraryhave not been enabled earlier in the install sequence, the import will fail.Add the missing hard module dependencies to make the config self-contained:
module: - link + - media + - media_library
17-57: Widget ordering looks sane – no further actionTitle → Icon → Summary → Link → Theme follows the intended authoring flow, and widget types align with field storage.
No issues spotted here.web/themes/contrib/civictheme/civictheme.info.yml (3)
655-655: Remove test webform from shipped production config
webform.webform.civictheme_test_webform_fieldsappears to be a sandbox/test artefact.
Shipping it with the theme will create an unnecessary Webform on live sites.Delete the reference (and the actual config file) unless it is genuinely required.
157-160: 👍 New form-display config correctly registered
core.entity_form_display.paragraph.civictheme_infographic_card.defaultis now included in the install list – this ensures the paragraph type is editable out of the box.
387-391: Field configs properly added – ensure list items allow the new bundleThe five
field.field.paragraph.civictheme_infographic_card.*configs are present.
Double-check thatfield_c_p_list_itemson the Manual list paragraph hascivictheme_infographic_cardinhandler_settings.target_bundles(config not shown here).
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/behat/features/paragraph.civictheme_fast_fact_card.fields.feature (1)
27-27: Fix incomplete theme field assertion.Line 27 contains an incomplete assertion for the theme field. The line is missing the closing part of the assertion.
Apply this diff to complete the theme field assertion:
- And I should see an "[name='field_c_n_components[0][subform][field_c_p_list_items][0][subform][field_c_p_theme]']" element + And I should see an "[name='field_c_n_components[0][subform][field_c_p_list_items][0][subform][field_c_p_theme]']" element + And I should not see an "[name='field_c_n_components[0][subform][field_c_p_list_items][0][subform][field_c_p_theme]'].required" element + And I should not see an "[name='field_c_n_components[0][subform][field_c_p_list_items][0][subform][field_c_p_theme]'][disabled]" element
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
tests/behat/features/paragraph.civictheme_fast_fact_card.fields.feature(1 hunks)web/themes/contrib/civictheme/civictheme.info.yml(4 hunks)web/themes/contrib/civictheme/civictheme.post_update.php(1 hunks)web/themes/contrib/civictheme/config/install/core.entity_form_display.paragraph.civictheme_fast_fact_card.default.yml(1 hunks)web/themes/contrib/civictheme/config/install/core.entity_view_display.paragraph.civictheme_fast_fact_card.default.yml(1 hunks)web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_fast_fact_card.field_c_p_icon.yml(1 hunks)web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_fast_fact_card.field_c_p_link.yml(1 hunks)web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_fast_fact_card.field_c_p_summary.yml(1 hunks)web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_fast_fact_card.field_c_p_theme.yml(1 hunks)web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_fast_fact_card.field_c_p_title.yml(1 hunks)web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_manual_list.field_c_p_list_items.yml(3 hunks)web/themes/contrib/civictheme/config/install/paragraphs.paragraphs_type.civictheme_fast_fact_card.yml(1 hunks)web/themes/contrib/civictheme/includes/cards.inc(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_fast_fact_card.field_c_p_title.yml
- web/themes/contrib/civictheme/config/install/paragraphs.paragraphs_type.civictheme_fast_fact_card.yml
- web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_fast_fact_card.field_c_p_theme.yml
- web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_fast_fact_card.field_c_p_icon.yml
- web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_fast_fact_card.field_c_p_summary.yml
- web/themes/contrib/civictheme/config/install/core.entity_form_display.paragraph.civictheme_fast_fact_card.default.yml
- web/themes/contrib/civictheme/config/install/core.entity_view_display.paragraph.civictheme_fast_fact_card.default.yml
- web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_fast_fact_card.field_c_p_link.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_manual_list.field_c_p_list_items.yml
- web/themes/contrib/civictheme/includes/cards.inc
- web/themes/contrib/civictheme/civictheme.info.yml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: richardgaunt
PR: civictheme/monorepo-drupal#1340
File: web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_message.field_c_p_content.yml:9-9
Timestamp: 2025-07-09T12:20:48.629Z
Learning: For paragraph field configurations in the civictheme Drupal theme, the configuration entity ID should follow the pattern `paragraph.bundle_name.field_name` rather than `field.field.paragraph.bundle_name.field_name`. This is specific to paragraph field configurations.
web/themes/contrib/civictheme/civictheme.post_update.php (1)
Learnt from: richardgaunt
PR: #1340
File: web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_message.field_c_p_content.yml:9-9
Timestamp: 2025-07-09T12:20:48.629Z
Learning: For paragraph field configurations in the civictheme Drupal theme, the configuration entity ID should follow the pattern paragraph.bundle_name.field_name rather than field.field.paragraph.bundle_name.field_name. This is specific to paragraph field configurations.
tests/behat/features/paragraph.civictheme_fast_fact_card.fields.feature (1)
Learnt from: richardgaunt
PR: #1340
File: web/themes/contrib/civictheme/config/install/field.field.paragraph.civictheme_message.field_c_p_content.yml:9-9
Timestamp: 2025-07-09T12:20:48.629Z
Learning: For paragraph field configurations in the civictheme Drupal theme, the configuration entity ID should follow the pattern paragraph.bundle_name.field_name rather than field.field.paragraph.bundle_name.field_name. This is specific to paragraph field configurations.
🧬 Code Graph Analysis (1)
web/themes/contrib/civictheme/civictheme.post_update.php (1)
tests/behat/bootstrap/FeatureContext.php (1)
paragraphsAddToParentEntityWithFields(188-217)
🪛 PHPMD (2.15.0)
web/themes/contrib/civictheme/civictheme.post_update.php
871-871: Avoid unused parameters such as '$sandbox'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ 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
🔇 Additional comments (4)
tests/behat/features/paragraph.civictheme_fast_fact_card.fields.feature (3)
1-11: Well-structured Behat feature setup.The feature definition, tags, and scenario setup follow proper Behat conventions and correctly establish the test context for the Fast fact card paragraph type.
12-14: Correct title field assertions.The assertions properly verify the title field presence, required status, and enabled state using the correct nested field name pattern.
16-26: Field assertions are correctly structured.The icon, summary, and link field assertions properly verify field presence and attributes using appropriate selectors for their respective field types.
web/themes/contrib/civictheme/civictheme.post_update.php (1)
872-911: Configuration definitions and field updates are well-structured.The configuration array includes all necessary components for a complete paragraph type implementation, and the field update logic properly handles enabling the new paragraph type in the manual list field with appropriate error checking and initialization.
joshua-salsadigital
added
State: Needs review
github-actions
bot
added
State: CONFLICT
and removed
State: Needs review
joshua-salsadigital
added
State: Needs review
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/themes/contrib/civictheme/package.json (1)
42-42: Prefer a release tag or published package over a raw Git commit for stability and supply-chain hygienePinning to a Git commit can cause CI flakiness (GitHub outages/rate limits), weaker auditing, and harder upgrades. If possible, depend on a tagged release (e.g., github:civictheme/uikit#vX.Y.Z) or an npm-published version. If a commit pin is necessary, document the reason and plan a follow-up to migrate to a release.
I can check available tags and propose the closest compatible tag, or open a task to migrate once a release is cut.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonweb/themes/contrib/civictheme/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)web/themes/contrib/civictheme/civictheme.post_update.php(1 hunks)web/themes/contrib/civictheme/package.json(1 hunks)web/themes/contrib/civictheme/templates/paragraphs/paragraph--civictheme-fast-fact-card.html.twig(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- package.json
- web/themes/contrib/civictheme/templates/paragraphs/paragraph--civictheme-fast-fact-card.html.twig
🚧 Files skipped from review as they are similar to previous changes (1)
- 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/behat/features/paragraph.civictheme_subject_card_ref.render.feature (1)
42-42: Subject card selector update LGTM; consider reducing future churn with a shared test step.Optional: Introduce a custom Behat step that parameterizes the card type to avoid repeating and editing CSS selector strings in multiple features when classes change (e.g., map “subject/promo/event/navigation/snippet” to the appropriate
.ct-*-card__title-linkselector in one place).tests/behat/features/paragraph.civictheme_subject_card_ref.fields.feature (1)
2-2: Align the Feature title with the filename to avoid confusion.The file is named “...fields.feature” but the Feature line says “render”. Consider either renaming the file to “...render.feature” or updating the Feature line to “fields” for consistency.
-Feature: subject reference card render +Feature: subject reference card fields
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (6)
tests/behat/features/paragraph.civictheme_event_card_ref.render.feature(1 hunks)tests/behat/features/paragraph.civictheme_navigation_card_ref.render.feature(1 hunks)tests/behat/features/paragraph.civictheme_promo_card_ref.render.feature(1 hunks)tests/behat/features/paragraph.civictheme_snippet_ref.render.feature(1 hunks)tests/behat/features/paragraph.civictheme_subject_card_ref.fields.feature(1 hunks)tests/behat/features/paragraph.civictheme_subject_card_ref.render.feature(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). (1)
- GitHub Check: commit
🔇 Additional comments (5)
tests/behat/features/paragraph.civictheme_event_card_ref.render.feature (1)
62-62: Selector rename verified — legacy selectors absent; please confirm Twig/UI kit markupQuick summary:
- No occurrences of the legacy selector (.ct-*-card__title__link) were found.
- New selector (.ct-*-card__title-link) appears in these test files:
- tests/behat/features/paragraph.civictheme_subject_card_ref.render.feature:42
- tests/behat/features/paragraph.civictheme_event_card_ref.render.feature:62
- tests/behat/features/paragraph.civictheme_subject_card_ref.fields.feature:42
- tests/behat/features/paragraph.civictheme_promo_card_ref.render.feature:49
- tests/behat/features/paragraph.civictheme_navigation_card_ref.render.feature:42
- The Twig templates search returned no matches for title link patterns, so I could not confirm templates/UI kit markup.
Action requested:
- Manually verify the relevant Twig templates / UI kit components to ensure they use .ct-*-card__title-link (if applicable).
tests/behat/features/paragraph.civictheme_navigation_card_ref.render.feature (1)
42-42: Consistent BEM selector update for navigation card.Matches the naming standard used across other card tests in this PR. Assuming templates were updated in tandem, this is good to go.
tests/behat/features/paragraph.civictheme_promo_card_ref.render.feature (1)
49-49: Promo card selector rename is correct and consistent.Test expectation now reflects
.ct-promo-card__title-link. No other issues spotted.tests/behat/features/paragraph.civictheme_snippet_ref.render.feature (1)
39-39: Snippet card selector updated as expected.Aligned with the cross-card selector standardization to
__title-link.tests/behat/features/paragraph.civictheme_subject_card_ref.fields.feature (1)
42-42: Rename verified — old selector removed; new .ct-subject-card__title-link present in testsNo occurrences of .ct-subject-card__title__link were found. The new selector appears in:
- tests/behat/features/paragraph.civictheme_subject_card_ref.fields.feature:42
- tests/behat/features/paragraph.civictheme_subject_card_ref.render.feature:42
I did not find template/SCSS references for the selector in this search — please confirm Twig/SCSS were updated where needed.
|
Updated failing test |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/behat/features/media.civictheme_remote_video.render.feature (1)
32-32: Reduce brittleness by targeting the link by text rather than classClasses tend to change with design refactors. Clicking by link text (“View transcript”) makes this step more resilient and decoupled from DOM structure changes, provided the text remains stable.
Apply this diff if your step definitions support following links by text (MinkContext usually does):
- And I click on ".ct-video-player__links-transcript a" element + And I follow "View transcript"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/behat/features/media.civictheme_remote_video.render.feature(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). (1)
- GitHub Check: commit
🔇 Additional comments (2)
tests/behat/features/media.civictheme_remote_video.render.feature (2)
32-32: Selector update aligns with UI Kit DOM changeThe new selector ".ct-video-player__links-transcript a" matches the refactor from "__links__transcript .ct-button" per the referenced UI Kit change. This should restore the click step reliability.
32-32: No stale transcript selectors found — ready to resolveSearch results:
- Old selector
.ct-video-player__links__transcript: 0 matches.- Old selector with
.ct-button: 0 matches.- New selector
.ct-video-player__links-transcriptfound at:
- tests/behat/features/media.civictheme_remote_video.render.feature:32 — And I click on ".ct-video-player__links-transcript a" element
github-actions
bot
added
State: CONFLICT
and removed
State: Needs review
joshua-salsadigital
force-pushed
the
feature/GOVCMSCT2-121
branch
from
6fc368c to
c8d8a55
Compare
- Add quant.type labels to docker-compose services - Add GitHub Actions build-deploy workflow - Update Drupal settings for Quant Cloud environment variables - Make Elasticsearch compatible with ECS Fargate - Convert post-rollout tasks to entrypoint scripts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (16)
.dockerignore (1)
32-32: Make reinclusion explicit for nested files.Docker’s ignore semantics can be surprising. Being explicit avoids context-related misses.
-!.docker/entrypoints +!.docker/entrypoints +!.docker/entrypoints/**.docker/entrypoints/cli/04-send-deployment-notifications.sh (1)
7-11: Quote vars and use an absolute path for notify.sh.Prevents word-splitting and CWD issues during entrypoint execution.
-if [ -n "$LAGOON_PR_NUMBER" ]; then export DREVOPS_NOTIFY_REF=$LAGOON_PR_NUMBER; export DREVOPS_NOTIFY_SHA=${LAGOON_PR_HEAD_SHA#origin/}; export DREVOPS_NOTIFY_BRANCH=$LAGOON_PR_HEAD_BRANCH; else export DREVOPS_NOTIFY_REF=$LAGOON_GIT_BRANCH; export DREVOPS_NOTIFY_SHA=$LAGOON_GIT_SHA; export DREVOPS_NOTIFY_BRANCH=$LAGOON_GIT_BRANCH; fi -DREVOPS_NOTIFY_EVENT=post_deployment \ -DREVOPS_NOTIFY_PROJECT=$LAGOON_PROJECT \ -DREVOPS_NOTIFY_ENVIRONMENT_URL=$LAGOON_ROUTE \ -./scripts/drevops/notify.sh +if [ -n "${LAGOON_PR_NUMBER:-}" ]; then + export DREVOPS_NOTIFY_REF="$LAGOON_PR_NUMBER" + export DREVOPS_NOTIFY_SHA="${LAGOON_PR_HEAD_SHA#origin/}" + export DREVOPS_NOTIFY_BRANCH="$LAGOON_PR_HEAD_BRANCH" +else + export DREVOPS_NOTIFY_REF="$LAGOON_GIT_BRANCH" + export DREVOPS_NOTIFY_SHA="$LAGOON_GIT_SHA" + export DREVOPS_NOTIFY_BRANCH="$LAGOON_GIT_BRANCH" +fi +DREVOPS_NOTIFY_EVENT=post_deployment \ +DREVOPS_NOTIFY_PROJECT="${LAGOON_PROJECT:-}" \ +DREVOPS_NOTIFY_ENVIRONMENT_URL="${LAGOON_ROUTE:-}" \ +/app/scripts/drevops/notify.shweb/sites/default/settings.php (1)
101-101: Pattern excludes the apex host.'^.+.apps.quantgovsites.com$' matches foo.apps.quantgovsites.com but not apps.quantgovsites.com. Include both if intended.
-$settings['trusted_host_patterns'][] = '^.+\.apps\.quantgovsites\.com$'; +$settings['trusted_host_patterns'][] = '^(apps\.quantgovsites\.com|.+\.apps\.quantgovsites\.com)$';.docker/entrypoints/php/00-fix-permissions.sh (1)
23-25: ShellCheck SC2043: single-item loop.Either drop the loop or make it a list for future paths; suppressing the warning is fine too.
-for path in "/app/web/sites/default/files/"; do +for path in /app/web/sites/default/files/; doOr:
-# Persistent volume paths from docker-compose lagoon.persistent labels -for path in "/app/web/sites/default/files/"; do +# Persistent volume paths from docker-compose lagoon.persistent labels +# shellcheck disable=SC2043 +for path in "/app/web/sites/default/files/"; do.docker/entrypoints/nginx/00-fix-permissions.sh (1)
23-25: SC2043: single-item loop.Tidy the loop or suppress as per rationale.
-for path in "/app/web/sites/default/files/"; do +for path in /app/web/sites/default/files/; doOr add:
# shellcheck disable=SC2043.docker/entrypoints/cli/00-fix-permissions.sh (1)
23-25: SC2043: single-item loop.Same nit as other entrypoints.
-for path in "/app/web/sites/default/files/"; do +for path in /app/web/sites/default/files/; doOr suppress with shellcheck directive.
.docker/entrypoints/cli/02-notify-about-pre-deployment.sh (4)
2-2: Harden script options: treat unset vars as errors.Use set -eu to fail fast on unset env vars (several LAGOON_* vars are optional).
-set -e +set -eu
4-4: Nit: Header says "Post-rollout" but this is a pre-deployment hook.Align the comment to avoid confusion.
-# Post-rollout task: Notify about pre-deployment. +# Pre-deployment task: Notify about upcoming deployment.
7-7: Improve readability, quoting, and safety of env detection.Expand the one-liner into a readable, quoted block; default empty when vars are unset.
-if [ -n "$LAGOON_PR_NUMBER" ]; then export DREVOPS_NOTIFY_REF=$LAGOON_PR_NUMBER;export DREVOPS_NOTIFY_SHA=${LAGOON_PR_HEAD_SHA#origin/};export DREVOPS_NOTIFY_BRANCH=$LAGOON_PR_HEAD_BRANCH;else export DREVOPS_NOTIFY_REF=$LAGOON_GIT_BRANCH;export DREVOPS_NOTIFY_SHA=$LAGOON_GIT_SHA;export DREVOPS_NOTIFY_BRANCH=$LAGOON_GIT_BRANCH;fi +if [ -n "${LAGOON_PR_NUMBER:-}" ]; then + export DREVOPS_NOTIFY_REF="${LAGOON_PR_NUMBER}" + export DREVOPS_NOTIFY_SHA="${LAGOON_PR_HEAD_SHA#origin/}" + export DREVOPS_NOTIFY_BRANCH="${LAGOON_PR_HEAD_BRANCH}" +else + export DREVOPS_NOTIFY_REF="${LAGOON_GIT_BRANCH:-}" + export DREVOPS_NOTIFY_SHA="${LAGOON_GIT_SHA:-}" + export DREVOPS_NOTIFY_BRANCH="${LAGOON_GIT_BRANCH:-}" +fi
8-10: Quote expansions when passing env to notify.sh.Prevents accidental word splitting if routes/project names have spaces.
-DREVOPS_NOTIFY_PROJECT=$LAGOON_PROJECT \ -DREVOPS_NOTIFY_ENVIRONMENT_URL=$LAGOON_ROUTE \ +DREVOPS_NOTIFY_PROJECT="${LAGOON_PROJECT:-}" \ +DREVOPS_NOTIFY_ENVIRONMENT_URL="${LAGOON_ROUTE:-}" \ DREVOPS_NOTIFY_EVENT=pre_deployment ./scripts/drevops/notify.sh || truePlease confirm ./scripts/drevops/notify.sh exists in the image and has +x. If not, add a chmod in the Dockerfile that copies it.
.docker/cli.dockerfile (1)
113-113: Copy the directory, not a glob, and ensure executability.Using a glob can fail if no match or miss hidden files. Copy the dir and (optionally) enforce +x.
-COPY .docker/entrypoints/cli/* /quant-entrypoint.d/ +COPY .docker/entrypoints/cli/ /quant-entrypoint.d/Optionally, ensure scripts are executable right after copy (outside this line range):
RUN chmod +x /quant-entrypoint.d/*.sh || true.github/workflows/build-deploy.yml (5)
1-18: Restrict GITHUB_TOKEN permissions (CodeQL hint).Add minimal permissions to follow GitHub hardening best practices.
name: Build and Push civictheme-monorepo-drupal to Quant Cloud 'on': @@ concurrency: group: build-and-push-${{ github.ref }} cancel-in-progress: true + +permissions: + contents: readOptionally, if pushing to GHCR in future, add packages: write; if using OIDC, add id-token: write.
39-61: Fix shell quoting and reduce multiple redirects in outputs step.Quotes $GITHUB_OUTPUT and batches writes; silences shellcheck SC2129/SC2086.
- - name: Override outputs for quant-cloud-migration branch + - name: Override outputs for quant-cloud-migration branch id: override-outputs run: |- - # Override outputs for quant-cloud-migration branch to treat it as production - if [[ "${{ github.ref }}" == "refs/heads/quant-cloud-migration" ]]; then - echo "image_suffix=-latest" >> $GITHUB_OUTPUT - echo "image_suffix_clean=latest" >> $GITHUB_OUTPUT - echo "is_production=true" >> $GITHUB_OUTPUT - echo "environment_name=production" >> $GITHUB_OUTPUT - echo "environment_exists=true" >> $GITHUB_OUTPUT - echo "Overriding outputs for quant-cloud-migration branch: using -latest suffix and production environment" - else - # Use the original action outputs - echo "image_suffix=${{ steps.init.outputs.image_suffix }}" >> $GITHUB_OUTPUT - # Remove leading hyphen from image_suffix for image_suffix parameter - suffix="${{ steps.init.outputs.image_suffix }}" - clean_suffix="${suffix#-}" - echo "image_suffix_clean=$clean_suffix" >> $GITHUB_OUTPUT - echo "is_production=${{ steps.init.outputs.is_production }}" >> $GITHUB_OUTPUT - echo "environment_name=${{ steps.init.outputs.environment_name }}" >> $GITHUB_OUTPUT - echo "environment_exists=${{ steps.init.outputs.environment_exists }}" >> $GITHUB_OUTPUT - fi + # Override outputs for quant-cloud-migration branch to treat it as production + if [[ "${{ github.ref }}" == "refs/heads/quant-cloud-migration" ]]; then + { + echo "image_suffix=-latest" + echo "image_suffix_clean=latest" + echo "is_production=true" + echo "environment_name=production" + echo "environment_exists=true" + } >> "$GITHUB_OUTPUT" + echo "Overriding outputs for quant-cloud-migration branch: using -latest suffix and production environment" + else + # Use the original action outputs + suffix="${{ steps.init.outputs.image_suffix }}" + clean_suffix="${suffix#-}" + { + echo "image_suffix=${{ steps.init.outputs.image_suffix }}" + echo "image_suffix_clean=${clean_suffix}" + echo "is_production=${{ steps.init.outputs.is_production }}" + echo "environment_name=${{ steps.init.outputs.environment_name }}" + echo "environment_exists=${{ steps.init.outputs.environment_exists }}" + } >> "$GITHUB_OUTPUT" + fiIf the runner shell isn’t guaranteed to be bash, set defaults.run.shell: bash at the workflow top.
110-120: Gate environment creation on non-PR events (consumes secrets).Aligns with above conditions.
- - name: Create environment if it doesn't exist - if: ${{ !startsWith(github.ref, 'refs/tags/') && steps.override-outputs.outputs.environment_exists == 'false' }} + - name: Create environment if it doesn't exist + if: ${{ github.event_name != 'pull_request' && !startsWith(github.ref, 'refs/tags/') && steps.override-outputs.outputs.environment_exists == 'false' }}
121-135: Gate database sync on non-PR events.- - name: Sync database from production to new environment - if: ${{ !startsWith(github.ref, 'refs/tags/') && steps.override-outputs.outputs.environment_exists == 'false' && steps.override-outputs.outputs.environment_name - != 'production' }} + - name: Sync database from production to new environment + if: ${{ github.event_name != 'pull_request' && !startsWith(github.ref, 'refs/tags/') && steps.override-outputs.outputs.environment_exists == 'false' && steps.override-outputs.outputs.environment_name != 'production' }}
136-145: Gate redeploy on non-PR events.- - name: Redeploy existing environment - if: ${{ !startsWith(github.ref, 'refs/tags/') && steps.override-outputs.outputs.environment_exists == 'true' }} + - name: Redeploy existing environment + if: ${{ github.event_name != 'pull_request' && !startsWith(github.ref, 'refs/tags/') && steps.override-outputs.outputs.environment_exists == 'true' }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.docker/cli.dockerfile(1 hunks).docker/entrypoints/cli/00-fix-permissions.sh(1 hunks).docker/entrypoints/cli/01-show-drevops-variables.sh(1 hunks).docker/entrypoints/cli/02-notify-about-pre-deployment.sh(1 hunks).docker/entrypoints/cli/03-provision-site.sh(1 hunks).docker/entrypoints/cli/04-send-deployment-notifications.sh(1 hunks).docker/entrypoints/nginx/00-fix-permissions.sh(1 hunks).docker/entrypoints/php/00-fix-permissions.sh(1 hunks).docker/php.dockerfile(1 hunks).dockerignore(1 hunks).github/workflows/build-deploy.yml(1 hunks)docker-compose.yml(3 hunks)web/sites/default/settings.php(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .docker/entrypoints/cli/03-provision-site.sh
🧰 Additional context used
🪛 Shellcheck (0.10.0)
.docker/entrypoints/nginx/00-fix-permissions.sh
[warning] 24-24: This loop will only ever run once. Bad quoting or missing glob/expansion?
(SC2043)
.docker/entrypoints/php/00-fix-permissions.sh
[warning] 24-24: This loop will only ever run once. Bad quoting or missing glob/expansion?
(SC2043)
.docker/entrypoints/cli/00-fix-permissions.sh
[warning] 24-24: This loop will only ever run once. Bad quoting or missing glob/expansion?
(SC2043)
🪛 GitHub Check: CodeQL
.github/workflows/build-deploy.yml
[warning] 21-144: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 actionlint (1.7.7)
.github/workflows/build-deploy.yml
21-21: label "sh-runner-1-arm64" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
41-41: shellcheck reported issue in this script: SC2129:style:3:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
41-41: shellcheck reported issue in this script: SC2086:info:3:34: Double quote to prevent globbing and word splitting
(shellcheck)
41-41: shellcheck reported issue in this script: SC2086:info:4:39: Double quote to prevent globbing and word splitting
(shellcheck)
41-41: shellcheck reported issue in this script: SC2086:info:5:32: Double quote to prevent globbing and word splitting
(shellcheck)
41-41: shellcheck reported issue in this script: SC2086:info:6:41: Double quote to prevent globbing and word splitting
(shellcheck)
41-41: shellcheck reported issue in this script: SC2086:info:7:37: Double quote to prevent globbing and word splitting
(shellcheck)
41-41: shellcheck reported issue in this script: SC2086:info:11:65: Double quote to prevent globbing and word splitting
(shellcheck)
41-41: shellcheck reported issue in this script: SC2129:style:15:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
41-41: shellcheck reported issue in this script: SC2086:info:15:46: Double quote to prevent globbing and word splitting
(shellcheck)
41-41: shellcheck reported issue in this script: SC2086:info:16:67: Double quote to prevent globbing and word splitting
(shellcheck)
41-41: shellcheck reported issue in this script: SC2086:info:17:73: Double quote to prevent globbing and word splitting
(shellcheck)
41-41: shellcheck reported issue in this script: SC2086:info:18:77: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ 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
🔇 Additional comments (5)
.docker/php.dockerfile (1)
18-18: Ensure entrypoint scripts are executable in final image.If repo exec bits aren't preserved by the build context, scripts in /quant-entrypoint.d may not run.
Apply:
COPY --from=cli /app /app -COPY .docker/entrypoints/php/* /quant-entrypoint.d/ +COPY .docker/entrypoints/php/* /quant-entrypoint.d/ +RUN chmod +x /quant-entrypoint.d/* || trueweb/sites/default/settings.php (1)
118-122: Consider broader env fallbacks; verify variable names.Many stacks use MYSQL_USER (not MYSQL_USERNAME). Add it as a fallback; same for DB_USER.
- 'username' => getenv('MYSQL_USERNAME') ?: getenv('MARIADB_USERNAME') ?: getenv('DB_USERNAME') ?: 'drupal', + 'username' => getenv('MYSQL_USERNAME') ?: getenv('MYSQL_USER') ?: getenv('MARIADB_USERNAME') ?: getenv('DB_USERNAME') ?: getenv('DB_USER') ?: 'drupal',docker-compose.yml (3)
146-146: LGTM: Label aligns mariadb with Quant metadata.Confirm Quant expects value "mysql" (not "mariadb") for this label in your deployment tooling.
159-159: LGTM: Non-deployable service correctly marked.
170-170: LGTM: Helper service excluded from deployment.
| echo "⚠️ VOLUME_USER not set: Using world-writable permissions for compatibility" | ||
| echo " (Consider setting VOLUME_USER and remapping container users to UID/GID 1000)" | ||
| PERMISSION_MODE="compatible" | ||
| fi |
There was a problem hiding this comment.
Gate world-writable fallback.
Avoid enabling 777/666 unless explicitly approved for the environment.
- PERMISSION_MODE="compatible"
+ PERMISSION_MODE="compatible"
+ if [ "${ALLOW_WORLD_WRITABLE:-0}" != "1" ]; then
+ echo " Skipping world-writable fallback (set ALLOW_WORLD_WRITABLE=1 to enable)."
+ exit 0
+ fi📝 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.
| echo "⚠️ VOLUME_USER not set: Using world-writable permissions for compatibility" | |
| echo " (Consider setting VOLUME_USER and remapping container users to UID/GID 1000)" | |
| PERMISSION_MODE="compatible" | |
| fi | |
| echo "⚠️ VOLUME_USER not set: Using world-writable permissions for compatibility" | |
| echo " (Consider setting VOLUME_USER and remapping container users to UID/GID 1000)" | |
| PERMISSION_MODE="compatible" | |
| if [ "${ALLOW_WORLD_WRITABLE:-0}" != "1" ]; then | |
| echo " Skipping world-writable fallback (set ALLOW_WORLD_WRITABLE=1 to enable)." | |
| exit 0 | |
| fi | |
| fi |
🤖 Prompt for AI Agents
.docker/entrypoints/cli/00-fix-permissions.sh around lines 18-21: the script
currently falls back to world-writable permissions unconditionally; change it to
require an explicit opt-in (e.g., ALLOW_WORLD_WRITABLE=true) before setting
PERMISSION_MODE="compatible", otherwise set a safer default (e.g., "restricted"
or specific 0755/0644) and emit a clear warning or fail fast; implement a simple
conditional that checks the opt-in environment variable, documents the behavior
in the log messages, and avoid applying 0777/0666 unless the opt-in flag is
present.
| # Post-rollout task: Show DrevOps variables. | ||
| # Generated from .lagoon.yml | ||
|
|
||
| env -0 | sort -z | tr '\0' '\n' | grep ^DREVOPS_ || true |
There was a problem hiding this comment.
Avoid GNU-only flags; add portable fallback.
env -0 and sort -z aren’t available in BusyBox/Alpine by default; with set -e this may silently noop or fail.
-env -0 | sort -z | tr '\0' '\n' | grep ^DREVOPS_ || true
+if env -0 >/dev/null 2>&1; then
+ env -0 | tr '\0' '\n' | sort | grep '^DREVOPS_' || true
+else
+ printenv | sort | grep '^DREVOPS_' || true
+fi📝 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.
| env -0 | sort -z | tr '\0' '\n' | grep ^DREVOPS_ || true | |
| if env -0 >/dev/null 2>&1; then | |
| env -0 | tr '\0' '\n' | sort | grep '^DREVOPS_' || true | |
| else | |
| printenv | sort | grep '^DREVOPS_' || true | |
| fi |
🤖 Prompt for AI Agents
In .docker/entrypoints/cli/01-show-drevops-variables.sh around line 7, the
pipeline uses GNU-only flags env -0 and sort -z which are missing on
BusyBox/Alpine; replace it with a portable implementation that first attempts
the null-delimited approach if available and otherwise falls back to a
POSIX-safe pipeline. Concretely, detect/support env -0 and sort -z (e.g., test
sort --version or attempt a harmless invocation) and use them when present;
otherwise use env | sort | grep '^DREVOPS_' || true so the script works on
minimal shells and still honors set -e by ensuring the final command never
fails.
| echo "⚠️ VOLUME_USER not set: Using world-writable permissions for compatibility" | ||
| echo " (Consider setting VOLUME_USER and remapping container users to UID/GID 1000)" | ||
| PERMISSION_MODE="compatible" | ||
| fi |
There was a problem hiding this comment.
Gate world-writable fallback.
Same security concern as PHP entrypoint; require explicit ALLOW_WORLD_WRITABLE=1.
- PERMISSION_MODE="compatible"
+ PERMISSION_MODE="compatible"
+ if [ "${ALLOW_WORLD_WRITABLE:-0}" != "1" ]; then
+ echo " Skipping world-writable fallback (set ALLOW_WORLD_WRITABLE=1 to enable)."
+ exit 0
+ fi📝 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.
| echo "⚠️ VOLUME_USER not set: Using world-writable permissions for compatibility" | |
| echo " (Consider setting VOLUME_USER and remapping container users to UID/GID 1000)" | |
| PERMISSION_MODE="compatible" | |
| fi | |
| echo "⚠️ VOLUME_USER not set: Using world-writable permissions for compatibility" | |
| echo " (Consider setting VOLUME_USER and remapping container users to UID/GID 1000)" | |
| PERMISSION_MODE="compatible" | |
| if [ "${ALLOW_WORLD_WRITABLE:-0}" != "1" ]; then | |
| echo " Skipping world-writable fallback (set ALLOW_WORLD_WRITABLE=1 to enable)." | |
| exit 0 | |
| fi | |
| fi |
🤖 Prompt for AI Agents
In .docker/entrypoints/nginx/00-fix-permissions.sh around lines 18 to 21, the
script currently falls back to setting world-writable permissions when
VOLUME_USER is not set; replace this unconditional fallback with an explicit
gate that requires ALLOW_WORLD_WRITABLE=1 to proceed. If ALLOW_WORLD_WRITABLE is
not set or not equal to "1", emit a clear error message and exit (or set a safer
default permission mode), and only set PERMISSION_MODE="compatible" when
ALLOW_WORLD_WRITABLE=1 is present; keep the existing informational echoes but
include the requirement to opt-in via ALLOW_WORLD_WRITABLE.
| echo "⚠️ VOLUME_USER not set: Using world-writable permissions for compatibility" | ||
| echo " (Consider setting VOLUME_USER and remapping container users to UID/GID 1000)" | ||
| PERMISSION_MODE="compatible" | ||
| fi |
There was a problem hiding this comment.
World-writable fallback needs an explicit opt-in.
777/666 on web writable dirs is risky in prod. Gate with an env flag.
- PERMISSION_MODE="compatible"
+ PERMISSION_MODE="compatible"
+ if [ "${ALLOW_WORLD_WRITABLE:-0}" != "1" ]; then
+ echo " Skipping world-writable fallback (set ALLOW_WORLD_WRITABLE=1 to enable)."
+ exit 0
+ fi📝 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.
| echo "⚠️ VOLUME_USER not set: Using world-writable permissions for compatibility" | |
| echo " (Consider setting VOLUME_USER and remapping container users to UID/GID 1000)" | |
| PERMISSION_MODE="compatible" | |
| fi | |
| echo "⚠️ VOLUME_USER not set: Using world-writable permissions for compatibility" | |
| echo " (Consider setting VOLUME_USER and remapping container users to UID/GID 1000)" | |
| PERMISSION_MODE="compatible" | |
| if [ "${ALLOW_WORLD_WRITABLE:-0}" != "1" ]; then | |
| echo " Skipping world-writable fallback (set ALLOW_WORLD_WRITABLE=1 to enable)." | |
| exit 0 | |
| fi | |
| fi |
🤖 Prompt for AI Agents
In .docker/entrypoints/php/00-fix-permissions.sh around lines 18 to 21, the
script currently falls back to world-writable permissions (777/666) when
VOLUME_USER is unset; change this so the world-writable fallback is only applied
when an explicit opt-in environment variable is set (e.g.,
VOLUME_ALLOW_WORLD_WRITABLE=true). Update the logic to: if VOLUME_USER unset
then check the opt-in flag — if opt-in is true set PERMISSION_MODE="compatible"
and print a clear warning that this is unsafe for production, otherwise set a
safer default (restrictive perms) and print an instruction message about setting
VOLUME_USER or enabling the opt-in flag; ensure the opt-in flag is documented in
the warning and exit logs remain informative.
| - name: Initialize Quant Cloud | ||
| uses: quantcdn/[email protected] | ||
| id: init | ||
| with: | ||
| quant_organization: ${{ secrets.QUANT_ORGANIZATION }} | ||
| quant_api_key: ${{ secrets.QUANT_API_KEY }} | ||
| quant_application: civictheme-monorepo-drupal | ||
| master_branch_override: main | ||
|
|
There was a problem hiding this comment.
Skip Quant init on pull_request to avoid secret access failures.
This step uses secrets; forks won’t have them.
- - name: Initialize Quant Cloud
+ - name: Initialize Quant Cloud
+ if: ${{ github.event_name != 'pull_request' }}
uses: quantcdn/[email protected]🤖 Prompt for AI Agents
.github/workflows/build-deploy.yml around lines 30-38: the Quant Cloud init step
runs for pull_request events and fails on forked PRs due to missing secrets; add
a job step-level conditional so the step only runs when the workflow is not
triggered by a pull_request (e.g., add if: github.event_name != 'pull_request'),
keeping the existing inputs intact so secrets are only referenced on trusted
events.
| - name: Build and push cli image | ||
| uses: docker/build-push-action@v5 | ||
| with: | ||
| context: . | ||
| file: ./.docker/cli.dockerfile | ||
| platforms: linux/arm64 | ||
| push: true | ||
| tags: ${{ steps.init.outputs.stripped_endpoint }}/${{ secrets.QUANT_ORGANIZATION }}/${{ steps.init.outputs.quant_application | ||
| }}:cli${{ steps.override-outputs.outputs.image_suffix }} | ||
| cache-from: |- | ||
| type=gha | ||
| type=registry,ref=${{ steps.init.outputs.stripped_endpoint }}/${{ secrets.QUANT_ORGANIZATION }}/${{ steps.init.outputs.quant_application }}:cli-cache | ||
| cache-to: type=gha,mode=max | ||
| build-args: CLI_IMAGE=${{ steps.init.outputs.stripped_endpoint }}/${{ secrets.QUANT_ORGANIZATION }}/${{ steps.init.outputs.quant_application | ||
| }}:cli${{ steps.override-outputs.outputs.image_suffix }} | ||
|
|
There was a problem hiding this comment.
Avoid pushing images on pull_request events.
Prevents failing builds from forks and unnecessary registry pollution.
- - name: Build and push cli image
+ - name: Build and push cli image
+ if: ${{ github.event_name != 'pull_request' }}
@@
- - name: Build and push nginx image
+ - name: Build and push nginx image
+ if: ${{ github.event_name != 'pull_request' }}
@@
- - name: Build and push php image
+ - name: Build and push php image
+ if: ${{ github.event_name != 'pull_request' }}Also applies to: 78-93, 94-109
🤖 Prompt for AI Agents
.github/workflows/build-deploy.yml lines 62-77 (and similarly 78-93, 94-109):
the workflow currently pushes built container images during pull_request events
which can fail for forked PRs and pollute registries; update the job so it does
not push on pull_request events by gating the push-related inputs (push, tags,
cache-from, cache-to, and any registry credentials) behind a conditional that
checks the event is not a pull_request (e.g., if: github.event_name !=
'pull_request') or split build and push into separate jobs where the push job
only runs for non-pull_request events, and ensure tags/build-args referencing
secrets are only used in the push job to avoid exposing secrets in PRs from
forks.
|
|
||
| // Base salt on the DB host name. | ||
| $settings['hash_salt'] = hash('sha256', getenv('MARIADB_HOST') ?: 'localhost'); | ||
| $settings['hash_salt'] = hash('sha256', getenv('MARIADB_HOST') ?: getenv('DB_HOST') ?: 'localhost'); |
There was a problem hiding this comment.
Use a dedicated HASH_SALT env var; host-derived salt weakens guarantees.
Deriving hash_salt from a predictable host reduces entropy. Prefer a dedicated secret.
-$settings['hash_salt'] = hash('sha256', getenv('MARIADB_HOST') ?: getenv('DB_HOST') ?: 'localhost');
+$settings['hash_salt'] = getenv('DRUPAL_HASH_SALT')
+ ?: hash('sha256', getenv('MARIADB_HOST') ?: getenv('DB_HOST') ?: 'localhost');📝 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.
| $settings['hash_salt'] = hash('sha256', getenv('MARIADB_HOST') ?: getenv('DB_HOST') ?: 'localhost'); | |
| $settings['hash_salt'] = getenv('DRUPAL_HASH_SALT') | |
| ?: hash('sha256', getenv('MARIADB_HOST') ?: getenv('DB_HOST') ?: 'localhost'); |
🤖 Prompt for AI Agents
In web/sites/default/settings.php at line 72, the current hash_salt is being
derived from the DB host which is predictable and weak; change it to read a
dedicated secret environment variable (e.g. HASH_SALT) and use that as the
primary value. If the env var is missing, fail fast or generate a
cryptographically secure random salt once and persist it (do not fall back to
host-based values); ensure the salt is long enough (e.g. 32+ bytes hex/base64)
and loaded securely so the application always uses a high-entropy,
non-predictable HASH_SALT.
- Add quant.type labels to docker-compose services - Add GitHub Actions build-deploy workflow - Update Drupal settings for Quant Cloud environment variables - Make Elasticsearch compatible with ECS Fargate - Convert post-rollout tasks to entrypoint scripts - Add nginx configuration for CDN header handling - Add Quant Cloud scripts (provision and database backup)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/build-deploy.yml (2)
41-62: Quote shell expansions when writing GitHub outputs.ShellCheck is right: the current script can word-split or glob if any of the action outputs contain spaces or wildcard characters. Quote the variables and batch the redirects.
- if [[ "${{ github.ref }}" == "refs/heads/quant-cloud-migration" ]]; then - echo "image_suffix=-latest" >> $GITHUB_OUTPUT - echo "image_suffix_clean=latest" >> $GITHUB_OUTPUT - echo "is_production=true" >> $GITHUB_OUTPUT - echo "environment_name=production" >> $GITHUB_OUTPUT - echo "environment_exists=true" >> $GITHUB_OUTPUT + if [[ "${{ github.ref }}" == "refs/heads/quant-cloud-migration" ]]; then + { + echo "image_suffix=-latest" + echo "image_suffix_clean=latest" + echo "is_production=true" + echo "environment_name=production" + echo "environment_exists=true" + } >> "$GITHUB_OUTPUT" @@ - echo "image_suffix=${{ steps.init.outputs.image_suffix }}" >> $GITHUB_OUTPUT + echo "image_suffix=${{ steps.init.outputs.image_suffix }}" >> "$GITHUB_OUTPUT" @@ - echo "image_suffix_clean=$clean_suffix" >> $GITHUB_OUTPUT - echo "is_production=${{ steps.init.outputs.is_production }}" >> $GITHUB_OUTPUT - echo "environment_name=${{ steps.init.outputs.environment_name }}" >> $GITHUB_OUTPUT - echo "environment_exists=${{ steps.init.outputs.environment_exists }}" >> $GITHUB_OUTPUT + { + echo "image_suffix_clean=$clean_suffix" + echo "is_production=${{ steps.init.outputs.is_production }}" + echo "environment_name=${{ steps.init.outputs.environment_name }}" + echo "environment_exists=${{ steps.init.outputs.environment_exists }}" + } >> "$GITHUB_OUTPUT"
21-24: Set explicitpermissionsfor the workflow.Without a permissions block, the default
GITHUB_TOKENscopes remain overly broad. Lock it down (e.g.,contents: read) to match least-privilege guidance and satisfy the CodeQL alert.jobs: build-and-push: + permissions: + contents: read runs-on: ubuntu-latest
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.docker/config/nginx/location_drupal_prepend_host.conf(1 hunks).docker/entrypoints/cli/03-provision-site.sh(1 hunks).docker/nginx-drupal.dockerfile(1 hunks).github/workflows/build-deploy.yml(1 hunks)scripts/quant/download-db.sh(1 hunks)scripts/quant/provision-quant.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
scripts/quant/provision-quant.sh
[warning] 12-12: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
🪛 GitHub Check: CodeQL
.github/workflows/build-deploy.yml
[warning] 23-146: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 actionlint (1.7.7)
.github/workflows/build-deploy.yml
43-43: shellcheck reported issue in this script: SC2129:style:3:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
43-43: shellcheck reported issue in this script: SC2086:info:3:34: Double quote to prevent globbing and word splitting
(shellcheck)
43-43: shellcheck reported issue in this script: SC2086:info:4:39: Double quote to prevent globbing and word splitting
(shellcheck)
43-43: shellcheck reported issue in this script: SC2086:info:5:32: Double quote to prevent globbing and word splitting
(shellcheck)
43-43: shellcheck reported issue in this script: SC2086:info:6:41: Double quote to prevent globbing and word splitting
(shellcheck)
43-43: shellcheck reported issue in this script: SC2086:info:7:37: Double quote to prevent globbing and word splitting
(shellcheck)
43-43: shellcheck reported issue in this script: SC2086:info:11:65: Double quote to prevent globbing and word splitting
(shellcheck)
43-43: shellcheck reported issue in this script: SC2129:style:15:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
43-43: shellcheck reported issue in this script: SC2086:info:15:46: Double quote to prevent globbing and word splitting
(shellcheck)
43-43: shellcheck reported issue in this script: SC2086:info:16:67: Double quote to prevent globbing and word splitting
(shellcheck)
43-43: shellcheck reported issue in this script: SC2086:info:17:73: Double quote to prevent globbing and word splitting
(shellcheck)
43-43: shellcheck reported issue in this script: SC2086:info:18:77: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ 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). (2)
- GitHub Check: build-and-push
- GitHub Check: commit
🔇 Additional comments (3)
.docker/entrypoints/cli/03-provision-site.sh (1)
35-40: Quant handoff guard looks goodThe executable check plus the clear failure message will make misconfigurations obvious while still delegating cleanly to the Quant-aware provisioner.
.github/workflows/build-deploy.yml (1)
64-110: Do not push container images for pull_request runs.These steps still push to the registry (and rely on secrets) during PR events, which both fails for forks and pollutes the registry with unmerged builds. Gate the push-oriented steps to run only on trusted events.
- name: Build and push cli image uses: docker/build-push-action@v5 + if: ${{ github.event_name != 'pull_request' }} @@ - name: Build and push nginx image uses: docker/build-push-action@v5 + if: ${{ github.event_name != 'pull_request' }} @@ - name: Build and push php image uses: docker/build-push-action@v5 + if: ${{ github.event_name != 'pull_request' }}.docker/nginx-drupal.dockerfile (1)
20-23: LGTM – configuration wired into the image cleanly.Copying the host-prepend config and locking its permissions before the final app sync looks correct and keeps the build deterministic.
| # Use X-Forwarded-Host when provided (reverse proxies) | ||
| if ($http_x_forwarded_host != "") { | ||
| set $final_host $http_x_forwarded_host; | ||
| } | ||
|
|
||
| # Override with Quant-Orig-Host when available | ||
| if ($http_quant_orig_host != "") { | ||
| set $final_host $http_quant_orig_host; | ||
| } | ||
|
|
||
| # Extract first host from comma-separated X-Forwarded-Host values | ||
| if ($final_host ~ "^([^,\s]+)") { | ||
| set $final_host $1; | ||
| } | ||
|
|
||
| # Always pass the calculated host; without override it matches the original Host header | ||
| fastcgi_param HTTP_HOST $final_host; |
There was a problem hiding this comment.
Don't trust raw X-Forwarded-Host (host‑header injection risk).
Any client can send an X-Forwarded-Host header when hitting the origin directly. With this block we now overwrite HTTP_HOST for PHP with that value, effectively reviving classic host-header attacks (poisoned absolute URLs, password-reset links, cache poisoning, etc.). Unless we positively identify and trust the intermediary that injected the header (e.g., by IP allowlisting or a signed header), we must not consume it blindly.
Please either remove this fallback or constrain it to a vetted allowlist/sanitised value before passing it to PHP. At minimum, drop the direct assignment shown below until a trust mechanism is implemented.
-# Use X-Forwarded-Host when provided (reverse proxies)
-if ($http_x_forwarded_host != "") {
- set $final_host $http_x_forwarded_host;
-}🤖 Prompt for AI Agents
In .docker/config/nginx/location_drupal_prepend_host.conf around lines 7-23, the
current logic blindly uses X-Forwarded-Host (and Quant-Orig-Host) to set
HTTP_HOST which introduces host‑header injection risks; remove the unconditional
override and instead only set $final_host from forwarded headers when the
request comes from a trusted intermediary (e.g., check remote_addr against a
proxy allowlist) or when the forwarded value matches an allowed/validated
hostname pattern or explicit whitelist, otherwise fall back to the original
$host; do not pass unvalidated forwarded headers to fastcgi_param HTTP_HOST
until you implement IP allowlisting or strict hostname validation.
| build-and-push: | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
Block the Quant deployment job on pull_request events.
Forked PRs cannot access the QUANT_* secrets, so the “Initialize Quant Cloud” step fails immediately and every forked PR run goes red. Guard the entire job (or all secrets-dependent steps) to skip execution on pull_request events.
build-and-push:
+ if: ${{ github.event_name != 'pull_request' }}
runs-on: ubuntu-latest📝 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.
| build-and-push: | |
| runs-on: ubuntu-latest | |
| steps: | |
| build-and-push: | |
| if: ${{ github.event_name != 'pull_request' }} | |
| runs-on: ubuntu-latest | |
| steps: |
🤖 Prompt for AI Agents
.github/workflows/build-deploy.yml around lines 22-24: the Quant deployment job
runs on pull_request where forked PRs lack QUANT_* secrets and fail; guard the
job (or all steps that use these secrets) with a condition that skips execution
for pull_request events (e.g., add an if that checks github.event_name !=
'pull_request' at the job level or on each secrets-dependent step) so the
job/steps are skipped for forked PRs.
| console.error(`[quant] Downloading backup to ${downloadPath}...`); | ||
| const response = await fetch(downloadUrl); | ||
| if (!response.ok || !response.body) { | ||
| console.error(`[quant] Failed to download backup: HTTP ${response.status}`); | ||
| process.exit(1); | ||
| } | ||
| const fileStream = fs.createWriteStream(downloadPath); | ||
| await pipeline(response.body, fileStream); | ||
| console.error('[quant] Download complete.'); |
There was a problem hiding this comment.
Fix the download pipeline to consume web streams correctly
node --input-type=module uses the WHATWG Fetch API where response.body is a web stream. Passing it directly into stream.pipeline throws ERR_INVALID_ARG_TYPE, so the script bails out before saving the backup.
-import { pipeline } from 'stream/promises';
+import { pipeline } from 'stream/promises';
+import { Readable } from 'stream';
@@
-await pipeline(response.body, fileStream);
+await pipeline(Readable.fromWeb(response.body), fileStream);📝 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.
| console.error(`[quant] Downloading backup to ${downloadPath}...`); | |
| const response = await fetch(downloadUrl); | |
| if (!response.ok || !response.body) { | |
| console.error(`[quant] Failed to download backup: HTTP ${response.status}`); | |
| process.exit(1); | |
| } | |
| const fileStream = fs.createWriteStream(downloadPath); | |
| await pipeline(response.body, fileStream); | |
| console.error('[quant] Download complete.'); | |
| // At the top of scripts/quant/download-db.sh (or .js) | |
| import { pipeline } from 'stream/promises'; | |
| import { Readable } from 'stream'; | |
| … | |
| console.error(`[quant] Downloading backup to ${downloadPath}...`); | |
| const response = await fetch(downloadUrl); | |
| if (!response.ok || !response.body) { | |
| console.error(`[quant] Failed to download backup: HTTP ${response.status}`); | |
| process.exit(1); | |
| } | |
| const fileStream = fs.createWriteStream(downloadPath); | |
| // Wrap the WHATWG ReadableStream into a Node.js Readable | |
| await pipeline(Readable.fromWeb(response.body), fileStream); | |
| console.error('[quant] Download complete.'); |
🤖 Prompt for AI Agents
In scripts/quant/download-db.sh around lines 170-178, the code passes the WHATWG
response.body (a web stream) directly into Node's stream.pipeline which causes
ERR_INVALID_ARG_TYPE; convert the web stream to a Node Readable and use the
promise-based pipeline. Import Readable from 'stream' (or require), import
pipeline from 'stream/promises', create a node stream with
Readable.fromWeb(response.body) and await
pipeline(Readable.fromWeb(response.body), fileStream) so the download consumes
and writes correctly.
| read -r config_source CONFIG_SOURCE_RULE <<<"$(resolve_config_source "${quant_env_type}" "${quant_env_name}")" | ||
|
|
||
| # Always pass an absolute path to Drush to avoid CWD/docroot ambiguity. | ||
| if [ -n "${config_source}" ] && [ "${config_source#/}" = "${config_source}" ]; then | ||
| # Convert relative path to absolute from repository root. | ||
| config_source="$(pwd -P)/${config_source#./}" | ||
| fi | ||
|
|
||
| info "Resolved config overlay mapping: type='${quant_env_type:-unset}', name='${quant_env_name:-unset}' -> '${config_source}' (rule: ${CONFIG_SOURCE_RULE:-default})." | ||
|
|
||
| if [ -z "${config_source}" ]; then | ||
| note "Environment-specific configuration mapping did not resolve to a directory; skipping partial import." | ||
| elif [ "${config_source}" = "${default_config_dir}" ]; then | ||
| note "Environment-specific configuration maps to ${config_source} (rule: ${CONFIG_SOURCE_RULE:-default}); base configuration already imported." | ||
| elif config_dir_has_files "${config_source}"; then | ||
| message_suffix="" | ||
| if [ -n "${quant_env_name}" ]; then | ||
| message_suffix=", name: ${quant_env_name}" | ||
| fi | ||
| info "Importing environment-specific configuration from ${config_source} (type: ${quant_env_type:-unset}${message_suffix})." | ||
| drush_quant config:import --partial --source="${config_source}" || { |
There was a problem hiding this comment.
Capture the rule emitted by resolve_config_source
read -r config_source CONFIG_SOURCE_RULE <<<"$(...)" only assigns the first line to config_source, leaving CONFIG_SOURCE_RULE empty and clobbering the rule you just computed. The overlay logs then fall back to “default”, hiding which branch actually ran.
resolve_config_source() {
local env_type="$1"
local env_name="$2"
local source="${default_config_dir}"
- # Normalise case and trim accidental whitespace.
- env_type="${env_type,,}"
- env_name="${env_name,,}"
+ local rule="default"
+
+ # Normalise case and trim accidental whitespace.
+ env_type="${env_type,,}"
+ env_name="${env_name,,}"
@@
- if contains_token "${env_type}" "${production_types}" || contains_token "${env_name}" "${production_names}"; then
- source="${production_config_dir}"
- CONFIG_SOURCE_RULE="production"
+ if contains_token "${env_type}" "${production_types}" || contains_token "${env_name}" "${production_names}"; then
+ source="${production_config_dir}"
+ rule="production"
@@
- elif contains_token "${env_name}" "${test_names}"; then
- source="${test_config_dir}"
- CONFIG_SOURCE_RULE="test"
+ elif contains_token "${env_name}" "${test_names}"; then
+ source="${test_config_dir}"
+ rule="test"
@@
- elif contains_token "${env_type}" "${development_types}" || contains_token "${env_name}" "${development_names}"; then
- source="${development_config_dir}"
- CONFIG_SOURCE_RULE="development-explicit"
+ elif contains_token "${env_type}" "${development_types}" || contains_token "${env_name}" "${development_names}"; then
+ source="${development_config_dir}"
+ rule="development-explicit"
@@
- elif ! contains_token "${env_name}" "${exclude_from_dev}"; then
- source="${development_config_dir}"
- CONFIG_SOURCE_RULE="development-fallback"
+ elif ! contains_token "${env_name}" "${exclude_from_dev}"; then
+ source="${development_config_dir}"
+ rule="development-fallback"
else
- CONFIG_SOURCE_RULE="excluded-from-development"
+ rule="excluded-from-development"
fi
# Print both values so caller can capture without relying on subshell state.
- printf '%s\n%s' "${source}" "${CONFIG_SOURCE_RULE}"
+ printf '%s\n%s' "${source}" "${rule}"
}
@@
- read -r config_source CONFIG_SOURCE_RULE <<<"$(resolve_config_source "${quant_env_type}" "${quant_env_name}")"
+ IFS=$'\n' read -r config_source CONFIG_SOURCE_RULE <<<"$(resolve_config_source "${quant_env_type}" "${quant_env_name}")"Committable suggestion skipped: line range outside the PR's diff.
…c4fbef60839c1855b2f06ff3cf88c
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
web/themes/contrib/civictheme/civictheme.post_update.php (1)
968-1010: Past review comment about batch processing pattern still applies.A previous review suggested following the batch processing pattern used elsewhere in this file. However, note that
civictheme_post_update_add_civictheme_message_paragraph()(lines 894-938) has a similar structure and also doesn't use batch processing, suggesting inconsistent patterns in this file.If the batch processing pattern is required for consistency, both functions should be updated. If config-only operations don't require it, consider adding
@SuppressWarnings(PHPMD.UnusedFormalParameter)to document why$sandboxis unused.
🧹 Nitpick comments (1)
web/themes/contrib/civictheme/includes/cards.inc (1)
208-217: Reorder helper calls for consistency.The theme helper is called first, but other card preprocessing functions in this file consistently call it last. While the order may not affect functionality, maintaining a consistent pattern improves code maintainability.
Apply this diff to align with the established pattern:
function civictheme_preprocess_paragraph__civictheme_fast_fact_card(array &$variables): void { - _civictheme_preprocess_paragraph__paragraph_field__theme($variables); _civictheme_preprocess_paragraph__paragraph_field__title($variables); _civictheme_preprocess_paragraph__paragraph_field__summary($variables); _civictheme_preprocess_paragraph__paragraph_field__link($variables); + _civictheme_preprocess_paragraph__paragraph_field__theme($variables); $media = civictheme_get_field_value($variables['paragraph'], 'field_c_p_icon', TRUE); if ($media) { $variables['image'] = civictheme_media_image_get_variables($media); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonweb/themes/contrib/civictheme/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
web/themes/contrib/civictheme/civictheme.info.yml(4 hunks)web/themes/contrib/civictheme/civictheme.post_update.php(1 hunks)web/themes/contrib/civictheme/includes/cards.inc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/themes/contrib/civictheme/civictheme.info.yml
🧰 Additional context used
🧬 Code graph analysis (1)
web/themes/contrib/civictheme/civictheme.post_update.php (1)
web/themes/contrib/civictheme/src/CivicthemeUpdateHelper.php (2)
CivicthemeUpdateHelper(20-302)createConfigs(133-147)
⏰ 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
joshua-salsadigital
added
the
State: Needs review
github-actions
bot
added
State: CONFLICT
and removed
State: Needs review
https://salsadigital.atlassian.net/browse/GOVCMSCT2-121
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
Release Notes
New Features
Tests