Switched to PHP attributes.#543
Merged
AlexSkrypnyk merged 1 commit intomainfrom Mar 18, 2026
Merged
Conversation
WalkthroughMigrates Behat step annotations from PHPDoc tags to PHP 8 attributes across many trait files; adds attribute-driven step extraction to docs.php and tests; updates config (Rector, PHPStan, composer, .ahoy.yml) and fixture-copy commands; minor test and fixture adjustments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 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 `@src/Drupal/DraggableviewsTrait.php`:
- Around line 29-30: The Behat step placeholder name is inconsistent with the
method parameter: update the step definition on function
draggableViewsSaveBundleOrder so the placeholder and argument match (either
change the attribute from ':views_display_id' to ':view_display_id' or rename
the method argument $view_display_id to $views_display_id); ensure both the
#[When(...)] pattern and the function signature use the same descriptive
placeholder name (preferably ':view_display_id' to match $view_display_id) so
documentation and step intent are consistent.
In `@src/Drupal/UserTrait.php`:
- Around line 371-372: Update the Given annotation for userCreateRole so it
follows prerequisite wording conventions: replace "the role :role_name with the
permissions :permissions" with wording that includes exists/have (for example
"the role :role_name exists with the permissions :permissions" or "a role
:role_name exists with permissions :permissions") so the Given step reads as a
precondition; leave the method name userCreateRole and its parameter list
unchanged.
In `@src/ElementTrait.php`:
- Around line 224-225: Update the Given step annotations to use repository
prerequisite-style wording (use "exists"/"have" patterns) instead of imperative
"Given I ..." — specifically change the annotation string on the method
elementAcceptConfirmation (currently #[Given('I accept all confirmation
dialogs')]) and the adjacent Given step later in the file to equivalent
prerequisite phrases per guidelines; leave the method names and logic intact,
only modify the annotation text to match the "Given ... exists/have" convention.
In `@src/FieldTrait.php`:
- Around line 345-347: The step annotations currently list tokens as ":option,
:selector" but the corresponding methods (e.g. fieldAssertSelectOptionExists)
declare parameters in the opposite order (and some use $value instead of
$option), causing mismatched bindings; fix by aligning parameter order and names
with the annotation tokens: change the method signatures for
fieldAssertSelectOptionExists and the other select-option step methods to accept
(string $option, string $selector) (rename any $value params to $option) so the
first token binds to $option and the second to $selector, or alternatively swap
the tokens in the #[Then(...)] attributes to match the existing parameter order
— pick one approach and apply it consistently to all select-option step methods
in this file.
In `@src/FileDownloadTrait.php`:
- Line 194: Replace the generic placeholder `:name` in the Behat step attributes
with a descriptive placeholder (e.g., `:expected_file_name`) in
FileDownloadTrait.php for both occurrences (the attribute on the method
annotated #[Then('the downloaded file name should be :name')] and the similar
attribute at line 212); update the corresponding method signatures/parameter
names to match the new placeholder (for example rename `$name` to
`$expectedFileName` in the methods that follow those attributes) and adjust any
internal references and docblocks to use the new, descriptive variable name so
the step definitions and generated docs are clear.
- Around line 228-230: Docstrings/Behat step texts in FileDownloadTrait (look
for the Behat step annotations and `@code` examples inside the FileDownloadTrait
docblock strings) use plain tables without the required phrase "the following";
update each occurrence (the table examples around the current table blocks near
the step descriptions and matching `@code` examples) to prepend "the following" so
the steps read e.g. "the following" before the fenced/table content, ensuring
all instances mentioned (tables near lines ~233, ~254-256, ~259, ~288-290, ~293)
are changed consistently in the step text and `@code` examples within
FileDownloadTrait.
In `@tests/behat/bootstrap/FeatureContextTrait.php`:
- Line 199: The throw expression uses RuntimeException without a namespace;
update the throw in FeatureContextTrait (the throw new
RuntimeException(sprintf('Module "%s" does not exist.', $name)); line) to
reference the global exception class by either adding a leading backslash
(\RuntimeException) or adding a use RuntimeException; import at the top of the
file so static analysis no longer flags the missing namespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd74aa9d-7650-44a3-9428-cd21556368f7
📒 Files selected for processing (46)
.ahoy.ymlSTEPS.mdcomposer.jsondocs.phpphpstan.neonrector.phpsrc/CookieTrait.phpsrc/DateTrait.phpsrc/Drupal/BlockTrait.phpsrc/Drupal/ContentBlockTrait.phpsrc/Drupal/ContentTrait.phpsrc/Drupal/DraggableviewsTrait.phpsrc/Drupal/EckTrait.phpsrc/Drupal/EmailTrait.phpsrc/Drupal/FileTrait.phpsrc/Drupal/MediaTrait.phpsrc/Drupal/MenuTrait.phpsrc/Drupal/ModuleTrait.phpsrc/Drupal/ParagraphsTrait.phpsrc/Drupal/SearchApiTrait.phpsrc/Drupal/TaxonomyTrait.phpsrc/Drupal/TimeTrait.phpsrc/Drupal/UserTrait.phpsrc/ElementTrait.phpsrc/FieldTrait.phpsrc/FileDownloadTrait.phpsrc/KeyboardTrait.phpsrc/LinkTrait.phpsrc/MetatagTrait.phpsrc/PathTrait.phpsrc/ResponseTrait.phpsrc/ResponsiveTrait.phpsrc/WaitTrait.phpsrc/XmlTrait.phptests/behat/bootstrap/BehatCliTrait.phptests/behat/bootstrap/FeatureContextTrait.phptests/behat/fixtures_drupal/d10/web/modules/custom/mysite_core/mysite_core.moduletests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/mysite_core.moduletests/phpunit/fixtures/docs/EmptyCommentTrait.phptests/phpunit/fixtures/docs/ExcludedTrait.phptests/phpunit/fixtures/docs/FirstTrait.phptests/phpunit/fixtures/docs/IncludedTrait.phptests/phpunit/fixtures/docs/MultiMethodTrait.phptests/phpunit/fixtures/docs/SampleTrait.phptests/phpunit/fixtures/docs/SecondTrait.phptests/phpunit/src/DocsTest.php
Comment on lines
+371
to
372
| #[Given('the role :role_name with the permissions :permissions')] | ||
| public function userCreateRole(string $role_name, string $permissions): void { |
There was a problem hiding this comment.
Align this Given step with prerequisite wording conventions.
Line 371 currently defines a prerequisite step without explicit exists/have wording, which makes it inconsistent with the repository’s Given style.
Suggested wording adjustment
- #[Given('the role :role_name with the permissions :permissions')]
+ #[Given('the role :role_name has the permissions :permissions')]As per coding guidelines "Given steps should define test prerequisites using words like 'exists' or 'have', avoiding 'should', 'should not', and 'Given I'."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[Given('the role :role_name with the permissions :permissions')] | |
| public function userCreateRole(string $role_name, string $permissions): void { | |
| #[Given('the role :role_name has the permissions :permissions')] | |
| public function userCreateRole(string $role_name, string $permissions): void { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Drupal/UserTrait.php` around lines 371 - 372, Update the Given annotation
for userCreateRole so it follows prerequisite wording conventions: replace "the
role :role_name with the permissions :permissions" with wording that includes
exists/have (for example "the role :role_name exists with the permissions
:permissions" or "a role :role_name exists with permissions :permissions") so
the Given step reads as a precondition; leave the method name userCreateRole and
its parameter list unchanged.
Comment on lines
+224
to
225
| #[Given('I accept all confirmation dialogs')] | ||
| public function elementAcceptConfirmation(): void { |
There was a problem hiding this comment.
Align Given step wording with repository convention.
Line 224 and Line 239 use imperative Given I ... phrasing; the repo guideline requires prerequisite-style Given wording (exists / have patterns).
Suggested step-phrase update
- #[Given('I accept all confirmation dialogs')]
+ #[Given('confirmation dialogs have automatic acceptance enabled')]
public function elementAcceptConfirmation(): void {
- #[Given('I do not accept any confirmation dialogs')]
+ #[Given('confirmation dialogs have automatic acceptance disabled')]
public function elementDeclineConfirmation(): void {As per coding guidelines: "Given steps should define test prerequisites using words like 'exists' or 'have', avoiding 'should', 'should not', and 'Given I'".
Also applies to: 239-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ElementTrait.php` around lines 224 - 225, Update the Given step
annotations to use repository prerequisite-style wording (use "exists"/"have"
patterns) instead of imperative "Given I ..." — specifically change the
annotation string on the method elementAcceptConfirmation (currently #[Given('I
accept all confirmation dialogs')]) and the adjacent Given step later in the
file to equivalent prerequisite phrases per guidelines; leave the method names
and logic intact, only modify the annotation text to match the "Given ...
exists/have" convention.
Comment on lines
+345
to
347
| #[Then('the option :option should exist within the select element :selector')] | ||
| public function fieldAssertSelectOptionExists(string $selector, string $option): void { | ||
| $select_element = $this->getSession()->getPage()->findField($selector); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import re
from pathlib import Path
text = Path("src/FieldTrait.php").read_text()
rx = re.compile(
r"#\[(?:Given|When|Then)\('([^']+)'\)\]\s+public function\s+([A-Za-z0-9_]+)\(([^)]*)\)",
re.MULTILINE
)
for pattern, method, args in rx.findall(text):
tokens = re.findall(r":([A-Za-z_][A-Za-z0-9_]*)", pattern)
params = []
for arg in args.split(","):
m = re.search(r"\$([A-Za-z_][A-Za-z0-9_]*)", arg)
if m:
params.append(m.group(1))
if tokens and tokens != params[:len(tokens)]:
print(f"{method}: tokens={tokens} params={params}")
PYRepository: drevops/behat-steps
Length of output: 434
Fix token-to-parameter mismatches in select-option steps.
Behat step tokens must align with method parameters by position. Lines 345, 365, 385, and 416 have mismatches: tokens are defined as :option, :selector but method parameters are in different order or have wrong names ($value instead of $option). This causes incorrect argument binding at runtime.
Fix: align tokens with parameters
- public function fieldAssertSelectOptionExists(string $selector, string $option): void {
+ public function fieldAssertSelectOptionExists(string $option, string $selector): void {
- public function fieldAssertSelectOptionNotExists(string $selector, string $option): void {
+ public function fieldAssertSelectOptionNotExists(string $option, string $selector): void {
- public function fieldAssertSelectOptionSelected(string $value, string $selector): void {
+ public function fieldAssertSelectOptionSelected(string $option, string $selector): void {
...
- $value,
+ $option,
...
- throw new ExpectationException(sprintf('The option "%s" was not selected on the page %s.', $value, $path), $this->getSession()->getDriver());
+ throw new ExpectationException(sprintf('The option "%s" was not selected on the page %s.', $option, $path), $this->getSession()->getDriver());
- public function fieldAssertSelectOptionNotSelected(string $value, string $selector): void {
+ public function fieldAssertSelectOptionNotSelected(string $option, string $selector): void {
...
- $option_field = $select_field->find('named', ['option', $value]);
+ $option_field = $select_field->find('named', ['option', $option]);
...
- throw new ExpectationException(sprintf('The option "%s" was not found in the select "%s" on the page %s.', $value, $selector, $path), $this->getSession()->getDriver());
+ throw new ExpectationException(sprintf('The option "%s" was not found in the select "%s" on the page %s.', $option, $selector, $path), $this->getSession()->getDriver());
...
- throw new ExpectationException(sprintf('The option "%s" was selected in the select "%s" on the page %s, but should not be.', $value, $selector, $path), $this->getSession()->getDriver());
+ throw new ExpectationException(sprintf('The option "%s" was selected in the select "%s" on the page %s, but should not be.', $option, $selector, $path), $this->getSession()->getDriver());🧰 Tools
🪛 PHPMD (2.15.0)
[error] 347-347: The variable $select_element 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 `@src/FieldTrait.php` around lines 345 - 347, The step annotations currently
list tokens as ":option, :selector" but the corresponding methods (e.g.
fieldAssertSelectOptionExists) declare parameters in the opposite order (and
some use $value instead of $option), causing mismatched bindings; fix by
aligning parameter order and names with the annotation tokens: change the method
signatures for fieldAssertSelectOptionExists and the other select-option step
methods to accept (string $option, string $selector) (rename any $value params
to $option) so the first token binds to $option and the second to $selector, or
alternatively swap the tokens in the #[Then(...)] attributes to match the
existing parameter order — pick one approach and apply it consistently to all
select-option step methods in this file.
| * | ||
| * @Then the downloaded file name should be :name | ||
| */ | ||
| #[Then('the downloaded file name should be :name')] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use descriptive placeholders instead of generic :name.
Line 194 and Line 212 use :name, which is too generic for generated docs and step readability. Prefer explicit placeholders (e.g., :expected_file_name, :file_name_part).
Proposed refactor
- #[Then('the downloaded file name should be :name')]
- public function fileDownloadAssertFileName(string $name): void {
+ #[Then('the downloaded file name should be :expected_file_name')]
+ public function fileDownloadAssertFileName(string $expected_file_name): void {
@@
- if ($name != $this->fileDownloadDownloadedFileInfo['file_name']) {
- throw new ExpectationException(sprintf('Downloaded file "%s", but expected "%s".', $this->fileDownloadDownloadedFileInfo['file_name'], $name), $this->getSession()->getDriver());
+ if ($expected_file_name != $this->fileDownloadDownloadedFileInfo['file_name']) {
+ throw new ExpectationException(sprintf('Downloaded file "%s", but expected "%s".', $this->fileDownloadDownloadedFileInfo['file_name'], $expected_file_name), $this->getSession()->getDriver());
}
}
@@
- #[Then('the downloaded file name should contain :name')]
- public function fileDownloadAssertFileNameContains(string $name): void {
+ #[Then('the downloaded file name should contain :file_name_part')]
+ public function fileDownloadAssertFileNameContains(string $file_name_part): void {
@@
- if (!str_contains((string) $this->fileDownloadDownloadedFileInfo['file_name'], $name)) {
- throw new ExpectationException(sprintf('Downloaded file name "%s" does not contain "%s".', $this->fileDownloadDownloadedFileInfo['file_name'], $name), $this->getSession()->getDriver());
+ if (!str_contains((string) $this->fileDownloadDownloadedFileInfo['file_name'], $file_name_part)) {
+ throw new ExpectationException(sprintf('Downloaded file name "%s" does not contain "%s".', $this->fileDownloadDownloadedFileInfo['file_name'], $file_name_part), $this->getSession()->getDriver());
}
}As per coding guidelines: Use descriptive placeholder names in Behat step definitions.
Also applies to: 212-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FileDownloadTrait.php` at line 194, Replace the generic placeholder
`:name` in the Behat step attributes with a descriptive placeholder (e.g.,
`:expected_file_name`) in FileDownloadTrait.php for both occurrences (the
attribute on the method annotated #[Then('the downloaded file name should be
:name')] and the similar attribute at line 212); update the corresponding method
signatures/parameter names to match the new placeholder (for example rename
`$name` to `$expectedFileName` in the methods that follow those attributes) and
adjust any internal references and docblocks to use the new, descriptive
variable name so the step definitions and generated docs are clear.
Comment on lines
+228
to
+230
| * | document.pdf | | ||
| * | image.jpg | | ||
| * | data.csv | |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Table-based steps should use the following wording.
Line 233, Line 259, and Line 293 (and matching @code examples) define tabled content without the following. This deviates from the project step wording convention.
Proposed wording update
- * Then the downloaded file should be a zip archive containing the files named:
+ * Then the downloaded file should be a zip archive containing the following files named:
@@
- #[Then('the downloaded file should be a zip archive containing the files named:')]
+ #[Then('the downloaded file should be a zip archive containing the following files named:')]
@@
- * Then the downloaded file should be a zip archive containing the files partially named:
+ * Then the downloaded file should be a zip archive containing the following files partially named:
@@
- #[Then('the downloaded file should be a zip archive containing the files partially named:')]
+ #[Then('the downloaded file should be a zip archive containing the following files partially named:')]
@@
- * Then the downloaded file should be a zip archive not containing the files partially named:
+ * Then the downloaded file should be a zip archive not containing the following files partially named:
@@
- #[Then('the downloaded file should be a zip archive not containing the files partially named:')]
+ #[Then('the downloaded file should be a zip archive not containing the following files partially named:')]As per coding guidelines: Use the following for tabled content in Behat steps.
Also applies to: 233-233, 254-256, 259-259, 288-290, 293-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FileDownloadTrait.php` around lines 228 - 230, Docstrings/Behat step
texts in FileDownloadTrait (look for the Behat step annotations and `@code`
examples inside the FileDownloadTrait docblock strings) use plain tables without
the required phrase "the following"; update each occurrence (the table examples
around the current table blocks near the step descriptions and matching `@code`
examples) to prepend "the following" so the steps read e.g. "the following"
before the fenced/table content, ensuring all instances mentioned (tables near
lines ~233, ~254-256, ~259, ~288-290, ~293) are changed consistently in the step
text and `@code` examples within FileDownloadTrait.
| $module_handler = \Drupal::service('module_handler'); | ||
| if (!$module_handler->moduleExists($name)) { | ||
| throw new \RuntimeException(sprintf('Module "%s" does not exist.', $name)); | ||
| throw new RuntimeException(sprintf('Module "%s" does not exist.', $name)); |
There was a problem hiding this comment.
Missing namespace prefix on RuntimeException.
The exception class is missing the leading backslash for the global namespace. Static analysis correctly flagged this as a missing import.
🐛 Proposed fix
- throw new RuntimeException(sprintf('Module "%s" does not exist.', $name));
+ throw new \RuntimeException(sprintf('Module "%s" does not exist.', $name));🧰 Tools
🪛 PHPMD (2.15.0)
[error] 199-199: Missing class import via use statement (line '199', column '17'). (undefined)
(MissingImport)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/behat/bootstrap/FeatureContextTrait.php` at line 199, The throw
expression uses RuntimeException without a namespace; update the throw in
FeatureContextTrait (the throw new RuntimeException(sprintf('Module "%s" does
not exist.', $name)); line) to reference the global exception class by either
adding a leading backslash (\RuntimeException) or adding a use RuntimeException;
import at the top of the file so static analysis no longer flags the missing
namespace.
AlexSkrypnyk
force-pushed
the
feature/switch-to-attributes
branch
from
f69de1f to
4a26212
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #543 +/- ##
=======================================
Coverage 99.83% 99.83%
=======================================
Files 35 35
Lines 2365 2385 +20
=======================================
+ Hits 2361 2381 +20
Misses 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Drupal/EmailTrait.php (1)
190-205:⚠️ Potential issue | 🟠 Major
$exactmode currently performs substring checks instead of exact checks.The exact-assertion paths still use
str_contains, so steps likeshould exactly be/should not beare not enforcing equality.🐛 Proposed fix for exact-comparison behavior
public function emailAssertMessageHeaderContains(string $header, PyStringNode $string, bool $exact = FALSE): void { $string_value = (string) $string; $string_value = $exact ? $string_value : $this->helperNormalizeWhitespace($string_value); foreach ($this->emailGetCollectedMessages() as $message) { $header_value = $message['headers'][$header] ?? ''; $header_value = $exact ? $header_value : $this->helperNormalizeWhitespace((string) $header_value); - if (str_contains((string) $header_value, (string) $string_value)) { + if ($exact ? ((string) $header_value === (string) $string_value) : str_contains((string) $header_value, (string) $string_value)) { return; } }public function emailAssertMessageFieldNotContains(string $field, PyStringNode $string, bool $exact = FALSE): void { @@ $string = strval($string); $string = $exact ? $string : $this->helperNormalizeWhitespace($string); foreach ($this->emailGetCollectedMessages() as $message) { - $value = $message[$field] ?? ''; - $field_string = $exact ? $value : $this->helperNormalizeWhitespace((string) $value); + $value = (string) ($message[$field] ?? ''); + $field_string = $exact ? $value : $this->helperNormalizeWhitespace($value); - if (str_contains((string) $field_string, (string) $string)) { + if ($exact ? ((string) $field_string === (string) $string) : str_contains((string) $field_string, (string) $string)) { throw new ExpectationException(sprintf('Found an email where the field "%s" contains%s text "%s" retrieved from test email collector, but it should not.', $field, ($exact ? ' exact' : ''), $string), $this->getSession()->getDriver()); } } }protected function emailFindMessage(string $field, PyStringNode $string, bool $exact = FALSE): ?array { @@ $string = (string) $string; - $string = $this->helperNormalizeWhitespace($string); + $string = $exact ? $string : $this->helperNormalizeWhitespace($string); foreach ($this->emailGetCollectedMessages() as $message) { - $field_string = $message[$field] ?? ''; - $field_string = $this->helperNormalizeWhitespace((string) $field_string); + $field_string = (string) ($message[$field] ?? ''); + $field_string = $exact ? $field_string : $this->helperNormalizeWhitespace($field_string); - if (str_contains((string) $field_string, (string) $string)) { + if ($exact ? ((string) $field_string === (string) $string) : str_contains((string) $field_string, (string) $string)) { return $message; } }Also applies to: 324-375, 686-700
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/EmailTrait.php` around lines 190 - 205, The exact-mode branch in emailAssertMessageHeaderContains is incorrectly using str_contains for exact equality; change the exact path to compare header_value and string_value with strict equality (===) instead of str_contains, and for the non-exact path keep the current normalized substring check; ensure the thrown ExpectationException message uses the original $string_value (or cast $string) consistently; apply the same change pattern to the other analogous assertion methods referenced (the other header/body assertion methods in the file).
♻️ Duplicate comments (9)
src/ElementTrait.php (1)
224-224:⚠️ Potential issue | 🟡 MinorRephrase Given steps to prerequisite-style wording.
Line 224 and Line 239 still use imperative
Given I ...phrasing, which conflicts with the repository’s Given-step convention.✏️ Suggested attribute wording
- #[Given('I accept all confirmation dialogs')] + #[Given('confirmation dialogs have automatic acceptance enabled')] public function elementAcceptConfirmation(): void { - #[Given('I do not accept any confirmation dialogs')] + #[Given('confirmation dialogs have automatic acceptance disabled')] public function elementDeclineConfirmation(): void {As per coding guidelines: "Given steps should define test prerequisites using words like 'exists' or 'have', avoiding 'should', 'should not', and 'Given I'."
Also applies to: 239-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ElementTrait.php` at line 224, Update the step attribute strings to use prerequisite-style wording instead of imperative "Given I ..." — for example change the attribute #[Given('I accept all confirmation dialogs')] to something like #[Given('all confirmation dialogs are accepted')] (and likewise update the other occurrence at the attribute currently containing 'I ...' on line 239) so they follow the repository convention of "Given" steps expressing state/prerequisites; ensure any matching method names or step definitions (the methods annotated by these attributes) keep their behavior but the attribute text is replaced accordingly.src/FileDownloadTrait.php (2)
194-213: 🛠️ Refactor suggestion | 🟠 MajorReplace generic
:nameplaceholders with descriptive names.The step placeholders are still too generic for generated docs and step readability.
Proposed fix
- #[Then('the downloaded file name should be :name')] - public function fileDownloadAssertFileName(string $name): void { + #[Then('the downloaded file name should be :expected_file_name')] + public function fileDownloadAssertFileName(string $expected_file_name): void { @@ - if ($name != $this->fileDownloadDownloadedFileInfo['file_name']) { - throw new ExpectationException(sprintf('Downloaded file "%s", but expected "%s".', $this->fileDownloadDownloadedFileInfo['file_name'], $name), $this->getSession()->getDriver()); + if ($expected_file_name != $this->fileDownloadDownloadedFileInfo['file_name']) { + throw new ExpectationException(sprintf('Downloaded file "%s", but expected "%s".', $this->fileDownloadDownloadedFileInfo['file_name'], $expected_file_name), $this->getSession()->getDriver()); } } @@ - #[Then('the downloaded file name should contain :name')] - public function fileDownloadAssertFileNameContains(string $name): void { + #[Then('the downloaded file name should contain :file_name_part')] + public function fileDownloadAssertFileNameContains(string $file_name_part): void { @@ - if (!str_contains((string) $this->fileDownloadDownloadedFileInfo['file_name'], $name)) { - throw new ExpectationException(sprintf('Downloaded file name "%s" does not contain "%s".', $this->fileDownloadDownloadedFileInfo['file_name'], $name), $this->getSession()->getDriver()); + if (!str_contains((string) $this->fileDownloadDownloadedFileInfo['file_name'], $file_name_part)) { + throw new ExpectationException(sprintf('Downloaded file name "%s" does not contain "%s".', $this->fileDownloadDownloadedFileInfo['file_name'], $file_name_part), $this->getSession()->getDriver()); } }As per coding guidelines: Use descriptive placeholder names in Behat step definitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FileDownloadTrait.php` around lines 194 - 213, The step definitions use a generic placeholder ":name" which reduces readability; update the attribute strings on the methods fileDownloadAssertFileName and fileDownloadAssertFileNameContains to use descriptive placeholders (for example change #[Then('the downloaded file name should be :name')] to #[Then('the downloaded file name should be :expectedFileName')] and #[Then('the downloaded file name should contain :name')] to #[Then('the downloaded file name should contain :expectedSubstring')]), keeping the method signatures the same so existing parameters still match.
227-233: 🛠️ Refactor suggestion | 🟠 MajorUse
the followingwording for table-based ZIP assertions.The table-driven steps and examples still omit the required
the followingphrase.Proposed fix
- * Then the downloaded file should be a zip archive containing the files named: + * Then the downloaded file should be a zip archive containing the following files named: @@ - #[Then('the downloaded file should be a zip archive containing the files named:')] + #[Then('the downloaded file should be a zip archive containing the following files named:')] @@ - * Then the downloaded file should be a zip archive containing the files partially named: + * Then the downloaded file should be a zip archive containing the following files partially named: @@ - #[Then('the downloaded file should be a zip archive containing the files partially named:')] + #[Then('the downloaded file should be a zip archive containing the following files partially named:')] @@ - * Then the downloaded file should be a zip archive not containing the files partially named: + * Then the downloaded file should be a zip archive not containing the following files partially named: @@ - #[Then('the downloaded file should be a zip archive not containing the files partially named:')] + #[Then('the downloaded file should be a zip archive not containing the following files partially named:')]As per coding guidelines: Use
the followingfor tabled content in Behat steps.Also applies to: 253-260, 287-294
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FileDownloadTrait.php` around lines 227 - 233, The step attribute strings in FileDownloadTrait.php use "containing the files named:" and similar table-driven wording; update each Behat #[Then(...)] attribute to include "the following" for table-based assertions (e.g. change "the downloaded file should be a zip archive containing the files named:" to "the downloaded file should be a zip archive containing the following files:"), and make the equivalent change for the other two occurrences noted (the attributes around lines 253-260 and 287-294) so all table-based step definitions follow the "the following" phrasing.src/FieldTrait.php (4)
345-356:⚠️ Potential issue | 🟠 MajorFix token-to-parameter order mismatch.
The step tokens
:option, :selectordon't match the parameter order$selector, $option. Behat binds tokens to parameters by position, so:optionwould incorrectly bind to$selector.Suggested fix - swap parameter order to match tokens
- public function fieldAssertSelectOptionExists(string $selector, string $option): void { + public function fieldAssertSelectOptionExists(string $option, string $selector): void { $select_element = $this->getSession()->getPage()->findField($selector);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FieldTrait.php` around lines 345 - 356, The step token order is mismatched with the method parameters in fieldAssertSelectOptionExists; change the method signature to accept (string $option, string $selector) so Behat binds :option to $option and :selector to $selector, leaving the rest of the function body (findField and option lookup using $selector and $option) unchanged.
385-407:⚠️ Potential issue | 🟠 MajorFix token name and parameter order mismatch.
The token
:optiondoesn't match parameter name$value, and the parameter order doesn't align with token order.Suggested fix
- public function fieldAssertSelectOptionSelected(string $value, string $selector): void { + public function fieldAssertSelectOptionSelected(string $option, string $selector): void { $select_field = $this->getSession()->getPage()->findField($selector); ... $option_field = $select_field->find('named', [ 'option', - $value, + $option, ]); ... - throw new ExpectationException(sprintf('The option "%s" was not selected on the page %s.', $value, $path), $this->getSession()->getDriver()); + throw new ExpectationException(sprintf('The option "%s" was not selected on the page %s.', $option, $path), $this->getSession()->getDriver());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FieldTrait.php` around lines 385 - 407, Rename and reorder the method parameters in fieldAssertSelectOptionSelected to match the Behat token order and name: change the first parameter from $value to $option and ensure the signature is (string $option, string $selector); update all internal uses ($option instead of $value) including the call to $select_field->find('named', ['option', $option]) and the sprintf messages so they reference $option, keeping the rest of the logic intact; this aligns the :option token with the parameter name and fixes the parameter order mismatch.
365-376:⚠️ Potential issue | 🟠 MajorFix token-to-parameter order mismatch.
Same issue as
fieldAssertSelectOptionExists- the parameter order doesn't match the token order in the attribute.Suggested fix
- public function fieldAssertSelectOptionNotExists(string $selector, string $option): void { + public function fieldAssertSelectOptionNotExists(string $option, string $selector): void { $select_element = $this->getSession()->getPage()->findField($selector);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FieldTrait.php` around lines 365 - 376, The step attribute token order (':option' then ':selector') doesn't match the method signature in fieldAssertSelectOptionNotExists (string $selector, string $option); swap the method parameters to (string $option, string $selector) or adjust the attribute to match—best fix: change the function signature to public function fieldAssertSelectOptionNotExists(string $option, string $selector): void so the :option and :selector tokens map correctly when Behat calls the step; keep the internal references to $select_element and $option_element as they are.
416-435:⚠️ Potential issue | 🟠 MajorFix token name and parameter order mismatch.
Same issue as
fieldAssertSelectOptionSelected- token:optiondoesn't match parameter$value.Suggested fix
- public function fieldAssertSelectOptionNotSelected(string $value, string $selector): void { + public function fieldAssertSelectOptionNotSelected(string $option, string $selector): void { ... - $option_field = $select_field->find('named', ['option', $value]); + $option_field = $select_field->find('named', ['option', $option]); ... - throw new ExpectationException(sprintf('The option "%s" was not found in the select "%s" on the page %s.', $value, $selector, $path), ...); + throw new ExpectationException(sprintf('The option "%s" was not found in the select "%s" on the page %s.', $option, $selector, $path), ...); ... - throw new ExpectationException(sprintf('The option "%s" was selected in the select "%s" on the page %s, but should not be.', $value, $selector, $path), ...); + throw new ExpectationException(sprintf('The option "%s" was selected in the select "%s" on the page %s, but should not be.', $option, $selector, $path), ...);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FieldTrait.php` around lines 416 - 435, The Step annotation uses token ":option" but the method signature uses parameter $value and the order must match the step tokens; rename the first parameter in fieldAssertSelectOptionNotSelected from string $value to string $option (and update all internal references $value -> $option) so the method reads fieldAssertSelectOptionNotSelected(string $option, string $selector), matching the annotation and mirroring fieldAssertSelectOptionSelected.src/Drupal/UserTrait.php (1)
371-372: 🧹 Nitpick | 🔵 TrivialConsider adjusting the
Givenstep wording to follow prerequisite conventions.The step
'the role :role_name with the permissions :permissions'doesn't explicitly use 'exists' or 'have' wording as recommended forGivensteps that define prerequisites.Suggested wording
- #[Given('the role :role_name with the permissions :permissions')] + #[Given('the role :role_name exists with the permissions :permissions')]As per coding guidelines: "Given steps should define test prerequisites using words like 'exists' or 'have'".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/UserTrait.php` around lines 371 - 372, Update the Given step annotation string on the userCreateRole method to use prerequisite wording (e.g., include "exists" or "has") so it reads like a precondition; change the attribute on userCreateRole (#[Given('the role :role_name with the permissions :permissions')]) to something like #[Given('the role :role_name exists with the permissions :permissions')] or #[Given('the role :role_name has the permissions :permissions')] and keep the method signature userCreateRole(string $role_name, string $permissions): void unchanged.src/Drupal/DraggableviewsTrait.php (1)
29-30:⚠️ Potential issue | 🟡 MinorFix the placeholder-to-parameter mismatch.
The step placeholder
:views_display_iddoes not match the method parameter$view_display_id(note the extra 's' in the placeholder).Suggested fix
- #[When('I save the draggable views items of the view :view_id and the display :views_display_id for the :bundle content in the following order:')] + #[When('I save the draggable views items of the view :view_id and the display :view_display_id for the :bundle content in the following order:')]As per coding guidelines: "Use descriptive placeholder names in Behat step definitions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/DraggableviewsTrait.php` around lines 29 - 30, The Behat step annotation on draggableViewsSaveBundleOrder has a placeholder mismatch (:views_display_id) that doesn't match the method parameter $view_display_id; update the step definition string to use :view_display_id (or rename the method parameter to match, but prefer fixing the annotation) so the placeholder and the parameter name align, ensuring the When(...) attribute matches the method signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.ahoy.yml:
- Line 165: The current ahoy command string "cmd: cp -Rf tests/behat/fixtures/.
build/web/sites/default/files/" overlays files and leaves deletions behind;
change the command so it mirrors the source into the destination (i.e., use a
tool like rsync with archive mode and the delete option) so removed fixture
files in tests/behat/fixtures are also removed from
build/web/sites/default/files; update the cmd value accordingly to use rsync
(preserve permissions, symlinks, timestamps) and enable deletion of extraneous
files.
In `@src/Drupal/ContentTrait.php`:
- Around line 52-53: The Behat step annotation placeholder :content_type is
inconsistent with the method parameter $type; update them to match by either
renaming the method parameter contentDelete(string $content_type, TableNode
$table) or changing the annotation to #[Given('the following :type content does
not exist:')], and then update all internal references/usages in contentDelete,
including any docblocks or calls, to use the chosen name (contentDelete,
:content_type and $content_type or :type and $type) so the placeholder and
parameter are aligned.
In `@src/Drupal/FileTrait.php`:
- Around line 202-213: The `@code` examples in the FileTrait.php docblock use the
wrong step phrase ("Given no managed files:") while the executable step is "the
following managed files do not exist:"; update both `@code` blocks to use the
exact step phrase "the following managed files do not exist:" and keep the
example tables (columns and URIs/filenames) unchanged so generated STEPS.md from
the FileTrait (and any methods referencing this trait) matches the real step
text used at runtime.
In `@src/Drupal/MediaTrait.php`:
- Around line 72-80: Update the example `@code` blocks in the doc comment for the
mediaCreate step so they match the new registered step text: change the
pre-migration wording in the two example blocks (one around the mediaCreate
method comment and the other at the later comment block) to the current step
signature/phrasing used by the #[Given('the following media :media_type
exist:')] attribute; ensure both occurrences referenced in the review are
updated so docs.php generates correct examples for mediaCreate and the examples
reflect the current :media_type parameter and table format.
In `@src/LinkTrait.php`:
- Around line 33-35: The Behat step attribute tokens use :link but the
corresponding method parameters are named $text, causing argument binding
failures; update the step attributes for the listed
methods—linkAssertTextWithHrefExists, linkAssertTextWithHrefWithinElementExists,
linkAssertTextWithHrefNotExists, linkAssertTextWithHrefWithinElementNotExists,
linkAssertLinkIsAbsolute, and linkAssertLinkIsNotAbsolute—so every occurrence of
:link is replaced with :text to match the parameter names and ensure proper
binding.
In `@tests/behat/features/drupal_file.feature`:
- Around line 23-24: The scenario titled "Assert \"When the following managed
files: With subdirectory path\"" only has the generic `@api` tag; add a
descriptive scenario-specific tag (for example `@managed-files` or `@file-subdir`)
immediately above the Scenario declaration so tests can be selectively run;
update the feature block that contains the Scenario title to include the chosen
tag alongside `@api` to follow the guideline of using descriptive tags for
selective test execution.
In `@tests/phpunit/src/DocsTest.php`:
- Around line 206-213: Rename the snake_case local variables in the test to
camelCase: change $given_method, $when_method, $then_method to $givenMethod,
$whenMethod, $thenMethod (and any other snake_case locals in the same block e.g.
at the other occurrence around 1587-1594) and update their usages in the
assertions and calls to extract_method_steps so the test still asserts the same
expected arrays for multimethodGivenItems, multimethodClickButton, and
multimethodAssertResultVisible.
---
Outside diff comments:
In `@src/Drupal/EmailTrait.php`:
- Around line 190-205: The exact-mode branch in emailAssertMessageHeaderContains
is incorrectly using str_contains for exact equality; change the exact path to
compare header_value and string_value with strict equality (===) instead of
str_contains, and for the non-exact path keep the current normalized substring
check; ensure the thrown ExpectationException message uses the original
$string_value (or cast $string) consistently; apply the same change pattern to
the other analogous assertion methods referenced (the other header/body
assertion methods in the file).
---
Duplicate comments:
In `@src/Drupal/DraggableviewsTrait.php`:
- Around line 29-30: The Behat step annotation on draggableViewsSaveBundleOrder
has a placeholder mismatch (:views_display_id) that doesn't match the method
parameter $view_display_id; update the step definition string to use
:view_display_id (or rename the method parameter to match, but prefer fixing the
annotation) so the placeholder and the parameter name align, ensuring the
When(...) attribute matches the method signature.
In `@src/Drupal/UserTrait.php`:
- Around line 371-372: Update the Given step annotation string on the
userCreateRole method to use prerequisite wording (e.g., include "exists" or
"has") so it reads like a precondition; change the attribute on userCreateRole
(#[Given('the role :role_name with the permissions :permissions')]) to something
like #[Given('the role :role_name exists with the permissions :permissions')] or
#[Given('the role :role_name has the permissions :permissions')] and keep the
method signature userCreateRole(string $role_name, string $permissions): void
unchanged.
In `@src/ElementTrait.php`:
- Line 224: Update the step attribute strings to use prerequisite-style wording
instead of imperative "Given I ..." — for example change the attribute
#[Given('I accept all confirmation dialogs')] to something like #[Given('all
confirmation dialogs are accepted')] (and likewise update the other occurrence
at the attribute currently containing 'I ...' on line 239) so they follow the
repository convention of "Given" steps expressing state/prerequisites; ensure
any matching method names or step definitions (the methods annotated by these
attributes) keep their behavior but the attribute text is replaced accordingly.
In `@src/FieldTrait.php`:
- Around line 345-356: The step token order is mismatched with the method
parameters in fieldAssertSelectOptionExists; change the method signature to
accept (string $option, string $selector) so Behat binds :option to $option and
:selector to $selector, leaving the rest of the function body (findField and
option lookup using $selector and $option) unchanged.
- Around line 385-407: Rename and reorder the method parameters in
fieldAssertSelectOptionSelected to match the Behat token order and name: change
the first parameter from $value to $option and ensure the signature is (string
$option, string $selector); update all internal uses ($option instead of $value)
including the call to $select_field->find('named', ['option', $option]) and the
sprintf messages so they reference $option, keeping the rest of the logic
intact; this aligns the :option token with the parameter name and fixes the
parameter order mismatch.
- Around line 365-376: The step attribute token order (':option' then
':selector') doesn't match the method signature in
fieldAssertSelectOptionNotExists (string $selector, string $option); swap the
method parameters to (string $option, string $selector) or adjust the attribute
to match—best fix: change the function signature to public function
fieldAssertSelectOptionNotExists(string $option, string $selector): void so the
:option and :selector tokens map correctly when Behat calls the step; keep the
internal references to $select_element and $option_element as they are.
- Around line 416-435: The Step annotation uses token ":option" but the method
signature uses parameter $value and the order must match the step tokens; rename
the first parameter in fieldAssertSelectOptionNotSelected from string $value to
string $option (and update all internal references $value -> $option) so the
method reads fieldAssertSelectOptionNotSelected(string $option, string
$selector), matching the annotation and mirroring
fieldAssertSelectOptionSelected.
In `@src/FileDownloadTrait.php`:
- Around line 194-213: The step definitions use a generic placeholder ":name"
which reduces readability; update the attribute strings on the methods
fileDownloadAssertFileName and fileDownloadAssertFileNameContains to use
descriptive placeholders (for example change #[Then('the downloaded file name
should be :name')] to #[Then('the downloaded file name should be
:expectedFileName')] and #[Then('the downloaded file name should contain
:name')] to #[Then('the downloaded file name should contain
:expectedSubstring')]), keeping the method signatures the same so existing
parameters still match.
- Around line 227-233: The step attribute strings in FileDownloadTrait.php use
"containing the files named:" and similar table-driven wording; update each
Behat #[Then(...)] attribute to include "the following" for table-based
assertions (e.g. change "the downloaded file should be a zip archive containing
the files named:" to "the downloaded file should be a zip archive containing the
following files:"), and make the equivalent change for the other two occurrences
noted (the attributes around lines 253-260 and 287-294) so all table-based step
definitions follow the "the following" phrasing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 140613e1-ad29-47f8-9257-bb4bcefd7963
⛔ Files ignored due to path filters (1)
tests/behat/fixtures/subdir/document.pdfis excluded by!**/*.pdf
📒 Files selected for processing (48)
.ahoy.ymlSTEPS.mdcomposer.jsondocs.phpphpstan.neonrector.phpscripts/provision.shsrc/CookieTrait.phpsrc/DateTrait.phpsrc/Drupal/BlockTrait.phpsrc/Drupal/ContentBlockTrait.phpsrc/Drupal/ContentTrait.phpsrc/Drupal/DraggableviewsTrait.phpsrc/Drupal/EckTrait.phpsrc/Drupal/EmailTrait.phpsrc/Drupal/FileTrait.phpsrc/Drupal/MediaTrait.phpsrc/Drupal/MenuTrait.phpsrc/Drupal/ModuleTrait.phpsrc/Drupal/ParagraphsTrait.phpsrc/Drupal/SearchApiTrait.phpsrc/Drupal/TaxonomyTrait.phpsrc/Drupal/TimeTrait.phpsrc/Drupal/UserTrait.phpsrc/ElementTrait.phpsrc/FieldTrait.phpsrc/FileDownloadTrait.phpsrc/KeyboardTrait.phpsrc/LinkTrait.phpsrc/MetatagTrait.phpsrc/PathTrait.phpsrc/ResponseTrait.phpsrc/ResponsiveTrait.phpsrc/WaitTrait.phpsrc/XmlTrait.phptests/behat/bootstrap/BehatCliTrait.phptests/behat/bootstrap/FeatureContextTrait.phptests/behat/features/drupal_file.featuretests/behat/fixtures_drupal/d10/web/modules/custom/mysite_core/mysite_core.moduletests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/mysite_core.moduletests/phpunit/fixtures/docs/EmptyCommentTrait.phptests/phpunit/fixtures/docs/ExcludedTrait.phptests/phpunit/fixtures/docs/FirstTrait.phptests/phpunit/fixtures/docs/IncludedTrait.phptests/phpunit/fixtures/docs/MultiMethodTrait.phptests/phpunit/fixtures/docs/SampleTrait.phptests/phpunit/fixtures/docs/SecondTrait.phptests/phpunit/src/DocsTest.php
| copy-files: | ||
| usage: Update fixture files to the current build. | ||
| cmd: find tests/behat/fixtures/ -maxdepth 1 -type f -exec cp {} build/web/sites/default/files/ \; | ||
| cmd: cp -Rf tests/behat/fixtures/. build/web/sites/default/files/ |
There was a problem hiding this comment.
Mirror fixture files instead of overlay-copying them.
cp -Rf only overlays and never removes deleted fixture files, so stale files can persist in build/web/sites/default/files/ and affect scenario outcomes.
🔧 Proposed fix
- cmd: cp -Rf tests/behat/fixtures/. build/web/sites/default/files/
+ cmd: |
+ mkdir -p build/web/sites/default/files
+ rsync -a --delete tests/behat/fixtures/ build/web/sites/default/files/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.ahoy.yml at line 165, The current ahoy command string "cmd: cp -Rf
tests/behat/fixtures/. build/web/sites/default/files/" overlays files and leaves
deletions behind; change the command so it mirrors the source into the
destination (i.e., use a tool like rsync with archive mode and the delete
option) so removed fixture files in tests/behat/fixtures are also removed from
build/web/sites/default/files; update the cmd value accordingly to use rsync
(preserve permissions, symlinks, timestamps) and enable deletion of extraneous
files.
Comment on lines
+52
to
53
| #[Given('the following :content_type content does not exist:')] | ||
| public function contentDelete(string $type, TableNode $table): void { |
There was a problem hiding this comment.
Align placeholder name with parameter name.
The step placeholder :content_type does not match the method parameter $type. This inconsistency can cause confusion in documentation and makes the code harder to follow.
Suggested fix
Either rename the parameter to match the placeholder:
- public function contentDelete(string $type, TableNode $table): void {
+ public function contentDelete(string $content_type, TableNode $table): void {Or rename the placeholder to match the parameter:
- #[Given('the following :content_type content does not exist:')]
+ #[Given('the following :type content does not exist:')]As per coding guidelines: "Use descriptive placeholder names in Behat step definitions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Drupal/ContentTrait.php` around lines 52 - 53, The Behat step annotation
placeholder :content_type is inconsistent with the method parameter $type;
update them to match by either renaming the method parameter
contentDelete(string $content_type, TableNode $table) or changing the annotation
to #[Given('the following :type content does not exist:')], and then update all
internal references/usages in contentDelete, including any docblocks or calls,
to use the chosen name (contentDelete, :content_type and $content_type or :type
and $type) so the placeholder and parameter are aligned.
Comment on lines
+33
to
35
| #[Then('the link :link with the href :href should exist')] | ||
| public function linkAssertTextWithHrefExists(string $text, string $href): void { | ||
| $this->linkAssertTextWithHrefWithinElementExists($text, $href, NULL); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify token/parameter alignment for attribute-based step definitions in src/LinkTrait.php.
# Expected: methods should not report unmatched tokens.
python - <<'PY'
import re
from pathlib import Path
path = Path("src/LinkTrait.php")
code = path.read_text()
pattern = re.compile(
r"#\[(Given|When|Then)\('([^']*)'\)\]\s*public function\s+([A-Za-z_]\w*)\s*\(([^)]*)\)",
re.S
)
def parse_params(raw):
names = []
for part in [p.strip() for p in raw.split(",") if p.strip()]:
part = part.split("=")[0].strip()
if "$" in part:
names.append(part.rsplit("$", 1)[1].strip().lstrip("&"))
return names
for kind, expr, fn, raw_params in pattern.findall(code):
tokens = re.findall(r":([A-Za-z_]\w*)", expr)
params = parse_params(raw_params)
unmatched = [t for t in tokens if t not in params]
if unmatched:
print(f"{fn}: tokens={tokens}, params={params}, unmatched={unmatched}")
PYRepository: drevops/behat-steps
Length of output: 699
Align step tokens with method parameter names in link assertions.
The step attributes use :link but the method parameters are $text. In Behat attribute-based step definitions, token names must match parameter names for proper argument binding.
Locations and fix
Affects 6 methods: linkAssertTextWithHrefExists (line 33), linkAssertTextWithHrefWithinElementExists (line 48), linkAssertTextWithHrefNotExists (line 86), linkAssertTextWithHrefWithinElementNotExists (line 101), linkAssertLinkIsAbsolute (line 172), linkAssertLinkIsNotAbsolute (line 194).
Replace :link with :text in all step attributes:
- #[Then('the link :link with the href :href should exist')]
+ #[Then('the link :text with the href :href should exist')]
public function linkAssertTextWithHrefExists(string $text, string $href): void {
- #[Then('the link :link with the href :href within the element :selector should exist')]
+ #[Then('the link :text with the href :href within the element :selector should exist')]
public function linkAssertTextWithHrefWithinElementExists(string $text, string $href, ?string $selector): void {
- #[Then('the link :link with the href :href should not exist')]
+ #[Then('the link :text with the href :href should not exist')]
public function linkAssertTextWithHrefNotExists(string $text, string $href): void {
- #[Then('the link :link with the href :href within the element :selector should not exist')]
+ #[Then('the link :text with the href :href within the element :selector should not exist')]
public function linkAssertTextWithHrefWithinElementNotExists(string $text, string $href, ?string $selector): void {
- #[Then('the link :link should be an absolute link')]
+ #[Then('the link :text should be an absolute link')]
public function linkAssertLinkIsAbsolute(string $text): void {
- #[Then('the link :link should not be an absolute link')]
+ #[Then('the link :text should not be an absolute link')]
public function linkAssertLinkIsNotAbsolute(string $text): void {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[Then('the link :link with the href :href should exist')] | |
| public function linkAssertTextWithHrefExists(string $text, string $href): void { | |
| $this->linkAssertTextWithHrefWithinElementExists($text, $href, NULL); | |
| #[Then('the link :text with the href :href should exist')] | |
| public function linkAssertTextWithHrefExists(string $text, string $href): void { | |
| $this->linkAssertTextWithHrefWithinElementExists($text, $href, NULL); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LinkTrait.php` around lines 33 - 35, The Behat step attribute tokens use
:link but the corresponding method parameters are named $text, causing argument
binding failures; update the step attributes for the listed
methods—linkAssertTextWithHrefExists, linkAssertTextWithHrefWithinElementExists,
linkAssertTextWithHrefNotExists, linkAssertTextWithHrefWithinElementNotExists,
linkAssertLinkIsAbsolute, and linkAssertLinkIsNotAbsolute—so every occurrence of
:link is replaced with :text to match the parameter names and ensure proper
binding.
Comment on lines
+23
to
+24
| @api | ||
| Scenario: Assert "When the following managed files: With subdirectory path" |
There was a problem hiding this comment.
Add a scenario-specific tag for selective execution.
The new scenario currently has only @api. Please add a descriptive tag (for example @managed-files or @file-subdir) to make targeted runs easier.
🏷️ Suggested update
- `@api`
+ `@api` `@managed-files` `@file-subdir`
Scenario: Assert "When the following managed files: With subdirectory path"As per coding guidelines, "Use descriptive tags (e.g., @datetime) in feature files to allow selective test execution".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @api | |
| Scenario: Assert "When the following managed files: With subdirectory path" | |
| `@api` `@managed-files` `@file-subdir` | |
| Scenario: Assert "When the following managed files: With subdirectory path" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/behat/features/drupal_file.feature` around lines 23 - 24, The scenario
titled "Assert \"When the following managed files: With subdirectory path\""
only has the generic `@api` tag; add a descriptive scenario-specific tag (for
example `@managed-files` or `@file-subdir`) immediately above the Scenario
declaration so tests can be selectively run; update the feature block that
contains the Scenario title to include the chosen tag alongside `@api` to follow
the guideline of using descriptive tags for selective test execution.
Comment on lines
+206
to
+213
| $given_method = $trait->getMethod('multimethodGivenItems'); | ||
| $this->assertEquals(['@Given the following items:'], extract_method_steps($given_method)); | ||
|
|
||
| $when_method = $trait->getMethod('multimethodClickButton'); | ||
| $this->assertEquals(['@When I click on :button'], extract_method_steps($when_method)); | ||
|
|
||
| $then_method = $trait->getMethod('multimethodAssertResultVisible'); | ||
| $this->assertEquals(['@Then the result should be visible'], extract_method_steps($then_method)); |
There was a problem hiding this comment.
Rename local variables to camelCase to satisfy PHPMD checks.
The new test locals use snake_case and are flagged by static analysis; please rename them to camelCase.
Proposed fix
- $given_method = $trait->getMethod('multimethodGivenItems');
- $this->assertEquals(['@Given the following items:'], extract_method_steps($given_method));
+ $givenMethod = $trait->getMethod('multimethodGivenItems');
+ $this->assertEquals(['@Given the following items:'], extract_method_steps($givenMethod));
- $when_method = $trait->getMethod('multimethodClickButton');
- $this->assertEquals(['@When I click on :button'], extract_method_steps($when_method));
+ $whenMethod = $trait->getMethod('multimethodClickButton');
+ $this->assertEquals(['@When I click on :button'], extract_method_steps($whenMethod));
- $then_method = $trait->getMethod('multimethodAssertResultVisible');
- $this->assertEquals(['@Then the result should be visible'], extract_method_steps($then_method));
+ $thenMethod = $trait->getMethod('multimethodAssertResultVisible');
+ $this->assertEquals(['@Then the result should be visible'], extract_method_steps($thenMethod));
@@
- $pystring_method = $result[$trait_name]['methods'][4];
- $this->assertIsString($pystring_method['example']);
- $this->assertStringContainsString('"""', $pystring_method['example']);
- $this->assertEquals('@Then the content should contain:', $pystring_method['steps'][0]);
+ $pystringMethod = $result[$trait_name]['methods'][4];
+ $this->assertIsString($pystringMethod['example']);
+ $this->assertStringContainsString('"""', $pystringMethod['example']);
+ $this->assertEquals('@Then the content should contain:', $pystringMethod['steps'][0]);
- $table_method = $result[$trait_name]['methods'][1];
- $this->assertEquals('@Given the following items exist:', $table_method['steps'][0]);
- $this->assertIsString($table_method['example']);
- $this->assertStringContainsString('| name | value |', $table_method['example']);
- $this->assertStringContainsString(' | name | value |', $table_method['example']);
+ $tableMethod = $result[$trait_name]['methods'][1];
+ $this->assertEquals('@Given the following items exist:', $tableMethod['steps'][0]);
+ $this->assertIsString($tableMethod['example']);
+ $this->assertStringContainsString('| name | value |', $tableMethod['example']);
+ $this->assertStringContainsString(' | name | value |', $tableMethod['example']);Also applies to: 1587-1594
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 206-206: The variable $given_method is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 209-209: The variable $when_method is not named in camelCase. (undefined)
(CamelCaseVariableName)
[error] 212-212: The variable $then_method 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 `@tests/phpunit/src/DocsTest.php` around lines 206 - 213, Rename the snake_case
local variables in the test to camelCase: change $given_method, $when_method,
$then_method to $givenMethod, $whenMethod, $thenMethod (and any other snake_case
locals in the same block e.g. at the other occurrence around 1587-1594) and
update their usages in the assertions and calls to extract_method_steps so the
test still asserts the same expected arrays for multimethodGivenItems,
multimethodClickButton, and multimethodAssertResultVisible.
4a26212 to
7ebdf04
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
src/FileDownloadTrait.php (2)
194-195:⚠️ Potential issue | 🟠 MajorReplace generic
:nameplaceholders with descriptive step parameters.Line 194 and Line 212 still use
:name, which is too generic for step docs/readability.Proposed fix
- #[Then('the downloaded file name should be :name')] - public function fileDownloadAssertFileName(string $name): void { + #[Then('the downloaded file name should be :expected_file_name')] + public function fileDownloadAssertFileName(string $expected_file_name): void { @@ - if ($name != $this->fileDownloadDownloadedFileInfo['file_name']) { - throw new ExpectationException(sprintf('Downloaded file "%s", but expected "%s".', $this->fileDownloadDownloadedFileInfo['file_name'], $name), $this->getSession()->getDriver()); + if ($expected_file_name != $this->fileDownloadDownloadedFileInfo['file_name']) { + throw new ExpectationException(sprintf('Downloaded file "%s", but expected "%s".', $this->fileDownloadDownloadedFileInfo['file_name'], $expected_file_name), $this->getSession()->getDriver()); } } @@ - #[Then('the downloaded file name should contain :name')] - public function fileDownloadAssertFileNameContains(string $name): void { + #[Then('the downloaded file name should contain :file_name_part')] + public function fileDownloadAssertFileNameContains(string $file_name_part): void { @@ - if (!str_contains((string) $this->fileDownloadDownloadedFileInfo['file_name'], $name)) { - throw new ExpectationException(sprintf('Downloaded file name "%s" does not contain "%s".', $this->fileDownloadDownloadedFileInfo['file_name'], $name), $this->getSession()->getDriver()); + if (!str_contains((string) $this->fileDownloadDownloadedFileInfo['file_name'], $file_name_part)) { + throw new ExpectationException(sprintf('Downloaded file name "%s" does not contain "%s".', $this->fileDownloadDownloadedFileInfo['file_name'], $file_name_part), $this->getSession()->getDriver()); } }As per coding guidelines: Use descriptive placeholder names in Behat step definitions.
Also applies to: 200-201, 212-213, 218-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FileDownloadTrait.php` around lines 194 - 195, Update the Behat step annotations to use descriptive parameter names instead of the generic :name placeholder; specifically change the annotation on the fileDownloadAssertFileName method (the #[Then('the downloaded file name should be :name')] declaration) and the other step annotations referenced (the annotations at the blocks around lines 200-201, 212-213, 218-219) to use clear placeholders such as :expectedFileName or :filename so step docs/readability follow guidelines and match the method parameter names.
227-230:⚠️ Potential issue | 🟠 MajorTable-based steps must include
the followingin step text and examples.The tabled steps/examples still omit required wording (
the following) in multiple places.Proposed wording update
- * Then the downloaded file should be a zip archive containing the files named: + * Then the downloaded file should be a zip archive containing the following files named: @@ - #[Then('the downloaded file should be a zip archive containing the files named:')] + #[Then('the downloaded file should be a zip archive containing the following files named:')] @@ - * Then the downloaded file should be a zip archive containing the files partially named: + * Then the downloaded file should be a zip archive containing the following files partially named: @@ - #[Then('the downloaded file should be a zip archive containing the files partially named:')] + #[Then('the downloaded file should be a zip archive containing the following files partially named:')] @@ - * Then the downloaded file should be a zip archive not containing the files partially named: + * Then the downloaded file should be a zip archive not containing the following files partially named: @@ - #[Then('the downloaded file should be a zip archive not containing the files partially named:')] + #[Then('the downloaded file should be a zip archive not containing the following files partially named:')]As per coding guidelines: Use
the followingfor tabled content in Behat steps.Also applies to: 233-233, 253-256, 259-259, 287-290, 293-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FileDownloadTrait.php` around lines 227 - 230, Several Behat table-based step texts in FileDownloadTrait.php are missing the required phrase "the following" (e.g., the step currently reads "Then the downloaded file should be a zip archive containing the files named:"). Update each table-based step and example to include "the following" (for example change that step to "Then the downloaded file should be a zip archive containing the following files:" or "Then the downloaded file should contain the following files:") and apply the same wording change to the other occurrences referenced in the review so all table steps use "the following" consistently.tests/behat/features/drupal_file.feature (1)
23-24:⚠️ Potential issue | 🟡 MinorAdd a descriptive tag for selective test execution.
The new scenario only has the generic
@apitag. Per coding guidelines, add a descriptive tag (e.g.,@file-subdiror@managed-files) to enable targeted test runs.Suggested fix
- `@api` + `@api` `@file-subdir` Scenario: Assert "When the following managed files: With subdirectory path"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/behat/features/drupal_file.feature` around lines 23 - 24, The scenario "Assert \"When the following managed files: With subdirectory path\"" only has the generic `@api` tag; add a descriptive tag (for example `@file-subdir` or `@managed-files`) directly above that Scenario declaration so tests can be selectively executed — locate the Scenario line in tests/behat/features/drupal_file.feature and insert the chosen tag on its own line immediately before the Scenario..ahoy.yml (1)
165-165:⚠️ Potential issue | 🟠 MajorUse
rsync --deleteto properly mirror fixture files.
cp -Rfonly overlays files and never removes deleted fixtures frombuild/web/sites/default/files/, which can cause stale files to persist and affect test outcomes.Proposed fix
- cmd: cp -Rf tests/behat/fixtures/. build/web/sites/default/files/ + cmd: | + mkdir -p build/web/sites/default/files + rsync -a --delete tests/behat/fixtures/ build/web/sites/default/files/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.ahoy.yml at line 165, The current .ahoy.yml "cmd" uses "cp -Rf" which only overlays files and leaves deleted fixtures behind; replace that command with an rsync mirror that removes stale files, e.g. use "rsync -a --delete tests/behat/fixtures/ build/web/sites/default/files/" (ensure the source has a trailing slash to copy contents). Update the "cmd" entry to call rsync with --delete so the fixtures directory on the target is a true mirror of the source.src/ElementTrait.php (1)
224-225:⚠️ Potential issue | 🟡 MinorUse prerequisite-style
Givenphrasing for confirmation dialog toggles.Line 224 and Line 239 still use imperative
Given I ...wording. Please switch both to prerequisite-style phrasing.Suggested change
- #[Given('I accept all confirmation dialogs')] + #[Given('confirmation dialogs have automatic acceptance enabled')] public function elementAcceptConfirmation(): void { - #[Given('I do not accept any confirmation dialogs')] + #[Given('confirmation dialogs have automatic acceptance disabled')] public function elementDeclineConfirmation(): void {As per coding guidelines: "Given steps should define test prerequisites using words like 'exists' or 'have', avoiding 'should', 'should not', and 'Given I'".
Also applies to: 239-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ElementTrait.php` around lines 224 - 225, Update the Given step strings for confirmation dialog toggles to use prerequisite-style phrasing instead of imperative "Given I ..." wording: change the attribute on elementAcceptConfirmation (#[Given('I accept all confirmation dialogs')]) and the attribute on the other confirmation toggle method (elementDisableConfirmation) to a prerequisite-style sentence (e.g., describe the existence/state like "Given confirmation dialogs are accepted" or "Given confirmation dialogs exist and are accepted") so both methods use the coding guideline's required phrasing.src/LinkTrait.php (1)
33-35:⚠️ Potential issue | 🟠 MajorAlign step tokens with method parameter names in link assertions.
The step attributes use
:linkbut the method parameters are$text. In Behat attribute-based step definitions, token names must match parameter names for proper argument binding.Proposed fix for affected methods
- #[Then('the link :link with the href :href should exist')] + #[Then('the link :text with the href :href should exist')] public function linkAssertTextWithHrefExists(string $text, string $href): void { - #[Then('the link :link with the href :href within the element :selector should exist')] + #[Then('the link :text with the href :href within the element :selector should exist')] public function linkAssertTextWithHrefWithinElementExists(string $text, string $href, ?string $selector): void { - #[Then('the link :link with the href :href should not exist')] + #[Then('the link :text with the href :href should not exist')] public function linkAssertTextWithHrefNotExists(string $text, string $href): void { - #[Then('the link :link with the href :href within the element :selector should not exist')] + #[Then('the link :text with the href :href within the element :selector should not exist')] public function linkAssertTextWithHrefWithinElementNotExists(string $text, string $href, ?string $selector): void { - #[Then('the link :link should be an absolute link')] + #[Then('the link :text should be an absolute link')] public function linkAssertLinkIsAbsolute(string $text): void { - #[Then('the link :link should not be an absolute link')] + #[Then('the link :text should not be an absolute link')] public function linkAssertLinkIsNotAbsolute(string $text): void {Also applies to: 48-48, 86-86, 101-101, 172-172, 194-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LinkTrait.php` around lines 33 - 35, The Behat step attributes use token :link but the corresponding method parameters are named $text (e.g., linkAssertTextWithHrefExists and linkAssertTextWithHrefWithinElementExists), so update the attribute tokens to match the parameter names (change :link to :text) for these methods and the other affected step methods mentioned; ensure all step attribute tokens exactly match their method parameter names (e.g., :text, :href, :element) so argument binding works correctly.tests/phpunit/src/DocsTest.php (1)
206-213: 🧹 Nitpick | 🔵 TrivialRename local variables to camelCase to satisfy PHPMD checks.
The snake_case variable names are flagged by static analysis. While the project uses snake_case for locals in
src/**/*.php, test files should follow standard PHP conventions.Proposed fix
- $given_method = $trait->getMethod('multimethodGivenItems'); - $this->assertEquals(['@Given the following items:'], extract_method_steps($given_method)); + $givenMethod = $trait->getMethod('multimethodGivenItems'); + $this->assertEquals(['@Given the following items:'], extract_method_steps($givenMethod)); - $when_method = $trait->getMethod('multimethodClickButton'); - $this->assertEquals(['@When I click on :button'], extract_method_steps($when_method)); + $whenMethod = $trait->getMethod('multimethodClickButton'); + $this->assertEquals(['@When I click on :button'], extract_method_steps($whenMethod)); - $then_method = $trait->getMethod('multimethodAssertResultVisible'); - $this->assertEquals(['@Then the result should be visible'], extract_method_steps($then_method)); + $thenMethod = $trait->getMethod('multimethodAssertResultVisible'); + $this->assertEquals(['@Then the result should be visible'], extract_method_steps($thenMethod));And at lines 1587-1593:
- $pystring_method = $result[$trait_name]['methods'][4]; + $pystringMethod = $result[$traitName]['methods'][4]; ... - $table_method = $result[$trait_name]['methods'][1]; + $tableMethod = $result[$traitName]['methods'][1];Also applies to: 1587-1593
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpunit/src/DocsTest.php` around lines 206 - 213, Rename the snake_case local variables used in the test to camelCase: change $given_method -> $givenMethod, $when_method -> $whenMethod, and $then_method -> $thenMethod and update all their usages (the ->getMethod(...) calls and the extract_method_steps(...) assertions) accordingly; also apply the same renaming for the duplicate occurrences mentioned around the other block that uses multimethodGivenItems/multimethodClickButton/multimethodAssertResultVisible so PHPMD passes, leaving function names like extract_method_steps unchanged.src/Drupal/UserTrait.php (1)
371-372:⚠️ Potential issue | 🟡 MinorUse prerequisite wording in this
Givenstep.Line 371 should include
existsorhaveto match repository Given-step conventions.Suggested fix
-#[Given('the role :role_name with the permissions :permissions')] +#[Given('the role :role_name has the permissions :permissions')]As per coding guidelines: "Given steps should define test prerequisites using words like 'exists' or 'have', avoiding 'should', 'should not', and 'Given I'".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/UserTrait.php` around lines 371 - 372, The Given-step annotation on the userCreateRole method uses non-prerequisite wording; update the step text in the #[Given(...)] attribute for userCreateRole to use prerequisite language such as including "exists" or "have" (for example: "the role :role_name exists with the permissions :permissions" or "a role :role_name has the permissions :permissions") so it follows repository Given-step conventions and coding guidelines.src/FieldTrait.php (1)
345-347:⚠️ Potential issue | 🔴 CriticalFix token-to-parameter binding order in select-option assertions.
Lines 345-417 define step tokens as
:option ... :selector, but method signatures take$selectorfirst (and$valuein two methods). This can bind arguments incorrectly at runtime.Suggested fix
#[Then('the option :option should exist within the select element :selector')] -public function fieldAssertSelectOptionExists(string $selector, string $option): void { +public function fieldAssertSelectOptionExists(string $option, string $selector): void { @@ #[Then('the option :option should not exist within the select element :selector')] -public function fieldAssertSelectOptionNotExists(string $selector, string $option): void { +public function fieldAssertSelectOptionNotExists(string $option, string $selector): void { @@ #[Then('the option :option should be selected within the select element :selector')] -public function fieldAssertSelectOptionSelected(string $value, string $selector): void { +public function fieldAssertSelectOptionSelected(string $option, string $selector): void { @@ - $value, + $option, @@ - throw new ExpectationException(sprintf('The option "%s" was not selected on the page %s.', $value, $path), $this->getSession()->getDriver()); + throw new ExpectationException(sprintf('The option "%s" was not selected on the page %s.', $option, $path), $this->getSession()->getDriver()); @@ #[Then('the option :option should not be selected within the select element :selector')] -public function fieldAssertSelectOptionNotSelected(string $value, string $selector): void { +public function fieldAssertSelectOptionNotSelected(string $option, string $selector): void { @@ - $option_field = $select_field->find('named', ['option', $value]); + $option_field = $select_field->find('named', ['option', $option]); @@ - throw new ExpectationException(sprintf('The option "%s" was not found in the select "%s" on the page %s.', $value, $selector, $path), $this->getSession()->getDriver()); + throw new ExpectationException(sprintf('The option "%s" was not found in the select "%s" on the page %s.', $option, $selector, $path), $this->getSession()->getDriver()); @@ - throw new ExpectationException(sprintf('The option "%s" was selected in the select "%s" on the page %s, but should not be.', $value, $selector, $path), $this->getSession()->getDriver()); + throw new ExpectationException(sprintf('The option "%s" was selected in the select "%s" on the page %s, but should not be.', $option, $selector, $path), $this->getSession()->getDriver());Run this read-only check to confirm token/parameter mismatches:
#!/bin/bash python - <<'PY' import re from pathlib import Path text = Path("src/FieldTrait.php").read_text() rx = re.compile( r"#\[(?:Given|When|Then)\('([^']+)'\)\]\s+public function\s+([A-Za-z_][A-Za-z0-9_]*)\(([^)]*)\)", re.MULTILINE ) targets = { "fieldAssertSelectOptionExists", "fieldAssertSelectOptionNotExists", "fieldAssertSelectOptionSelected", "fieldAssertSelectOptionNotSelected", } for pattern, method, args in rx.findall(text): if method not in targets: continue tokens = re.findall(r":([A-Za-z_][A-Za-z0-9_]*)", pattern) params = re.findall(r"\$([A-Za-z_][A-Za-z0-9_]*)", args) print(f"{method}: tokens={tokens}, params={params}") PYExpected: token order and parameter order should match (
option,selector) for all four methods.Also applies to: 365-367, 385-387, 416-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FieldTrait.php` around lines 345 - 347, The step token order for the select-option steps doesn't match the method parameter order: update the signatures for fieldAssertSelectOptionExists, fieldAssertSelectOptionNotExists, fieldAssertSelectOptionSelected, and fieldAssertSelectOptionNotSelected so the first parameter is the option and the second is the selector (e.g. change from (string $selector, string $option) or (string $selector, string $value) to (string $option, string $selector) and rename $value -> $option where present), then adjust the body of each method to use the reordered variables (ensure calls like findField(...) use $selector and assertions reference $option).src/Drupal/ContentTrait.php (1)
52-53:⚠️ Potential issue | 🟡 MinorAlign step token and method argument naming in
contentDelete.Line 52 uses
:content_type, while Line 53 uses$type. Keep these aligned to reduce ambiguity in step docs and maintenance.Suggested fix
#[Given('the following :content_type content does not exist:')] -public function contentDelete(string $type, TableNode $table): void { +public function contentDelete(string $content_type, TableNode $table): void { foreach ($table->getHash() as $node_hash) { - $nids = $this->contentLoadMultiple($type, $node_hash); + $nids = $this->contentLoadMultiple($content_type, $node_hash);As per coding guidelines: "Use descriptive placeholder names in Behat step definitions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Drupal/ContentTrait.php` around lines 52 - 53, The Behat step token and method parameter are misaligned: the attribute uses ":content_type" while the method signature uses "$type"; update the method signature in contentDelete to use a descriptive parameter name that matches the step token (e.g., rename the parameter to $content_type) so the step definition and method argument are consistent with the Behat placeholder and coding guidelines.
🤖 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/FieldTrait.php`:
- Around line 230-231: The step definition method fieldFillColor currently
declares its optional parameter as ?string $value = NULL; change it to use an
empty-string default per repo rules by updating the signature to string $value =
'' (remove the nullable ? and NULL default) and adjust any internal checks in
fieldFillColor to treat '' as the "no value" case instead of null so behavior
remains the same.
---
Duplicate comments:
In @.ahoy.yml:
- Line 165: The current .ahoy.yml "cmd" uses "cp -Rf" which only overlays files
and leaves deleted fixtures behind; replace that command with an rsync mirror
that removes stale files, e.g. use "rsync -a --delete tests/behat/fixtures/
build/web/sites/default/files/" (ensure the source has a trailing slash to copy
contents). Update the "cmd" entry to call rsync with --delete so the fixtures
directory on the target is a true mirror of the source.
In `@src/Drupal/ContentTrait.php`:
- Around line 52-53: The Behat step token and method parameter are misaligned:
the attribute uses ":content_type" while the method signature uses "$type";
update the method signature in contentDelete to use a descriptive parameter name
that matches the step token (e.g., rename the parameter to $content_type) so the
step definition and method argument are consistent with the Behat placeholder
and coding guidelines.
In `@src/Drupal/UserTrait.php`:
- Around line 371-372: The Given-step annotation on the userCreateRole method
uses non-prerequisite wording; update the step text in the #[Given(...)]
attribute for userCreateRole to use prerequisite language such as including
"exists" or "have" (for example: "the role :role_name exists with the
permissions :permissions" or "a role :role_name has the permissions
:permissions") so it follows repository Given-step conventions and coding
guidelines.
In `@src/ElementTrait.php`:
- Around line 224-225: Update the Given step strings for confirmation dialog
toggles to use prerequisite-style phrasing instead of imperative "Given I ..."
wording: change the attribute on elementAcceptConfirmation (#[Given('I accept
all confirmation dialogs')]) and the attribute on the other confirmation toggle
method (elementDisableConfirmation) to a prerequisite-style sentence (e.g.,
describe the existence/state like "Given confirmation dialogs are accepted" or
"Given confirmation dialogs exist and are accepted") so both methods use the
coding guideline's required phrasing.
In `@src/FieldTrait.php`:
- Around line 345-347: The step token order for the select-option steps doesn't
match the method parameter order: update the signatures for
fieldAssertSelectOptionExists, fieldAssertSelectOptionNotExists,
fieldAssertSelectOptionSelected, and fieldAssertSelectOptionNotSelected so the
first parameter is the option and the second is the selector (e.g. change from
(string $selector, string $option) or (string $selector, string $value) to
(string $option, string $selector) and rename $value -> $option where present),
then adjust the body of each method to use the reordered variables (ensure calls
like findField(...) use $selector and assertions reference $option).
In `@src/FileDownloadTrait.php`:
- Around line 194-195: Update the Behat step annotations to use descriptive
parameter names instead of the generic :name placeholder; specifically change
the annotation on the fileDownloadAssertFileName method (the #[Then('the
downloaded file name should be :name')] declaration) and the other step
annotations referenced (the annotations at the blocks around lines 200-201,
212-213, 218-219) to use clear placeholders such as :expectedFileName or
:filename so step docs/readability follow guidelines and match the method
parameter names.
- Around line 227-230: Several Behat table-based step texts in
FileDownloadTrait.php are missing the required phrase "the following" (e.g., the
step currently reads "Then the downloaded file should be a zip archive
containing the files named:"). Update each table-based step and example to
include "the following" (for example change that step to "Then the downloaded
file should be a zip archive containing the following files:" or "Then the
downloaded file should contain the following files:") and apply the same wording
change to the other occurrences referenced in the review so all table steps use
"the following" consistently.
In `@src/LinkTrait.php`:
- Around line 33-35: The Behat step attributes use token :link but the
corresponding method parameters are named $text (e.g.,
linkAssertTextWithHrefExists and linkAssertTextWithHrefWithinElementExists), so
update the attribute tokens to match the parameter names (change :link to :text)
for these methods and the other affected step methods mentioned; ensure all step
attribute tokens exactly match their method parameter names (e.g., :text, :href,
:element) so argument binding works correctly.
In `@tests/behat/features/drupal_file.feature`:
- Around line 23-24: The scenario "Assert \"When the following managed files:
With subdirectory path\"" only has the generic `@api` tag; add a descriptive tag
(for example `@file-subdir` or `@managed-files`) directly above that Scenario
declaration so tests can be selectively executed — locate the Scenario line in
tests/behat/features/drupal_file.feature and insert the chosen tag on its own
line immediately before the Scenario.
In `@tests/phpunit/src/DocsTest.php`:
- Around line 206-213: Rename the snake_case local variables used in the test to
camelCase: change $given_method -> $givenMethod, $when_method -> $whenMethod,
and $then_method -> $thenMethod and update all their usages (the
->getMethod(...) calls and the extract_method_steps(...) assertions)
accordingly; also apply the same renaming for the duplicate occurrences
mentioned around the other block that uses
multimethodGivenItems/multimethodClickButton/multimethodAssertResultVisible so
PHPMD passes, leaving function names like extract_method_steps unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1f9e491-a12b-4b5a-8c7e-dbeeb85594c7
⛔ Files ignored due to path filters (1)
tests/behat/fixtures/subdir/document.pdfis excluded by!**/*.pdf
📒 Files selected for processing (48)
.ahoy.ymlSTEPS.mdcomposer.jsondocs.phpphpstan.neonrector.phpscripts/provision.shsrc/CookieTrait.phpsrc/DateTrait.phpsrc/Drupal/BlockTrait.phpsrc/Drupal/ContentBlockTrait.phpsrc/Drupal/ContentTrait.phpsrc/Drupal/DraggableviewsTrait.phpsrc/Drupal/EckTrait.phpsrc/Drupal/EmailTrait.phpsrc/Drupal/FileTrait.phpsrc/Drupal/MediaTrait.phpsrc/Drupal/MenuTrait.phpsrc/Drupal/ModuleTrait.phpsrc/Drupal/ParagraphsTrait.phpsrc/Drupal/SearchApiTrait.phpsrc/Drupal/TaxonomyTrait.phpsrc/Drupal/TimeTrait.phpsrc/Drupal/UserTrait.phpsrc/ElementTrait.phpsrc/FieldTrait.phpsrc/FileDownloadTrait.phpsrc/KeyboardTrait.phpsrc/LinkTrait.phpsrc/MetatagTrait.phpsrc/PathTrait.phpsrc/ResponseTrait.phpsrc/ResponsiveTrait.phpsrc/WaitTrait.phpsrc/XmlTrait.phptests/behat/bootstrap/BehatCliTrait.phptests/behat/bootstrap/FeatureContextTrait.phptests/behat/features/drupal_file.featuretests/behat/fixtures_drupal/d10/web/modules/custom/mysite_core/mysite_core.moduletests/behat/fixtures_drupal/d11/web/modules/custom/mysite_core/mysite_core.moduletests/phpunit/fixtures/docs/EmptyCommentTrait.phptests/phpunit/fixtures/docs/ExcludedTrait.phptests/phpunit/fixtures/docs/FirstTrait.phptests/phpunit/fixtures/docs/IncludedTrait.phptests/phpunit/fixtures/docs/MultiMethodTrait.phptests/phpunit/fixtures/docs/SampleTrait.phptests/phpunit/fixtures/docs/SecondTrait.phptests/phpunit/src/DocsTest.php
Comment on lines
+230
to
231
| #[When('I fill in the color field :field with the value :value')] | ||
| public function fieldFillColor(string $field, ?string $value = NULL): mixed { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use empty-string default for optional step argument in fieldFillColor.
Line 231 uses ?string $value = NULL; repository rules require empty string defaults for optional step parameters.
Suggested fix
-public function fieldFillColor(string $field, ?string $value = NULL): mixed {
+public function fieldFillColor(string $field, string $value = ''): mixed {As per coding guidelines: "Optional parameters in step definitions should use empty string defaults, not PHP optional parameters".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[When('I fill in the color field :field with the value :value')] | |
| public function fieldFillColor(string $field, ?string $value = NULL): mixed { | |
| #[When('I fill in the color field :field with the value :value')] | |
| public function fieldFillColor(string $field, string $value = ''): mixed { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FieldTrait.php` around lines 230 - 231, The step definition method
fieldFillColor currently declares its optional parameter as ?string $value =
NULL; change it to use an empty-string default per repo rules by updating the
signature to string $value = '' (remove the nullable ? and NULL default) and
adjust any internal checks in fieldFillColor to treat '' as the "no value" case
instead of null so behavior remains the same.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Dependencies
Tests
Chores