ProxyGenerator::isShortIdentifierGetter is too aggressive in skipping get<identifier> methods

[peterjmit]

We have been relying on the behaviour of SomeEntityProxy::getId() not triggering n+1 queries (in the ORM), and recently with some refactoring I saw an endpoint go through the roof in terms of number of queries made.

Turns out it was due to a refactor of the following method on our entity

public function getId()
{
    return $this->id;
}

After doing some digging I found that any variations on this method prevent the proxy generator from adding the optimization to the proxy class for an entity

1

public function getId()
{
    // this is a comment
    return $this->id;
}

2

public function getId()
{
    return (int) $this->id;
}

3

public function getId()
{
    return $this->id . ' hello!';
}

4 (this example doesn't even make it to the regex portion of the check, due to exceeding 4 lines)

public function getId()
{
    // this 
    // is 
    // a 
    // long 
    // comment
    return $this->id;
}

My documentation searching skills are likely poor, but this feature seems to be rather sparsely explained which may contribute to the difficulty in understanding the behaviour here.

It seems to me like the above examples should all be valid (and should allow getId() to skip proxy initialization)

I understand the need to prevent introduction of bugs through unexpected proxy behaviour

5

public function getId()
{
    return $this->id . $this->name;
}

However I think my first 4 examples are much more likely to occur (and erroneously break the n+1 block) rather than the edge case presented in example 5.

There are a few options I see for resolving this (and I would be happy to contribute if there is consensus)

1. Better documentation on this functionality and caveats (assuming my failure to find any is valid)
2. Loosen the restrictions on "cheap check" to allow for comments
3. Modify (or abandon) the regex and strip the code of comments before applying the regex. A couple of options for modification off the top of my head:
   1. Allow for type casting and primitive concatenation (complex to build a regex for this?)
   2. Simpler: Look for occurrences of other mapped properties in the method body, i.e. $this->xxx where xxx != <identifier>, if there is a match then return false from isShortIdentifierGetter

[Ocramius]

This would easily be solved by doctrine/doctrine2#1241

[stof]

anything doing more than returning the id property directly means that the real method must be called, and so that the proxy must be initialized (the id property is still null for non-loaded proxies).

[bravik]

Is it possible to generate correct proxy getId() method without triggering lazy-loading for the case when id is a Value Object?

class SomeEntity
{
    /**
     * @ORM\Embedded(class=Id::class, columnPrefix=false)
     */
    protected ?Id $id;

    public function getId(): Id
    {
        return $this->id;
    }

// ....
}



/**
 * @ORM\Embeddable
 */
final class Id
{
    /**
     * @ORM\Column(name="id", type="integer", nullable=false)
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private int $value;

    public function __construct(int $value)
    {
        $this->value = $value;
    }

    public function getValue(): int
    {
        return $this->value;
    }
}

As I figured out the $class->getIdentifier() array for the embeded Entity would have id.value item. And identifier in this case would still be id, so the $cheapCheck would be false.

  $cheapCheck = $method->getNumberOfParameters() === 0
      && substr($method->getName(), 0, 3) === 'get'
      && in_array($identifier, $class->getIdentifier(), true)
      && $class->hasField($identifier)
      && ($endLine - $startLine <= 4);