doctrine / doctrine/common

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

Open
#368 3 comments 0 reactions 0 assignees View on GitHub
Dominant language
PHP
Stars
5.8k
Forks
284
PR merge metrics
No merged PRs in 30d

Description

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

``` php
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

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

2

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

3

``` php
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)

``` php
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

``` php
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 != `, if there is a match then return false from `isShortIdentifierGetter`

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.