Fixed more coding standards and removed PHPMD. #373
Conversation
WalkthroughThis update removes all integration and configuration for PHP Mess Detector (PHPMD) from the project. The PHPMD configuration file ( Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (18)
💤 Files with no reviewable changes (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (7)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
b1faed4 to
834e551
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/behat/bootstrap/BehatCliTrait.php (1)
172-178:⚠️ Potential issueStray quote breaks generated Gherkin feature
behatCliWriteScenarioSteps()writesFeature: Stub feature';(note the extra').
When this stub is parsed by Behat it yields “unexpected string” syntax errors and the scenario won’t run.-Feature: Stub feature'; +Feature: Stub featurePlease remove the stray quote.
🧹 Nitpick comments (4)
src/WaitTrait.php (1)
23-23: Method name changed to improve clarity.The method was renamed from
waitSecondstowaitWaitForSecondsto better align with its functionality. However, the method name now contains a redundant "Wait" term.Consider renaming to just
waitForSecondsto avoid the repetition while maintaining clarity about the method's purpose.-public function waitWaitForSeconds(int|string $seconds): void { +public function waitForSeconds(int|string $seconds): void {tests/behat/bootstrap/FeatureContextTrait.php (1)
3-10: Doc block closing tag is malformedThere is an extra space before the terminating
*/, leaving the file-level doc-block as* */.
Some tools (e.g. phpcs, PhpStorm inspections) flag this as an “unterminated comment” and may ignore the entire header.- * */ + */tests/behat/bootstrap/BehatCliTrait.php (1)
315-332: Consider adding an explicit return type
behatCliCopyFixtures()performs copying but does not declare a return type.
For consistency with other methods in the trait (all of which declare: void) and to leverage strict-types, add it here as well:-protected function behatCliCopyFixtures() { +protected function behatCliCopyFixtures(): void {This tiny change improves static-analysis coverage without altering behaviour.
tests/behat/bootstrap/BehatCliContext.php (1)
3-24: Duplicate file-level doc-blocksTwo consecutive
@fileheaders (lines 3-7 and 10-24) describe the same purpose.
Keeping only one avoids needless noise and prevents tooling from reporting “duplicate comment”/“missing short description” issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.ahoy.yml(1 hunks).gitattributes(1 hunks)phpmd.xml(0 hunks)src/BigPipeTrait.php(0 hunks)src/BlockContentTrait.php(0 hunks)src/BlockTrait.php(0 hunks)src/ContentTrait.php(1 hunks)src/CookieTrait.php(1 hunks)src/FileTrait.php(1 hunks)src/KeyboardTrait.php(0 hunks)src/MediaTrait.php(1 hunks)src/ParagraphsTrait.php(0 hunks)src/SearchApiTrait.php(0 hunks)src/WaitTrait.php(1 hunks)steps.md(1 hunks)tests/behat/bootstrap/BehatCliContext.php(3 hunks)tests/behat/bootstrap/BehatCliTrait.php(8 hunks)tests/behat/bootstrap/FeatureContextTrait.php(13 hunks)tests/behat/fixtures/d10/.rsync-exclude(0 hunks)tests/behat/fixtures/d10/composer.json(0 hunks)
💤 Files with no reviewable changes (9)
- src/SearchApiTrait.php
- src/BlockTrait.php
- src/ParagraphsTrait.php
- tests/behat/fixtures/d10/.rsync-exclude
- tests/behat/fixtures/d10/composer.json
- phpmd.xml
- src/KeyboardTrait.php
- src/BlockContentTrait.php
- src/BigPipeTrait.php
🔇 Additional comments (8)
src/ContentTrait.php (1)
51-52: Great variable renamingChanging
$nodeHashto$node_hashimproves consistency with Drupal coding standards, which prefer snake_case for local variables..gitattributes (1)
2-20: Export ignore list updates look goodThe updates to the export-ignore list are well organized:
- Added new configuration files like
.docker,.github,docs.php, etc.- Removed
phpmd.xmlwhich aligns with removing PHP Mess Detector- Improved organization of the list entries
These changes ensure appropriate files are excluded from distribution packages.
src/FileTrait.php (1)
68-69: Good variable naming standardizationRenaming
$nodeHashto$node_hashimproves consistency with Drupal coding standards, which prefer snake_case for local variables.src/MediaTrait.php (1)
72-73: Consistent variable naming improvementChanging
$nodeHashto$node_hashmaintains coding standard consistency across all traits, improving code readability and adherence to Drupal's preferred snake_case naming convention for local variables..ahoy.yml (2)
113-113: Updated lint command to remove PHPMD.The PHPMD configuration file (
phpmd.xml) has been removed from the list of files being copied to the build directory for linting, aligning with the PR objective of removing PHPMD from the project's toolchain.
122-122: Updated lint-fix command to remove PHPMD.Similarly, the
phpmd.xmlfile has been removed from the configuration files copied during the lint-fix process, maintaining consistency with the removal of PHPMD from the project.src/CookieTrait.php (1)
101-101: Standardized step definition format.The annotation has been updated to remove the optional parentheses around "the", making the step definition more explicit and consistent with other step definitions.
steps.md (1)
335-335: Updated documentation to match code change.The step definition in the documentation has been updated to match the annotation change in
CookieTrait.php, ensuring consistency between code and documentation.
834e551 to
62023e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/behat/bootstrap/BehatCliContext.php (2)
3-8: Remove duplicated file-level DocBlockThe file contains two very similar
@fileDocBlocks (lines 3-8 and 10-24). Having duplicates makes automated tooling (e.g., API doc generators) choose one arbitrarily and confuses readers.Please delete the first one (lines 3-8) or merge any missing details into the second to keep a single authoritative header.
17-18: Fix typo in comment
calss→classNit-level, but worth cleaning because this file is explicitly marked “DO NOT MODIFY … TO KEEP IT SYNCED WITH UPSTREAM!”.
- * Changes made to this calss: + * Changes made to this class:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (19)
.ahoy.yml(1 hunks).gitattributes(1 hunks)phpmd.xml(0 hunks)src/BigPipeTrait.php(0 hunks)src/BlockContentTrait.php(0 hunks)src/BlockTrait.php(0 hunks)src/ContentTrait.php(1 hunks)src/CookieTrait.php(1 hunks)src/FileTrait.php(1 hunks)src/KeyboardTrait.php(0 hunks)src/MediaTrait.php(1 hunks)src/ParagraphsTrait.php(0 hunks)src/SearchApiTrait.php(0 hunks)src/WaitTrait.php(1 hunks)steps.md(1 hunks)tests/behat/bootstrap/BehatCliContext.php(3 hunks)tests/behat/bootstrap/FeatureContextTrait.php(13 hunks)tests/behat/fixtures/d10/.rsync-exclude(0 hunks)tests/behat/fixtures/d10/composer.json(0 hunks)
💤 Files with no reviewable changes (9)
- src/ParagraphsTrait.php
- src/BigPipeTrait.php
- src/KeyboardTrait.php
- src/SearchApiTrait.php
- tests/behat/fixtures/d10/composer.json
- src/BlockContentTrait.php
- src/BlockTrait.php
- tests/behat/fixtures/d10/.rsync-exclude
- phpmd.xml
🚧 Files skipped from review as they are similar to previous changes (9)
- src/WaitTrait.php
- src/ContentTrait.php
- src/MediaTrait.php
- src/CookieTrait.php
- src/FileTrait.php
- .ahoy.yml
- steps.md
- .gitattributes
- tests/behat/bootstrap/FeatureContextTrait.php
| * @var string | ||
| */ | ||
| private $answerString; | ||
|
|
There was a problem hiding this comment.
Typed property $answerString is never initialised
declare(strict_types=1) + an uninitialised typed property triggers a fatal error on first access:
private string $answerString; // uninitialised
...
if (!empty($this->answerString)) { // Fatal ErrorInitialise it to an empty string so iRunBehat() can safely access it before any interactive run.
- private $answerString;
+ private string $answerString = '';| public function aNoopFeature() { | ||
| $filename = 'features/bootstrap/FeatureContext.php'; | ||
| $content = <<<'EOL' | ||
| Feature: | ||
| Scenario: | ||
| When this scenario executes | ||
| EOL; | ||
| $this->createFile($this->workingDir . '/' . $filename, $content); | ||
| } | ||
|
|
||
| /** | ||
| * Moves user to the specified path. | ||
| * | ||
| * @Given /^I am in the "([^"]*)" path$/ | ||
| * | ||
| * @param string $path | ||
| */ | ||
| public function iAmInThePath($path) | ||
| { | ||
| $this->moveToNewPath($path); | ||
| $this->createFile($this->workingDir . '/' . $filename, $content); | ||
| } |
There was a problem hiding this comment.
aNoopFeature() writes Gherkin to a .php file
The method description says “Creates a noop feature”, but it creates
features/bootstrap/FeatureContext.php and writes Gherkin text into it.
That will be interpreted as PHP by autoloaders and as a feature by none,
leading to bewildering failures.
Proposed fix: place the feature in features/noop.feature.
- $filename = 'features/bootstrap/FeatureContext.php';
+ $filename = 'features/noop.feature';Optionally move it under a features/ sub-directory of your choosing.
| public function iRunBehatInteractively($answerString, $argumentsString) { | ||
| $this->env['SHELL_INTERACTIVE'] = TRUE; | ||
|
|
||
| $this->answerString = $answerString; | ||
|
|
||
| $this->options = '--format-settings=\'{"timer": false}\''; | ||
| $this->iRunBehat($argumentsString); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Cast SHELL_INTERACTIVE to string before passing to Process
Symfony\Component\Process\Process::setEnv() expects string|null values.
Assigning a boolean (TRUE) will work today but emits a deprecation notice in Symfony 6 and becomes an InvalidArgumentException in Symfony 7.
- $this->env['SHELL_INTERACTIVE'] = TRUE;
+ $this->env['SHELL_INTERACTIVE'] = '1';🏁 Script executed:
#!/bin/bash
# Locate the iRunBehat method to see how $this->env is used, and search for setEnv calls
rg -n "function iRunBehat" -n tests/behat/bootstrap/BehatCliContext.php
rg -n "new Process" -n tests/behat/bootstrap/BehatCliContext.php
rg -n "setEnv" -n tests/behat/bootstrap/BehatCliContext.phpLength of output: 468
Cast SHELL_INTERACTIVE to string before passing to Process
Symfony’s Process::setEnv() only accepts string|null. Using a boolean now works but will trigger a deprecation in Symfony 6 and throw in Symfony 7.
Please update in tests/behat/bootstrap/BehatCliContext.php:
--- a/tests/behat/bootstrap/BehatCliContext.php
+++ b/tests/behat/bootstrap/BehatCliContext.php
@@ public function iRunBehatInteractively($answerString, $argumentsString) {
- $this->env['SHELL_INTERACTIVE'] = TRUE;
+ $this->env['SHELL_INTERACTIVE'] = '1';62023e1 to
1ca34f7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.x #373 +/- ##
=======================================
Coverage 81.54% 81.54%
=======================================
Files 31 31
Lines 1680 1680
=======================================
Hits 1370 1370
Misses 310 310 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Chores
Style
Documentation
Refactor