Conversation
…uilder subclasses Closes #930 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…acheability parameter Covers both OOP (#[Hook] attribute) and procedural hook implementations. Procedural rule uses version_compare rather than ReflectionProvider since api.php files are not loaded by default. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds new PHPStan bleeding-edge rules to detect Drupal 11.3.x cacheability-signature gaps where cacheability metadata is not bubbled due to outdated method/hook signatures.
Changes:
- Introduces
EntityListBuilderOperationsCacheabilityRuleto flagEntityListBuilder/EntityListBuilderInterfacesubclasses overridinggetOperations()/getDefaultOperations()without the new cacheability parameter (Drupal 11.3.x only). - Adds hook-focused cacheability rules for OOP (
#[Hook]) and procedural hook implementations ofhook_entity_operation/hook_entity_operation_alter. - Registers the new rules behind a bleeding-edge flag and adds PHPUnit fixtures/tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Rules/Drupal/EntityListBuilderOperationsCacheabilityRule.php | New rule to detect missing cacheability parameter in EntityListBuilder overrides (version-gated). |
| src/Rules/Drupal/HookEntityOperationCacheabilityRule.php | New rule to detect missing cacheability parameter in #[Hook] OOP hook methods. |
| src/Rules/Drupal/ProceduralHookEntityOperationCacheabilityRule.php | New rule to detect missing cacheability parameter in procedural hook implementations in .module/.inc. |
| tests/src/Rules/EntityListBuilderOperationsCacheabilityRuleTest.php | Tests for the EntityListBuilder cacheability rule. |
| tests/src/Rules/HookEntityOperationCacheabilityRuleTest.php | Tests for the OOP hook cacheability rule. |
| tests/src/Rules/ProceduralHookEntityOperationCacheabilityRuleTest.php | Tests for the procedural hook cacheability rule. |
| tests/src/Rules/data/entity-list-builder-operations-cacheability.php | Fixture classes for list builder override detection. |
| tests/src/Rules/data/hook-entity-operation-cacheability.php | Fixture classes for #[Hook]-based hook detection. |
| tests/src/Rules/data/mymodule.module | Fixture procedural hook implementations for .module detection. |
| rules.neon | Registers the new rules and wires them behind a conditional flag. |
| extension.neon | Adds a new configuration parameter and schema entry for the rule flag. |
| bleedingEdge.neon | Enables the new rule flag in bleeding-edge defaults. |
The CI tests against Drupal ~11.2.0 where the version gate causes rules to return no errors. Make expected errors conditional on the running Drupal version so tests pass across the full version matrix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sCacheabilityRule
mglaman
changed the title
…d simplify helper signature
Replace duplicated isValidCacheabilityParam() in three rules with a general-purpose ParamHelper::isValidParam() that accepts any expected FQCN and nullable constraint. Callers now own bounds checking via count(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Hooks have no shared interface contract, so PHPStan native method-signature checking cannot catch missing cacheability params in Drupal 12+. Keep the rule firing for any Drupal >= 11.3. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
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
Closes #930
Drupal 11.3 introduced a deprecation for
EntityListBuilder::getOperations()andEntityListBuilder::getDefaultOperations()— both methods now accept an optional?CacheableMetadata $cacheability = NULLsecond parameter (via afunc_get_args()workaround until it becomes formal in Drupal 12.0.0). Custom subclasses that override these methods without the new parameter silently miss cacheability bubbling for access checks.Similarly,
hook_entity_operationandhook_entity_operation_alternow also accept aCacheableMetadataargument.This PR adds rules to detect these outdated overrides and hook implementations:
•
EntityListBuilderOperationsCacheabilityRule: detects outdated overrides inEntityListBuildersubclasses.•
HookEntityOperationCacheabilityRuleandProceduralHookEntityOperationCacheabilityRule: detects outdatedhook_entity_operation(_alter)implementations.All rules are registered behind the
entityOperationsCacheabilityRulebleeding-edge flag.• Version-gated: only fires for Drupal 11.3.x by reading
\Drupal::VERSION; skips< 11.3(parameter didn't exist yet) and>= 12(PHPStan's native signature checking handles it once the parameter is formal).• Error includes a tip linking to the change record https://www.drupal.org/node/3533080
Test plan
[ ]
php vendor/bin/phpunit --filter=EntityListBuilderOperationsCacheabilityRuleTest— new test passes[ ]
php vendor/bin/phpunit— full suite green[ ]
php vendor/bin/phpstan analyze— no errors[ ]
php vendor/bin/phpcs— clean