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

[RFC] Load parent classes when loading metadata from cache #369

Closed
wants to merge 1 commit into from
Closed

[RFC] Load parent classes when loading metadata from cache #369

wants to merge 1 commit into from

Conversation

gonzalovilaseca
Copy link

This can be done more neatly, but before putting more time in it I'd like to check if it's a good idea.
I came across this solution because we had a problem with Gedmo extensions when caching metadata in Redis, this is our scenario:

Class A extends B;

Class B has a Gedmo extension mapped field.

On first request metadata for both Class A and Class B is cached in Redis.

On second request AbstractClassMetadataFactory loads Class A from Redis.

Gedmo extension loops through all Class A parents and calls method hasMetadataFor($parentClass) in AbstractClassMetadataFactory, only Class A metadata has been loaded previously, therefore hasMetadataFor($parentClass) returns false, and Gedmo then skips the parent class.

Should Gedmo extension load the metadata for the parent class or should it be done as proposed in this PR?

This can be done more neatly, but before putting more time in it I'd like to check if it's a good idea.
I came across this solution because we had a problem with Gedmo extensions when caching metadata in Redis, this is our scenario:
```
Class A extends B;
```
```
``Class B`` has a ``Gedmo`` mapped field.

On first request metadata for both ``A`` and ``B`` is cached in Redis.

On second request ``AbstractClassMetadataFactory`` loads ``Class A`` from Redis.

Gedmo extension loops through all ``Class A`` parents and calls method ``hasMetadataFor($parentClass)`` in ``AbstractClassMetadataFactory``, only ``Class A`` metadata has been loaded previously, therefore ``hasMetadataFor($parentClass)`` returns false, and ``Gedmo`` then skips the parent class.

Should Gedmo exension load the metadata for the parent class or should it be done as proposed in this PR?
@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DCOM-287

We use Jira to track the state of pull requests and the versions they got
included in.

@Ocramius
Copy link
Member


Nvm, I get what the problem is, but a test case should be in place to reproduce that scenario.

@gonzalovilaseca
Copy link
Author

No, it's not the order.
With this PR the parent classes are also pulled from cache.

So when this class saves to cache, it saves all parent classes too, but when it reads from cache, it only reads the class metadata, not it's parents, and that's what I'm doing here.

@gonzalovilaseca
Copy link
Author

ping

@Ocramius
Copy link
Member

Ocramius commented Dec 3, 2015

Needs test scenario that validates the use-case (failing before patch)

@Majkl578
Copy link
Contributor

Hello,
we've recently split Persistence component into a separate doctrine/persistence repository.
If your PR is still relevant, please open new one to the new repository.
Thanks.

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

Successfully merging this pull request may close these issues.

None yet

4 participants