property instead of array access rule

[dereuromark]

What do you think of adding this?

<?php

declare(strict_types=1);

namespace App\PHPStan\Rules;

use PhpParser\Node;
use PHPStan\Analyser\Scope;
use PHPStan\Node\VirtualNode;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\ObjectType;
use PHPStan\Type\TypeWithClassName;
use PHPStan\Reflection\ClassReflection;
use PhpParser\Node\Expr\ArrayDimFetch;
use PhpParser\Node\Scalar\String_;

/**
 * Detects array access on object properties when the property is defined.
 *
 * Reports: $entity['property'] when property exists
 * Suggests: $entity->property
 *
 * Allows: $entity['dynamicKey'] when property doesn't exist
 */
class NoArrayAccessOnDefinedObjectPropertiesRule implements Rule
{
    public function getNodeType(): string
    {
        return ArrayDimFetch::class;
    }

    /**
     * @param ArrayDimFetch $node
     * @param Scope $scope
     * @return array<\PHPStan\Rules\RuleError>
     */
    public function processNode(Node $node, Scope $scope): array
    {
        // We only care about string keys like $obj['property']
        if (!$node->dim instanceof String_) {
            return [];
        }

        $key = $node->dim->value;

        // Get the type of the variable being accessed
        $varType = $scope->getType($node->var);

        // Check if it's an object type
        if (!$varType instanceof TypeWithClassName) {
            return [];
        }

        $classReflection = $varType->getClassReflection();
        if ($classReflection === null) {
            return [];
        }

        // For CakePHP entities, check even though they implement ArrayAccess
        $isCakeEntity = $this->isCakeEntity($classReflection);

        // Skip if class implements ArrayAccess (legitimate use) UNLESS it's a Cake entity
        if (!$isCakeEntity && $this->implementsArrayAccess($classReflection)) {
            return [];
        }

        // Check if the property exists as a real property or via annotations
        if (!$this->hasProperty($classReflection, $key)) {
            // Property doesn't exist - this is dynamic, allow it
            return [];
        }

        // Property exists! This should use -> instead
        $varName = $this->getVariableName($node->var);

        return [
            RuleErrorBuilder::message(
                sprintf(
                    'Use object property access %s->%s instead of array access %s[\'%s\']',
                    $varName,
                    $key,
                    $varName,
                    $key
                )
            )
            ->identifier('app.objectPropertyAccess')
            ->build(),
        ];
    }

    private function isCakeEntity(ClassReflection $classReflection): bool
    {
        // Check if this is a CakePHP entity
        $className = $classReflection->getName();

        // Direct check
        if ($className === 'Cake\\ORM\\Entity' || $className === 'Cake\\Datasource\\EntityInterface') {
            return true;
        }

        // Check parent classes
        $parentClass = $classReflection->getParentClass();
        while ($parentClass !== null) {
            if ($parentClass->getName() === 'Cake\\ORM\\Entity') {
                return true;
            }
            $parentClass = $parentClass->getParentClass();
        }

        // Check interfaces
        foreach ($classReflection->getInterfaces() as $interface) {
            if ($interface->getName() === 'Cake\\Datasource\\EntityInterface') {
                return true;
            }
        }

        return false;
    }

    private function implementsArrayAccess(ClassReflection $classReflection): bool
    {
        foreach ($classReflection->getInterfaces() as $interface) {
            if ($interface->getName() === \ArrayAccess::class) {
                return true;
            }
        }

        return false;
    }

    private function hasProperty(ClassReflection $classReflection, string $propertyName): bool
    {
        // Check native properties
        if ($classReflection->hasNativeProperty($propertyName)) {
            return true;
        }

        // Check @property annotations
        if ($classReflection->hasProperty($propertyName)) {
            return true;
        }

        // Check parent classes
        $parentClass = $classReflection->getParentClass();
        if ($parentClass !== null) {
            return $this->hasProperty($parentClass, $propertyName);
        }

        return false;
    }

    private function getVariableName(Node $node): string
    {
        if ($node instanceof Node\Expr\Variable && is_string($node->name)) {
            return '$' . $node->name;
        }

        if ($node instanceof Node\Expr\PropertyFetch) {
            return '$...->' . ($node->name instanceof Node\Identifier ? $node->name->toString() : '?');
        }

        if ($node instanceof Node\Expr\MethodCall) {
            return '$...->method()';
        }

        return '$var';
    }
}

I tried it out and it seems to work.
Shows all unnecessary array usage on Entities.

[rochamarcelo]

We have DisallowEntityArrayAccessRule, what is the difference?

[rochamarcelo]

btw, DisallowEntityArrayAccessRule is an optional rule.

[dereuromark]

Key Differences

CakeDC's DisallowEntityArrayAccessRule

• Scope: CakePHP entities only
• Approach: Blanket ban on ALL array access for entities
• Logic: If it's an entity → disallow $entity['anything']
• Dynamic properties: Also blocked (even if property doesn't exist)
• Configuration: Enabled by default, can disable with:
  parameters:
  cakeDC:
  disallowEntityArrayAccessRule: false

this NoArrayAccessOnDefinedObjectPropertiesRule

• Scope: Any object type (not just entities)
• Approach: Smart detection - only flags when property is defined
• Logic:
  • Checks if property exists (native property or @property annotation)
  • If property exists → error (should use ->property)
  • If property doesn't exist → allows it (legitimate dynamic access)
• Special handling: Specifically targets CakePHP entities even though they implement ArrayAccess
• More lenient: Allows legitimate dynamic property access

Key Technical Differences

Proposed one:
// Check if the property exists as a real property or via annotations
if (!$this->hasProperty($classReflection, $key)) {
// Property doesn't exist - this is dynamic, allow it
return [];
}

This allows cases like:
$entity['dynamic_key']; // ✓ Allowed if 'dynamic_key' not defined
$entity['name']; // ✗ Error if 'name' property exists

The CakeDC rule would block both cases.

Example of where we had to use custom one:

// _matchingData special property (1 error in UsersTable.php:1637)
  $entity['_matchingData']['HomesModules']  // Must stay - CakePHP documented pattern

PS: Are you releasing a new minor soon on the things in pipeline?

[dereuromark]

But I guess one can manually add the properties for the custom ones?

1. _matchingData - for matching() queries (found in UsersTable.php:1637)
2. _joinData - for belongsToMany associations (defined in entities but uses object notation ->_joinData, not array access)
3. _ids - used in form data for belongsToMany (in $data['modules']['_ids'] - but this is on regular arrays, not entities)

Then yours would still suffice I guess.

[rochamarcelo]

If I were to ignore property not defined I would preferer only for specific known keys (_matchingData, _joinData and _ids)

The existing rule could be enabled by default, if the user prefer to ignore it they can disable it.