Skip to content

Conversation

@alan-cole
Copy link
Collaborator

@alan-cole alan-cole commented Sep 30, 2025

https://www.drupal.org/project/civictheme/issues/3549220

Checklist before requesting a review

  • I have formatted the subject to include ticket number as Issue #123456 by drupal_org_username: Issue title
  • I have added a link to the issue tracker
  • I have provided information in Changed section about WHY something was done if this was not a normal implementation
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have run new and existing relevant tests locally with my changes, and they passed
  • I have provided screenshots, where applicable

Changed

  1. Updated search page results message to use "Showing @start - @end of @total @keywords" instead of "Search results..."
  2. Updated automated list count to use "Showing @start - @end of @total @keywords"
  3. Added html preprocess to include @Keywords if present. Added cache context for url.query_args.
  4. Added views preprocess to add @keywords if present.
  5. Added theme settings to define what fields should be used for @Keywords.
  6. Added config to set theme settings to "keywords" (/search page) and "title" (automated list).
  7. Updated search and auto listing to use the 'inline filter' - Note! this requires changes in UI Kit ([#769] Added inline filter component for search page. uikit#788) to work. I have updated the install on this branch to the required commit. This will need to be reverted back to develop once [#769] Added inline filter component for search page. uikit#788 is merged.

Screenshots

Screenshots for Points 1-6.
3549220

Screenshot for Point 7.
image

Summary by CodeRabbit

  • New Features

    • Theme settings: added a collapsible Search component to configure keyword fields.
    • Search pages append entered keywords to page titles.
    • Views result headers now include searched keywords (e.g., "Showing @start - @EnD of @ToTal @Keywords").
    • Inline filter layout for single-keyword exposed filters (Search and Automated List).
    • Form fields support a title_size property for improved label styling; result counts and titles respect URL query parameters.
  • Chores

    • Updated UI Kit dependency to a newer commit.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 30, 2025

Walkthrough

Adds configurable keyword-based search fields and UI, injects searched keywords into head titles and Views headers, enables single-field inline exposed filters, forwards title_size to field templates, extends cache contexts for URL query args, and updates the UI kit dependency.

Changes

Cohort / File(s) Summary of Changes
Theme bootstrap
web/themes/contrib/civictheme/civictheme.theme
Import Drupal\Core\Cache\Cache; call _civictheme_preprocess_html__search_head_title() from HTML preprocess; merge cache contexts with url.query_args.
Search logic & head title
web/themes/contrib/civictheme/includes/search.inc
New _civictheme_preprocess_html__search_head_title(array &$variables): void using CivicthemeUtility to read configured keyword fields, extract the first matching query param, and append a sanitized "Searching for '...'" suffix to the head title.
Views preprocess & exposed form
web/themes/contrib/civictheme/includes/views.inc, web/themes/contrib/civictheme/config/optional/views.view.civictheme_search.yml, .../config/install/views.view.civictheme_automated_list.yml
Add _civictheme_preprocess_views_view__results_count() to replace @keywords in results_count, extend cache contexts with url.query_args, extract keywords from exposed input, adjust exposed-form preprocessing to enable single-field inline filters for specific views; Views header content simplified to a plain string with @keywords.
Theme settings UI & config
web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionComponents.php, web/themes/contrib/civictheme/config/install/civictheme.settings.yml, web/themes/contrib/civictheme/config/schema/civictheme.schema.yml
Add "Search" settings section with keyword_fields textarea (multiline ↔ array conversion via CivicthemeUtility), store under components.search.keyword_fields, add submitSearch handler, and add schema entry for the new config.
Exposed form template (inline filter)
web/themes/contrib/civictheme/templates/views/views-exposed-form.html.twig
Add inline_filter branch to render civictheme:inline-filter with filter_items and submit_text when applicable; otherwise fall back to default {{ form }} rendering.
Form element title size
web/themes/contrib/civictheme/includes/form_element.inc, web/themes/contrib/civictheme/templates/form/form-element--civictheme-field.html.twig
Propagate new #title_size into preprocess variables and pass title_size into the civictheme field template include.
Utility usage
web/themes/contrib/civictheme/includes/*.inc, web/themes/contrib/civictheme/src/Settings/...
Introduce/use CivicthemeUtility helpers for converting multiline config ↔ arrays and reading theme settings.
Dependency
web/themes/contrib/civictheme/package.json
Update @civictheme/uikit dependency pin to commit bd1d7944ff7f1030359d31c61c9dd88f64038d8d.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Browser
  participant Drupal as Drupal (render)
  participant Theme as CivicTheme
  participant Config as ThemeConfig

  Browser->>Drupal: GET /search?q=term
  Drupal->>Theme: civictheme_preprocess_html(variables)
  Theme->>Config: load components.search.keyword_fields
  Theme->>Theme: find first matching query param
  alt keyword found
    Theme->>Drupal: append "Searching for '…'" to head_title
    Theme->>Drupal: merge cache contexts with url.query_args
  else no keyword
    Theme-->>Drupal: no head_title change
  end
Loading
sequenceDiagram
  autonumber
  participant Views as Views (preprocess)
  participant Theme as CivicTheme
  participant Config as ThemeConfig
  participant Twig as Twig Template

  Views->>Theme: civictheme_preprocess_views_view(variables)
  Theme->>Theme: _civictheme_preprocess_views_view__results_count()
  Theme->>Config: read keyword_fields
  Theme->>Theme: extract keywords from exposed input
  alt keywords found and @keywords present
    Theme->>Views: replace @keywords in results_count
    Theme->>Views: add url.query_args to cache contexts
  end
  Theme->>Theme: determine inline_filter eligibility
  alt inline_filter true
    Theme->>Twig: render civictheme:inline-filter with filter_items, submit_text
  else
    Theme->>Twig: render default exposed form
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • richardgaunt

Poem

I twitch my whiskers at query light,
I sniff the keywords through the night.
Titles echo what users seek,
Inline filters small and sleek.
Configs hop, deps snug—hop, delight! 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly describes the primary change by specifying updates to both the search and auto list page titles along with the search results message, which directly matches the core objectives of the pull request.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/3549220-search-results-message

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b8d9a1b and 3957fa8.

📒 Files selected for processing (5)
  • web/themes/contrib/civictheme/civictheme.theme (3 hunks)
  • web/themes/contrib/civictheme/config/schema/civictheme.schema.yml (1 hunks)
  • web/themes/contrib/civictheme/includes/search.inc (2 hunks)
  • web/themes/contrib/civictheme/includes/views.inc (4 hunks)
  • web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionComponents.php (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionComponents.php
  • web/themes/contrib/civictheme/includes/search.inc
⏰ 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 (5)
web/themes/contrib/civictheme/civictheme.theme (1)

11-11: LGTM!

The Cache import, search head title preprocessing call, and corrected cache context merging all integrate properly with the new keyword-based search functionality.

Also applies to: 165-165, 280-280

web/themes/contrib/civictheme/config/schema/civictheme.schema.yml (1)

228-236: LGTM!

The schema correctly defines keyword_fields as a sequence of strings, aligning with its usage in the preprocessing logic where it's converted via CivicthemeUtility::multilineToArray().

web/themes/contrib/civictheme/includes/views.inc (3)

11-11: LGTM!

The new imports support the keyword field utilities and view instance type checking introduced in this PR.

Also applies to: 16-16


26-26: LGTM!

The preprocessing flow correctly prioritizes inline filtering for single-field exposed forms before falling back to the single filter approach.

Also applies to: 166-169


179-215: LGTM!

The inline filter preprocessing includes proper guards (ViewExecutable instance check, view ID validation, field type verification) and correctly extracts the submit button text when available.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@alan-cole alan-cole changed the title Issue #3549220 by alan.cole: Search & Auto list page title and search results message Issue #3549220 by alan.cole: Search & Auto list page title and search results message. Sep 30, 2025
@alan-cole alan-cole marked this pull request as ready for review September 30, 2025 06:48
@alan-cole alan-cole added the State: Needs review Pull requests needs a review from assigned developers label Sep 30, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
web/themes/contrib/civictheme/includes/search.inc (1)

68-72: Review sanitization approach for consistency with Drupal's translation system.

The code uses htmlspecialchars() before passing to t(). However, Drupal's t() function with '@' placeholders already escapes HTML by default, which may result in double-escaping (e.g., & becoming &).

Consider removing the explicit htmlspecialchars() call and relying on t()'s built-in escaping:

-  $sanitized_keywords = htmlspecialchars($keywords, ENT_QUOTES, 'UTF-8');
   $variables['head_title']['title'] = t("@title - Searching for '@keywords'", [
     '@title' => $head_title,
-    '@keywords' => $sanitized_keywords,
+    '@keywords' => $keywords,
   ]);

If additional sanitization is required, verify this doesn't cause double-escaping in the browser page title.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 748d00d and 1d4e4c9.

⛔ Files ignored due to path filters (1)
  • web/themes/contrib/civictheme/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (9)
  • web/themes/contrib/civictheme/civictheme.theme (3 hunks)
  • web/themes/contrib/civictheme/config/install/civictheme.settings.yml (1 hunks)
  • web/themes/contrib/civictheme/config/install/views.view.civictheme_automated_list.yml (1 hunks)
  • web/themes/contrib/civictheme/config/optional/views.view.civictheme_search.yml (1 hunks)
  • web/themes/contrib/civictheme/includes/search.inc (2 hunks)
  • web/themes/contrib/civictheme/includes/views.inc (4 hunks)
  • web/themes/contrib/civictheme/package.json (1 hunks)
  • web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionComponents.php (1 hunks)
  • web/themes/contrib/civictheme/templates/views/views-exposed-form.html.twig (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionComponents.php (2)
web/themes/contrib/civictheme/src/CivicthemeUtility.php (2)
  • CivicthemeUtility (10-157)
  • arrayToMultiline (23-25)
web/themes/contrib/civictheme/src/CivicthemeConfigManager.php (1)
  • load (67-74)
⏰ 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 (8)
web/themes/contrib/civictheme/civictheme.theme (2)

11-11: LGTM!

The Cache class import is necessary for the cache context merging functionality added later in the file.


165-165: Verified: _civictheme_preprocess_html__search_head_title is defined in web/themes/contrib/civictheme/includes/search.inc (line 44) and correctly extracts keywords and formats the head title.

web/themes/contrib/civictheme/config/install/civictheme.settings.yml (1)

70-73: Verified keyword_fields match exposed filter identifiers in the search and automated list views.

web/themes/contrib/civictheme/includes/views.inc (3)

165-177: Good implementation of inline filter logic.

The early return pattern and view-specific filtering logic is clean and appropriate.


299-299: Correct cache context added for query arguments.

The addition of url.query_args cache context ensures proper cache variation based on query parameters.


302-312: Verify Twig escaping for results_count
I couldn’t locate where results_count is rendered in the theme; please inspect the Twig template that outputs this variable and ensure it’s in an auto-escaped context (or explicitly passed through the |escape filter) to prevent XSS from user-supplied keywords.

web/themes/contrib/civictheme/config/optional/views.view.civictheme_search.yml (1)

221-230: Configuration changes align with keyword support.

The header configuration correctly changes from a custom area to a result summary plugin with the @keywords placeholder, which will be processed by _civictheme_preprocess_views_view__results_count() in views.inc.

web/themes/contrib/civictheme/includes/search.inc (1)

55-72: Cache context for query parameters is correctly applied
The url.query_args context is already being merged into $attachments['#cache']['contexts'] in web/themes/contrib/civictheme/civictheme.theme (around line 280).

Comment on lines +32 to +36
{% elseif inline_filter %}
{% include 'civictheme:inline-filter' with {
theme: theme,
items: filter_items,
} only %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify inline-filter component exists and preprocessing sets inline_filter variable.

The template now includes a civictheme:inline-filter component. Ensure:

  1. The component exists in the updated UIKit dependency
  2. The inline_filter variable is properly set in includes/views.inc preprocessing
  3. The filter_items variable is populated with the correct structure expected by the component

🏁 Script executed:

#!/bin/bash
# Check if inline-filter component exists in the components directory after uikit-install
fd -t d "inline-filter" components/

# Search for inline_filter variable assignment in views preprocessing
rg -n --type=php "inline_filter" web/themes/contrib/civictheme/includes/views.inc -C5

Length of output: 713


🏁 Script executed:

#!/bin/bash
# Locate directories named inline-filter
fd -t d inline-filter

# Locate twig templates named inline-filter
fd -t f "*inline-filter*.twig"

# Find any files referencing inline-filter
rg -l "inline-filter"

Length of output: 374


🏁 Script executed:

#!/bin/bash
# Search for inline-filter or inline_filter twig templates in the theme
find web/themes/contrib/civictheme/templates -type f \( -iname "*inline-filter*.twig" -o -iname "*inline_filter*.twig" \)

Length of output: 0


Add missing inline-filter component template
The include in web/themes/contrib/civictheme/templates/views/views-exposed-form.html.twig references civictheme:inline-filter, but no corresponding Twig template or component directory exists. Add a templates/components/inline-filter.html.twig (or your project’s component path) to define this component. Variables inline_filter and filter_items are already set in includes/views.inc.

🤖 Prompt for AI Agents
In web/themes/contrib/civictheme/templates/views/views-exposed-form.html.twig
around lines 32 to 36 the template includes "civictheme:inline-filter" but no
component template exists; create a new Twig component at
web/themes/contrib/civictheme/templates/components/inline-filter.html.twig (or
your project's component path) that accepts the passed-in variables (theme and
filter_items / inline_filter), loops over filter_items to render each filter
control with appropriate markup/classes (respect theme if needed), and returns
the expected HTML structure used by the views-exposed-form include; after adding
the file clear Drupal caches so the new component is discoverable.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
web/themes/contrib/civictheme/includes/views.inc (1)

165-165: Fix typo in inline-filter comment

Replace “exsits” with “exists” to tidy the comment.

-    // Use inline filter on search and auto pages if a single text field exsits.
+    // Use inline filter on search and auto pages if a single text field exists.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1d4e4c9 and 394ad2d.

📒 Files selected for processing (4)
  • web/themes/contrib/civictheme/includes/form_element.inc (1 hunks)
  • web/themes/contrib/civictheme/includes/views.inc (4 hunks)
  • web/themes/contrib/civictheme/package.json (1 hunks)
  • web/themes/contrib/civictheme/templates/form/form-element--civictheme-field.html.twig (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

@github-actions github-actions bot added State: CONFLICT and removed State: Needs review Pull requests needs a review from assigned developers labels Oct 8, 2025
@alan-cole alan-cole force-pushed the feature/3549220-search-results-message branch from 394ad2d to 08d8f19 Compare October 10, 2025 05:41
@alan-cole alan-cole added the State: Needs review Pull requests needs a review from assigned developers label Oct 10, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
web/themes/contrib/civictheme/civictheme.theme (1)

280-280: Critical: Fix incorrect variable name in cache context merging.

Line 280 references $variables but the function parameter is $attachments. This will cause the cache context merging to fail or produce an undefined variable error.

Apply this diff:

- $attachments['#cache']['contexts'] = Cache::mergeContexts($variables['#cache']['contexts'] ?? [], ['url.query_args']);
+ $attachments['#cache']['contexts'] = Cache::mergeContexts($attachments['#cache']['contexts'] ?? [], ['url.query_args']);
web/themes/contrib/civictheme/includes/views.inc (1)

165-165: Fix typo in comment.

"exsits" should be "exists".

Apply this diff:

-    // Use inline filter on search and auto pages if a single text field exsits.
+    // Use inline filter on search and auto pages if a single text field exists.
🧹 Nitpick comments (1)
web/themes/contrib/civictheme/includes/search.inc (1)

68-72: Consider removing redundant escaping.

The t() function with @ placeholders automatically escapes values using Html::escape(), which internally uses htmlspecialchars(). Manually calling htmlspecialchars() before passing to t() results in double-escaping, though it's not harmful.

Apply this diff to simplify:

- $sanitized_keywords = htmlspecialchars($keywords, ENT_QUOTES, 'UTF-8');
  $variables['head_title']['title'] = t("@title - Searching for '@keywords'", [
    '@title' => $head_title,
-   '@keywords' => $sanitized_keywords,
+   '@keywords' => $keywords,
  ]);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 394ad2d and 08d8f19.

📒 Files selected for processing (11)
  • web/themes/contrib/civictheme/civictheme.theme (3 hunks)
  • web/themes/contrib/civictheme/config/install/civictheme.settings.yml (1 hunks)
  • web/themes/contrib/civictheme/config/install/views.view.civictheme_automated_list.yml (1 hunks)
  • web/themes/contrib/civictheme/config/optional/views.view.civictheme_search.yml (1 hunks)
  • web/themes/contrib/civictheme/includes/form_element.inc (1 hunks)
  • web/themes/contrib/civictheme/includes/search.inc (2 hunks)
  • web/themes/contrib/civictheme/includes/views.inc (4 hunks)
  • web/themes/contrib/civictheme/package.json (1 hunks)
  • web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionComponents.php (2 hunks)
  • web/themes/contrib/civictheme/templates/form/form-element--civictheme-field.html.twig (1 hunks)
  • web/themes/contrib/civictheme/templates/views/views-exposed-form.html.twig (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • web/themes/contrib/civictheme/templates/form/form-element--civictheme-field.html.twig
  • web/themes/contrib/civictheme/config/install/views.view.civictheme_automated_list.yml
  • web/themes/contrib/civictheme/includes/form_element.inc
  • web/themes/contrib/civictheme/templates/views/views-exposed-form.html.twig
  • web/themes/contrib/civictheme/config/install/civictheme.settings.yml
🧰 Additional context used
🧬 Code graph analysis (1)
web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionComponents.php (2)
web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionOptout.php (2)
  • form (27-60)
  • multilineToArray (65-68)
web/themes/contrib/civictheme/src/CivicthemeUtility.php (3)
  • CivicthemeUtility (10-157)
  • arrayToMultiline (23-25)
  • multilineToArray (36-40)
⏰ 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 (4)
web/themes/contrib/civictheme/config/optional/views.view.civictheme_search.yml (1)

221-230: LGTM!

The header configuration changes correctly introduce the @keywords placeholder, which aligns with the PR's goal of adding keyword support. The placeholder will be dynamically replaced by the preprocessor in web/themes/contrib/civictheme/includes/views.inc.

web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionComponents.php (1)

606-616: LGTM!

The submitSearch handler correctly converts the multiline textarea input to an array before saving, addressing the past review comment. The implementation follows the established pattern and integrates seamlessly with the auto-discovery mechanism.

web/themes/contrib/civictheme/includes/views.inc (2)

166-178: LGTM!

The instanceof check at line 166 correctly guards against null/invalid View objects before calling $view->id(), addressing the past review concern. The inline filter logic properly handles single-field exposed forms for search and automated list views.


283-316: LGTM!

The _civictheme_preprocess_views_view__results_count function is well-structured with appropriate early returns, proper cache context handling, and correct keyword extraction logic. The implementation aligns with the PR's objectives for keyword-based search result messaging.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 08d8f19 and 78796ac.

📒 Files selected for processing (2)
  • web/themes/contrib/civictheme/civictheme.theme (3 hunks)
  • web/themes/contrib/civictheme/includes/search.inc (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • web/themes/contrib/civictheme/civictheme.theme
⏰ 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 (2)
web/themes/contrib/civictheme/includes/search.inc (2)

11-11: LGTM!

The import of CivicthemeUtility is appropriate for the new functionality.


44-49: Verify cache context in preprocess_html
No civictheme_preprocess_html implementation was found; ensure your theme’s preprocess_html hook adds the url.query_args cache context to prevent serving stale cached pages for different query parameters.

Comment on lines +54 to +62
foreach ($search_fields as $field) {
$query_field = \Drupal::request()->query->get($field);
if (!empty($query_field)) {
$keywords = $query_field;
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add input validation and error handling for query parameters.

The code retrieves query parameters without validation or error handling, which could lead to issues:

  1. No input validation: Query parameter values are not validated for length or type. A malicious user could pass extremely long strings that might cause memory issues or unexpected behavior.
  2. No null-safety: If \Drupal::request() returns null (though rare), this will cause a fatal error.

Apply this diff to add validation:

   $keywords = NULL;
   $search_fields = CivicthemeUtility::multilineToArray($setting_keyword_fields);
   foreach ($search_fields as $field) {
-    $query_field = \Drupal::request()->query->get($field);
+    $request = \Drupal::request();
+    if (!$request) {
+      continue;
+    }
+    $query_field = $request->query->get($field);
     if (!empty($query_field)) {
+      // Limit keyword length to prevent abuse.
+      $query_field = is_string($query_field) ? mb_substr($query_field, 0, 255) : '';
       $keywords = $query_field;
       break;
     }
   }
🤖 Prompt for AI Agents
In web/themes/contrib/civictheme/includes/search.inc around lines 54 to 60, the
code reads query parameters without null-safety or validation; update it to
first ensure \Drupal::request() returns a Request object
(null-check/type-check), then retrieve each $field value safely, ensure it is a
string, trim it, enforce a maximum length (e.g. 1024 chars) and optional
pattern/type checks, and only assign $keywords and break when the value passes
validation; if validation fails, skip that param and optionally log a warning or
set a safe default so no fatal error or unbounded input is accepted.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
web/themes/contrib/civictheme/includes/views.inc (1)

191-214: LGTM with a minor suggestion!

The function correctly guards against invalid views and implements the inline filter logic. The past review concern about null guards has been addressed.

Consider adding a null check before accessing the submit button:

   $variables['inline_filter'] = TRUE;
   $keyword_field['#title_size'] = 'extra-large';
   $variables['filter_items'] = $keyword_field;
-  $submit_field = $variables['form']['actions']['submit'] ?? NULL;
-  if (!empty($submit_field)) {
+  if (!empty($variables['form']['actions']['submit'])) {
+    $submit_field = $variables['form']['actions']['submit'];
     $variables['submit_text'] = $submit_field['#value'] ?? '';
   }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 78796ac and 99169c4.

📒 Files selected for processing (2)
  • web/themes/contrib/civictheme/includes/search.inc (2 hunks)
  • web/themes/contrib/civictheme/includes/views.inc (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • web/themes/contrib/civictheme/includes/search.inc
⏰ 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/includes/views.inc (3)

11-11: LGTM!

The import is necessary for the multilineToArray() method used in the new results count preprocessor.


25-25: LGTM!

The preprocessor call is correctly integrated following the existing pattern.


165-169: LGTM!

The inline filter check correctly returns early when applicable, preventing the fallback to single filter processing. The typo and guard issues from previous reviews have been addressed.

Comment on lines +316 to +347
function _civictheme_preprocess_views_view__results_count(array &$variables): void {
if (empty($variables['results_count'])) {
return;
}
if (!str_contains($variables['results_count'], '@keywords')) {
return;
}

// Load search fields from theme settings.
$setting_keyword_fields = civictheme_get_theme_config_manager()->load('components.search.keyword_fields') ?? '';
if (empty($setting_keyword_fields)) {
// Strip the @keywords token because it exists but won't be populated.
$variables['results_count'] = str_replace('@keywords', '', $variables['results_count']);
return;
}

$variables['#cache']['contexts'] = Cache::mergeContexts($variables['#cache']['contexts'] ?? [], ['url.query_args']);

$keywords = '';
$exposed_input = $variables['view']->getExposedInput();
$search_fields = CivicthemeUtility::multilineToArray($setting_keyword_fields);

foreach ($search_fields as $field) {
if (!empty($exposed_input[$field])) {
$keywords = t("for '@keywords'", ['@keywords' => $exposed_input[$field]])->__toString();
break;
}
}

$variables['results_count'] = str_replace('@keywords', $keywords, $variables['results_count']);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Guard against missing view object.

Line 335 calls $variables['view']->getExposedInput() without verifying that $variables['view'] exists, which could cause a fatal error.

Apply this diff to add a guard:

   if (!str_contains($variables['results_count'], '@keywords')) {
     return;
   }
+  if (empty($variables['view'])) {
+    $variables['results_count'] = str_replace('@keywords', '', $variables['results_count']);
+    return;
+  }
 
   // Load search fields from theme settings.

Verify the keyword extraction logic.

Lines 338-343 extract keywords from the first matching field and break immediately. If multiple search fields are configured (e.g., "keywords" and "title"), only the first field with a value will be used. Please confirm this is the intended behavior rather than concatenating or prioritizing keywords.

Run the following script to check how keyword_fields are configured and used:


🏁 Script executed:

#!/bin/bash
# Description: Check keyword_fields configuration and usage

# Find theme settings configuration
fd -e yml -e yaml . | xargs rg -l "keyword_fields" | head -5

# Check how keyword_fields are used in preprocessing
rg -A 5 "keyword_fields" --type=php

Length of output: 2731


🏁 Script executed:

#!/bin/bash
# Inspect default config for keyword_fields
echo "== YAML snippet for keyword_fields =="
rg -n "keyword_fields" -C2 web/themes/contrib/civictheme/config/install/civictheme.settings.yml

# Locate multilineToArray implementation
echo "== multilineToArray implementation =="
rg -n "function multilineToArray" -C5 web/themes/contrib/civictheme/src/Utility

Length of output: 258


Add guard for missing view and confirm keyword field priority.

  • After the str_contains check, guard against a missing view to avoid a fatal error:
   if (!str_contains($variables['results_count'], '@keywords')) {
     return;
   }
+  if (empty($variables['view'])) {
+    $variables['results_count'] = str_replace('@keywords', '', $variables['results_count']);
+    return;
+  }
  • Confirm that only the first non-empty field from components.search.keyword_fields is intended to replace @keywords rather than combining multiple fields.
📝 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
function _civictheme_preprocess_views_view__results_count(array &$variables): void {
if (empty($variables['results_count'])) {
return;
}
if (!str_contains($variables['results_count'], '@keywords')) {
return;
}
// Load search fields from theme settings.
$setting_keyword_fields = civictheme_get_theme_config_manager()->load('components.search.keyword_fields') ?? '';
if (empty($setting_keyword_fields)) {
// Strip the @keywords token because it exists but won't be populated.
$variables['results_count'] = str_replace('@keywords', '', $variables['results_count']);
return;
}
$variables['#cache']['contexts'] = Cache::mergeContexts($variables['#cache']['contexts'] ?? [], ['url.query_args']);
$keywords = '';
$exposed_input = $variables['view']->getExposedInput();
$search_fields = CivicthemeUtility::multilineToArray($setting_keyword_fields);
foreach ($search_fields as $field) {
if (!empty($exposed_input[$field])) {
$keywords = t("for '@keywords'", ['@keywords' => $exposed_input[$field]])->__toString();
break;
}
}
$variables['results_count'] = str_replace('@keywords', $keywords, $variables['results_count']);
}
function _civictheme_preprocess_views_view__results_count(array &$variables): void {
if (empty($variables['results_count'])) {
return;
}
if (!str_contains($variables['results_count'], '@keywords')) {
return;
}
if (empty($variables['view'])) {
// Strip the @keywords token because no view is available to provide keywords.
$variables['results_count'] = str_replace('@keywords', '', $variables['results_count']);
return;
}
// Load search fields from theme settings.
$setting_keyword_fields = civictheme_get_theme_config_manager()->load('components.search.keyword_fields') ?? '';
if (empty($setting_keyword_fields)) {
// Strip the @keywords token because it exists but won't be populated.
$variables['results_count'] = str_replace('@keywords', '', $variables['results_count']);
return;
}
$variables['#cache']['contexts'] = Cache::mergeContexts($variables['#cache']['contexts'] ?? [], ['url.query_args']);
$keywords = '';
$exposed_input = $variables['view']->getExposedInput();
$search_fields = CivicthemeUtility::multilineToArray($setting_keyword_fields);
foreach ($search_fields as $field) {
if (!empty($exposed_input[$field])) {
$keywords = t("for '@keywords'", ['@keywords' => $exposed_input[$field]])->__toString();
break;
}
}
$variables['results_count'] = str_replace('@keywords', $keywords, $variables['results_count']);
}
🤖 Prompt for AI Agents
In web/themes/contrib/civictheme/includes/views.inc around lines 316 to 346, add
a null/instance guard after the str_contains check to ensure $variables['view']
exists and is an object before calling getExposedInput() to avoid a fatal error
(return early if missing), and make the replacement behavior explicit: either
keep the current logic which uses only the first non-empty field from
components.search.keyword_fields (document this with a short inline comment) or,
if the intent is to combine multiple fields, change the loop to accumulate
non-empty fields into $keywords (joined with a separator) and use that combined
string when replacing @keywords; also preserve cache context merging and the
existing token stripping fallback.

@alan-cole alan-cole force-pushed the feature/3549220-search-results-message branch from b8d9a1b to 3957fa8 Compare October 10, 2025 06:48
@github-actions github-actions bot added State: CONFLICT and removed State: Needs review Pull requests needs a review from assigned developers labels Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants