Conversation
WalkthroughAdds a new Drupal TimeTrait with Behat steps to set/reset mock system time, feature tests, and Drupal 10/11 test fixtures (service, interface, implementation, controller, route) that read/write a Changes
Sequence Diagram(s)sequenceDiagram
participant Behat as "Behat Test"
participant Trait as "TimeTrait (FeatureContext)"
participant State as "Drupal State"
participant TimeSvc as "mysite_core Time Service"
participant Core as "Core Time Service"
Behat->>Trait: "I set system time to :value"
Trait->>State: set `testing.time` = timestamp
State-->>Trait: ack
Behat->>TimeSvc: request current time (via controller/request)
TimeSvc->>State: read `testing.time`
State-->>TimeSvc: return override (numeric)
TimeSvc-->>Behat: return overridden time
Behat->>Trait: "I reset system time"
Trait->>State: delete `testing.time`
State-->>Trait: ack
Behat->>TimeSvc: request current time
TimeSvc->>State: read `testing.time`
State-->>TimeSvc: not found
TimeSvc->>Core: delegate to core time service
Core-->>TimeSvc: real system time
TimeSvc-->>Behat: return real time
Note over Trait,Behat: AfterScenario hook calls timeCleanup unless scenario tagged to skip
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/Drupal/TimeTrait.php`:
- Around line 35-49: Update the `@When` placeholder in the TimeTrait::timeSet
docblock from ":value" to ":timestamp" and change example accordingly; add input
validation in the timeSet(string $value): void method to ensure $value is a
numeric integer timestamp (e.g. ctype_digit or is_numeric with integer cast) and
non-negative, and if validation fails throw an InvalidArgumentException (or
\RuntimeException) instead of silently casting to 0; only after validation cast
to (int) and call \Drupal::state()->set('testing.time', $timestamp).
In `@tests/behat/features/drupal_time.feature`:
- Around line 1-4: Add a descriptive `@time` tag to the feature/scenario to enable
selective test execution: update the Feature header and each Scenario that tests
TimeTrait (e.g., the "Assert system time can be overridden" Scenario and any
other time-related scenarios in this file) to include the `@time` tag so that
behat can run time-related tests selectively.
In
`@tests/behat/fixtures_drupal/d10/web/modules/custom/mysite_core/src/Time/Time.php`:
- Around line 43-50: The micro-time methods getRequestMicroTime() and
getCurrentMicroTime() currently read an integer timestamp from
$this->state->get(self::STATE_KEY) which loses sub-second precision; update the
override handling in class Time so the stored override is a float (microsecond
precision) or explicitly cast/convert the retrieved override to float before
returning, and ensure the state write path (where STATE_KEY is set) stores a
float value; adjust both getRequestMicroTime and getCurrentMicroTime to use the
float override consistently.
| /** | ||
| * Sets the system time for testing. | ||
| * | ||
| * @param string $value | ||
| * The time value as Unix timestamp. | ||
| * | ||
| * @When I set system time to :value | ||
| * | ||
| * @code | ||
| * When I set system time to "1737849900" | ||
| * @endcode | ||
| */ | ||
| public function timeSet(string $value): void { | ||
| \Drupal::state()->set('testing.time', (int) $value); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the current state of the file at the specified lines
head -n 55 src/Drupal/TimeTrait.php | tail -n 25Repository: drevops/behat-steps
Length of output: 285
🏁 Script executed:
# Check for other step definitions using a generic :value placeholder in PHP files
rg -n ':value' src -g '*.php' -B 2 -A 2Repository: drevops/behat-steps
Length of output: 285
🏁 Script executed:
# Check the overall structure of TimeTrait.php to understand context
wc -l src/Drupal/TimeTrait.phpRepository: drevops/behat-steps
Length of output: 285
Use a descriptive placeholder and validate timestamp input.
The placeholder :value is too generic; use :timestamp for clarity as required by coding guidelines. Additionally, casting arbitrary strings to int silently maps invalid input to 0, causing tests to proceed with incorrect time values. Add validation to fail fast.
Suggested change
- * `@param` string $value
- * The time value as Unix timestamp.
+ * `@param` string $timestamp
+ * The time value as Unix timestamp.
@@
- * `@When` I set system time to :value
+ * `@When` I set system time to :timestamp
@@
- public function timeSet(string $value): void {
- \Drupal::state()->set('testing.time', (int) $value);
+ public function timeSet(string $timestamp): void {
+ if (!is_numeric($timestamp)) {
+ throw new \InvalidArgumentException('System time must be a Unix timestamp.');
+ }
+ \Drupal::state()->set('testing.time', (int) $timestamp);
}🤖 Prompt for AI Agents
In `@src/Drupal/TimeTrait.php` around lines 35 - 49, Update the `@When` placeholder
in the TimeTrait::timeSet docblock from ":value" to ":timestamp" and change
example accordingly; add input validation in the timeSet(string $value): void
method to ensure $value is a numeric integer timestamp (e.g. ctype_digit or
is_numeric with integer cast) and non-negative, and if validation fails throw an
InvalidArgumentException (or \RuntimeException) instead of silently casting to
0; only after validation cast to (int) and call
\Drupal::state()->set('testing.time', $timestamp).
| Feature: Ensure TimeTrait works. | ||
|
|
||
| @api | ||
| Scenario: Assert system time can be overridden |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a descriptive tag for selective test execution.
As per coding guidelines, feature files should use descriptive tags to allow selective test execution. Consider adding a @time tag to enable running only time-related tests.
♻️ Proposed fix
Feature: Ensure TimeTrait works.
+ `@time`
`@api`
Scenario: Assert system time can be overriddenApply the same @time tag to the other scenarios as well.
📝 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.
| Feature: Ensure TimeTrait works. | |
| @api | |
| Scenario: Assert system time can be overridden | |
| Feature: Ensure TimeTrait works. | |
| `@time` | |
| `@api` | |
| Scenario: Assert system time can be overridden |
🤖 Prompt for AI Agents
In `@tests/behat/features/drupal_time.feature` around lines 1 - 4, Add a
descriptive `@time` tag to the feature/scenario to enable selective test
execution: update the Feature header and each Scenario that tests TimeTrait
(e.g., the "Assert system time can be overridden" Scenario and any other
time-related scenarios in this file) to include the `@time` tag so that behat can
run time-related tests selectively.
tests/behat/fixtures_drupal/d10/web/modules/custom/mysite_core/src/Time/Time.php
Show resolved
Hide resolved
AlexSkrypnyk
force-pushed
the
feature/time-trait
branch
from
afca69b to
843d561
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@STEPS.md`:
- Around line 3394-3402: Normalize the TimeTrait blockquote formatting: remove
trailing double spaces and normalize blank lines, fix the list indentation so
each list item is on its own properly-indented line (e.g., "- Time service:", "-
Time interface:", "- Service registration:"), and replace the three bare URLs
after those list items with markdown links (e.g., "[Time service](https://...)",
"[Time interface](https://...)", "[Service registration](https://...)") so the
blockquote no longer triggers markdownlint for trailing spaces, indentation, or
bare URLs; update the lines that mention "Time service", "Time interface", and
"Service registration" accordingly.
In
`@tests/behat/fixtures_drupal/d10/web/modules/custom/mysite_core/src/Controller/TestContent.php`:
- Around line 24-27: The container lookup in TestContent::create uses the
concrete service id 'mysite_core.time' instead of the interface alias; update
the factory to fetch the service by its interface (use the TimeInterface alias)
in the create(ContainerInterface $container) method (replace the
container->get('mysite_core.time') call with
container->get(TimeInterface::class)) and add the corresponding use/import so
the TestContent class references the interface alias rather than the hardcoded
service id.
In
`@tests/behat/fixtures_drupal/d10/web/modules/custom/mysite_core/src/Time/TimeInterface.php`:
- Around line 7-12: PHPStan is failing on the fixture interface declaration
because it cannot resolve Drupal\Component\Datetime\TimeInterface (referenced as
CoreTimeInterface) in tests/behat fixtures; either exclude fixture directories
from PHPStan's analyzed paths or add an ignoreErrors pattern to suppress
undefined class/interface errors for these fixtures (e.g., match "Class or
interface Drupal\\Component\\Datetime\\TimeInterface not found" or a broader
"Class or interface Drupal\\.* not found" rule). Update your
phpstan.neon/phpstan.neon.dist accordingly and ensure the rule targets the
fixtures directory so symbols like TimeInterface / CoreTimeInterface in mys
ite_core fixtures are ignored.
In
`@tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/src/Controller/TestContent.php`:
- Around line 24-27: The service lookup in TestContent::create uses the concrete
service id 'mysite_core.time' instead of the interface alias; change the call to
$container->get(TimeInterface::class) (and add the appropriate use/import for
TimeInterface) so the factory resolves via the interface alias; ensure the
constructor parameter type still matches the interface used.
♻️ Duplicate comments (2)
src/Drupal/TimeTrait.php (1)
35-48: Use a descriptive placeholder and validate timestamp input.The placeholder
:valueis too generic and the method silently casts invalid input to0. Use:timestampand validate numeric input before setting state.🐛 Proposed fix
- * `@param` string $value - * The time value as Unix timestamp. + * `@param` string $timestamp + * The time value as Unix timestamp. @@ - * `@When` I set system time to :value + * `@When` I set system time to :timestamp @@ - public function timeSet(string $value): void { - \Drupal::state()->set('testing.time', (int) $value); + public function timeSet(string $timestamp): void { + if (!is_numeric($timestamp)) { + throw new \InvalidArgumentException('System time must be a Unix timestamp.'); + } + \Drupal::state()->set('testing.time', (int) $timestamp); }As per coding guidelines.
tests/behat/features/drupal_time.feature (1)
1-9: Add a descriptive@timetag for selective test execution.As per coding guidelines, feature files should use descriptive tags to allow selective test execution. Consider adding a
@timetag at the Feature level or to each scenario.
| > Control system time in tests using Drupal state overrides. | ||
| > <br/><br/> | ||
| > IMPORTANT: This trait requires your application to use a mockable time | ||
| > service that checks Drupal state for time overrides. | ||
| > <br/><br/> | ||
| > Example implementation: | ||
| > - Time service: https://github.com/drevops/behat-steps/blob/main/tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/src/Time/Time.php | ||
| > - Time interface: https://github.com/drevops/behat-steps/blob/main/tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/src/Time/TimeInterface.php | ||
| > - Service registration: https://github.com/drevops/behat-steps/blob/main/tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/mysite_core.services.yml |
There was a problem hiding this comment.
Fix markdownlint violations in the TimeTrait blockquote.
The blockquote uses double spaces, list indentation, and bare URLs that trip markdownlint. Please normalize the spacing and convert URLs to links.
🧹 Suggested formatting fix
-> Control system time in tests using Drupal state overrides.
-> <br/><br/>
-> IMPORTANT: This trait requires your application to use a mockable time
-> service that checks Drupal state for time overrides.
-> <br/><br/>
-> Example implementation:
-> - Time service: https://github.com/drevops/behat-steps/blob/main/tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/src/Time/Time.php
-> - Time interface: https://github.com/drevops/behat-steps/blob/main/tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/src/Time/TimeInterface.php
-> - Service registration: https://github.com/drevops/behat-steps/blob/main/tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/mysite_core.services.yml
+> Control system time in tests using Drupal state overrides.
+> <br/><br/>
+> IMPORTANT: This trait requires your application to use a mockable time
+> service that checks Drupal state for time overrides.
+> <br/><br/>
+> Example implementation:
+> - [Time service](https://github.com/drevops/behat-steps/blob/main/tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/src/Time/Time.php)
+> - [Time interface](https://github.com/drevops/behat-steps/blob/main/tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/src/Time/TimeInterface.php)
+> - [Service registration](https://github.com/drevops/behat-steps/blob/main/tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/mysite_core.services.yml)🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
3394-3394: Multiple spaces after blockquote symbol
(MD027, no-multiple-space-blockquote)
3395-3395: Multiple spaces after blockquote symbol
(MD027, no-multiple-space-blockquote)
3396-3396: Multiple spaces after blockquote symbol
(MD027, no-multiple-space-blockquote)
3397-3397: Multiple spaces after blockquote symbol
(MD027, no-multiple-space-blockquote)
3398-3398: Multiple spaces after blockquote symbol
(MD027, no-multiple-space-blockquote)
3399-3399: Multiple spaces after blockquote symbol
(MD027, no-multiple-space-blockquote)
3400-3400: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
3400-3400: Multiple spaces after blockquote symbol
(MD027, no-multiple-space-blockquote)
3400-3400: Bare URL used
(MD034, no-bare-urls)
3401-3401: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
3401-3401: Multiple spaces after blockquote symbol
(MD027, no-multiple-space-blockquote)
3401-3401: Bare URL used
(MD034, no-bare-urls)
3402-3402: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
3402-3402: Multiple spaces after blockquote symbol
(MD027, no-multiple-space-blockquote)
3402-3402: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In `@STEPS.md` around lines 3394 - 3402, Normalize the TimeTrait blockquote
formatting: remove trailing double spaces and normalize blank lines, fix the
list indentation so each list item is on its own properly-indented line (e.g.,
"- Time service:", "- Time interface:", "- Service registration:"), and replace
the three bare URLs after those list items with markdown links (e.g., "[Time
service](https://...)", "[Time interface](https://...)", "[Service
registration](https://...)") so the blockquote no longer triggers markdownlint
for trailing spaces, indentation, or bare URLs; update the lines that mention
"Time service", "Time interface", and "Service registration" accordingly.
| public static function create(ContainerInterface $container): self { | ||
| return new self( | ||
| $container->get('mysite_core.time'), | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer interface alias in service lookup (same as d11 fixture).
♻️ Proposed change
- $container->get('mysite_core.time'),
+ $container->get(TimeInterface::class),📝 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.
| public static function create(ContainerInterface $container): self { | |
| return new self( | |
| $container->get('mysite_core.time'), | |
| ); | |
| public static function create(ContainerInterface $container): self { | |
| return new self( | |
| $container->get(TimeInterface::class), | |
| ); |
🧰 Tools
🪛 PHPStan (2.1.33)
26-26: Parameter #1 $time of class Drupal\mysite_core\Controller\TestContent constructor expects Drupal\mysite_core\Time\TimeInterface, object given.
(argument.type)
🤖 Prompt for AI Agents
In
`@tests/behat/fixtures_drupal/d10/web/modules/custom/mysite_core/src/Controller/TestContent.php`
around lines 24 - 27, The container lookup in TestContent::create uses the
concrete service id 'mysite_core.time' instead of the interface alias; update
the factory to fetch the service by its interface (use the TimeInterface alias)
in the create(ContainerInterface $container) method (replace the
container->get('mysite_core.time') call with
container->get(TimeInterface::class)) and add the corresponding use/import so
the TestContent class references the interface alias rather than the hardcoded
service id.
| use Drupal\Component\Datetime\TimeInterface as CoreTimeInterface; | ||
|
|
||
| /** | ||
| * Time service interface with state-based override support. | ||
| */ | ||
| interface TimeInterface extends CoreTimeInterface { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for Drupal core/component deps in composer.json
rg -n '"drupal/(core|core-recommended|component|datetime)"' composer.json || true
# Inspect PHPStan config for autoload/scan/exclude settings
fd -a 'phpstan.*\.neon' -x rg -n 'autoload|scanFiles|scanDirectories|excludePaths|fixtures_drupal|ignoreErrors' {}Repository: drevops/behat-steps
Length of output: 386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# View full phpstan config
echo "=== phpstan.neon content ==="
fd -a 'phpstan.*\.neon' -x cat {} | head -100
echo ""
echo "=== Checking composer.json for Drupal deps ==="
# Extract require and require-dev sections
jq '.require, .["require-dev"]' composer.json 2>/dev/null | head -40
echo ""
echo "=== Check if fixtures are in scanDirectories or excludePaths ==="
fd -a 'phpstan.*\.neon' -x grep -A 5 'scanDirectories:' {}Repository: drevops/behat-steps
Length of output: 3175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for phpstan extension configurations
echo "=== Looking for phpstan extension/drupal configs ==="
fd -a '\.neon' -x grep -l 'drupal\|extension' {} 2>/dev/null || true
echo ""
echo "=== Check if there are other phpstan config files ==="
fd -a 'phpstan.*' -type f
echo ""
echo "=== Check if mglaman/phpstan-drupal is properly configured ==="
cat vendor/mglaman/phpstan-drupal/extension.neon 2>/dev/null | head -50 || echo "File not found or not readable"
echo ""
echo "=== Look for any drupal stubs or autoload ==="
ls -la vendor/mglaman/phpstan-drupal/ 2>/dev/null | head -20 || echo "Directory listing failed"Repository: drevops/behat-steps
Length of output: 390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check vendor structure for phpstan-drupal ==="
ls -la vendor/mglaman/phpstan-drupal/ 2>/dev/null | head -30 || echo "Not found"
echo ""
echo "=== Check all neon files ==="
find . -name "*.neon" -type f 2>/dev/null
echo ""
echo "=== Check if there's an extension.neon in phpstan-drupal ==="
cat vendor/mglaman/phpstan-drupal/extension.neon 2>/dev/null | head -30 || echo "Not readable"
echo ""
echo "=== Look at the fixture file to see what it imports ==="
cat tests/behat/fixtures_drupal/d10/web/modules/custom/mysite_core/src/Time/TimeInterface.phpRepository: drevops/behat-steps
Length of output: 2982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if there are similar TimeInterface definitions ==="
find tests/behat/fixtures_drupal -name "TimeInterface.php" -o -name "*TimeInterface*"
echo ""
echo "=== Check if any ignoreErrors patterns match Drupal Component interfaces ==="
grep -A 50 'ignoreErrors:' phpstan.neon | grep -i 'drupal\|component\|interface'
echo ""
echo "=== Check the drupal-autoloader to see what it loads ==="
cat vendor/mglaman/phpstan-drupal/drupal-autoloader.php | head -40Repository: drevops/behat-steps
Length of output: 1331
Add PHPStan ignoreErrors pattern or exclude fixtures from analysis.
Fixtures are included in PHPStan's paths for analysis, but without drupal/core installed, PHPStan will not resolve Drupal\Component\Datetime\TimeInterface and will report it as unknown. The current ignoreErrors rule for fixtures only suppresses specific patterns (iterable types, WebDriver methods) and does not cover undefined classes. Either add an ignoreErrors pattern to suppress undefined Drupal interfaces in fixtures, or exclude fixture directories from analysis.
🧰 Tools
🪛 PHPStan (2.1.33)
12-12: Interface Drupal\mysite_core\Time\TimeInterface extends unknown interface Drupal\Component\Datetime\TimeInterface.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(interface.notFound)
🤖 Prompt for AI Agents
In
`@tests/behat/fixtures_drupal/d10/web/modules/custom/mysite_core/src/Time/TimeInterface.php`
around lines 7 - 12, PHPStan is failing on the fixture interface declaration
because it cannot resolve Drupal\Component\Datetime\TimeInterface (referenced as
CoreTimeInterface) in tests/behat fixtures; either exclude fixture directories
from PHPStan's analyzed paths or add an ignoreErrors pattern to suppress
undefined class/interface errors for these fixtures (e.g., match "Class or
interface Drupal\\Component\\Datetime\\TimeInterface not found" or a broader
"Class or interface Drupal\\.* not found" rule). Update your
phpstan.neon/phpstan.neon.dist accordingly and ensure the rule targets the
fixtures directory so symbols like TimeInterface / CoreTimeInterface in mys
ite_core fixtures are ignored.
| public static function create(ContainerInterface $container): self { | ||
| return new self( | ||
| $container->get('mysite_core.time'), | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer interface alias in service lookup.
Using the interface alias keeps the factory aligned with service wiring and is easier to refactor later.
♻️ Proposed change
- $container->get('mysite_core.time'),
+ $container->get(TimeInterface::class),🧰 Tools
🪛 PHPStan (2.1.33)
26-26: Parameter #1 $time of class Drupal\mysite_core\Controller\TestContent constructor expects Drupal\mysite_core\Time\TimeInterface, object given.
(argument.type)
🤖 Prompt for AI Agents
In
`@tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/src/Controller/TestContent.php`
around lines 24 - 27, The service lookup in TestContent::create uses the
concrete service id 'mysite_core.time' instead of the interface alias; change
the call to $container->get(TimeInterface::class) (and add the appropriate
use/import for TimeInterface) so the factory resolves via the interface alias;
ensure the constructor parameter type still matches the interface used.
843d561 to
c77b288
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/src/Time/Time.php`:
- Around line 32-74: Extract the repeated override-checking logic into a private
helper (e.g., a method like fetchOverride()) that reads
$this->state->get(self::STATE_KEY) and returns the raw override or null/false if
not numeric; then update getRequestTime, getRequestMicroTime, getCurrentTime,
and getCurrentMicroTime to call fetchOverride() and only cast/return the
override when numeric (casting to int or float as appropriate) otherwise
delegating to $this->coreTime->...; this centralizes the is_numeric check and
state access while preserving existing behavior.
♻️ Duplicate comments (4)
STEPS.md (1)
3390-3431: TimeTrait documentation is comprehensive; formatting issues previously flagged.The documentation content is complete with source/example links and clear step definitions for
@When I set system time to :valueand@When I reset system time.The markdownlint violations (double spaces after
>, list indentation, bare URLs) in the blockquote section (lines 3394-3402) were already identified in a previous review.tests/behat/features/drupal_time.feature (1)
1-23: Add descriptive@timetag for selective test execution.Per coding guidelines, feature files should use descriptive tags to allow selective test execution. Consider adding a
@timetag at the feature level.♻️ Proposed fix
Feature: Ensure TimeTrait works. + `@time` `@api` Scenario: Assert system time can be overriddentests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/src/Controller/TestContent.php (1)
24-27: Prefer using the interface alias for service lookup.This keeps the factory aligned with the service alias and easier to refactor later.
♻️ Suggested change
- $container->get('mysite_core.time'), + $container->get(TimeInterface::class),src/Drupal/TimeTrait.php (1)
35-49: Use descriptive placeholder:timestampand validate input.Per coding guidelines, use descriptive placeholder names. The generic
:valueshould be:timestamp. Additionally, casting arbitrary strings tointsilently maps invalid input to0, causing tests to proceed with incorrect time values.♻️ Proposed fix
/** * Sets the system time for testing. * - * `@param` string $value + * `@param` string $timestamp * The time value as Unix timestamp. * - * `@When` I set system time to :value + * `@When` I set system time to :timestamp * * `@code` * When I set system time to "1737849900" * `@endcode` */ - public function timeSet(string $value): void { - \Drupal::state()->set('testing.time', (int) $value); + public function timeSet(string $timestamp): void { + if (!is_numeric($timestamp)) { + throw new \InvalidArgumentException('System time must be a numeric Unix timestamp.'); + } + \Drupal::state()->set('testing.time', (int) $timestamp); }
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function getRequestTime(): int { | ||
| $override = $this->state->get(self::STATE_KEY); | ||
| if (is_numeric($override)) { | ||
| return (int) $override; | ||
| } | ||
| return $this->coreTime->getRequestTime(); | ||
| } | ||
|
|
||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function getRequestMicroTime(): float { | ||
| $override = $this->state->get(self::STATE_KEY); | ||
| if (is_numeric($override)) { | ||
| return (float) $override; | ||
| } | ||
| return $this->coreTime->getRequestMicroTime(); | ||
| } | ||
|
|
||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function getCurrentTime(): int { | ||
| $override = $this->state->get(self::STATE_KEY); | ||
| if (is_numeric($override)) { | ||
| return (int) $override; | ||
| } | ||
| return $this->coreTime->getCurrentTime(); | ||
| } | ||
|
|
||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function getCurrentMicroTime(): float { | ||
| $override = $this->state->get(self::STATE_KEY); | ||
| if (is_numeric($override)) { | ||
| return (float) $override; | ||
| } | ||
| return $this->coreTime->getCurrentMicroTime(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting repeated override logic.
All four methods (getRequestTime, getRequestMicroTime, getCurrentTime, getCurrentMicroTime) repeat the same override-checking pattern. While acceptable for test fixture clarity, you could extract a helper method if this pattern needs to evolve.
♻️ Optional refactor
+ /**
+ * Gets the override value if set.
+ *
+ * `@return` int|float|null
+ * The override value or NULL if not set.
+ */
+ protected function getOverride(): int|float|null {
+ $override = $this->state->get(self::STATE_KEY);
+ return is_numeric($override) ? $override : NULL;
+ }
+
/**
* {`@inheritdoc`}
*/
public function getRequestTime(): int {
- $override = $this->state->get(self::STATE_KEY);
- if (is_numeric($override)) {
- return (int) $override;
- }
- return $this->coreTime->getRequestTime();
+ return ($o = $this->getOverride()) !== NULL ? (int) $o : $this->coreTime->getRequestTime();
}🧰 Tools
🪛 PHPStan (2.1.33)
36-36: Call to method get() on an unknown class Drupal\Core\State\StateInterface.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
40-40: Call to method getRequestTime() on an unknown class Drupal\Component\Datetime\TimeInterface.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
47-47: Call to method get() on an unknown class Drupal\Core\State\StateInterface.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
51-51: Call to method getRequestMicroTime() on an unknown class Drupal\Component\Datetime\TimeInterface.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
58-58: Call to method get() on an unknown class Drupal\Core\State\StateInterface.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
62-62: Call to method getCurrentTime() on an unknown class Drupal\Component\Datetime\TimeInterface.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
69-69: Call to method get() on an unknown class Drupal\Core\State\StateInterface.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
73-73: Call to method getCurrentMicroTime() on an unknown class Drupal\Component\Datetime\TimeInterface.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
🤖 Prompt for AI Agents
In
`@tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/src/Time/Time.php`
around lines 32 - 74, Extract the repeated override-checking logic into a
private helper (e.g., a method like fetchOverride()) that reads
$this->state->get(self::STATE_KEY) and returns the raw override or null/false if
not numeric; then update getRequestTime, getRequestMicroTime, getCurrentTime,
and getCurrentMicroTime to call fetchOverride() and only cast/return the
override when numeric (casting to int or float as appropriate) otherwise
delegating to $this->coreTime->...; this centralizes the is_numeric check and
state access while preserving existing behavior.
c77b288 to
d9f8cbe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Drupal/TimeTrait.php`:
- Line 32: Extract the repeated literal 'testing.time' into a trait constant on
TimeTrait so it matches the Time service's STATE_KEY; add a constant (e.g.
STATE_KEY) to TimeTrait and replace all direct uses of 'testing.time' (calls
like \Drupal::state()->delete('testing.time'), get(), set(), etc.) with
self::STATE_KEY to ensure consistency with Time::STATE_KEY and avoid typos.
♻️ Duplicate comments (5)
STEPS.md (1)
3394-3402: Fix markdownlint violations in the TimeTrait blockquote.The blockquote uses double spaces, improper list indentation, and bare URLs that trip markdownlint. Normalize spacing and convert bare URLs to markdown links.
tests/behat/features/drupal_time.feature (1)
1-4: Add a@timetag for selective test execution.As per coding guidelines, feature files should use descriptive tags to allow selective test execution. Add a
@timetag to enable running only time-related tests.tests/behat/fixtures_drupal/d10/web/modules/custom/mysite_core/src/Time/TimeInterface.php (1)
7-14: PHPStan error is a false positive for test fixtures.The interface correctly extends
Drupal\Component\Datetime\TimeInterface. PHPStan reports the base interface as unknown because Drupal core isn't available during static analysis of test fixtures. This was addressed in previous review comments regarding PHPStan configuration for fixtures.src/Drupal/TimeTrait.php (1)
35-49: Use a descriptive placeholder name and add input validation.Per coding guidelines, use descriptive placeholder names in Behat step definitions. The placeholder
:valueshould be:timestampfor clarity. Additionally, casting arbitrary strings tointsilently converts invalid input to0, which could cause tests to pass with incorrect time values.tests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/src/Controller/TestContent.php (1)
15-28: Consider using the interface alias for service lookup.Using
TimeInterface::classinstead of the service ID string aligns with the interface alias defined in services.yml and is easier to refactor. However, the current approach using'mysite_core.time'is consistent with the D10 fixture and works correctly.
| return; | ||
| } | ||
|
|
||
| \Drupal::state()->delete('testing.time'); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the state key to a constant.
The state key 'testing.time' is used in three places within this trait and must match the STATE_KEY constant in the Time service implementation. Extracting it to a trait constant improves maintainability and reduces the risk of typos.
♻️ Suggested refactor
trait TimeTrait {
+ /**
+ * State key for time override, must match Time service implementation.
+ */
+ protected const TIME_STATE_KEY = 'testing.time';
+
/**
* Cleans up testing.time state after each scenario.
*/
#[AfterScenario]
public function timeCleanup(AfterScenarioScope $scope): void {
if ($scope->getScenario()->hasTag('behat-steps-skip:' . __FUNCTION__)) {
return;
}
- \Drupal::state()->delete('testing.time');
+ \Drupal::state()->delete(self::TIME_STATE_KEY);
}
// ... in timeSet():
- \Drupal::state()->set('testing.time', (int) $value);
+ \Drupal::state()->set(self::TIME_STATE_KEY, (int) $value);
// ... in timeReset():
- \Drupal::state()->delete('testing.time');
+ \Drupal::state()->delete(self::TIME_STATE_KEY);Also applies to: 48-48, 61-61
🤖 Prompt for AI Agents
In `@src/Drupal/TimeTrait.php` at line 32, Extract the repeated literal
'testing.time' into a trait constant on TimeTrait so it matches the Time
service's STATE_KEY; add a constant (e.g. STATE_KEY) to TimeTrait and replace
all direct uses of 'testing.time' (calls like
\Drupal::state()->delete('testing.time'), get(), set(), etc.) with
self::STATE_KEY to ensure consistency with Time::STATE_KEY and avoid typos.
d9f8cbe to
7f521cf
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #524 +/- ##
===========================================
- Coverage 100.00% 99.95% -0.05%
===========================================
Files 34 35 +1
Lines 2351 2356 +5
===========================================
+ Hits 2351 2355 +4
- Misses 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.