fix: resolve pre-existing phpstan and phpcs CI failures - #28
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe pull request extends static-analysis suppressions, normalizes custom formatter derivative definitions to arrays, sets Drupal 11.3 for lint assembly, and updates functional and kernel test account handling. ChangesAnalysis and test compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
21bb160 to
9cff333
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.1.x #28 +/- ##
==========================================
+ Coverage 78.54% 78.55% +0.01%
==========================================
Files 25 25
Lines 1729 1730 +1
Branches 64 64
==========================================
+ Hits 1358 1359 +1
Misses 368 368
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a3c664a to
fefad1d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/src/Functional/FormatterSettingUiTest.php`:
- Line 39: Remove the redundant AccountInterface instanceof assertions in
tests/src/Functional/FormatterSettingUiTest.php lines 39-39 and
tests/src/Kernel/FormatterSettingAccessControlTest.php lines 63-68. Update the
affected test setup methods to use the users returned by drupalCreateUser() and
UserCreationTrait::createUser() directly, preserving all subsequent behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ae08a69-66f0-4b41-8bce-d2612d32280b
📒 Files selected for processing (9)
.github/workflows/test.ymlphpstan.neonsrc/Plugin/CustomFormatters/FormatterType/Php.phpsrc/Plugin/Derivative/CustomFormatters.phptests/src/Functional/CustomFormattersTestBase.phptests/src/Functional/FormatterSettingFormTest.phptests/src/Functional/FormatterSettingUiTest.phptests/src/Kernel/FormatterPreviewFormTest.phptests/src/Kernel/FormatterSettingAccessControlTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- phpstan.neon
- src/Plugin/Derivative/CustomFormatters.php
- src/Plugin/CustomFormatters/FormatterType/Php.php
| 'administer node display', | ||
| ]); | ||
| $this->assertNotFalse($admin); | ||
| \assert($admin instanceof AccountInterface); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)tests/src/(Functional|Kernel)/FormatterSetting(Ui|AccessControl)Test\.php$' || true
echo "== file outlines/sections =="
for f in $(git ls-files | rg '(^|/)tests/src/(Functional|Kernel)/FormatterSetting(Ui|AccessControl)Test\.php$'); do
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,120p' "$f" | cat -n
done
echo "== locate AccountInterface and class declarations =="
rg -n "use .*\bAccountInterface|class .*extends|function createUser|drupalLogin|assert\\(" tests/src tests -S || true
echo "== PHP/PHPUnit available? =="
which php || true
php -v 2>/dev/null || trueRepository: Decipher/custom_formatters
Length of output: 21642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate Composer dependencies / Drupal packages =="
fd -a 'composer\.(json|lock)$' . | sed 's#^\./##'
for f in $(fd 'composer\.(json|lock)$' .); do
echo "--- $f ---"
if [[ "$f" == *composer.json ]]; then
python3 - <<'PY' "$f"
import json, sys
p=sys.argv[1]
data=json.load(open(p))
print("name", data.get("name"))
print("require", data.get("require", {}))
print("require-dev", data.get("require-dev", {}))
PY
else
rg -n '"drupal/core"|"drush/drush"|"phpoffice/phpstan-drupal"|"phpunit/phpunit"|drupal/user' "$f" | head -80 || true
fi
done
echo "== package manager presence =="
command -v composer || true
composer -V 2>/dev/null || true
echo "== inspect Drupal core drupal_createUser signature if vendored =="
rg -n "function drupalCreateUser|createUser\(" vendor -S 2>/dev/null | head -80 || trueRepository: Decipher/custom_formatters
Length of output: 568
🌐 Web query:
Drupal 9 BrowserTestBase drupalCreateUser return TypeHandler::createUser UserInterface AccountInterface docblock
💡 Result:
In Drupal 9, BrowserTestBase provides the drupalCreateUser method as an alias for the createUser method defined in the UserCreationTrait [1][2][3]. The return type specified in the docblock for UserCreationTrait::createUser is \Drupal\user\UserInterface|false (or \Drupal\user\UserInterface in more recent versions) [4][5]. While the method returns a fully loaded user entity (which implements UserInterface), the docblock often notes that it returns FALSE if account creation fails [4][6]. Key details regarding the interfaces: 1. UserInterface: This is the interface that defines a user entity in Drupal [7][8]. It extends AccountInterface, ContentEntityInterface, and EntityChangedInterface [7]. 2. AccountInterface: This is a foundational interface representing a user's session and account properties (such as user ID, roles, and permissions) [9]. Because UserInterface extends AccountInterface, a user object returned by drupalCreateUser satisfies both type hints [7]. In practice, the object returned by drupalCreateUser is an instance of \Drupal\user\Entity\User, which implements UserInterface [4][5]. The docblock in UserCreationTrait specifically documents the return type as \Drupal\user\UserInterface to maintain standard interface-based type hinting for entity objects, rather than relying on the concrete entity class [4][5].
Citations:
- 1: https://api.drupal.org/api/drupal/core!tests!Drupal!Tests!BrowserTestBase.php/class/BrowserTestBase/9
- 2: https://api.drupal.org/api/drupal/core%21tests%21Drupal%21Tests%21BrowserTestBase.php/9
- 3: https://api.drupal.org/api/drupal/core!tests!Drupal!Tests!BrowserTestBase.php/class/BrowserTestBase/10
- 4: https://api.drupal.org/api/drupal/core%21modules%21user%21tests%21src%21Traits%21UserCreationTrait.php/trait/UserCreationTrait/9
- 5: https://api.drupal.org/api/drupal/core%21modules%21user%21tests%21src%21Traits%21UserCreationTrait.php/function/UserCreationTrait%3A%3AcreateUser/main
- 6: https://api.druphelp.com/api/zircon_profile/core%21modules%21simpletest%21src%21BrowserTestBase.php/function/BrowserTestBase%3A%3AdrupalCreateUser/8
- 7: https://api.drupal.org/api/drupal/core%21modules%21user%21src%21UserInterface.php/interface/UserInterface/10
- 8: https://github.com/drupal/drupal/blob/3659d1b9/core/modules/user/src/UserInterface.php
- 9: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Session%21AccountInterface.php/9
🌐 Web query:
Drupal UserInterface extends AccountInterface
💡 Result:
Yes, the Drupal UserInterface extends AccountInterface [1][2][3]. In the Drupal API, the UserInterface (found at \Drupal\user\UserInterface) defines a user entity and extends several interfaces, including ContentEntityInterface, EntityChangedInterface, and AccountInterface [1][2][4]. The AccountInterface (found at \Drupal\Core\Session\AccountInterface) is designed to represent the current user and defines methods related to account identity, roles, and access checking [5][6]. Because UserInterface extends AccountInterface, any user entity object inherently satisfies the requirements of an account [7]. When writing code that specifically requires access checking (such as verifying permissions), Drupal best practices recommend type-hinting for AccountInterface rather than UserInterface to ensure broader compatibility, as other objects (like the global session) also implement AccountInterface without being full user entities [7][5].
Citations:
- 1: https://api.drupal.org/api/drupal/core%21modules%21user%21src%21UserInterface.php/interface/UserInterface/main
- 2: https://api.drupal.org/api/drupal/core%21modules%21user%21src%21UserInterface.php/11.x
- 3: https://api.drupal.org/api/drupal/core%21modules%21user%21src%21UserInterface.php/interface/UserInterface/11.x
- 4: https://api.drupal.org/api/drupal/core%21modules%21user%21src%21UserInterface.php/10
- 5: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Session%21AccountInterface.php/interface/AccountInterface/main
- 6: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Session%21AccountInterface.php/interface/AccountInterface/11.x
- 7: https://api.drupal.org/api/drupal/core%21core.api.php/group/user_api/main
🌐 Web query:
Drupal Core phpstan-drupal assert instanceof AccountInterface UserInterface always true UserCreationTrait createUser
💡 Result:
In Drupal test environments, when using UserCreationTrait::createUser, PHPStan may report an instanceof.alwaysTrue error if the returned object is already known by the static analyzer to implement AccountInterface or UserInterface [1][2]. This occurs because PHPStan has identified the return type of createUser—which returns an object implementing these interfaces—and determines that the subsequent instanceof check is redundant [1]. The UserCreationTrait::createUser method is defined in Drupal core to return a user entity (or false in older versions), and its type information is explicitly provided via PHPDoc [3][4]. When PHPStan tracks this type correctly, it concludes that the variable satisfies the interface requirements by definition [1]. To resolve this issue, you have several options: 1. Remove the redundant check: Since PHPStan has determined the check is always true, the safest and cleanest approach is to remove the unnecessary instanceof condition entirely [1]. If the code requires assurance about the type, PHPStan already possesses that knowledge [1]. 2. Use PHPStan ignore comments: If you must retain the check for runtime safety (e.g., in edge cases where the trait might be extended or overridden unexpectedly), you can suppress the warning using a comment above the code line [1]: // @phpstan-ignore instanceof.alwaysTrue if ($user instanceof AccountInterface) {... } 3. Configure phpstan.neon: You can disable this rule globally or per-file in your phpstan.neon configuration if it frequently triggers false positives in your test suite [1][2]: parameters: ignoreErrors: - identifier: instanceof.alwaysTrue Note that Drupal core has actively worked to clean up these types of PHPStan errors in test traits to improve developer experience and baseline stability [5][6][7]. If you are working on a Drupal module, ensure you are using the modern signature of createUser (array $permissions = [], $name = NULL, $admin = FALSE, array $values = []) to avoid deprecation issues and improve type accuracy [7].
Citations:
- 1: https://phpstan.org/error-identifiers/instanceof.alwaysTrue
- 2: I don't understand the "Always true" rules phpstan/phpstan#7242
- 3: https://api.drupal.org/api/drupal/core%21modules%21user%21tests%21src%21Traits%21UserCreationTrait.php/trait/UserCreationTrait/11.x
- 4: https://api.drupal.org/api/drupal/core%21modules%21user%21tests%21src%21Traits%21UserCreationTrait.php/9
- 5: https://www.drupal.org/project/drupal/issues/3319683
- 6: https://www.drupal.org/project/drupal/issues/3324384
- 7: https://www.drupal.org/node/3330762
Remove the redundant AccountInterface assertions.
drupalCreateUser() in CustomFormattersTestBase returns a Drupal user object that already implements AccountInterface, and UserCreationTrait::createUser() in FormatterSettingAccessControlTest returns \Drupal\user\UserInterface, which also extends AccountInterface. Drop these instanceof checks to avoid redundant assertions/PHPStan instanceof.alwaysTrue noise.
🧰 Tools
🪛 GitHub Check: lint
[failure] 39-39:
Instanceof between Drupal\user\UserInterface and Drupal\Core\Session\AccountInterface will always evaluate to true.
[failure] 39-39:
Call to function assert() with true will always evaluate to true.
📍 Affects 2 files
tests/src/Functional/FormatterSettingUiTest.php#L39-L39(this comment)tests/src/Kernel/FormatterSettingAccessControlTest.php#L63-L68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/Functional/FormatterSettingUiTest.php` at line 39, Remove the
redundant AccountInterface instanceof assertions in
tests/src/Functional/FormatterSettingUiTest.php lines 39-39 and
tests/src/Kernel/FormatterSettingAccessControlTest.php lines 63-68. Update the
affected test setup methods to use the users returned by drupalCreateUser() and
UserCreationTrait::createUser() directly, preserving all subsequent behavior.
Source: Linters/SAST tools
fefad1d to
81e9403
Compare
- Php.php: correct phpcs sniff name (DrupalPractice.CodeAnalysis) - CustomFormatters deriver: cast base_plugin_definition to array - Test files: use \assert() for type narrowing on drupalCreateUser() return (returns User|false on Drupal 11.3, the $CORE_SUPPORTED version) - CustomFormattersTestBase: fix property type to AccountInterface - GitHub CI: pin lint job to DRUPAL_VERSION=11.3 to match drupalcode.org $CORE_SUPPORTED, ensuring consistent type inference across environments - phpstan.neon: targeted ignores for deprecated node_access_rebuild() and _contextual_links_to_id() (no replacement available in Drupal 11) - phpstan.neon: ignore entityStorageDirectInjection in ListBuilder (required by ConfigEntityListBuilder parent constructor)
81e9403 to
e6edab9
Compare
- FormatterListBuilder entityStorageDirectInjection ignore now uses a leading-wildcard path so it matches under both GitHub Actions' build/ layout and Drupal.org's module-root layout; the rooted path only matched the former, so the DrupalCI-equivalent pipeline still failed. - Remove instanceof/assert(AccountInterface) checks in test setUp() methods now that phpstan proves the returned type is already narrowed, and drop the resulting unused imports.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/src/Kernel/FormatterSettingAccessControlTest.php`:
- Around line 62-65: Update the test setup around createUser() so both returned
User|false values are explicitly validated before assignment to adminUser and
regularUser. Fail the test immediately when either creation returns false, then
assign the narrowed user values and preserve the existing access-test setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0138b03-a101-42dd-9fc3-81032b95a8b8
📒 Files selected for processing (9)
.github/workflows/test.ymlphpstan.neonsrc/Plugin/CustomFormatters/FormatterType/Php.phpsrc/Plugin/Derivative/CustomFormatters.phptests/src/Functional/CustomFormattersTestBase.phptests/src/Functional/FormatterSettingFormTest.phptests/src/Functional/FormatterSettingUiTest.phptests/src/Kernel/FormatterPreviewFormTest.phptests/src/Kernel/FormatterSettingAccessControlTest.php
💤 Files with no reviewable changes (3)
- tests/src/Functional/FormatterSettingUiTest.php
- tests/src/Functional/FormatterSettingFormTest.php
- tests/src/Kernel/FormatterPreviewFormTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Plugin/CustomFormatters/FormatterType/Php.php
- .github/workflows/test.yml
- phpstan.neon
| $admin = $this->createUser(['administer custom formatters']); | ||
| $this->adminUser = $admin; | ||
| $regular = $this->createUser([]); | ||
| $this->regularUser = $regular; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore type narrowing after createUser().
createUser() returns User|false, but these assignments claim AccountInterface. This currently fails PHPStan and can pass false into access() if setup fails. Add an explicit failure check before assigning each user.
Proposed fix
$admin = $this->createUser(['administer custom formatters']);
+ if (!$admin instanceof \Drupal\Core\Session\AccountInterface) {
+ throw new \RuntimeException('Failed to create the admin test user.');
+ }
$this->adminUser = $admin;
$regular = $this->createUser([]);
+ if (!$regular instanceof \Drupal\Core\Session\AccountInterface) {
+ throw new \RuntimeException('Failed to create the regular test user.');
+ }
$this->regularUser = $regular;📝 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.
| $admin = $this->createUser(['administer custom formatters']); | |
| $this->adminUser = $admin; | |
| $regular = $this->createUser([]); | |
| $this->regularUser = $regular; | |
| $admin = $this->createUser(['administer custom formatters']); | |
| if (!$admin instanceof \Drupal\Core\Session\AccountInterface) { | |
| throw new \RuntimeException('Failed to create the admin test user.'); | |
| } | |
| $this->adminUser = $admin; | |
| $regular = $this->createUser([]); | |
| if (!$regular instanceof \Drupal\Core\Session\AccountInterface) { | |
| throw new \RuntimeException('Failed to create the regular test user.'); | |
| } | |
| $this->regularUser = $regular; |
🧰 Tools
🪛 GitHub Check: lint
[failure] 65-65:
Property Drupal\Tests\custom_formatters\Kernel\FormatterSettingAccessControlTest::$regularUser (Drupal\Core\Session\AccountInterface) does not accept Drupal\user\Entity\User|false.
[failure] 63-63:
Property Drupal\Tests\custom_formatters\Kernel\FormatterSettingAccessControlTest::$adminUser (Drupal\Core\Session\AccountInterface) does not accept Drupal\user\Entity\User|false.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/Kernel/FormatterSettingAccessControlTest.php` around lines 62 - 65,
Update the test setup around createUser() so both returned User|false values are
explicitly validated before assignment to adminUser and regularUser. Fail the
test immediately when either creation returns false, then assign the narrowed
user values and preserve the existing access-test setup.
Source: Linters/SAST tools
The previous commit removed these as redundant per a CodeRabbit suggestion, verified locally with an unpinned DRUPAL_VERSION. Both GitHub Actions (pinned to 11.3) and gitlab.local's DrupalCI-equivalent pipeline disagree: on 11.3, drupalCreateUser()/createUser() return User|false, so the assert()/instanceof was legitimately narrowing away the false case, not redundant. Removing it broke assign.propertyType and argument.type checks on both real pipelines. The FormatterListBuilder phpstan.neon path fix from the previous commit is unaffected and stays.
Drupal.org's actual pipeline (git.drupalcode.org MR !13, job 11227419) confirms these are currently redundant: the resolved drupal/core UserCreationTrait::createUser() stub no longer includes |false, so phpstan reports instanceof.alwaysTrue / function.alreadyNarrowedType. GitHub Actions and the gitlab.local mirror disagreed only because they resolved a different drupal/core patch at a different point in time (this project doesn't commit a composer.lock, so each CI run resolves fresh). Drupal.org is the actual target, so it wins.
DRUPAL_VERSION=11.3.0 was pinned to match drupalcode.org's $CORE_SUPPORTED at the time, but core has since moved on to 11.4 as current stable — drupal.org's own MR pipeline now resolves drupal/core 11.4.4, not 11.3.x. The pin doesn't track that; it just goes stale again every time core releases a new minor. Drop the override and let it fall back to devtools/assemble's default (~11, i.e. latest compatible 11.x), matching drupal.org's own behavior of always resolving current stable rather than a fixed patch level.
|
Landed via drupal.org MR !13 (squash-merged into 4.1.x as f6fae76), then synced back here. Closing rather than merging since the content is already in 4.1.x. |
1 participant
Problem
The
4.1.xbranch has pre-existing phpstan and phpcs failures that block CI on all PRs.Changes
phpcs:ignoresniff name (DrupalPractice.CodeAnalysisvariant)base_plugin_definitionto array before offset accessassertNotFalse/assertwith proper\assert()type narrowing ondrupalCreateUser()returnUserInterface, remove dead!== FALSEguardnode_access_rebuild()and_contextual_links_to_id()(no replacement available in Drupal 11)entityStorageDirectInjectioninFormatterListBuilder(required byConfigEntityListBuilderparent constructor)Why separate PR?
These issues are unrelated to any feature work. Splitting them out unblocks CI for all future PRs.
Summary by CodeRabbit