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

#5854 workaround to avoid populating Second Level Cache from DQL queries with multiple nested DQL aliases #5856

Conversation

Ocramius
Copy link
Member

@Ocramius Ocramius commented Jun 5, 2016

Provides workaround for #5854

Doesn't really fix the issue though.

The problem is following:

SELECT
    a, b, c
FROM
    A a
JOIN
    a.b b
JOIN
    b.c c

Caching a query like this one via SLC will have the DefaultQueryCache loop over associations of A, but using metadata of all DQL aliases/associations.

This workaround prevents that, but also disables caching of fetched c instances from this example.

Note: no tests provided, as existing tests already show the regression.

@Ocramius Ocramius added this to the 2.6.0 milestone Jun 5, 2016
@Ocramius Ocramius changed the title #5854 - simple workaround to avoid populating SLC cache from DQL queries with multiple nested DQL aliases #5854 workaround to avoid populating Second Level Cache from DQL queries with multiple nested DQL aliases Jun 5, 2016
@@ -5,6 +5,7 @@ php:
- 5.5
- 5.6
- 7.0
- 7.1
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, not yet available. I guess nightly would be an alternative

Copy link
Member

Choose a reason for hiding this comment

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

Indeed

@lcobucci
Copy link
Member

lcobucci commented Jun 6, 2016

I think that we also have a small thing on https://github.com/doctrine/doctrine2/blob/3bc61d5f5e4aa75479dfe141e52326243972f530/tests/Doctrine/Tests/ORM/Cache/DefaultQueryCacheTest.php#L134. Shouldn't it be $state->getName() instead of $city->getName()?

@Ocramius
Copy link
Member Author

Ocramius commented Jun 6, 2016

Yep, that needs fixing too.

@lcobucci
Copy link
Member

lcobucci commented Jun 6, 2016

I believe we can fix those small problems in order to prevent that Notice and then think on a permanent solution.

@FabioBatSilva FabioBatSilva force-pushed the fix/#5854-default-query-cache-test-using-wrong-reflection-instance branch from c386bb9 to 5533cec Compare June 17, 2016 04:09
@FabioBatSilva FabioBatSilva force-pushed the fix/#5854-default-query-cache-test-using-wrong-reflection-instance branch from 5533cec to 163dac4 Compare June 17, 2016 04:11
@FabioBatSilva
Copy link
Member

@Ocramius I made a couple changes here, hopefully will fix the issue.

@Ocramius
Copy link
Member Author

Ok, seems like I'll have to do some git bisect on master: loads of issues popping up lately...

This patch is good to go meanwhile, thanks @FabioBatSilva!

@Ocramius Ocramius merged commit 765e102 into master Jun 19, 2016
@Ocramius Ocramius deleted the fix/#5854-default-query-cache-test-using-wrong-reflection-instance branch June 19, 2016 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants