Conversation
There was a problem hiding this comment.
Claude's review below, seems to be some blockers which I find to be valid.
Required Changes (Blockers)
- Nonexistent method names in
SAFE_DELEGATING_METHODS
The list includes:
'buildReferencedEntityLabelsPlainTextTags',
'buildReferencedEntityLabelsButtonTags',These methods don't exist anywhere in the codebase or PEVB contrib. They should be removed or documented with a comment explaining their anticipated origin. Including phantom method names in a safety allowlist is a maintenance hazard.
- False negative in
allLoadsAreSafeDelegated()with multiple entity loads
The line-number heuristic (a safe-delegating call appears after an entity load) doesn't verify the safe call actually receives that load's result. With two loads in one method where only one is safely delegated, both loads may be marked
safe. Example:
$items = $entity->get('field_items')->referencedEntities(); // line 10 — NOT safely delegated
$labels = array_map(fn($i) => $i->label(), $items); // mutable, needs cache!
$others = $entity->get('field_others')->referencedEntities(); // line 12
$built = $this->buildEntities($others, 'teaser'); // line 13 — safeHere buildEntities at line 13 > line 10, so $items is incorrectly considered safe. At minimum, add a comment documenting this known limitation.
3. Confirm basename() safety for multibyte filenames
Drupal's FileSystem::basename() uses a regex (locale-independent). PHP's basename() is locale-sensitive. If migration file paths can contain non-ASCII characters, the swap may introduce a regression. Please confirm this is safe in practice.
^ This can probably be ignored. As Drupal core is shifting to basename()
Suggested Improvements (Non-blocking)
A. No tests for the rule — This is the most complex rule in the project. A RuleTester-based test covering each gate would significantly reduce regression risk.
B. NodeFinder instantiated on every processNode() call — Consider making it a constructor-injected dependency or class property to avoid repeated instantiation across all EVB methods during analysis.
C. Import ordering — Per CLAUDE.md, imports should be sorted alphabetically. use PhpParser\Node; should come before use PhpParser\Node\Expr\Assign; etc. Current order is:
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Expr\Variable;
use PhpParser\Node; // ← out of orderD. Inline referencedEntities() calls — When called inline in a foreach expression (no assignment node), findAssignedVariable() returns NULL and the code falls through correctly to gate 5. This is correct behavior but worth a comment since it's non-obvious.
| private const ENTITY_LOAD_METHODS = [ | ||
| 'referencedEntities', | ||
| 'loadMultiple', | ||
| 'loadByProperties', |
There was a problem hiding this comment.
| 'loadByProperties', | |
| 'loadByProperties', | |
| 'load', | |
| 'accessCheck', |
What about single entity loads? That could also be a thing right? Also added accessCheck as a suggestion maybe, so we can cover entity queries.
There was a problem hiding this comment.
I'm actually not familiar with how the checks are being done so not sure if entity queries and single entity loads can be supported by existing code.
If possible, we should use a phpstan rule to check for missing render array caching.