New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DDC-2556] Fix generating non-loading proxy methods when the identifier field is defined in a trait #861

Merged
merged 3 commits into from Dec 4, 2018

Conversation

Projects
None yet
2 participants
@Jalle19
Contributor

Jalle19 commented Dec 4, 2018

I stumbled upon this issue today while dealing with a nasty performance issue. After some digging around in the various bug trackers I found doctrine/doctrine2#3282 which illustrates the issue, however it doesn't make it clear what the root cause is.

The problem is that the code that gets checked against the regular expression when determining if a getter is "a short identifier getter" is not the right code. With this patch the actual method in the trait gets checked, not the various use statements in the class that uses said trait.

In my particular case, the lines that got considered where these:

use XXX\ContentApi\Domain\Traits\HasLikes;
use XXX\ContentApi\Domain\Traits\Identifiable;
use XXX\ContentApi\Infrastructure\Traits\AutoIncrements;
use XXX\ContentApi\Infrastructure\Traits\Timestamps;

Here, the line numbers are correct, but the file is wrong, which ultimately causes the check to fail.

@Jalle19 Jalle19 changed the title from [DDC-2556] Don't lazy-load entities for d to [DDC-2556] Fix generating non-loading proxy methods when the identifier field is defined in a trait Dec 4, 2018

@Ocramius

This comment has been minimized.

Member

Ocramius commented Dec 4, 2018

This is already fixed in doctrine/orm:master, where the entire proxy layer is phased out

@Jalle19

This comment has been minimized.

Contributor

Jalle19 commented Dec 4, 2018

Managed to hit enter before I could write a proper description...

@Ocramius yes I noticed that this class is deprecated, however, as far as I can tell, Doctrine 3 is not released yet?

@Ocramius

This comment has been minimized.

Member

Ocramius commented Dec 4, 2018

@Jalle19 yeah, I think the change can make it into doctrine/common in the next minor, since the change is trivial and clear

Jalle19 added some commits Dec 4, 2018

Add a failing test case to illustrate how identifier fields from trai…
…ts never pass the isShortIdentifierGetter() check
Fix identifier fields from traits not passing isShortIdentifierGetter()
The root cause is that the code snippet that is checked against the regular expression (which checks if a method is a simple getter) ends up being a bunch of "use" statements in the class the trait is used in. This is fixed by not calling getDeclaringClass() and instead relying on ReflectionClass to (correctly) figure out that the actual code for the method resides in the trait, not the implementing class.

Fixes doctrine/doctrine2#3282

@Jalle19 Jalle19 force-pushed the Jalle19:fix-ddc-2556 branch from 63097b5 to 5937459 Dec 4, 2018

@Jalle19

This comment has been minimized.

Contributor

Jalle19 commented Dec 4, 2018

@Jalle19 yeah, I think the change can make it into doctrine/common in the next minor, since the change is trivial and clear

This would be more than great, that way I wouldn't have to potentially resort to pulling in my own fork just for this fix.

@Ocramius Ocramius self-assigned this Dec 4, 2018

@Ocramius Ocramius added the Improvement label Dec 4, 2018

@Ocramius Ocramius added this to the 2.11.0 milestone Dec 4, 2018

@Ocramius

🚢 thanks @Jalle19!

@Ocramius Ocramius merged commit c7a23e3 into doctrine:master Dec 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Jalle19

This comment has been minimized.

Contributor

Jalle19 commented Dec 4, 2018

No problem, thanks for reviewing and merging so quickly!

@Jalle19 Jalle19 deleted the Jalle19:fix-ddc-2556 branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment