Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds RSS feed support for the blog: a page attachments hook for RSS autodiscovery, a Views feed at /blog/feed, paragraph preprocess to inject an RSS button in blog lists, a Twig extension exposing theme assets, service registration, component/twig icon overrides, and unit/Behat tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Browser
participant Drupal
participant PageHook as PageAttachmentsHook
participant Alias as AliasManager
participant Path as CurrentPathStack
participant Config as ConfigFactory
participant Views
Browser->>Drupal: GET /blog
Drupal->>PageHook: Invoke hook_page_attachments()
PageHook->>Path: Get current path
PageHook->>Alias: Resolve alias
Alias-->>PageHook: "/blog" or "/blog/..."
PageHook->>Config: Load system.site.name
PageHook-->>Drupal: Attach RSS autodiscovery link -> attachments['#attached']['html_head']
Drupal-->>Browser: Return page with RSS link
Browser->>Drupal: GET /blog/feed
Drupal->>Views: Execute feed_1 display
Views->>Views: Query nodes (filters, sort)
Views-->>Browser: Return RSS (application/rss+xml)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
StevenWalker98
force-pushed
the
feature/140-blog-rss-feed
branch
from
ebfd762 to
57a43ef
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #148 +/- ##
============================================
+ Coverage 0.00% 66.03% +66.03%
============================================
Files 3 6 +3
Lines 15 53 +38
============================================
+ Hits 0 35 +35
- Misses 15 18 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/modules/custom/do_base/css/blog-rss-feed.css`:
- Line 14: The CSS uses the value keyword "currentColor" for the border property
which violates the value-keyword-case rule; update all occurrences where the
border property uses currentColor (e.g., the border declarations shown) to use
the lowercase keyword "currentcolor" instead, and run stylelint to confirm the
lint failure is resolved (also ensure you change the other occurrence of
currentColor in the same stylesheet).
- Around line 12-24: The RSS link color contrast is too low for
.do-blog-rss__link (normal and :hover/:focus) at 0.875rem; update the CSS so
both default and hover/focus color pairs meet WCAG AA (4.5:1) contrast.
Specifically, modify the color values used in the .do-blog-rss__link rule and
the .do-blog-rss__link:hover, .do-blog-rss__link:focus rules (or swap to a
darker text color) so the foreground/background combinations pass 4.5:1—verify
with a contrast checker and apply the new hex values in those selectors.
In `@web/modules/custom/do_base/src/Controller/BlogRssFeedController.php`:
- Around line 51-54: Replace direct request and string concatenation in
BlogRssFeedController with Drupal URL generation APIs: use
Url::fromRoute('do_base.blog_rss_feed', [], ['absolute' => TRUE])->toString()
for the feed URL and Url::fromUri('base:/blog', ['absolute' =>
TRUE])->toString() for the blog URL instead of
\Drupal::request()->getSchemeAndHttpHost() . Also update the node link
generation at the later use (the code around line 61) to use
$node->toUrl('canonical', ['absolute' => TRUE])->toString() so base-path and
language-prefix handling are preserved.
- Around line 47-60: The public RSS generator in BlogRssFeedController iterates
$nodes and can expose nodes without enforcing view permissions; inside the
foreach over $nodes (and after the instanceof NodeInterface check) call the node
access check (e.g., use $node->access('view') / $node->access('view', NULL,
TRUE) as appropriate) and skip nodes that fail the check so only viewable
content is added to $items. Ensure the access check is applied before building
the feed item for each NodeInterface instance.
In `@web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php`:
- Line 17: The docblock line in PageAttachmentsHook.php ("Adds an RSS
autodiscovery link on the blog listing and individual blog posts.") exceeds the
80-char limit; wrap it into shorter lines under 80 characters in the docblock
(split the sentence into two lines) so it satisfies the configured 80-char check
for the file/class PageAttachmentsHook and its related docblock.
- Around line 21-22: PageAttachmentsHook uses service-locator calls
(\Drupal::service, \Drupal::config) and a hardcoded "/blog/feed" URL; refactor
by adding constructor injection for the needed services (e.g.
PathCurrentInterface or current_path service, PathAliasManagerInterface, and
ConfigFactoryInterface as required) and store them as private properties,
replace calls to \Drupal::service('path.current')->getPath() and
\Drupal::service('path_alias.manager')->getAliasByPath(...) with calls on those
injected properties inside PageAttachmentsHook::alterAttachments (or the
existing method), and replace the hardcoded href with
Url::fromRoute('do_base.blog_rss_feed')->toString() so the hook uses autowired
services and the route-based RSS URL.
In `@web/modules/custom/do_base/src/Plugin/Block/BlogRssFeedBlock.php`:
- Around line 24-31: In BlogRssFeedBlock::build replace the hardcoded
"/blog/feed" with a route-generated URL: call
\Drupal\Core\Url::fromRoute('do_base.blog_rss_feed')->toString() in the build()
method, add that string into the render array via '#context' (e.g. 'feed_url' =>
$urlString), and update the inline_template '#template' to use the context
variable (e.g. {{ feed_url }}) for the anchor href so the link is route-aware
and works on subdirectory deployments.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
config/default/block.block.drevops_blog_rss_feed.ymlweb/modules/custom/do_base/css/blog-rss-feed.cssweb/modules/custom/do_base/do_base.libraries.ymlweb/modules/custom/do_base/do_base.routing.ymlweb/modules/custom/do_base/src/Controller/BlogRssFeedController.phpweb/modules/custom/do_base/src/Hook/PageAttachmentsHook.phpweb/modules/custom/do_base/src/Plugin/Block/BlogRssFeedBlock.php
| color: #e8620e; | ||
| text-decoration: none; | ||
| border: 1px solid currentColor; | ||
| border-radius: 4px; | ||
| padding: 0.4em 0.8em; | ||
| transition: background-color 0.15s ease, color 0.15s ease; | ||
| } | ||
|
|
||
| .do-blog-rss__link:hover, | ||
| .do-blog-rss__link:focus { | ||
| background-color: #e8620e; | ||
| color: #fff; | ||
| text-decoration: none; |
There was a problem hiding this comment.
Increase color contrast for RSS link states.
At 0.875rem, the #e8620e ↔ #fff contrast is below WCAG AA (4.5:1), so both default and hover/focus states are hard to read.
♿ Suggested contrast-safe color update
.do-blog-rss__link {
display: inline-flex;
align-items: center;
gap: 0.4em;
font-size: 0.875rem;
- color: `#e8620e`;
+ color: `#b34700`;
text-decoration: none;
border: 1px solid currentColor;
border-radius: 4px;
padding: 0.4em 0.8em;
transition: background-color 0.15s ease, color 0.15s ease;
}
.do-blog-rss__link:hover,
.do-blog-rss__link:focus {
- background-color: `#e8620e`;
+ background-color: `#b34700`;
color: `#fff`;
text-decoration: none;
}📝 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.
| color: #e8620e; | |
| text-decoration: none; | |
| border: 1px solid currentColor; | |
| border-radius: 4px; | |
| padding: 0.4em 0.8em; | |
| transition: background-color 0.15s ease, color 0.15s ease; | |
| } | |
| .do-blog-rss__link:hover, | |
| .do-blog-rss__link:focus { | |
| background-color: #e8620e; | |
| color: #fff; | |
| text-decoration: none; | |
| color: `#b34700`; | |
| text-decoration: none; | |
| border: 1px solid currentColor; | |
| border-radius: 4px; | |
| padding: 0.4em 0.8em; | |
| transition: background-color 0.15s ease, color 0.15s ease; | |
| } | |
| .do-blog-rss__link:hover, | |
| .do-blog-rss__link:focus { | |
| background-color: `#b34700`; | |
| color: `#fff`; | |
| text-decoration: none; |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 14-14: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/css/blog-rss-feed.css` around lines 12 - 24, The
RSS link color contrast is too low for .do-blog-rss__link (normal and
:hover/:focus) at 0.875rem; update the CSS so both default and hover/focus color
pairs meet WCAG AA (4.5:1) contrast. Specifically, modify the color values used
in the .do-blog-rss__link rule and the .do-blog-rss__link:hover,
.do-blog-rss__link:focus rules (or swap to a darker text color) so the
foreground/background combinations pass 4.5:1—verify with a contrast checker and
apply the new hex values in those selectors.
| $nodes = $nids ? $this->entityTypeManager()->getStorage('node')->loadMultiple($nids) : []; | ||
|
|
||
| $site_config = $this->config('system.site'); | ||
| $site_name = $site_config->get('name') ?? 'DrevOps'; | ||
| $request = \Drupal::request(); | ||
| $base_url = $request->getSchemeAndHttpHost(); | ||
| $feed_url = $base_url . '/blog/feed'; | ||
| $blog_url = $base_url . '/blog'; | ||
|
|
||
| $items = ''; | ||
| foreach ($nodes as $node) { | ||
| if (!$node instanceof NodeInterface) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the BlogRssFeedController file
find . -name "BlogRssFeedController.php" -type fRepository: drevops/website
Length of output: 128
🏁 Script executed:
# Also search for any RSS-related controller files
find . -path "*/do_base/*" -name "*Controller.php" -type f | head -20Repository: drevops/website
Length of output: 128
🏁 Script executed:
cat -n "./web/modules/custom/do_base/src/Controller/BlogRssFeedController.php"Repository: drevops/website
Length of output: 4115
🏁 Script executed:
# Search for route definition for this controller
find ./web/modules/custom/do_base -name "*.routing.yml" -o -name "*.yml" | xargs grep -l "BlogRssFeedController\|blog.*feed" 2>/dev/nullRepository: drevops/website
Length of output: 158
🏁 Script executed:
# Also check for services or module configuration
find ./web/modules/custom/do_base -name "*.services.yml" -o -name "*.info.yml" | head -10Repository: drevops/website
Length of output: 104
🏁 Script executed:
cat ./web/modules/custom/do_base/do_base.routing.ymlRepository: drevops/website
Length of output: 252
Enforce node view access before adding items to the public feed.
The route is public (_access: 'TRUE') and the SQL query bypasses the node_access table. Without a view permission check, nodes that are published but restricted by access control (via node_access grants, content moderation, roles, or other mechanisms) can be exposed in the RSS feed.
Add the access check to the loop condition:
🔒 Suggested access-control guard
$items = '';
foreach ($nodes as $node) {
- if (!$node instanceof NodeInterface) {
+ if (!$node instanceof NodeInterface || !$node->access('view')) {
continue;
}🧰 Tools
🪛 GitHub Actions: Database, Build, Test and Deploy
[warning] 51-51: Drupal calls should be avoided in classes, use dependency injection instead (DrupalPractice.Objects.GlobalDrupal.GlobalDrupal).
🪛 PHPMD (2.15.0)
[error] 49-49: The variable $site_config is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 50-50: The variable $site_name is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 51-51: Avoid using static access to class '\Drupal' in method 'feed'. (undefined)
(StaticAccess)
[error] 52-52: The variable $base_url is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 53-53: The variable $feed_url is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 54-54: The variable $blog_url is not named in camelCase. (undefined)
(CamelCaseVariableName)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/src/Controller/BlogRssFeedController.php` around
lines 47 - 60, The public RSS generator in BlogRssFeedController iterates $nodes
and can expose nodes without enforcing view permissions; inside the foreach over
$nodes (and after the instanceof NodeInterface check) call the node access check
(e.g., use $node->access('view') / $node->access('view', NULL, TRUE) as
appropriate) and skip nodes that fail the check so only viewable content is
added to $items. Ensure the access check is applied before building the feed
item for each NodeInterface instance.
| $request = \Drupal::request(); | ||
| $base_url = $request->getSchemeAndHttpHost(); | ||
| $feed_url = $base_url . '/blog/feed'; | ||
| $blog_url = $base_url . '/blog'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n web/modules/custom/do_base/src/Controller/BlogRssFeedController.phpRepository: drevops/website
Length of output: 4115
🏁 Script executed:
find web/modules/custom/do_base -name "*.routing.yml" -o -name "do_base.routing.yml"Repository: drevops/website
Length of output: 105
🏁 Script executed:
cat -n web/modules/custom/do_base/do_base.routing.ymlRepository: drevops/website
Length of output: 301
Replace static request access and manual URL concatenation with Drupal URL generation APIs.
Avoid \Drupal::request()->getSchemeAndHttpHost() combined with string concatenation, which bypasses Drupal's base-path and language-prefix handling. The suggested approaches are more robust:
- Use
Url::fromRoute('do_base.blog_rss_feed', [], ['absolute' => TRUE])->toString()for the feed URL (route exists) - Use
Url::fromUri('base:/blog', ['absolute' => TRUE])->toString()for the blog URL - Use
$node->toUrl('canonical', ['absolute' => TRUE])->toString()for node URLs
Apply changes to lines 51-54 and 61.
🧰 Tools
🪛 GitHub Actions: Database, Build, Test and Deploy
[warning] 51-51: Drupal calls should be avoided in classes, use dependency injection instead (DrupalPractice.Objects.GlobalDrupal.GlobalDrupal).
🪛 PHPMD (2.15.0)
[error] 51-51: Avoid using static access to class '\Drupal' in method 'feed'. (undefined)
(StaticAccess)
[error] 52-52: The variable $base_url is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 53-53: The variable $feed_url is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 54-54: The variable $blog_url is not named in camelCase. (undefined)
(CamelCaseVariableName)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/src/Controller/BlogRssFeedController.php` around
lines 51 - 54, Replace direct request and string concatenation in
BlogRssFeedController with Drupal URL generation APIs: use
Url::fromRoute('do_base.blog_rss_feed', [], ['absolute' => TRUE])->toString()
for the feed URL and Url::fromUri('base:/blog', ['absolute' =>
TRUE])->toString() for the blog URL instead of
\Drupal::request()->getSchemeAndHttpHost() . Also update the node link
generation at the later use (the code around line 61) to use
$node->toUrl('canonical', ['absolute' => TRUE])->toString() so base-path and
language-prefix handling are preserved.
| $current_path = \Drupal::service('path.current')->getPath(); | ||
| $alias = \Drupal::service('path_alias.manager')->getAliasByPath($current_path); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n web/modules/custom/do_base/src/Hook/PageAttachmentsHook.phpRepository: drevops/website
Length of output: 1435
🏁 Script executed:
# Search for route definition
fd -t f "routing" web/modules/custom/do_base/Repository: drevops/website
Length of output: 105
🏁 Script executed:
# Look for any YAML route definitions
find web/modules/custom/do_base/ -name "*.routing.yml" -o -name "*.yml" | head -20Repository: drevops/website
Length of output: 198
🏁 Script executed:
# Search for "do_base.blog_rss_feed" route in the codebase
rg "blog_rss_feed|blog.*feed" web/modules/custom/do_base/Repository: drevops/website
Length of output: 1899
🏁 Script executed:
# Search for other hook classes to see if they use DI
find web/modules/custom -name "*Hook.php" -type fRepository: drevops/website
Length of output: 233
🏁 Script executed:
# Check if there are any hook classes with constructors
rg -A 10 "#\[Hook" web/modules/custom --type php | head -50Repository: drevops/website
Length of output: 2829
🏁 Script executed:
# Check FormAlterHook for constructor usage
cat -n web/modules/custom/do_base/src/Hook/FormAlterHook.phpRepository: drevops/website
Length of output: 974
🏁 Script executed:
# Check LibraryInfoAlterHook for constructor usage
cat -n web/modules/custom/do_base/src/Hook/LibraryInfoAlterHook.phpRepository: drevops/website
Length of output: 945
🏁 Script executed:
# Search for documentation or examples of hook attribute with DI in Drupal
web_search "Drupal #[Hook] attribute constructor dependency injection"Repository: drevops/website
Length of output: 109
🌐 Web query:
Drupal 10 11 #[Hook] attribute constructor dependency injection
💡 Result:
In Drupal core, you cannot dependency-inject into the #[Hook] attribute’s constructor (it’s just metadata). Dependency injection happens on the hook class (service) constructor, not on the attribute. In Drupal 11.1+, hook classes are auto-registered as autowired services, so constructor DI “just works” as long as the class is in the right namespace/path. (drupal.org)
Drupal 11.1+ (core OOP hooks + constructor DI)
Create a class in src/Hook/ with namespace Drupal\<module>\Hook, add #[Hook('...')] on a method, and inject services via the class constructor:
<?php
namespace Drupal\mymodule\Hook;
use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\Core\Hook\Attribute\Hook;
use Drupal\node\NodeInterface;
final class NodeHooks {
public function __construct(
private readonly EntityTypeManagerInterface $entityTypeManager,
) {}
#[Hook('node_insert')]
public function onInsert(NodeInterface $node): void {
// Use $this->entityTypeManager ...
}
}Drupal 11.1+ discovers these automatically and wires the constructor. (drupal.org)
Drupal 10.x and Drupal 11.0 (core does not execute #[Hook] yet)
Core ships the Hook attribute class mainly as a shim, but it has no effect until Drupal 11.1.0. (api.drupal.org)
If you want one codebase that works on 10.1–11.0 and uses OOP hooks, the recommended pattern is:
- Register the hook class as a service (
autowire: true) - Keep a small procedural hook implementation that forwards to the service, marked
#[LegacyHook]so Drupal 11.1+ won’t call it. (drupal.org)
(Example structure is shown in the change record.) (drupal.org)
Citations:
- 1: https://www.drupal.org/node/3442349?utm_source=openai
- 2: https://www.drupal.org/node/3442349?utm_source=openai
- 3: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Hook%21Attribute%21Hook.php/class/Hook/10?utm_source=openai
- 4: https://www.drupal.org/node/3442349?utm_source=openai
- 5: https://www.drupal.org/node/3442349?utm_source=openai
🏁 Script executed:
# Check the Drupal version this project uses
cat web/composer.json | grep -A 5 '"drupal/core"'Repository: drevops/website
Length of output: 108
🏁 Script executed:
# Check if there's a services.yml that registers the hook class
find web/modules/custom/do_base -name "*.services.yml" -o -name "*services*"Repository: drevops/website
Length of output: 41
🏁 Script executed:
# Find composer.json
find . -maxdepth 3 -name "composer.json" -type fRepository: drevops/website
Length of output: 74
🏁 Script executed:
# Check if do_base has a services.yml or needs one
ls -la web/modules/custom/do_base/Repository: drevops/website
Length of output: 656
🏁 Script executed:
# Check Drupal version
cat composer.json | grep -i drupal/coreRepository: drevops/website
Length of output: 240
🏁 Script executed:
# Check the do_base.module file to see if it registers the hook class
cat web/modules/custom/do_base/do_base.moduleRepository: drevops/website
Length of output: 529
🏁 Script executed:
# Check if there's a services.yml needed or if it's auto-discovered in 11.3
# Verify the src/Hook directory structure
find web/modules/custom/do_base/src -type f -name "*.php"Repository: drevops/website
Length of output: 366
🏁 Script executed:
# Verify all \Drupal:: calls in the PageAttachmentsHook file
rg "\\\\Drupal::" web/modules/custom/do_base/src/Hook/PageAttachmentsHook.phpRepository: drevops/website
Length of output: 282
🏁 Script executed:
# Check if there are any other \Drupal:: calls in lines 28-38
sed -n '28,38p' web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php | cat -nRepository: drevops/website
Length of output: 493
Replace global \Drupal::* calls with constructor dependency injection.
The code uses service locator pattern (\Drupal::service() and \Drupal::config()) instead of constructor DI. In Drupal 11.3.3, hook classes in src/Hook/ are auto-discovered and autowired, so services can be injected directly. This improves testability and aligns with modern Drupal practices.
Additionally, replace the hardcoded /blog/feed href with Url::fromRoute('do_base.blog_rss_feed')->toString() to use the defined route.
🛠️ Suggested refactoring
use Drupal\Core\Hook\Attribute\Hook;
+use Drupal\Core\Config\ConfigFactoryInterface;
+use Drupal\Core\Path\AliasManagerInterface;
+use Drupal\Core\Path\CurrentPathStack;
+use Drupal\Core\Url;
...
final class PageAttachmentsHook {
+ public function __construct(
+ private readonly CurrentPathStack $currentPath,
+ private readonly AliasManagerInterface $aliasManager,
+ private readonly ConfigFactoryInterface $configFactory,
+ ) {}
...
#[Hook('page_attachments')]
public function pageAttachments(array &$attachments): void {
- $current_path = \Drupal::service('path.current')->getPath();
- $alias = \Drupal::service('path_alias.manager')->getAliasByPath($current_path);
+ $currentPath = $this->currentPath->getPath();
+ $alias = $this->aliasManager->getAliasByPath($currentPath);
...
- $site_name = \Drupal::config('system.site')->get('name') ?? 'DrevOps';
+ $siteName = $this->configFactory->get('system.site')->get('name') ?? 'DrevOps';
...
- 'title' => $site_name . ' Blog RSS Feed',
- 'href' => '/blog/feed',
+ 'title' => $siteName . ' Blog RSS Feed',
+ 'href' => Url::fromRoute('do_base.blog_rss_feed')->toString(),🧰 Tools
🪛 PHPMD (2.15.0)
[error] 21-21: The variable $current_path is not named in camelCase. (undefined)
(CamelCaseVariableName)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php` around lines 21
- 22, PageAttachmentsHook uses service-locator calls (\Drupal::service,
\Drupal::config) and a hardcoded "/blog/feed" URL; refactor by adding
constructor injection for the needed services (e.g. PathCurrentInterface or
current_path service, PathAliasManagerInterface, and ConfigFactoryInterface as
required) and store them as private properties, replace calls to
\Drupal::service('path.current')->getPath() and
\Drupal::service('path_alias.manager')->getAliasByPath(...) with calls on those
injected properties inside PageAttachmentsHook::alterAttachments (or the
existing method), and replace the hardcoded href with
Url::fromRoute('do_base.blog_rss_feed')->toString() so the hook uses autowired
services and the route-based RSS URL.
| public function build(): array { | ||
| return [ | ||
| '#type' => 'inline_template', | ||
| '#template' => '<div class="do-blog-rss"><a href="/blog/feed" class="do-blog-rss__link" title="Subscribe to the DrevOps Blog RSS Feed" rel="alternate" type="application/rss+xml"><svg class="do-blog-rss__icon" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 448 512" width="16" height="16" aria-hidden="true" focusable="false"><path d="M0 64C0 46.3 14.3 32 32 32c229.8 0 416 186.2 416 416c0 17.7-14.3 32-32 32s-32-14.3-32-32C384 253.6 226.4 96 32 96C14.3 96 0 81.7 0 64zM0 416a64 64 0 1 1 128 0A64 64 0 1 1 0 416zM32 160c159.1 0 288 128.9 288 288c0 17.7-14.3 32-32 32s-32-14.3-32-32c0-123.7-100.3-224-224-224c-17.7 0-32-14.3-32-32s14.3-32 32-32z"/></svg> RSS Feed</a></div>', | ||
| '#attached' => [ | ||
| 'library' => [ | ||
| 'do_base/blog_rss_feed', | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine its content
cat -n web/modules/custom/do_base/src/Plugin/Block/BlogRssFeedBlock.phpRepository: drevops/website
Length of output: 1783
🏁 Script executed:
# Check for route definitions in the module
find web/modules/custom/do_base -name "*.yml" -o -name "*.yaml" | xargs grep -l "blog_rss_feed" 2>/dev/null || echo "No routes with blog_rss_feed found"Repository: drevops/website
Length of output: 154
🏁 Script executed:
# Check the structure of the module and look for any routing files
find web/modules/custom/do_base -type f \( -name "routing.yml" -o -name "*routing*" \)Repository: drevops/website
Length of output: 105
🏁 Script executed:
# Search for any references to "/blog/feed" in the codebase to understand how this route is defined
rg "/blog/feed" web/modules/custom/do_base/Repository: drevops/website
Length of output: 1076
🏁 Script executed:
# Examine the routing file to see the exact route definition
cat -n web/modules/custom/do_base/do_base.routing.ymlRepository: drevops/website
Length of output: 301
🏁 Script executed:
# Also check the PageAttachmentsHook and BlogRssFeedController to understand the full context
head -50 web/modules/custom/do_base/src/Hook/PageAttachmentsHook.phpRepository: drevops/website
Length of output: 1127
🏁 Script executed:
# Check the BlogRssFeedController
head -80 web/modules/custom/do_base/src/Controller/BlogRssFeedController.phpRepository: drevops/website
Length of output: 2714
🏁 Script executed:
# Quick verification: Check if Url class is already imported or used elsewhere in similar blocks
rg "use Drupal\\Core\\Url" web/modules/custom/do_base/src/Plugin/Block/ || echo "Url not currently imported in blocks"Repository: drevops/website
Length of output: 208
🏁 Script executed:
# Verify this is a consistent pattern issue by checking if other blocks use Url::fromRoute
rg "Url::fromRoute" web/modules/custom/do_base/src/Plugin/Block/ || echo "No Url::fromRoute usage in blocks yet"Repository: drevops/website
Length of output: 96
Replace hardcoded /blog/feed with route-based URL generation.
The path is hardcoded and will break on subdirectory deployments. Use Url::fromRoute('do_base.blog_rss_feed') to generate the URL dynamically, then pass it to the template via context.
🔧 Suggested route-aware update
use Drupal\Core\Block\Attribute\Block;
use Drupal\Core\Block\BlockBase;
use Drupal\Core\StringTranslation\TranslatableMarkup;
+use Drupal\Core\Url;
...
public function build(): array {
+ $feedUrl = Url::fromRoute('do_base.blog_rss_feed')->toString();
return [
'#type' => 'inline_template',
- '#template' => '<div class="do-blog-rss"><a href="/blog/feed" class="do-blog-rss__link" title="Subscribe to the DrevOps Blog RSS Feed" rel="alternate" type="application/rss+xml"><svg class="do-blog-rss__icon" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 448 512" width="16" height="16" aria-hidden="true" focusable="false"><path d="M0 64C0 46.3 14.3 32 32 32c229.8 0 416 186.2 416 416c0 17.7-14.3 32-32 32s-32-14.3-32-32C384 253.6 226.4 96 32 96C14.3 96 0 81.7 0 64zM0 416a64 64 0 1 1 128 0A64 64 0 1 1 0 416zM32 160c159.1 0 288 128.9 288 288c0 17.7-14.3 32-32 32s-32-14.3-32-32c0-123.7-100.3-224-224-224c-17.7 0-32-14.3-32-32s14.3-32 32-32z"/></svg> RSS Feed</a></div>',
+ '#template' => '<div class="do-blog-rss"><a href="{{ feed_url }}" class="do-blog-rss__link" title="Subscribe to the DrevOps Blog RSS Feed" rel="alternate" type="application/rss+xml"><svg class="do-blog-rss__icon" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 448 512" width="16" height="16" aria-hidden="true" focusable="false"><path d="M0 64C0 46.3 14.3 32 32 32c229.8 0 416 186.2 416 416c0 17.7-14.3 32-32 32s-32-14.3-32-32C384 253.6 226.4 96 32 96C14.3 96 0 81.7 0 64zM0 416a64 64 0 1 1 128 0A64 64 0 1 1 0 416zM32 160c159.1 0 288 128.9 288 288c0 17.7-14.3 32-32 32s-32-14.3-32-32c0-123.7-100.3-224-224-224c-17.7 0-32-14.3-32-32s14.3-32 32-32z"/></svg> RSS Feed</a></div>',
+ '#context' => [
+ 'feed_url' => $feedUrl,
+ ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/src/Plugin/Block/BlogRssFeedBlock.php` around
lines 24 - 31, In BlogRssFeedBlock::build replace the hardcoded "/blog/feed"
with a route-generated URL: call
\Drupal\Core\Url::fromRoute('do_base.blog_rss_feed')->toString() in the build()
method, add that string into the render array via '#context' (e.g. 'feed_url' =>
$urlString), and update the inline_template '#template' to use the context
variable (e.g. {{ feed_url }}) for the anchor href so the link is route-aware
and works on subdirectory deployments.
57a43ef to
d69755d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
web/modules/custom/do_base/css/blog-rss-feed.css (2)
14-14: Unresolvedvalue-keyword-caselint issue (currentColor).Stylelint previously flagged this and it is still present on Line 14 and Line 28 (
currentcolorexpected).Also applies to: 28-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/css/blog-rss-feed.css` at line 14, The stylelint error is caused by using the camelCase keyword "currentColor"; update the CSS in blog-rss-feed.css to use the lowercase keyword "currentcolor" wherever it's used (replace occurrences at the selector rules that contain "border: 1px solid currentColor;" and the other rule flagged on Line 28), and run stylelint again to confirm no other instances remain.
12-12: Unresolved contrast issue for RSS link colors.This still appears unresolved from earlier review feedback: the orange/white combinations on Line 12 and Lines 22-23 need WCAG AA verification for 0.875rem text.
Also applies to: 22-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/css/blog-rss-feed.css` at line 12, The RSS link color declaration using the hex value "#e8620e" in blog-rss-feed.css fails WCAG AA for 0.875rem text; update the CSS declarations that set color: `#e8620e` (and any other occurrences in this stylesheet) to a higher-contrast color (choose a darker hex that passes WCAG AA contrast ratio against the current background) or replace with a semantic variable (e.g., --link-color) and set that variable to an accessible value, then verify with a contrast checker and adjust hover/focus states accordingly so the link text meets WCAG AA at 0.875rem.web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php (1)
21-22: Previously raised DI + route-generation concern remains unresolved.The earlier feedback about replacing service-locator calls and hardcoded feed path is still applicable at these lines.
Also applies to: 28-28, 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php` around lines 21 - 22, Replace the direct service-locator calls and hardcoded feed path in PageAttachmentsHook.php by injecting the needed services via the class constructor (e.g., inject the path.current and path_alias.manager services into the PageAttachmentsHook class) and use Drupal's route/URL generation APIs instead of string paths (e.g., Url::fromRoute or routeMatch->getRouteName where appropriate) when building the feed link; update all usages that call \Drupal::service('path.current') and \Drupal::service('path_alias.manager') (and the hardcoded feed path references at the other spots noted) to use the injected $this->pathCurrent and $this->pathAliasManager and generated route URLs. Ensure the service definitions are added/updated so PageAttachmentsHook is constructed with these dependencies.web/modules/custom/do_base/src/Plugin/Block/BlogRssFeedBlock.php (1)
27-27: Unresolved hardcoded feed URL in block markup.The Line 27 hardcoded
/blog/feedconcern was already raised in prior review and still appears unresolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/src/Plugin/Block/BlogRssFeedBlock.php` at line 27, The block currently hardcodes "/blog/feed" inside the '#template' string in BlogRssFeedBlock which repeats the prior review concern; change the markup to build the feed link dynamically using Drupal URL generation (e.g. \Drupal\Core\Url::fromRoute('your.feed.route')->toString() or Url::fromUri('internal:/blog/feed')->toString()) and inject that value into the template (or better, return a render array Link object instead of raw HTML) so the feed path comes from a route/uri generator rather than a hardcoded string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/default/views.view.blog_rss.yml`:
- Line 28: Replace all instances of the empty-map inline braces that have extra
spaces (e.g., the "fields: { }" entries) with the compact form "{}" to satisfy
yamllint braces rules; search for occurrences of "{ }" (including places where
the key name is "fields") and normalize each to "{}".
- Line 12: Replace the placeholder default_config_hash in the
views.view.blog_rss.yml export by re-exporting the blog RSS view from Drupal
(via UI or drush config-export) and copy the exported default_config_hash value
into the file’s default_config_hash field so the YAML contains the actual
Drupal-exported hash instead of the placeholder.
In `@tests/behat/features/blog_rss.feature`:
- Around line 39-42: Update the test scenario "RSS icon block is NOT visible on
non-blog pages" to use the /about-us path instead of /; change the step "When I
go to \"/\"" to "When I go to \"/about-us\"" so the test matches the stated
acceptance criteria and the PR objective validating non-blog visibility on
/about-us.
- Around line 25-31: The scenario "RSS autodiscovery link is present on blog
listing page" currently asserts generic strings and can be fooled by body
content; update the test in tests/behat/features/blog_rss.feature to assert the
actual head link element instead of raw substrings—replace the two "And the
response should contain" checks with a single assertion that the response
contains the exact <link rel="alternate" type="application/rss+xml"
href="/blog/feed" /> (or a regex matching
<head>.*<link[^>]*rel=["']alternate["'][^>]*type=["']application/rss\+xml["'][^>]*href=["']/blog/feed["'][^>]*>.*</head>)
so the test verifies the <link> tag in the head (scenario: RSS autodiscovery
link is present on blog listing page).
In `@web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php`:
- Line 21: Local variable names in PageAttachmentsHook.php use snake_case and
violate PHPMD naming rules; rename occurrences of $current_path to $currentPath
and $site_name to $siteName (and any other snake_case locals noted at lines 28
and 36) and update all references where they are assigned (e.g., the call to
\Drupal::service('path.current')->getPath()) and subsequently used in methods
inside the PageAttachmentsHook class so callers and concatenations use the new
camelCase names.
---
Duplicate comments:
In `@web/modules/custom/do_base/css/blog-rss-feed.css`:
- Line 14: The stylelint error is caused by using the camelCase keyword
"currentColor"; update the CSS in blog-rss-feed.css to use the lowercase keyword
"currentcolor" wherever it's used (replace occurrences at the selector rules
that contain "border: 1px solid currentColor;" and the other rule flagged on
Line 28), and run stylelint again to confirm no other instances remain.
- Line 12: The RSS link color declaration using the hex value "#e8620e" in
blog-rss-feed.css fails WCAG AA for 0.875rem text; update the CSS declarations
that set color: `#e8620e` (and any other occurrences in this stylesheet) to a
higher-contrast color (choose a darker hex that passes WCAG AA contrast ratio
against the current background) or replace with a semantic variable (e.g.,
--link-color) and set that variable to an accessible value, then verify with a
contrast checker and adjust hover/focus states accordingly so the link text
meets WCAG AA at 0.875rem.
In `@web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php`:
- Around line 21-22: Replace the direct service-locator calls and hardcoded feed
path in PageAttachmentsHook.php by injecting the needed services via the class
constructor (e.g., inject the path.current and path_alias.manager services into
the PageAttachmentsHook class) and use Drupal's route/URL generation APIs
instead of string paths (e.g., Url::fromRoute or routeMatch->getRouteName where
appropriate) when building the feed link; update all usages that call
\Drupal::service('path.current') and \Drupal::service('path_alias.manager') (and
the hardcoded feed path references at the other spots noted) to use the injected
$this->pathCurrent and $this->pathAliasManager and generated route URLs. Ensure
the service definitions are added/updated so PageAttachmentsHook is constructed
with these dependencies.
In `@web/modules/custom/do_base/src/Plugin/Block/BlogRssFeedBlock.php`:
- Line 27: The block currently hardcodes "/blog/feed" inside the '#template'
string in BlogRssFeedBlock which repeats the prior review concern; change the
markup to build the feed link dynamically using Drupal URL generation (e.g.
\Drupal\Core\Url::fromRoute('your.feed.route')->toString() or
Url::fromUri('internal:/blog/feed')->toString()) and inject that value into the
template (or better, return a render array Link object instead of raw HTML) so
the feed path comes from a route/uri generator rather than a hardcoded string.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
config/default/block.block.drevops_blog_rss_feed.ymlconfig/default/views.view.blog_rss.ymltests/behat/features/blog_rss.featureweb/modules/custom/do_base/css/blog-rss-feed.cssweb/modules/custom/do_base/do_base.libraries.ymlweb/modules/custom/do_base/src/Hook/PageAttachmentsHook.phpweb/modules/custom/do_base/src/Plugin/Block/BlogRssFeedBlock.php
| - taxonomy | ||
| - user | ||
| _core: | ||
| default_config_hash: blog_rss_feed_view_config_hash_placeholder |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify placeholder hashes in default config
echo "=== Searching for placeholder hashes in config/default ==="
rg -n "default_config_hash: .*placeholder" config/default
# Also check the specific file mentioned
echo ""
echo "=== Content of config/default/views.view.blog_rss.yml around line 12 ==="
if [ -f config/default/views.view.blog_rss.yml ]; then
sed -n '1,30p' config/default/views.view.blog_rss.yml
else
echo "File not found"
fi
# Check if file exists and get its full size
echo ""
echo "=== File info ==="
if [ -f config/default/views.view.blog_rss.yml ]; then
wc -l config/default/views.view.blog_rss.yml
else
echo "File not found"
fiRepository: drevops/website
Length of output: 973
Replace placeholder default_config_hash with the exported hash value.
Line 12 contains a placeholder value instead of an actual Drupal-exported hash. Re-export this view configuration from Drupal to populate the correct hash and ensure proper config-sync integrity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/default/views.view.blog_rss.yml` at line 12, Replace the placeholder
default_config_hash in the views.view.blog_rss.yml export by re-exporting the
blog RSS view from Drupal (via UI or drush config-export) and copy the exported
default_config_hash value into the file’s default_config_hash field so the YAML
contains the actual Drupal-exported hash instead of the placeholder.
| position: 0 | ||
| display_options: | ||
| title: 'Blog RSS Feed' | ||
| fields: { } |
There was a problem hiding this comment.
Fix YAML empty-map brace spacing to satisfy linting.
These lines use { }, which fails yamllint braces rules; use {} consistently.
🧹 Suggested normalization
- fields: { }
+ fields: {}
...
- options: { }
+ options: {}Also applies to: 50-50, 68-68, 91-91, 108-109, 132-132, 150-151, 172-172, 190-191, 201-201, 216-221, 229-229, 238-238, 242-242, 261-261
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 28-28: too many spaces inside empty braces
(braces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/default/views.view.blog_rss.yml` at line 28, Replace all instances of
the empty-map inline braces that have extra spaces (e.g., the "fields: { }"
entries) with the compact form "{}" to satisfy yamllint braces rules; search for
occurrences of "{ }" (including places where the key name is "fields") and
normalize each to "{}".
| */ | ||
| #[Hook('page_attachments')] | ||
| public function pageAttachments(array &$attachments): void { | ||
| $current_path = \Drupal::service('path.current')->getPath(); |
There was a problem hiding this comment.
Use camelCase local variables to satisfy PHPMD naming rules.
$current_path / $site_name should be camelCase ($currentPath, $siteName) for consistency with the configured lint rule.
🧩 Minimal rename update
- $current_path = \Drupal::service('path.current')->getPath();
- $alias = \Drupal::service('path_alias.manager')->getAliasByPath($current_path);
+ $currentPath = \Drupal::service('path.current')->getPath();
+ $alias = \Drupal::service('path_alias.manager')->getAliasByPath($currentPath);
...
- $site_name = \Drupal::config('system.site')->get('name') ?? 'DrevOps';
+ $siteName = \Drupal::config('system.site')->get('name') ?? 'DrevOps';
...
- 'title' => $site_name . ' Blog RSS Feed',
+ 'title' => $siteName . ' Blog RSS Feed',Also applies to: 28-28, 36-36
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 21-21: The variable $current_path is not named in camelCase. (undefined)
(CamelCaseVariableName)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php` at line 21,
Local variable names in PageAttachmentsHook.php use snake_case and violate PHPMD
naming rules; rename occurrences of $current_path to $currentPath and $site_name
to $siteName (and any other snake_case locals noted at lines 28 and 36) and
update all references where they are assigned (e.g., the call to
\Drupal::service('path.current')->getPath()) and subsequently used in methods
inside the PageAttachmentsHook class so callers and concatenations use the new
camelCase names.
d69755d to
4a046e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php (1)
34-34:$topic_idsuses snake_case — same PHPMD naming pattern asPageAttachmentsHook.For consistency, apply camelCase (
$topicIds) here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php` at line 34, Rename the local variable $topic_ids to camelCase $topicIds in PreprocessParagraphHook (where it's created with array_column) and update all subsequent references in that function to use $topicIds to match the naming convention used in PageAttachmentsHook; ensure no other behavior changes or missing references remain after the rename.web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php (2)
41-48: Snake_case local variables still present — previously flagged.
$current_path(line 41) and$site_name(line 48) remain snake_case despite PHPMD violations and prior review feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php` around lines 41 - 48, Local variable names are still snake_case; rename $current_path to $currentPath and $site_name to $siteName throughout the PageAttachmentsHook method where you call $this->currentPathStack->getPath(), $this->aliasManager->getAliasByPath(...), and $this->configFactory->get('system.site')->get('name') to satisfy PHPMD/coding standards; update every reference and any related docblocks or inline comments in the same scope and scan nearby code for other snake_case locals to convert to camelCase.
44-62: 🧹 Nitpick | 🔵 TrivialConsider extracting the hardcoded
/blog/feedpath to a shared constant.Both
PageAttachmentsHook(line 58) andPreprocessParagraphHook(line 46) hardcode the string'/blog/feed'. If the Views feed path ever changes, both locations need manual updates. A class constant (e.g., on a shared module class or interface) would reduce this coupling.Alternatively, use
Url::fromRoute('view.blog_rss.feed_1')->toString()to derive the path from the Views route, though that adds a runtime dependency on the view existing.♻️ Minimal constant extraction example
// In a shared location, e.g., a constants class or the module's .module file: const DO_BASE_BLOG_RSS_FEED_PATH = '/blog/feed';Then in both hooks:
- 'href' => '/blog/feed', + 'href' => DO_BASE_BLOG_RSS_FEED_PATH,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php` around lines 44 - 62, Extract the hardcoded '/blog/feed' into a shared constant and use it in PageAttachmentsHook and PreprocessParagraphHook: add a constant (e.g., DO_BASE_BLOG_RSS_FEED_PATH = '/blog/feed') in a shared class or the module file, then replace the literal '/blog/feed' in PageAttachmentsHook (attachments['#attached'] entry) and the matching string in PreprocessParagraphHook with that constant; alternatively, if you prefer deriving the path from Views, replace the literal with Url::fromRoute('view.blog_rss.feed_1')->toString() where used, ensuring the view route exists before calling it.config/default/views.view.blog_rss.yml (1)
26-26: YAML{ }brace spacing already flagged in a prior review.The double-spaced empty braces (
{ }) throughout this file are Drupal's default config export format. This was noted previously.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/default/views.view.blog_rss.yml` at line 26, The YAML entry "fields: { }" uses double spaces inside the empty braces; update all occurrences (e.g., the "fields" mapping in views.view.blog_rss.yml) to use compact empty mapping syntax "fields: {}" (remove the extra spaces) so the file conforms to standard YAML brace spacing used elsewhere.web/modules/custom/do_base/css/blog-rss-feed.css (1)
7-25: Color contrast concern for#e8620eremains unaddressed.The default-state
#e8620eon white background and the hover/focus-state#fffon#e8620ebackground both fall below the WCAG AA 4.5:1 contrast ratio at0.875remfont size. This was flagged in a previous review and appears to still be present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/css/blog-rss-feed.css` around lines 7 - 25, The .do-blog-rss__link styles use `#e8620e` on white and white on `#e8620e` on hover/focus which both fail WCAG AA 4.5:1 at 0.875rem; update the CSS to use accessible color pairs (either pick a darker orange for the default state or a darker text color on default, and a darker hover background or non-white text on hover) so that .do-blog-rss__link and its :hover/:focus states meet a 4.5:1 contrast ratio; test the chosen colors with a contrast checker and replace `#e8620e/`#fff accordingly in the .do-blog-rss__link and .do-blog-rss__link:hover, .do-blog-rss__link:focus rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/default/views.view.blog_rss.yml`:
- Around line 151-195: The view currently hardcodes taxonomy term id "1" in the
tid filter (plugin_id taxonomy_index_tid, vocabulary civictheme_topics), which
is brittle; either replace that static TID by resolving the correct term id at
runtime (look up the "Blog" term by stable identifier such as its UUID or
machine name and inject its tid into the view/filter or into the code that
builds the feed) — update the code in PreprocessParagraphHook.php to load the
taxonomy term by UUID/machine name and use its tid instead of a literal 1 — or,
if you cannot change filter behaviour, add a clear inline comment in the view
description and in PreprocessParagraphHook.php documenting the coupling and why
TID 1 is assumed so maintainers know to update both places when the term is
re-created.
In `@tests/behat/features/blog_rss.feature`:
- Around line 17-22: The scenario "RSS feed contains blog posts but not services
pages" relies on pre-existing blog content but doesn't create any; update the
feature by either adding explicit setup steps (e.g., a Given step to create one
or more published blog posts such as "Given there is a published blog post
titled '...'" or a Background that seeds published posts) so the Then "response
should contain '<item>'" passes, or, if relying on a seeded test DB, add a clear
comment above the Scenario noting the dependency on seeded blog content and the
required state (published posts present) so future readers know the assumption.
In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php`:
- Line 44: The assignment to $variables['footer'] in PreprocessParagraphHook.php
overwrites any existing footer content; change it to preserve and merge existing
data instead (e.g., check if $variables['footer'] is set/array and merge new
footer pieces into it, or append to it) so you don't discard prior values added
by the paragraph entity or other preprocess hooks; if overwriting is
intentional, add a brief inline comment on
PreprocessParagraphHook::preprocessParagraph (or the relevant method) explaining
that behaviour.
- Around line 44-53: The inline_template assigned to $variables['footer'] in
PreprocessParagraphHook contains a huge single-line SVG; extract that HTML/SVG
into a Twig template (e.g., templates/blog-rss-feed-link.html.twig) and replace
the inline_template entry with a theme/template render array that references the
new Twig template (use '#theme' => 'blog_rss_feed_link' or '#template' =>
'blog-rss-feed-link' and keep '#context' => ['title' => $this->t(...)]) and
preserve the '#attached' => ['library' => ['do_base/blog_rss_feed']]; create the
Twig file with the markup (use {{ title }} and translatable link text) so the
theme layer can override it and the PHP stays clean.
---
Duplicate comments:
In `@config/default/views.view.blog_rss.yml`:
- Line 26: The YAML entry "fields: { }" uses double spaces inside the empty
braces; update all occurrences (e.g., the "fields" mapping in
views.view.blog_rss.yml) to use compact empty mapping syntax "fields: {}"
(remove the extra spaces) so the file conforms to standard YAML brace spacing
used elsewhere.
In `@web/modules/custom/do_base/css/blog-rss-feed.css`:
- Around line 7-25: The .do-blog-rss__link styles use `#e8620e` on white and white
on `#e8620e` on hover/focus which both fail WCAG AA 4.5:1 at 0.875rem; update the
CSS to use accessible color pairs (either pick a darker orange for the default
state or a darker text color on default, and a darker hover background or
non-white text on hover) so that .do-blog-rss__link and its :hover/:focus states
meet a 4.5:1 contrast ratio; test the chosen colors with a contrast checker and
replace `#e8620e/`#fff accordingly in the .do-blog-rss__link and
.do-blog-rss__link:hover, .do-blog-rss__link:focus rules.
In `@web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php`:
- Around line 41-48: Local variable names are still snake_case; rename
$current_path to $currentPath and $site_name to $siteName throughout the
PageAttachmentsHook method where you call $this->currentPathStack->getPath(),
$this->aliasManager->getAliasByPath(...), and
$this->configFactory->get('system.site')->get('name') to satisfy PHPMD/coding
standards; update every reference and any related docblocks or inline comments
in the same scope and scan nearby code for other snake_case locals to convert to
camelCase.
- Around line 44-62: Extract the hardcoded '/blog/feed' into a shared constant
and use it in PageAttachmentsHook and PreprocessParagraphHook: add a constant
(e.g., DO_BASE_BLOG_RSS_FEED_PATH = '/blog/feed') in a shared class or the
module file, then replace the literal '/blog/feed' in PageAttachmentsHook
(attachments['#attached'] entry) and the matching string in
PreprocessParagraphHook with that constant; alternatively, if you prefer
deriving the path from Views, replace the literal with
Url::fromRoute('view.blog_rss.feed_1')->toString() where used, ensuring the view
route exists before calling it.
In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php`:
- Line 34: Rename the local variable $topic_ids to camelCase $topicIds in
PreprocessParagraphHook (where it's created with array_column) and update all
subsequent references in that function to use $topicIds to match the naming
convention used in PageAttachmentsHook; ensure no other behavior changes or
missing references remain after the rename.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
config/default/views.view.blog_rss.ymltests/behat/features/blog_rss.featureweb/modules/custom/do_base/css/blog-rss-feed.cssweb/modules/custom/do_base/do_base.libraries.ymlweb/modules/custom/do_base/src/Hook/PageAttachmentsHook.phpweb/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php
| tid: | ||
| id: tid | ||
| table: taxonomy_index | ||
| field: tid | ||
| relationship: none | ||
| group_type: group | ||
| admin_label: '' | ||
| plugin_id: taxonomy_index_tid | ||
| operator: or | ||
| value: | ||
| 1: 1 | ||
| group: 1 | ||
| exposed: false | ||
| expose: | ||
| operator_id: tid_op | ||
| label: Topics | ||
| description: '' | ||
| use_operator: false | ||
| operator: tid_op | ||
| operator_limit_selection: false | ||
| operator_list: { } | ||
| identifier: tid | ||
| required: false | ||
| remember: false | ||
| multiple: false | ||
| remember_roles: | ||
| authenticated: authenticated | ||
| reduce: false | ||
| is_grouped: false | ||
| group_info: | ||
| label: '' | ||
| description: '' | ||
| identifier: '' | ||
| optional: true | ||
| widget: select | ||
| multiple: false | ||
| remember: false | ||
| default_group: All | ||
| default_group_multiple: { } | ||
| group_items: { } | ||
| reduce_duplicates: false | ||
| vid: civictheme_topics | ||
| type: select | ||
| hierarchy: false | ||
| limit: true |
There was a problem hiding this comment.
Hardcoded taxonomy term ID 1 for the Blog topic is fragile.
The tid filter hardcodes 1: 1 (line 161) to match the "Blog" term in civictheme_topics. If the taxonomy term is ever recreated or the database is re-seeded with a different auto-increment value, the feed silently breaks. The same hardcoded TID appears in PreprocessParagraphHook.php (line 40).
Consider documenting this coupling prominently (e.g., in a README or inline comment in the Views description field) so future maintainers are aware. Alternatively, if Drupal's taxonomy term UUID can be leveraged, that would be more resilient — though Views' taxonomy_index_tid plugin doesn't natively support UUID filtering.
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 171-171: too many spaces inside empty braces
(braces)
[error] 189-189: too many spaces inside empty braces
(braces)
[error] 190-190: too many spaces inside empty braces
(braces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/default/views.view.blog_rss.yml` around lines 151 - 195, The view
currently hardcodes taxonomy term id "1" in the tid filter (plugin_id
taxonomy_index_tid, vocabulary civictheme_topics), which is brittle; either
replace that static TID by resolving the correct term id at runtime (look up the
"Blog" term by stable identifier such as its UUID or machine name and inject its
tid into the view/filter or into the code that builds the feed) — update the
code in PreprocessParagraphHook.php to load the taxonomy term by UUID/machine
name and use its tid instead of a literal 1 — or, if you cannot change filter
behaviour, add a clear inline comment in the view description and in
PreprocessParagraphHook.php documenting the coupling and why TID 1 is assumed so
maintainers know to update both places when the term is re-created.
| @api | ||
| Scenario: RSS feed contains blog posts but not services pages | ||
| Given I am an anonymous user | ||
| When I go to "/blog/feed" | ||
| Then the response should contain "<item>" | ||
| And the response should not contain "/services/" |
There was a problem hiding this comment.
Scenario depends on pre-existing blog content in the test environment.
There are no Given steps to create blog posts, so the <item> assertion (line 21) will fail against a fresh install or any environment without published blog content. If this is intentional (tests run against a seeded database), consider adding a brief comment to the feature file documenting that assumption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/behat/features/blog_rss.feature` around lines 17 - 22, The scenario
"RSS feed contains blog posts but not services pages" relies on pre-existing
blog content but doesn't create any; update the feature by either adding
explicit setup steps (e.g., a Given step to create one or more published blog
posts such as "Given there is a published blog post titled '...'" or a
Background that seeds published posts) so the Then "response should contain
'<item>'" passes, or, if relying on a seeded test DB, add a clear comment above
the Scenario noting the dependency on seeded blog content and the required state
(published posts present) so future readers know the assumption.
| return; | ||
| } | ||
|
|
||
| $variables['footer'] = [ |
There was a problem hiding this comment.
Overwriting $variables['footer'] may discard existing content.
This assignment replaces any prior footer content. If $variables['footer'] could already be populated by the paragraph entity or another preprocess hook, this would silently drop it. If this is intentional for this paragraph type, a brief inline comment would clarify the intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php` at line 44,
The assignment to $variables['footer'] in PreprocessParagraphHook.php overwrites
any existing footer content; change it to preserve and merge existing data
instead (e.g., check if $variables['footer'] is set/array and merge new footer
pieces into it, or append to it) so you don't discard prior values added by the
paragraph entity or other preprocess hooks; if overwriting is intentional, add a
brief inline comment on PreprocessParagraphHook::preprocessParagraph (or the
relevant method) explaining that behaviour.
| $variables['footer'] = [ | ||
| '#type' => 'inline_template', | ||
| '#template' => '<div class="do-blog-rss"><a href="/blog/feed" class="do-blog-rss__link" title="{{ title }}" rel="alternate" type="application/rss+xml"><svg class="do-blog-rss__icon" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 448 512" width="16" height="16" aria-hidden="true" focusable="false"><path d="M0 64C0 46.3 14.3 32 32 32c229.8 0 416 186.2 416 416c0 17.7-14.3 32-32 32s-32-14.3-32-32C384 253.6 226.4 96 32 96C14.3 96 0 81.7 0 64zM0 416a64 64 0 1 1 128 0A64 64 0 1 1 0 416zM32 160c159.1 0 288 128.9 288 288c0 17.7-14.3 32-32 32s-32-14.3-32-32c0-123.7-100.3-224-224-224c-17.7 0-32-14.3-32-32s14.3-32 32-32z"/></svg> RSS Feed</a></div>', | ||
| '#context' => [ | ||
| 'title' => $this->t('Subscribe to the DrevOps Blog RSS Feed'), | ||
| ], | ||
| '#attached' => [ | ||
| 'library' => ['do_base/blog_rss_feed'], | ||
| ], | ||
| ]; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the large inline SVG template to a Twig template file.
Line 46 is an extremely long single line (~600+ characters) containing HTML markup with an embedded SVG path. This is difficult to read, review, and maintain. A dedicated Twig template (e.g., templates/block--blog-rss-feed.html.twig) would be cleaner and allow the theme layer to override it.
♻️ Suggested approach
Create a template file, e.g., web/modules/custom/do_base/templates/blog-rss-feed-link.html.twig:
<div class="do-blog-rss">
<a href="/blog/feed"
class="do-blog-rss__link"
title="{{ title }}"
rel="alternate"
type="application/rss+xml">
<svg class="do-blog-rss__icon"
xmlns="http://www.w3.org/2000/svg"
viewBox="0 0 448 512"
width="16" height="16"
aria-hidden="true"
focusable="false">
<path d="M0 64C0 46.3 ..."/>
</svg>
{{ 'RSS Feed'|t }}
</a>
</div>Then reference it in PHP:
$variables['footer'] = [
- '#type' => 'inline_template',
- '#template' => '<div class="do-blog-rss">...',
+ '#theme' => 'blog_rss_feed_link',
'#context' => [
'title' => $this->t('Subscribe to the DrevOps Blog RSS Feed'),
],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php` around lines
44 - 53, The inline_template assigned to $variables['footer'] in
PreprocessParagraphHook contains a huge single-line SVG; extract that HTML/SVG
into a Twig template (e.g., templates/blog-rss-feed-link.html.twig) and replace
the inline_template entry with a theme/template render array that references the
new Twig template (use '#theme' => 'blog_rss_feed_link' or '#template' =>
'blog-rss-feed-link' and keep '#context' => ['title' => $this->t(...)]) and
preserve the '#attached' => ['library' => ['do_base/blog_rss_feed']]; create the
Twig file with the markup (use {{ title }} and translatable link text) so the
theme layer can override it and the PHP stays clean.
4a046e8 to
b3f0a14
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php (2)
41-48:⚠️ Potential issue | 🟡 MinorUse camelCase locals to keep PHPMD green.
Line 41 (
$current_path) and Line 48 ($site_name) violate the configured camelCase variable rule and will keep static analysis red.Proposed fix
- $current_path = $this->currentPathStack->getPath(); - $alias = $this->aliasManager->getAliasByPath($current_path); + $currentPath = $this->currentPathStack->getPath(); + $alias = $this->aliasManager->getAliasByPath($currentPath); @@ - $site_name = $this->configFactory->get('system.site')->get('name') + $siteName = $this->configFactory->get('system.site')->get('name') ?? 'DrevOps'; @@ - 'title' => $site_name . ' Blog RSS Feed', + 'title' => $siteName . ' Blog RSS Feed',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php` around lines 41 - 48, The locals $current_path and $site_name violate camelCase rules; rename them to $currentPath and $siteName respectively, update the assignments where you call $this->currentPathStack->getPath() and $this->configFactory->get('system.site')->get('name'), and adjust every subsequent usage in the same method (including the alias check using $this->aliasManager->getAliasByPath($currentPath) and any later references) to the new camelCase names so PHPMD passes.
58-58: 🧹 Nitpick | 🔵 TrivialAvoid duplicating the feed URL literal across hooks.
Line 58 hardcodes
/blog/feed, and the same literal exists inweb/modules/custom/do_base/src/Hook/PreprocessParagraphHook.phpLine 42. Extracting a shared constant/config value avoids drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php` at line 58, The feed URL '/blog/feed' is duplicated between PageAttachmentsHook and PreprocessParagraphHook; extract it into a single shared constant (e.g., FEED_PATH) on a common class or config holder used by the module, then replace the hardcoded literal in PageAttachmentsHook and PreprocessParagraphHook with that constant (reference the class name you choose and the constant, e.g., DoBaseConfig::FEED_PATH or DoBaseHooks::FEED_PATH) so both hooks read the value from one location.web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php (1)
38-44:⚠️ Potential issue | 🟡 MinorDirectly overwriting
footercan discard existing render content.Line 38 replaces
$variables['footer']unconditionally. If another preprocess/provider sets footer content first, it gets lost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php` around lines 38 - 44, Don't unconditionally overwrite $variables['footer']; instead detect existing footer renderable and merge or append the RSS render array so other providers' content is preserved. In PreprocessParagraphHook (where $variables['footer'] is set) check if $variables['footer'] already exists and is an array, then add this RSS element as a new keyed child (e.g. 'rss' or append to numeric items) or merge using array_merge_recursive to combine render arrays rather than replacing the entire $variables['footer'] value.config/default/views.view.blog_rss.yml (1)
26-259:⚠️ Potential issue | 🟡 MinorNormalize empty-map braces to pass yamllint.
There are repeated
{ }forms (for example Line 26, Line 48, Line 49, Line 67, and others). These should be{}to satisfy the configured braces rule.Example normalization
- fields: { } + fields: {} @@ - options: { } + options: {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/default/views.view.blog_rss.yml` around lines 26 - 259, Replace all occurrences of the expanded empty-map form "{ }" with the compact "{}" to satisfy yamllint's braces rule; specifically update entries such as fields (under the main display), cache.options, style.options.grouping, display_extenders, grouping under feed_1.display_options.style.options, row.options.relationship/view_mode blocks, query.options.query_tags, relationships, header, footer, display_extenders, and any cache_metadata.tags lines so every "{ }" becomes "{}" consistently throughout the file.tests/behat/features/blog_rss.feature (1)
18-22:⚠️ Potential issue | 🟡 MinorThis scenario depends on seeded blog content.
Line 21 (
<item>) will fail on a clean dataset unless published blog posts are created first. Please add setup steps or explicitly document seeded-data dependency in the feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/behat/features/blog_rss.feature` around lines 18 - 22, The scenario "RSS feed contains blog posts but not services pages" relies on seeded blog posts and will fail on a clean DB; either add explicit setup steps (e.g., a Given step that creates/publishes at least one blog post before visiting "/blog/feed") or mark the scenario with a clear tag/comment such as `@requires-seed` and document the dependency; update the feature to include the new Given step name (e.g., "Given there is a published blog post" or a Background that creates published posts) so the expectation for "<item>" in the response is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php`:
- Line 40: The template string for '#template' hardcodes "RSS Feed"; update the
Twig snippet to make the label translatable by replacing the literal with the
Twig translation filter (e.g. use 'RSS Feed'|t). Ensure the replacement occurs
inside the "#template" assignment in PreprocessParagraphHook (the
include('@atoms/button/button.twig', { ..., text: ... }, false) call) and that
the PHP string quoting is adjusted/escaped as needed so the resulting Twig uses
'RSS Feed'|t.
- Around line 32-35: The variable $topic_ids uses snake_case and violates PHPMD
naming rules; rename it to camelCase (e.g., $topicIds) where it's defined (the
array_column call on $paragraph->field_c_p_list_topics->getValue()) and update
all subsequent references (such as the array_map('intval', ...) and in_array
checks) to use $topicIds so behavior remains identical but the variable name
conforms to linting.
In
`@web/modules/custom/do_base/tests/src/Unit/Hook/PreprocessParagraphHookTest.php`:
- Around line 29-117: Tests use an outdated API: remove calls to nonexistent
setStringTranslation() and rename tests to call the current hook method used by
PreprocessParagraphHook (replace preprocessParagraphCivicthemeAutomatedList()
with the hook method implemented in PreprocessParagraphHook), and stub the
Paragraph::bundle() call to return 'civictheme_automated_list' where needed so
the positive-path logic is exercised; update all test cases (including
testRssIconInjectedForBlogList, testNoRssIconForNonBlogList,
testSkipParagraphWithoutTopicField, testBlogTopicWithMultipleTopics) to mock
bundle(), remove setStringTranslation() usage, and call the actual
PreprocessParagraphHook method name present in the class.
In `@web/themes/custom/drevops/components/00-base/icon/icon.twig`:
- Around line 19-26: When assets_dir is provided but the requested icon file is
missing the current template sets source only from the custom path and never
falls back; update the logic in icon.twig so after setting source from the
custom assets_dir (using set source = source(assets_dir ~ '/icons/' ~ symbol ~
'.svg', true)) you check if source is empty and, if so, attempt the existing
fallbacks (set source = source('@drevops/../assets/icons/' ~ symbol ~ '.svg',
true) and if still empty set source = source('@civictheme/../assets/icons/' ~
symbol ~ '.svg', true)); keep the same true argument and reuse the symbol
variable so missing custom icons correctly fall back to drevops/civictheme.
---
Duplicate comments:
In `@config/default/views.view.blog_rss.yml`:
- Around line 26-259: Replace all occurrences of the expanded empty-map form "{
}" with the compact "{}" to satisfy yamllint's braces rule; specifically update
entries such as fields (under the main display), cache.options,
style.options.grouping, display_extenders, grouping under
feed_1.display_options.style.options, row.options.relationship/view_mode blocks,
query.options.query_tags, relationships, header, footer, display_extenders, and
any cache_metadata.tags lines so every "{ }" becomes "{}" consistently
throughout the file.
In `@tests/behat/features/blog_rss.feature`:
- Around line 18-22: The scenario "RSS feed contains blog posts but not services
pages" relies on seeded blog posts and will fail on a clean DB; either add
explicit setup steps (e.g., a Given step that creates/publishes at least one
blog post before visiting "/blog/feed") or mark the scenario with a clear
tag/comment such as `@requires-seed` and document the dependency; update the
feature to include the new Given step name (e.g., "Given there is a published
blog post" or a Background that creates published posts) so the expectation for
"<item>" in the response is satisfied.
In `@web/modules/custom/do_base/src/Hook/PageAttachmentsHook.php`:
- Around line 41-48: The locals $current_path and $site_name violate camelCase
rules; rename them to $currentPath and $siteName respectively, update the
assignments where you call $this->currentPathStack->getPath() and
$this->configFactory->get('system.site')->get('name'), and adjust every
subsequent usage in the same method (including the alias check using
$this->aliasManager->getAliasByPath($currentPath) and any later references) to
the new camelCase names so PHPMD passes.
- Line 58: The feed URL '/blog/feed' is duplicated between PageAttachmentsHook
and PreprocessParagraphHook; extract it into a single shared constant (e.g.,
FEED_PATH) on a common class or config holder used by the module, then replace
the hardcoded literal in PageAttachmentsHook and PreprocessParagraphHook with
that constant (reference the class name you choose and the constant, e.g.,
DoBaseConfig::FEED_PATH or DoBaseHooks::FEED_PATH) so both hooks read the value
from one location.
In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php`:
- Around line 38-44: Don't unconditionally overwrite $variables['footer'];
instead detect existing footer renderable and merge or append the RSS render
array so other providers' content is preserved. In PreprocessParagraphHook
(where $variables['footer'] is set) check if $variables['footer'] already exists
and is an array, then add this RSS element as a new keyed child (e.g. 'rss' or
append to numeric items) or merge using array_merge_recursive to combine render
arrays rather than replacing the entire $variables['footer'] value.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
web/themes/custom/drevops/assets/icons/rss.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
config/default/views.view.blog_rss.ymlphpstan.neontests/behat/features/blog_rss.featureweb/modules/custom/do_base/src/Hook/PageAttachmentsHook.phpweb/modules/custom/do_base/src/Hook/PreprocessParagraphHook.phpweb/modules/custom/do_base/tests/src/Unit/Hook/PageAttachmentsHookTest.phpweb/modules/custom/do_base/tests/src/Unit/Hook/PreprocessParagraphHookTest.phpweb/themes/custom/drevops/components/00-base/icon/icon.component.ymlweb/themes/custom/drevops/components/00-base/icon/icon.twig
| $topic_ids = array_column($paragraph->field_c_p_list_topics->getValue(), 'target_id'); | ||
|
|
||
| if (!in_array(1, array_map('intval', $topic_ids), TRUE)) { | ||
| return; |
There was a problem hiding this comment.
Rename $topic_ids to camelCase for lint compliance.
Line 32 currently uses snake_case and will trip PHPMD’s variable naming rule.
Proposed fix
- $topic_ids = array_column($paragraph->field_c_p_list_topics->getValue(), 'target_id');
+ $topicIds = array_column($paragraph->field_c_p_list_topics->getValue(), 'target_id');
@@
- if (!in_array(1, array_map('intval', $topic_ids), TRUE)) {
+ if (!in_array(1, array_map('intval', $topicIds), TRUE)) {
return;
}📝 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.
| $topic_ids = array_column($paragraph->field_c_p_list_topics->getValue(), 'target_id'); | |
| if (!in_array(1, array_map('intval', $topic_ids), TRUE)) { | |
| return; | |
| $topicIds = array_column($paragraph->field_c_p_list_topics->getValue(), 'target_id'); | |
| if (!in_array(1, array_map('intval', $topicIds), TRUE)) { | |
| return; |
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 32-32: The variable $topic_ids is not named in camelCase. (undefined)
(CamelCaseVariableName)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php` around lines
32 - 35, The variable $topic_ids uses snake_case and violates PHPMD naming
rules; rename it to camelCase (e.g., $topicIds) where it's defined (the
array_column call on $paragraph->field_c_p_list_topics->getValue()) and update
all subsequent references (such as the array_map('intval', ...) and in_array
checks) to use $topicIds so behavior remains identical but the variable name
conforms to linting.
|
|
||
| $variables['footer'] = [ | ||
| '#type' => 'inline_template', | ||
| '#template' => "{{ include('@atoms/button/button.twig', {kind: 'link', type: 'secondary', icon: 'rss', icon_placement: 'before', text: 'RSS Feed', url: url}, false) }}", |
There was a problem hiding this comment.
Make the RSS label translatable.
Line 40 hardcodes "RSS Feed" in English. Use Twig translation so this remains localizable.
Proposed fix
- '#template' => "{{ include('@atoms/button/button.twig', {kind: 'link', type: 'secondary', icon: 'rss', icon_placement: 'before', text: 'RSS Feed', url: url}, false) }}",
+ '#template' => "{{ include('@atoms/button/button.twig', {kind: 'link', type: 'secondary', icon: 'rss', icon_placement: 'before', text: 'RSS Feed'|t, url: url}, false) }}",📝 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.
| '#template' => "{{ include('@atoms/button/button.twig', {kind: 'link', type: 'secondary', icon: 'rss', icon_placement: 'before', text: 'RSS Feed', url: url}, false) }}", | |
| '#template' => "{{ include('@atoms/button/button.twig', {kind: 'link', type: 'secondary', icon: 'rss', icon_placement: 'before', text: 'RSS Feed'|t, url: url}, false) }}", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php` at line 40,
The template string for '#template' hardcodes "RSS Feed"; update the Twig
snippet to make the label translatable by replacing the literal with the Twig
translation filter (e.g. use 'RSS Feed'|t). Ensure the replacement occurs inside
the "#template" assignment in PreprocessParagraphHook (the
include('@atoms/button/button.twig', { ..., text: ... }, false) call) and that
the PHP string quoting is adjusted/escaped as needed so the resulting Twig uses
'RSS Feed'|t.
web/modules/custom/do_base/tests/src/Unit/Hook/PreprocessParagraphHookTest.php
Outdated
Show resolved
Hide resolved
| {%- if assets_dir is defined and assets_dir is not empty -%} | ||
| {%- set source = source(assets_dir ~ '/icons/' ~ symbol ~ '.svg', true) -%} | ||
| {%- else -%} | ||
| {%- set source = source('@drevops/../assets/icons/' ~ symbol ~ '.svg', true) -%} | ||
| {%- if source is empty -%} | ||
| {%- set source = source('@civictheme/../assets/icons/' ~ symbol ~ '.svg', true) -%} | ||
| {%- endif -%} | ||
| {%- endif -%} |
There was a problem hiding this comment.
assets_dir path misses fallback when the requested icon is absent.
If Line 20 returns empty for a custom assets_dir, rendering stops without trying drevops/civictheme sources.
Proposed fix
{%- if assets_dir is defined and assets_dir is not empty -%}
{%- set source = source(assets_dir ~ '/icons/' ~ symbol ~ '.svg', true) -%}
+ {%- if source is empty -%}
+ {%- set source = source('@drevops/../assets/icons/' ~ symbol ~ '.svg', true) -%}
+ {%- if source is empty -%}
+ {%- set source = source('@civictheme/../assets/icons/' ~ symbol ~ '.svg', true) -%}
+ {%- endif -%}
+ {%- endif -%}
{%- else -%}
{%- set source = source('@drevops/../assets/icons/' ~ symbol ~ '.svg', true) -%}
{%- if source is empty -%}
{%- set source = source('@civictheme/../assets/icons/' ~ symbol ~ '.svg', true) -%}
{%- endif -%}
{%- endif -%}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/themes/custom/drevops/components/00-base/icon/icon.twig` around lines 19
- 26, When assets_dir is provided but the requested icon file is missing the
current template sets source only from the custom path and never falls back;
update the logic in icon.twig so after setting source from the custom assets_dir
(using set source = source(assets_dir ~ '/icons/' ~ symbol ~ '.svg', true)) you
check if source is empty and, if so, attempt the existing fallbacks (set source
= source('@drevops/../assets/icons/' ~ symbol ~ '.svg', true) and if still empty
set source = source('@civictheme/../assets/icons/' ~ symbol ~ '.svg', true));
keep the same true argument and reuse the symbol variable so missing custom
icons correctly fall back to drevops/civictheme.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/modules/custom/do_base/do_base.services.yml`:
- Line 6: The YAML inline map for the service should include spaces inside the
braces to satisfy the 'braces' lint rule; change the inline map that references
twig.extension from "{name: twig.extension}" to "{ name: twig.extension }"
(i.e., add a space after "{" and before "}") so the entry with the name
"twig.extension" passes YAMLlint.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
web/modules/custom/do_base/do_base.services.ymlweb/modules/custom/do_base/src/Twig/IconAssetsExtension.php
| class: Drupal\do_base\Twig\IconAssetsExtension | ||
| arguments: ['@theme.manager'] | ||
| tags: | ||
| - { name: twig.extension } |
There was a problem hiding this comment.
Fix YAML braces spacing to satisfy lint.
Line 6 uses inline map spacing that violates your YAMLlint rule (braces), which can fail CI.
Proposed fix
- - { name: twig.extension }
+ - {name: twig.extension}📝 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.
| - { name: twig.extension } | |
| - {name: twig.extension} |
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 6-6: too many spaces inside braces
(braces)
[error] 6-6: too many spaces inside braces
(braces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/do_base.services.yml` at line 6, The YAML inline
map for the service should include spaces inside the braces to satisfy the
'braces' lint rule; change the inline map that references twig.extension from
"{name: twig.extension}" to "{ name: twig.extension }" (i.e., add a space after
"{" and before "}") so the entry with the name "twig.extension" passes YAMLlint.
|
Fixed the CI failures and CodeRabbit issues:
@coderabbitai review |
|
✅ PHPStan fixes — Good catch on aligning the test with the actual implementation by using ✅ Line length — Extracting the template strings into ✅ Magic property access — Using ✅ Naming conventions — You're absolutely right; ✅ CSS lint — Confirmed that I'll review the updated changes now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php (2)
39-45:⚠️ Potential issue | 🟡 Minor
$variables['footer']is overwritten unconditionally.Any footer content set by the paragraph entity or an earlier preprocess hook is silently discarded. If this is intentional (i.e. this is the only footer content ever expected for
civictheme_automated_listblog paragraphs), a brief inline comment would make that explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php` around lines 39 - 45, The code unconditionally replaces $variables['footer'] in PreprocessParagraphHook.php which discards any footer set earlier; update the logic to preserve existing footer content by checking if $variables['footer'] is already set and either append/merge the RSS button into the existing render array (use array_merge_recursive or combine the '#type' and '#context' appropriately) or only assign when no footer exists, and if the overwrite is intentional add a clear inline comment above the assignment explaining that this paragraph type owns the footer and will replace previous values; reference $variables['footer'] and the RSS button assignment block when making the change.
38-38:⚠️ Potential issue | 🟡 Minor
'RSS Feed'is not translatable.The string is embedded raw inside a Twig template snippet and will never pass through Drupal's translation pipeline.
♻️ Proposed fix
- $rss_button_args = "{kind: 'link', type: 'secondary', icon: 'rss', icon_placement: 'before', text: 'RSS Feed', url: url}"; + $rss_button_args = "{kind: 'link', type: 'secondary', icon: 'rss', icon_placement: 'before', text: 'RSS Feed'|t, url: url}";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php` at line 38, The hardcoded label 'RSS Feed' inside the $rss_button_args string is not translatable; replace the inline JS-like string with a PHP-built structure that passes the label through Drupal's translation system (e.g. use t('RSS Feed') or \Drupal::translation()->translate('RSS Feed') when constructing the button args) and then serialize (json_encode) or render that into the Twig snippet so the translated string is used; update the $rss_button_args assignment in PreprocessParagraphHook::... to build the args array/object with the translated text instead of embedding the raw 'RSS Feed' literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php`:
- Line 34: Replace the magic literal 1 used to identify the Blog taxonomy term
with a named constant: add a class constant (e.g., BLOG_TID or BLOG_TERM_ID) to
PreprocessParagraphHook and use that constant in the in_array check (replace the
literal in the expression using $topic_ids). Also update any tests that assert
against the literal 1 to reference the new constant (import or reference
PreprocessParagraphHook::BLOG_TID) so the identifier is defined in one place and
easily configurable.
In `@web/themes/custom/drevops/components/00-base/icon/icon.component.yml`:
- Around line 9-11: The schema currently declares the property "symbol" but
doesn't mark it required; update icon.component.yml to add a required array that
includes "symbol" so SDC will validate it (e.g., add required: ["symbol"] at the
component root while keeping the existing symbol property as type: string); this
enforces the template expectation (icon.twig reference to symbol) and prevents
empty/misconfigured usages from slipping through.
---
Duplicate comments:
In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php`:
- Around line 39-45: The code unconditionally replaces $variables['footer'] in
PreprocessParagraphHook.php which discards any footer set earlier; update the
logic to preserve existing footer content by checking if $variables['footer'] is
already set and either append/merge the RSS button into the existing render
array (use array_merge_recursive or combine the '#type' and '#context'
appropriately) or only assign when no footer exists, and if the overwrite is
intentional add a clear inline comment above the assignment explaining that this
paragraph type owns the footer and will replace previous values; reference
$variables['footer'] and the RSS button assignment block when making the change.
- Line 38: The hardcoded label 'RSS Feed' inside the $rss_button_args string is
not translatable; replace the inline JS-like string with a PHP-built structure
that passes the label through Drupal's translation system (e.g. use t('RSS
Feed') or \Drupal::translation()->translate('RSS Feed') when constructing the
button args) and then serialize (json_encode) or render that into the Twig
snippet so the translated string is used; update the $rss_button_args assignment
in PreprocessParagraphHook::... to build the args array/object with the
translated text instead of embedding the raw 'RSS Feed' literal.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.phpweb/modules/custom/do_base/tests/src/Unit/Hook/PreprocessParagraphHookTest.phpweb/themes/custom/drevops/components/00-base/icon/icon.component.ymlweb/themes/custom/drevops/components/00-base/icon/icon.twig
|
|
||
| $topic_ids = array_column($paragraph->get('field_c_p_list_topics')->getValue(), 'target_id'); | ||
|
|
||
| if (!in_array(1, array_map(intval(...), $topic_ids), TRUE)) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Define the blog TID as a named constant instead of a magic number.
The literal 1 appears in both the hook and the tests with no explanation. If the Blog taxonomy term ID ever changes or needs to be configurable, every reference must be tracked down manually.
♻️ Proposed fix
+ /**
+ * Blog topic term ID.
+ */
+ private const BLOG_TOPIC_TID = 1;
+
#[Hook('preprocess_paragraph')]
public function preprocessParagraph(array &$variables): void {- if (!in_array(1, array_map(intval(...), $topic_ids), TRUE)) {
+ if (!in_array(self::BLOG_TOPIC_TID, array_map(intval(...), $topic_ids), TRUE)) {📝 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.
| if (!in_array(1, array_map(intval(...), $topic_ids), TRUE)) { | |
| /** | |
| * Blog topic term ID. | |
| */ | |
| private const BLOG_TOPIC_TID = 1; | |
| #[Hook('preprocess_paragraph')] | |
| public function preprocessParagraph(array &$variables): void { | |
| // ... earlier code ... | |
| if (!in_array(self::BLOG_TOPIC_TID, array_map(intval(...), $topic_ids), TRUE)) { | |
| return; | |
| } | |
| // ... rest of method ... | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/src/Hook/PreprocessParagraphHook.php` at line 34,
Replace the magic literal 1 used to identify the Blog taxonomy term with a named
constant: add a class constant (e.g., BLOG_TID or BLOG_TERM_ID) to
PreprocessParagraphHook and use that constant in the in_array check (replace the
literal in the expression using $topic_ids). Also update any tests that assert
against the literal 1 to reference the new constant (import or reference
PreprocessParagraphHook::BLOG_TID) so the identifier is defined in one place and
easily configurable.
| symbol: | ||
| type: string | ||
| title: Symbol |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider marking symbol as required.
The template produces no output when symbol is empty (line 19 of icon.twig). Declaring it required in the schema makes that constraint explicit and enables SDC validation to catch misconfigured usages earlier.
♻️ Proposed fix
symbol:
type: string
title: Symbol
+ required: true📝 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.
| symbol: | |
| type: string | |
| title: Symbol | |
| symbol: | |
| type: string | |
| title: Symbol | |
| required: true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/themes/custom/drevops/components/00-base/icon/icon.component.yml` around
lines 9 - 11, The schema currently declares the property "symbol" but doesn't
mark it required; update icon.component.yml to add a required array that
includes "symbol" so SDC will validate it (e.g., add required: ["symbol"] at the
component root while keeping the existing symbol property as type: string); this
enforces the template expectation (icon.twig reference to symbol) and prevents
empty/misconfigured usages from slipping through.
This comment has been minimized.
This comment has been minimized.
|
AlexSkrypnyk
force-pushed
the
feature/140-blog-rss-feed
branch
from
fbebdf0 to
21dba71
Compare
3 participants
Summary
Implements #140 — RSS feed for the blog.
What was implemented
/blog/feed— filterscivictheme_pagenodes tagged with the Blog topic (TID 1), sorted by created date DESC<link>injected in<head>on/blogand/blog/*pages viaPageAttachmentsHookfooterslot of thecivictheme_automated_listcomponent viaPreprocessParagraphHook— only when Blog topic (TID 1) is presentdrevops:iconSDC component that overridescivictheme:icon— checksdrevops/assets/icons/first, falls back to civictheme assetstests/behat/features/blog_rss.featureHow it works
PreprocessParagraphHook::preprocessParagraph()checks if the paragraph iscivictheme_automated_listwith Blog topic TID 1. If yes, sets$variables['footer']to an inline template rendering@atoms/button/button.twigwithtype: secondary,icon: rss.The
rss.svglives inweb/themes/custom/drevops/assets/icons/(tracked). Thedrevops:iconSDC component (atcomponents/00-base/icon/) overridescivictheme:iconwith a fallback lookup — drevops assets first, then civictheme — so no contrib files need modifying.How to test
Verified working
/blog/feedreturns valid RSS with blog posts only<link rel="alternate">in<head>on/blog<footer class="ct-list__footer">drevops:iconSDC overrideScreenshots
Closes #140
Summary by CodeRabbit
New Features
Tests
Chores