[#610] Added 'StateTrait' with Drupal State API management steps#621
[#610] Added 'StateTrait' with Drupal State API management steps#621AlexSkrypnyk merged 8 commits intomainfrom
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:
WalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant Scenario as Behat Scenario
participant Before as stateBeforeScenario()
participant Steps as Step Methods
participant StateAPI as Drupal::state()
participant After as stateAfterScenario()
Scenario->>Before: start scenario
Note over Before: clear per-scenario registry
Scenario->>Steps: execute step (set / delete / assert / set multiple)
Steps->>StateAPI: has/get/set/delete key(s)
Steps->>Before: on first touch, record original existence/value
Scenario->>Steps: additional steps...
Scenario->>After: end scenario
Note over After: skip revert if scenario tagged to skip
After->>After: iterate registry
After->>StateAPI: restore original values or delete keys
After->>Scenario: cleanup complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Poem
🚥 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)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #621 +/- ##
==========================================
- Coverage 97.47% 97.42% -0.05%
==========================================
Files 42 43 +1
Lines 2928 2992 +64
==========================================
+ Hits 2854 2915 +61
- Misses 74 77 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Drupal/StateTrait.php`:
- Around line 211-214: The json_decode call in StateTrait (the branch that
detects JSON via $trimmed[0] === '{' || $trimmed[0] === '[') is passing TRUE
which forces objects into associative arrays; remove the TRUE parameter so
json_decode($trimmed) returns objects and preserves types, keep the
json_last_error() === JSON_ERROR_NONE check and return $decoded as before to
maintain the normalization contract.
- Around line 134-160: stateAssertHasValue and stateAssertNotExists currently
call \Drupal::state()->get($name) directly which conflates a stored NULL with a
missing key; update both methods to use the same sentinel lookup pattern used in
stateStoreOriginalValue to detect existence explicitly: call
\Drupal::state()->get($name, $sentinel) with a unique sentinel value, check
whether the returned value === $sentinel to detect non-existence, and only then
treat a NULL as a legitimate stored value (normalize expected with
stateNormaliseValue and compare stringified values with stateStringifyValue as
before). Ensure you reference and reuse the sentinel approach so an actual NULL
stored value does not get treated as missing.
In `@tests/behat/features/drupal_state.feature`:
- Line 1: Add a descriptive feature-level tag to the feature that tests
StateTrait so it can be run selectively; update the feature header for "Feature:
Check that StateTrait works" to include a tag such as `@state` or `@drupal_state`
(in addition to the existing `@api/`@trait:Drupal\StateTrait) so test runners can
target this suite independently.
- Around line 16-24: Add two new Behat scenarios to exercise the missing
branches: one where the step "Given the state :name has the value :value" is
invoked with the value "null" (e.g., Given the state
"behat_steps_test.null_value" has the value "null" and Then it should have the
value "null") and one where the value is a JSON object (e.g., Given the state
"behat_steps_test.object" has the value "{\"a\":1}" and Then it should have the
value "{\"a\":1}"). Ensure the scenario titles match the existing pattern
("Assert \"Given the state :name has the value :value\" ...") so they exercise
the same step definitions and trigger the null and JSON object normalization
branches.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65625b09-6cc8-477e-8938-7862c2a093b3
📒 Files selected for processing (5)
README.mdSTEPS.mdsrc/Drupal/StateTrait.phptests/behat/bootstrap/FeatureContext.phptests/behat/features/drupal_state.feature
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/behat/features/drupal_state.feature (1)
22-31:⚠️ Potential issue | 🟡 MinorAdd a happy-path scenario for JSON object normalization.
The feature covers JSON arrays and
null, but it still misses a positive scenario for object input (e.g.,{"a":1}), which is part of the documented normalization behavior.➕ Proposed scenario
`@api` Scenario: Assert "Given the state :name has the value :value" sets a JSON array state value Given the state "behat_steps_test.list" has the value "[1,2,3]" Then the state "behat_steps_test.list" should have the value "[1,2,3]" + `@api` + Scenario: Assert "Given the state :name has the value :value" sets a JSON object state value + Given the state "behat_steps_test.object" has the value "{\"a\":1}" + Then the state "behat_steps_test.object" should have the value "{\"a\":1}" + `@api` Scenario: Assert "Given the state :name has the value :value" sets a null state value Given the state "behat_steps_test.nullable" has the value "null" Then the state "behat_steps_test.nullable" should have the value "null"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/behat/features/drupal_state.feature` around lines 22 - 31, Add a happy-path Behat scenario that asserts JSON object normalization by reusing the existing steps "Given the state :name has the value :value" and "Then the state :name should have the value :value"; create a new Scenario (e.g., description: Assert "Given the state :name has the value :value" sets a JSON object state value) that sets the key "behat_steps_test.object" to the JSON object string "{\"a\":1}" and then asserts the same value, ensuring the feature tests object normalization alongside the existing array and null scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Drupal/StateTrait.php`:
- Around line 57-63: The return in stateAfterScenario prevents clearing the
snapshot cache and lets $this->stateOriginalValues leak into later scenarios;
modify stateAfterScenario (and the similar early-return path referenced around
the other occurrence) to always reset/clear $this->stateOriginalValues (e.g. set
it to an empty array or null) before any early return so cleanup runs even when
the method is skipped, ensuring no leftover snapshot state is reused across
scenarios.
---
Duplicate comments:
In `@tests/behat/features/drupal_state.feature`:
- Around line 22-31: Add a happy-path Behat scenario that asserts JSON object
normalization by reusing the existing steps "Given the state :name has the value
:value" and "Then the state :name should have the value :value"; create a new
Scenario (e.g., description: Assert "Given the state :name has the value :value"
sets a JSON object state value) that sets the key "behat_steps_test.object" to
the JSON object string "{\"a\":1}" and then asserts the same value, ensuring the
feature tests object normalization alongside the existing array and null
scenarios.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 535edb91-d722-4ddc-b494-af09ba8b2f6c
📒 Files selected for processing (2)
src/Drupal/StateTrait.phptests/behat/features/drupal_state.feature
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 `@src/Drupal/StateTrait.php`:
- Around line 22-24: The docblock claims tag-controlled skipping for
stateBeforeScenario/stateAfterScenario but the method stateBeforeScenario
unconditionally resets $this->stateOriginalValues; restore tag-based skipping by
checking for the presence of `@behat-steps-skip`:stateBeforeScenario (and/or the
aggregate `@behat-steps-skip`:StateTrait) in the scenario tags before modifying
$this->stateOriginalValues, or alternatively update the docblock and any
generated step docs to remove the claim that
stateBeforeScenario/stateAfterScenario can be skipped by those tags; locate the
behavior in the stateBeforeScenario() method and the $this->stateOriginalValues
assignment to implement the conditional tag check or to update the docblock text
accordingly.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd63f488-ad49-42b0-8b8a-320a08332800
📒 Files selected for processing (1)
src/Drupal/StateTrait.php
Pull request was closed
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 `@src/Drupal/StateTrait.php`:
- Around line 233-235: The numeric normalization currently uses
str_contains($trimmed, '.') to decide between int and float in
stateNormaliseValue(), which miscasts scientific notation like "1e3"; update the
logic in stateNormaliseValue() so that after is_numeric($trimmed) you use a
regex check (e.g. preg_match('/^[+-]?\d+$/', $trimmed)) to detect pure integers
and cast those to (int), otherwise cast to (float) (this will correctly handle
floats and exponent notation).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b8fc51d-2eff-4107-b9a6-1798383b658d
📒 Files selected for processing (2)
STEPS.mdsrc/Drupal/StateTrait.php
| if (is_numeric($trimmed)) { | ||
| return str_contains($trimmed, '.') ? (float) $trimmed : (int) $trimmed; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PHP 8.x, what does (int) "1e3"return, and what is the recommended way to parse numeric strings that may include exponent notation (e.g.,1e3)?
💡 Result:
-
(int) "1e3"in PHP 8.x returns:1(the cast tointparses only the leading decimal digits and stops at the non-integer part, so the exponent is not applied). [1] -
Recommended way to parse numeric strings that may include exponent notation (like
1e3):- Validate as a numeric/float string, then parse as float, then convert as needed:
$f = filter_var($s, FILTER_VALIDATE_FLOAT); if ($f === false) { /* invalid */ } $i = (int)$f; // "1e3" -> 1000
FILTER_VALIDATE_FLOATis intended for safe float validation+conversion. [3] - Alternatively, for a quick check,
is_numeric($s)accepts scientific notation, then you can cast to float:[2]if (!is_numeric($s)) { /* invalid */ } $i = (int)(float)$s;
- Validate as a numeric/float string, then parse as float, then convert as needed:
Sources: PHP “Numeric strings” manual (scientific notation rules) [1], is_numeric() manual [2], PHP RFC noting FILTER_VALIDATE_INT/FLOAT for validated casts [3].
🏁 Script executed:
# Examine the actual file around the specified lines
cd /tmp/repo 2>/dev/null || cd .
if [ -f "src/Drupal/StateTrait.php" ]; then
echo "=== Lines 225-245 from StateTrait.php ==="
sed -n '225,245p' src/Drupal/StateTrait.php
else
echo "File not found, searching for StateTrait.php..."
find . -name "StateTrait.php" -type f 2>/dev/null | head -5
fiRepository: drevops/behat-steps
Length of output: 567
🏁 Script executed:
# Also check the full method signature and context around lines 233-235
if [ -f "src/Drupal/StateTrait.php" ]; then
echo "=== Full stateNormaliseValue method ==="
rg -A 30 "function stateNormaliseValue" src/Drupal/StateTrait.php
fiRepository: drevops/behat-steps
Length of output: 777
🏁 Script executed:
# Search for any test coverage of numeric normalization
if [ -d "tests" ] || [ -d "test" ]; then
echo "=== Search for tests covering numeric normalization ==="
rg "stateNormaliseValue|1e3|scientific|exponent" tests/ 2>/dev/null || \
rg "stateNormaliseValue|1e3|scientific|exponent" test/ 2>/dev/null || \
echo "No tests found with those patterns"
fiRepository: drevops/behat-steps
Length of output: 372
Handle scientific-notation numbers correctly during normalization.
In stateNormaliseValue(), the expression is_numeric('1e3') returns true, but the check str_contains($trimmed, '.') is false, so the code executes (int) '1e3', which returns 1 instead of 1000. This silently miscasts valid numeric strings with exponent notation.
Use preg_match('/^[+-]?\d+$/', $trimmed) to distinguish pure integers from other numeric formats (floats, exponent notation), routing the latter to (float) conversion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Drupal/StateTrait.php` around lines 233 - 235, The numeric normalization
currently uses str_contains($trimmed, '.') to decide between int and float in
stateNormaliseValue(), which miscasts scientific notation like "1e3"; update the
logic in stateNormaliseValue() so that after is_numeric($trimmed) you use a
regex check (e.g. preg_match('/^[+-]?\d+$/', $trimmed)) to detect pure integers
and cast those to (int), otherwise cast to (float) (this will correctly handle
floats and exponent notation).
…ypes, added feature tag and null scenario.
a4ef9b2 to
1604909
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Drupal/StateTrait.php (1)
233-235:⚠️ Potential issue | 🟡 MinorHandle scientific-notation values in numeric normalization.
is_numeric('1e3')istrue, but the current dot-check routes it to(int)and returns1instead of1000.🔧 Suggested fix
- if (is_numeric($trimmed)) { - return str_contains($trimmed, '.') ? (float) $trimmed : (int) $trimmed; - } + if (is_numeric($trimmed)) { + return preg_match('/^[+-]?\d+$/', $trimmed) === 1 + ? (int) $trimmed + : (float) $trimmed; + }In PHP 8.x, what does `(int) "1e3"` return, and what is the recommended way to distinguish integer-only numeric strings from float/scientific notation strings?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/StateTrait.php` around lines 233 - 235, The numeric normalization currently uses is_numeric($trimmed) then checks for a dot via str_contains to decide int vs float, which misclassifies scientific notation like "1e3"; update the logic in StateTrait (the numeric normalization block) to treat only strings that are pure integer digits as ints and everything else numeric (including decimals and scientific notation) as floats — e.g. replace the str_contains('.') check with a strict integer-only test such as preg_match('/^[+-]?\d+$/', $trimmed) (or ctype_digit on a trimmed/sign-handled string) and return (int)$trimmed only for that case, otherwise return (float)$trimmed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Drupal/StateTrait.php`:
- Around line 233-235: The numeric normalization currently uses
is_numeric($trimmed) then checks for a dot via str_contains to decide int vs
float, which misclassifies scientific notation like "1e3"; update the logic in
StateTrait (the numeric normalization block) to treat only strings that are pure
integer digits as ints and everything else numeric (including decimals and
scientific notation) as floats — e.g. replace the str_contains('.') check with a
strict integer-only test such as preg_match('/^[+-]?\d+$/', $trimmed) (or
ctype_digit on a trimmed/sign-handled string) and return (int)$trimmed only for
that case, otherwise return (float)$trimmed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66ff0c27-2763-45a8-8b5e-37e1a03fc491
📒 Files selected for processing (2)
STEPS.mdsrc/Drupal/StateTrait.php
Summary
Adds a new
Drupal/StateTraitthat exposes set, delete, bulk set, and positive/negative assertion steps for values stored in the Drupal State API (\Drupal::state()).Touched keys are snapshotted on first access via
@BeforeScenarioand reverted (or deleted, if the key did not previously exist) in@AfterScenario, so scenarios can flip feature flags, maintenance mode, last-cron timestamps, and similar runtime values without leaking state between tests.Values captured from step arguments and table cells are normalised through
stateNormaliseValue(): booleans,null, integers, floats, and JSON-encoded arrays/objects are decoded; plain strings are kept as-is. Consumers can override this helper to shape values for their own use cases.Fixes #610.
Acceptance criteria
src/Drupal/StateTrait.php.#[Given(...)]/#[Then(...)]tuple annotations.@BeforeScenariosnapshot +@AfterScenariorevert of touched keys.behat-steps-skip:StateTraittag disables the revert hook.tests/behat/features/drupal_state.feature.@trait:StateTraitnegative tests for missing keys / wrong values.stateNormaliseValue()protected helper for consumer overrides.ahoy update-docsrun to regenerateSTEPS.md.ahoy lintpasses.use StateTrait;toFeatureContext.php.Test plan
ahoy test-bdd tests/behat/features/drupal_state.featureahoy lintahoy update-docsand verifySTEPS.mddiff looks rightOverview
This PR adds a new Drupal StateTrait to manage Drupal State API values during Behat scenarios. It provides steps to set, delete, bulk-set, and assert state keys, snapshots touched keys on first use, and reverts or deletes them after each scenario (with tags to skip revert). Documentation and feature tests (including negative cases) are included.
Changes
New Trait: StateTrait (src/Drupal/StateTrait.php)
Tests
Documentation
Integration
Compliance with CONTRIBUTING.md
I reviewed the repository's step-definition rules in CONTRIBUTING.md. The trait follows the guidelines:
No violations were found. (Critical: none)
Remaining tasks / Checklist items
Public API / Exported Entities