Add rule to detect missing CacheableMetadata parameter in EntityListBuilder subclasses#979
Add rule to detect missing CacheableMetadata parameter in EntityListBuilder subclasses#979
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. |
| mglaman\PHPStanDrupal\Rules\Drupal\EntityListBuilderOperationsCacheabilityRule: | ||
| phpstan.rules.rule: %drupal.rules.entityListBuilderOperationsCacheabilityRule% | ||
| mglaman\PHPStanDrupal\Rules\Drupal\HookEntityOperationCacheabilityRule: | ||
| phpstan.rules.rule: %drupal.rules.entityListBuilderOperationsCacheabilityRule% | ||
| mglaman\PHPStanDrupal\Rules\Drupal\ProceduralHookEntityOperationCacheabilityRule: | ||
| phpstan.rules.rule: %drupal.rules.entityListBuilderOperationsCacheabilityRule% |
There was a problem hiding this comment.
These two hook-related rules are registered behind the entityListBuilderOperationsCacheabilityRule flag. That flag name (and the PR title/description) suggests it should only gate the EntityListBuilder rule, so enabling it will unexpectedly also enable hook checks. Consider either (a) introducing a separate config flag for the hook rules (and documenting it), or (b) wiring these two rules behind the existing hookRules flag / a more appropriately named shared flag.
| // Second parameter (CacheableMetadata) already present — no issue. | ||
| if (count($node->params) >= 2) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
This rule treats “has 2+ parameters” as sufficient, but doesn’t verify that the 2nd parameter is actually ?CacheableMetadata $cacheability = NULL. A subclass could already have a second optional parameter of a different type/name and would be incorrectly skipped, leaving the cacheability bubbling bug undetected. Consider validating the second parameter’s type (allowing nullable) and default value rather than only counting parameters.
| if ($hookInstance->hook === 'entity_operation' && count($node->params) < 2) { | ||
| $errors[] = RuleErrorBuilder::message( | ||
| sprintf( | ||
| 'Method %s::%s() implements hook_entity_operation but is missing the CacheableMetadata parameter added in Drupal 11.3. Update the signature to include ?\Drupal\Core\Cache\CacheableMetadata $cacheability as the second parameter.', | ||
| $classReflection->getName(), | ||
| $methodName, | ||
| ) | ||
| ) | ||
| ->tip('See https://www.drupal.org/node/3533080') | ||
| ->identifier('drupal.hookEntityOperationMissingCacheabilityParameter') | ||
| ->build(); | ||
| } | ||
|
|
||
| if ($hookInstance->hook === 'entity_operation_alter' && count($node->params) < 3) { | ||
| $errors[] = RuleErrorBuilder::message( | ||
| sprintf( | ||
| 'Method %s::%s() implements hook_entity_operation_alter but is missing the CacheableMetadata parameter added in Drupal 11.3. Update the signature to include ?\Drupal\Core\Cache\CacheableMetadata $cacheability as the third parameter.', | ||
| $classReflection->getName(), | ||
| $methodName, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The suggested signature in this error message only mentions adding a nullable CacheableMetadata parameter, but omits the = NULL default. Without a default, implementations can break when running on Drupal versions that don’t pass the new argument. Consider updating the guidance to recommend ?\Drupal\Core\Cache\CacheableMetadata $cacheability = NULL (and align test expectations accordingly).
| if ($hookName === 'hook_entity_operation' && count($node->params) < 2) { | ||
| return [ | ||
| RuleErrorBuilder::message( | ||
| sprintf( | ||
| 'Function %s() implements hook_entity_operation but is missing the CacheableMetadata parameter added in Drupal 11.3. Update the signature to: %s(\Drupal\Core\Entity\EntityInterface $entity, \Drupal\Core\Cache\CacheableMetadata $cacheability).', | ||
| $functionName, | ||
| $functionName, | ||
| ) |
There was a problem hiding this comment.
The signature guidance here recommends a non-nullable CacheableMetadata $cacheability with no default. For cross-version compatibility (and to match the recommended pattern used elsewhere in this PR), the added parameter should be nullable and default to NULL (e.g., ?CacheableMetadata $cacheability = NULL). As written, following this advice can cause ArgumentCountError on Drupal versions that don’t pass the new argument, and it’s also inconsistent with the OOP hook fixture that uses = null.
| [ | ||
| 'Function mymodule_entity_operation() implements hook_entity_operation but is missing the CacheableMetadata parameter added in Drupal 11.3. Update the signature to: mymodule_entity_operation(\Drupal\Core\Entity\EntityInterface $entity, \Drupal\Core\Cache\CacheableMetadata $cacheability).', | ||
| 7, | ||
| 'See https://www.drupal.org/node/3533080', | ||
| ], | ||
| [ | ||
| 'Function mymodule_entity_operation_alter() implements hook_entity_operation_alter but is missing the CacheableMetadata parameter added in Drupal 11.3. Update the signature to: mymodule_entity_operation_alter(array &$operations, \Drupal\Core\Entity\EntityInterface $entity, \Drupal\Core\Cache\CacheableMetadata $cacheability).', | ||
| 12, | ||
| 'See https://www.drupal.org/node/3533080', | ||
| ], |
There was a problem hiding this comment.
This test hardcodes the exact error message text including the suggested signature without a nullable/default CacheableMetadata argument. If the rule message is corrected to recommend ?CacheableMetadata $cacheability = NULL for compatibility, update the expected strings here accordingly (or consider asserting only stable substrings/identifiers to reduce churn when wording changes).
| [ | ||
| 'Method BadEntityOperationHook::entityOperation() implements hook_entity_operation but is missing the CacheableMetadata parameter added in Drupal 11.3. Update the signature to include ?\Drupal\Core\Cache\CacheableMetadata $cacheability as the second parameter.', | ||
| 10, | ||
| 'See https://www.drupal.org/node/3533080', | ||
| ], | ||
| [ | ||
| 'Method BadEntityOperationAlterHookOneParam::entityOperationAlter() implements hook_entity_operation_alter but is missing the CacheableMetadata parameter added in Drupal 11.3. Update the signature to include ?\Drupal\Core\Cache\CacheableMetadata $cacheability as the third parameter.', | ||
| 20, | ||
| 'See https://www.drupal.org/node/3533080', | ||
| ], | ||
| [ | ||
| 'Method BadEntityOperationAlterHookTwoParams::entityOperationAlter() implements hook_entity_operation_alter but is missing the CacheableMetadata parameter added in Drupal 11.3. Update the signature to include ?\Drupal\Core\Cache\CacheableMetadata $cacheability as the third parameter.', | ||
| 29, | ||
| 'See https://www.drupal.org/node/3533080', | ||
| ], |
There was a problem hiding this comment.
This test asserts the full error message text, including signature guidance that currently omits the = NULL default for the CacheableMetadata argument. If the rule message is updated to recommend ?CacheableMetadata $cacheability = NULL for cross-version compatibility, the expected strings here will need to be updated too (or assert on identifiers/stable substrings to reduce churn).
| [ | ||
| 'Method MissingCacheabilityGetOperations::getOperations() is missing the CacheableMetadata parameter added in Drupal 11.3. Update the signature to: getOperations(\Drupal\Core\Entity\EntityInterface $entity, ?\Drupal\Core\Cache\CacheableMetadata $cacheability = NULL).', | ||
| 10, | ||
| 'See https://www.drupal.org/node/3533080', | ||
| ], | ||
| [ | ||
| 'Method MissingCacheabilityGetDefaultOperations::getDefaultOperations() is missing the CacheableMetadata parameter added in Drupal 11.3. Update the signature to: getDefaultOperations(\Drupal\Core\Entity\EntityInterface $entity, ?\Drupal\Core\Cache\CacheableMetadata $cacheability = NULL).', | ||
| 19, | ||
| 'See https://www.drupal.org/node/3533080', | ||
| ], | ||
| [ | ||
| 'Method MissingCacheabilityBoth::getOperations() is missing the CacheableMetadata parameter added in Drupal 11.3. Update the signature to: getOperations(\Drupal\Core\Entity\EntityInterface $entity, ?\Drupal\Core\Cache\CacheableMetadata $cacheability = NULL).', | ||
| 28, | ||
| 'See https://www.drupal.org/node/3533080', | ||
| ], | ||
| [ | ||
| 'Method MissingCacheabilityBoth::getDefaultOperations() is missing the CacheableMetadata parameter added in Drupal 11.3. Update the signature to: getDefaultOperations(\Drupal\Core\Entity\EntityInterface $entity, ?\Drupal\Core\Cache\CacheableMetadata $cacheability = NULL).', | ||
| 33, | ||
| 'See https://www.drupal.org/node/3533080', | ||
| ], |
There was a problem hiding this comment.
This test hardcodes the full error message text. If the rule is tightened to validate the 2nd parameter’s type/default (and/or the message is updated for consistency), these exact-string assertions will become brittle. Consider asserting identifiers and key message fragments rather than the entire suggested signature.
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>
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.This PR adds
EntityListBuilderOperationsCacheabilityRule, a bleeding-edge rule that detects these outdated overrides:\Drupal::VERSION; skips< 11.3(parameter didn't exist yet) and>= 12(PHPStan's native signature checking handles it once the parameter is formal)getOperations()on anyEntityListBuilderInterfacesubclassgetDefaultOperations()on anyEntityListBuildersubclass (method is not on the interface)EntityListBuilderitself, which has thefunc_get_args()workaround intentionallyentityListBuilderOperationsCacheabilityRulebleeding-edge flagTest plan
php vendor/bin/phpunit --filter=EntityListBuilderOperationsCacheabilityRuleTest— new test passesphp vendor/bin/phpunit— full suite greenphp vendor/bin/phpstan analyze— no errorsphp vendor/bin/phpcs src/Rules/Drupal/EntityListBuilderOperationsCacheabilityRule.php— clean🤖 Generated with Claude Code