test: add comprehensive coverage for path translator subscribers#3
test: add comprehensive coverage for path translator subscribers#3Decipher wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe pull request enables PHPUnit coverage reporting, makes CORS configuration handling null-safe, resolves translated URLs from route names, and adds unit and kernel tests for service-provider, path-translation, and request-path behavior. ChangesDruxt behavior and validation
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 1.2.x #3 +/- ##
===========================================
+ Coverage 42.66% 95.11% +52.44%
===========================================
Files 5 5
Lines 225 225
===========================================
+ Hits 96 214 +118
+ Misses 129 11 -118 ☔ View full report in Codecov by Harness. 🚀 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: 1
🧹 Nitpick comments (1)
tests/src/Unit/DruxtServiceProviderTest.php (1)
22-35: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for an array missing
enabled.The current non-array test does not catch the original undefined-key case (
['allowedHeaders' => ...]withoutenabled). Add an explicit test to lock in the behavior changed atsrc/DruxtServiceProvider.php:23.Suggested test
+ public function testAlterEnablesCorsWhenEnabledKeyIsMissing(): void { + $container = new ContainerBuilder(); + $container->setParameter('cors.config', ['allowedHeaders' => ['x-custom']]); + + (new DruxtServiceProvider())->alter($container); + + $config = $container->getParameter('cors.config'); + $this->assertTrue($config['enabled']); + $this->assertSame(['x-custom'], $config['allowedHeaders']); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/Unit/DruxtServiceProviderTest.php` around lines 22 - 35, Add a regression test alongside testAlterWithNonArrayCorsConfig that sets cors.config to an array containing allowedHeaders but no enabled key, invokes DruxtServiceProvider::alter, and asserts the resulting configuration supplies the default enabled value while preserving the provided allowedHeaders.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/EventSubscriber/WildcardPathTranslatorSubscriber.php`:
- Around line 55-57: Update the URL reconstruction in
WildcardPathTranslatorSubscriber to extract only matched route path variables
from $match_info and pass them as Url::fromRoute() parameters instead of an
empty array. Preserve existing behavior for query or non-route metadata, and
update the related test expectations for parameterized routes such as /node/1
and /page/123 from 500 to 200.
---
Nitpick comments:
In `@tests/src/Unit/DruxtServiceProviderTest.php`:
- Around line 22-35: Add a regression test alongside
testAlterWithNonArrayCorsConfig that sets cors.config to an array containing
allowedHeaders but no enabled key, invokes DruxtServiceProvider::alter, and
asserts the resulting configuration supplies the default enabled value while
preserving the provided allowedHeaders.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc511178-e2b8-440e-b3c1-4e264f576a4d
📒 Files selected for processing (10)
.gitlab-ci.ymlphpunit.gitlab-ci.xmlsrc/DruxtServiceProvider.phpsrc/EventSubscriber/ContactPathTranslatorSubscriber.phpsrc/EventSubscriber/WildcardPathTranslatorSubscriber.phptests/src/Kernel/ContactPathTranslationKernelTest.phptests/src/Kernel/DruxtRequestPathKernelTest.phptests/src/Kernel/ViewsPathTranslationKernelTest.phptests/src/Kernel/WildcardPathTranslationKernelTest.phptests/src/Unit/DruxtServiceProviderTest.php
| $route_name = $match_info[RouteObjectInterface::ROUTE_NAME]; | ||
| try { | ||
| $resolved_url = Url::fromRoute($route, [], ['absolute' => TRUE])->toString(TRUE); | ||
| $resolved_url = Url::fromRoute($route_name, [], ['absolute' => TRUE])->toString(TRUE); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/EventSubscriber/WildcardPathTranslatorSubscriber.php --items all
sed -n '34,70p' src/EventSubscriber/WildcardPathTranslatorSubscriber.php
sed -n '89,105p' tests/src/Kernel/WildcardPathTranslationKernelTest.php
# Inspect existing route-name URL generation patterns and their parameter inputs.
rg -n -C 3 --glob '*.php' 'Url::fromRoute\(\$route_name' src testsRepository: Decipher/druxt_drupal
Length of output: 4715
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,130p' src/EventSubscriber/WildcardPathTranslatorSubscriber.php
sed -n '1,130p' src/EventSubscriber/ViewsPathTranslatorSubscriber.php
sed -n '120,200p' tests/src/Kernel/WildcardPathTranslationKernelTest.phpRepository: Decipher/druxt_drupal
Length of output: 7700
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,180p' tests/src/Kernel/WildcardPathTranslationKernelTest.php
cat <<'PHP' >/tmp/generate_route_params.php
<?php
$paths = [
'/node/1',
'/page/123',
'/admin/config/system/site-information',
'/user/login',
];
foreach ($paths as $path) {
$routePath = str_replace('}', '', str_replace('{', '', $path));
$tokens = preg_split('/[\/a-zA-Z0-9]+/', $routePath, -1, PREG_SPLIT_NO_EMPTY);
echo "$path\t" . implode(' ', $tokens) . "\n";
}
PHP
php /tmp/generate_route_params.phpRepository: Decipher/druxt_drupal
Length of output: 232
Preserve matched route parameters when rebuilding the URL.
/node/1 and /page/123 now return 500 because the matched parameters such as {node} are dropped before Url::fromRoute() is called. Filter the matched route variables to route path parameters only and pass them to Url::fromRoute() instead of an empty array; then update the related test expectation to 200.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/EventSubscriber/WildcardPathTranslatorSubscriber.php` around lines 55 -
57, Update the URL reconstruction in WildcardPathTranslatorSubscriber to extract
only matched route path variables from $match_info and pass them as
Url::fromRoute() parameters instead of an empty array. Preserve existing
behavior for query or non-route metadata, and update the related test
expectations for parameterized routes such as /node/1 and /page/123 from 500 to
200.
17417a4 to
e91b2a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/src/Kernel/ContactPathTranslationKernelTest.php`:
- Around line 62-76: Update translatePath to handle the subscriber’s
early-return case before calling getResponse(), ensuring unset paths receive an
explicit response by dispatching the controller endpoint or setting/checking the
response. Keep the existing status and JSON data extraction for initialized
responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 707e5f5e-073a-496b-9c8c-80a6df8f110c
📒 Files selected for processing (9)
.gitlab-ci.ymlphpunit.gitlab-ci.xmlsrc/DruxtServiceProvider.phpsrc/EventSubscriber/ContactPathTranslatorSubscriber.phpsrc/EventSubscriber/WildcardPathTranslatorSubscriber.phptests/src/Kernel/ContactPathTranslationKernelTest.phptests/src/Kernel/DruxtRequestPathKernelTest.phptests/src/Kernel/WildcardPathTranslationKernelTest.phptests/src/Unit/DruxtServiceProviderTest.php
🚧 Files skipped from review as they are similar to previous changes (7)
- src/DruxtServiceProvider.php
- src/EventSubscriber/WildcardPathTranslatorSubscriber.php
- src/EventSubscriber/ContactPathTranslatorSubscriber.php
- .gitlab-ci.yml
- tests/src/Unit/DruxtServiceProviderTest.php
- tests/src/Kernel/WildcardPathTranslationKernelTest.php
- tests/src/Kernel/DruxtRequestPathKernelTest.php
| private function translatePath(string $path): array { | ||
| $request = Request::create('/router/translate-path', 'GET'); | ||
| $event = new PathTranslatorEvent( | ||
| $this->container->get('http_kernel'), | ||
| $request, | ||
| HttpKernelInterface::MAIN_REQUEST, | ||
| $path | ||
| ); | ||
| $this->container->get('druxt.contact_path_translator.subscriber')->onPathTranslation($event); | ||
|
|
||
| $response = $event->getResponse(); | ||
| return [ | ||
| 'status' => $response->getStatusCode(), | ||
| 'data' => json_decode((string) $response->getContent(), TRUE), | ||
| ]; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 4 'class PathTranslatorEvent|function getResponse|function setResponse|onPathTranslation' .Repository: Decipher/druxt_drupal
Length of output: 7799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect PathTranslatorEvent class and getResponse implementation without running repository code.
rg -n -C 6 'class PathTranslatorEvent|function .*\bgetResponse\b' .
# Read focused test and subscriber contexts.
for f in $(git ls-files | rg '(^|/)PathTranslatorEvent\.php$|tests/src/Kernel/(Contact|Views|Wildcard|DruxtRequestPath)PathTranslationKernelTest\.php$'); do
echo "===== $f ====="
sed -n '1,220p' "$f"
doneRepository: Decipher/druxt_drupal
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
grep -RInE 'class PathTranslatorEvent|function .*getResponse|getResponse\(' . || true
git ls-files | grep -E '(^|/)PathTranslatorEvent\.php$|tests/src/Kernel/(Contact|Views|Wildcard|DruxtRequestPath)PathTranslationKernelTest\.php$|EventSubscriber/(Contact|Views|Wildcard)PathTranslatorSubscriber\.php$' || trueRepository: Decipher/druxt_drupal
Length of output: 1324
🌐 Web query:
php drupal decoupled router PathTranslatorEvent getResponse
💡 Result:
In the Drupal Decoupled Router module, PathTranslatorEvent is a custom kernel event used to process path translation requests [1][2]. The getResponse method is a member function of this event class that retrieves the response object associated with the path translation process [1]. When a request is made to the path translation endpoint, the PathTranslator controller dispatches a PathTranslatorEvent [1]. Event subscribers, such as RouterPathTranslatorSubscriber or RedirectPathTranslatorSubscriber, are responsible for handling this event and setting the appropriate response (typically a CacheableJsonResponse) onto the event object [1][2][3]. After the event is dispatched, the PathTranslator controller calls $event->getResponse to retrieve this response object [1]. The controller then further modifies or returns this response to the consumer, ensuring that necessary cacheability metadata and headers (such as Content-Type: application/json) are attached [1]. In summary, the workflow is as follows: 1. The controller creates a PathTranslatorEvent and dispatches it [1]. 2. Event subscribers perform logic to resolve the path and call $event->setResponse($response) to attach the results [1]. 3. The controller retrieves the result using $event->getResponse and returns it to the client [1].
Citations:
- 1: https://api.druphelp.com/api/decoupled_router/src!Controller!PathTranslator.php/class/PathTranslator/2.x
- 2: https://www.drupalarchitect.info/articles/decoupled-router
- 3: https://github.com/Realityloop/decoupled_router/blob/8.x-1.x/decoupled_router.services.yml
🌐 Web query:
github drupydrup/decoupled_router PathTranslatorEvent getResponse setResponse
💡 Result:
In the Drupal Decoupled Router module, PathTranslatorEvent is a custom event class used to facilitate the translation of paths [1][2]. It extends Symfony's KernelEvent and is dispatched by the PathTranslator controller [1][2]. The PathTranslatorEvent class provides methods to manage the response during the path translation process, specifically getResponse and setResponse [1]. These methods allow event subscribers (such as those handling routers or redirects) to intervene in the translation process and provide a response if they can successfully resolve the path [1][2]. When the PathTranslator::translate method is called: 1. A new PathTranslatorEvent object is created [1]. 2. The event is dispatched via the event dispatcher [1]. 3. Event subscribers listen for this event and use setResponse to attach a result (typically a cacheable JSON response) if they identify a valid route or redirect for the requested path [1][2]. 4. Finally, the controller retrieves the response using getResponse and returns it to the client [1]. If no subscriber sets a response, the controller continues its logic to return a default or "not found" response [1].
Citations:
- 1: https://api.druphelp.com/api/decoupled_router/src!Controller!PathTranslator.php/class/PathTranslator/2.x
- 2: https://www.drupalarchitect.info/articles/decoupled-router
🌐 Web query:
github Realityloop/decoupled_router PathTranslatorEvent.php
💡 Result:
The file PathTranslatorEvent.php is a component of the Drupal module Decoupled Router (maintained by Realityloop) [1][2]. This file defines the PathTranslatorEvent class, which is a KernelEvent used within the module to manage the process of translating incoming front-end paths into Drupal entity information [3]. Key technical details include: - Functionality: The Decoupled Router module provides an API endpoint (typically /router/translate-path) that resolves URL aliases and redirects to their underlying Drupal entities [3][1]. - Role of the Event: When the PathTranslator controller processes a request, it instantiates a PathTranslatorEvent [4]. This event is then dispatched via the event dispatcher to various subscribers (such as RouterPathTranslatorSubscriber and RedirectPathTranslatorSubscriber) that perform the actual resolution logic [5][3]. - Event Lifecycle: Subscribers listening to this event are responsible for setting the appropriate response (typically including cacheability metadata) on the event object [4][3]. The module is designed for decoupled applications, allowing them to resolve paths without needing to be aware of Drupal's internal routing structure [1][2]. It is currently compatible with Drupal 10 and 11 [6][7].
Citations:
- 1: https://www.drupal.org/project/decoupled_router
- 2: https://www.drupalreleases.com/project/decoupled-router
- 3: https://www.drupalarchitect.info/articles/decoupled-router
- 4: https://api.druphelp.com/api/decoupled_router/src!Controller!PathTranslator.php/class/PathTranslator/2.x
- 5: https://github.com/Realityloop/decoupled_router/blob/8.x-1.x/decoupled_router.services.yml
- 6: https://www.drupal.org/project/decoupled_router/releases/2.0.6
- 7: https://www.drupal.org/project/decoupled_router/releases/2.x-dev
🌐 Web query:
site:github.com/Realityloop/decoupled_router "class PathTranslatorEvent" "getResponse"
💡 Result:
There is no standard or widely documented class named PathTranslatorEvent with a getResponse method in common PHP or Drupal frameworks. The available search results identify event subscribers related to path translation in the decoupled_router module for Drupal [1], but they do not define or reference a class named PathTranslatorEvent [1]. It is possible that this class is part of a custom, non-public, or project-specific codebase.
Citations:
🌐 Web query:
"PathTranslatorEvent" "isPropagationStopped" "getResponse"
💡 Result:
PathTranslatorEvent is a custom event class used in the Decoupled Router module for Drupal to handle path translation requests [1][2]. While it is not a core Symfony event, it follows standard patterns common to Symfony-based architectures [1]. In this context: getResponse: This method is used to retrieve the response object associated with the event [2]. Event subscribers are responsible for setting this response, which may include specific cacheability metadata [1][2]. isPropagationStopped: This is a standard method inherited from the Symfony event dispatcher contract (or the underlying event class) [3][4]. It returns a boolean value indicating whether further event listeners should be triggered for this event [3][4]. Typically, when a subscriber processes the PathTranslatorEvent, it performs the necessary translation and uses a method like setResponse to attach the result [2]. Setting a response in such events often implicitly or explicitly involves stopping propagation to ensure no other listeners attempt to override the generated response [3][5][6]. top_results: [1][2][3][4]
Citations:
- 1: https://www.drupalarchitect.info/articles/decoupled-router
- 2: https://api.druphelp.com/api/decoupled_router/src!Controller!PathTranslator.php/class/PathTranslator/2.x
- 3: https://docs.w3cub.com/symfony~4.1/symfony/component/httpkernel/event/getresponseevent
- 4: https://phpdox.net/demo/Symfony2/classes/Symfony_Component_HttpKernel_Event_FilterResponseEvent.xhtml
- 5: https://api.druphelp.com/api/zircon_profile/vendor%21symfony%21http-kernel%21Event%21GetResponseForControllerResultEvent.php/class/GetResponseForControllerResultEvent/8
- 6: https://www.teichner.biz/api-docs/classes/Symfony-Component-HttpKernel-Event-GetResponseEvent.html
🌐 Web query:
GitHub decoupled_router PathTranslatorEvent setResponse \$response = null
💡 Result:
In the context of the Drupal Decoupled Router module, the PathTranslatorEvent is a custom event class used to handle the translation of path aliases and redirects into entity-related information [1][2]. The event is dispatched by the PathTranslator controller [3]. Event subscribers, such as RouterPathTranslatorSubscriber and RedirectPathTranslatorSubscriber, listen for this event and are responsible for setting the response by calling the setResponse method on the PathTranslatorEvent instance [3][1][4]. Regarding your specific query about "$response = null", the PathTranslatorEvent class typically includes a protected $response property, which is initialized to null by default [5]. This property is designed to be populated by the relevant event subscribers [3]. If a subscriber determines that it can resolve the path, it calls $event->setResponse($response), where $response is a Symfony Response object (often a CacheableJsonResponse) [3]. If no subscriber handles the event, the $response property remains null [5]. In the PathTranslator controller's translate method, the code retrieves the response using $event->getResponse [3]. If it is still null at that point, the code will likely result in an error or unexpected behavior, as it subsequently attempts to add headers and cacheability metadata to that response object [3]. Therefore, checking if the response is null is an important aspect of ensuring the path translation process has succeeded [3]. top_results: [3][1][4][2][5]
Citations:
- 1: https://www.drupalarchitect.info/articles/decoupled-router
- 2: https://www.drupal.org/project/decoupled_router
- 3: https://api.druphelp.com/api/decoupled_router/src!Controller!PathTranslator.php/class/PathTranslator/2.x
- 4: https://github.com/Realityloop/decoupled_router/blob/8.x-1.x/decoupled_router.services.yml
- 5: https://api.druphelp.com/api/zircon_profile/vendor%21symfony%21http-kernel%21Event%21FilterResponseEvent.php/8.0
Handle the early-return response before dereferencing.
PathTranslatorEvent does not initialize a default 404 response; unset paths need an explicit response before callers can safely read status/content. In the short-circuit test, dispatch the controller endpoint or set/check the response explicitly after the subscriber returns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/Kernel/ContactPathTranslationKernelTest.php` around lines 62 - 76,
Update translatePath to handle the subscriber’s early-return case before calling
getResponse(), ensuring unset paths receive an explicit response by dispatching
the controller endpoint or setting/checking the response. Keep the existing
status and JSON data extraction for initialized responses.
Fix D11 TypeError in Contact and Wildcard subscribers (same bug as #3558380 in Views subscriber) by passing route name instead of Route object to Url::fromRoute(). Fix undefined array key warning in DruxtServiceProvider when cors.config parameter is null or missing 'enabled' key. Add kernel/unit tests covering all src/ files (95.1% line coverage): - DruxtServiceProviderTest (unit, CORS defaults) - ContactPathTranslationKernelTest (contact form translation) - WildcardPathTranslationKernelTest (generic route translation) - DruxtRequestPathKernelTest (condition plugin bypass) Enable GitLab CI coverage reporting via Cobertura XML artifacts.
e91b2a9 to
c0f8a4b
Compare
1 participant
Coverage: 0% → 95.1%
Fixes D11 TypeError in Contact + Wildcard subscribers (same bug as #3558380) and undefined array key in DruxtServiceProvider.
Changes
Url::fromRoute()in Contact + Wildcard subscriberscors.config['enabled']in DruxtServiceProviderCoverage breakdown
Remaining 11 uncovered lines are dead code (MethodNotAllowedException catch) and untestable defensive checks.
Summary by CodeRabbit
Bug Fixes
Testing