Skip to content
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

Use getMultiple() to fetch associations as well in L2 cache fetch operations #6245

Merged
merged 1 commit into from Jun 24, 2017

Conversation

lcobucci
Copy link
Member

Just as it was done on #5831

@lcobucci lcobucci added this to the 2.6.0 milestone Jan 20, 2017
@lcobucci lcobucci self-assigned this Jan 20, 2017
@lcobucci
Copy link
Member Author

btw I know this is a bit messy, so please suggest a nice way to improve this (I'm a bit sleepy right now)

@@ -178,14 +176,20 @@ public function get(QueryCacheKey $key, ResultSetMapping $rsm, array $hints = []
continue;
}

$collection = new PersistentCollection($this->em, $assocMetadata, new ArrayCollection());
$generateKeys = function ($id) use ($assocMetadata): EntityCacheKey {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP7 only feature!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

master already dropped PHP 5.6

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guilhermeblanco we dropped PHP 5 support in master as well

@LuffyLyon
Copy link

Hello,

This PR will be merged to the 2.6 version that's right?
Because I did the same work in my project to improve performance, I didn't see this PR before, so II've overriden the DefaultQueryCache class.

The PR is still in improvement?

@lcobucci
Copy link
Member Author

lcobucci commented Apr 4, 2017

@LuffyLyon this is only for 2.6 indeed and @Ocramius wanted to have some tests but we bumped into mocking hell. We still need to decide what to do with it (there're some duplications that I'm not really happy with)

@LuffyLyon
Copy link

Ok thank you, which duplications are you talking about? Maybe I can contribute to improving this feature

@lcobucci lcobucci removed this from Review in ORM v2.6.x Jun 1, 2017
@lcobucci lcobucci added this to Review in ORM v2.7.x Jun 1, 2017
@lcobucci lcobucci modified the milestones: 2.7.0, 2.6.0 Jun 1, 2017
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@Ocramius Ocramius changed the title Use getMultiple() to fetch associations as well Use getMultiple() to fetch associations as well in L2 cache fetch operations Jun 24, 2017
@Ocramius Ocramius merged commit 388afb4 into doctrine:master Jun 24, 2017
@Ocramius Ocramius modified the milestones: 2.6.0, 2.7.0 Jun 24, 2017
@Ocramius Ocramius removed this from Review in ORM v2.7.x Jun 24, 2017
@lcobucci lcobucci added this to Done in ORM v2.6.x Jun 26, 2017
@lcobucci lcobucci deleted the l2c-use-getMultiple branch June 26, 2017 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants