[Mapping] Metadata for inherited mapped classes are not well collected

[gaelreyrol]

Subject

The ExtensionMetadataFactory is not collecting as expected configuration from metadata in particular conditions.

Here are the conditions I managed to define to get this unexpected behavior:

• Entity using ORM Inherited entity / Mapped super class with any extensions.
• Cache change between entity manipulations.

This issue is similar but has been closed considering that specifying the metadata cache pool solves the problem, which is true but does not solves the root cause of the problem.

@dpfaffenbauer pointed out this issue from doctrine/orm reporting that not all classes metadata are loaded in production and derives on the ClassMetadataFactory interface method hasMetadataFor usage. The method name is misleading its usage as the interface comments states:

Checks whether the factory has the metadata for a class loaded already.

This method does not return false if the entity has no metadata, it returns false if the cache does not contain any metadata for the given class which in our case can occur for mapped super classes / inherited entities.

Therefor the method isTransient from the same interface can be used in combination to check if mapped super class metadata should be loaded and cached which is exactly what is missing in the ExtensionMetadataFactory to fully collect metadata for mapped super classes.

Minimal repository with the bug

https://github.com/gaelreyrol/DoctrineExtensions/tree/fix-inherited-classes-metadata-collection

Steps to reproduce

Run the test MappingEventSubscriberTest::testGetMetadataFactoryCacheFromDoctrineForSuperClassExtension().

https://github.com/gaelreyrol/DoctrineExtensions/blob/fix-inherited-classes-metadata-collection/tests/Gedmo/Mapping/MappingEventSubscriberTest.php#L71

Expected results

With this fix applied in ExtensionMetadataFactory::getExtensionMetadata() the test should pass.

If this explanation is self sufficient, I can submit a PR with the branch I created containing the fix and the related test.

Regards

[phansys]

Hi @gaelreyrol.

Is the metadata cache configuration intended to be updated at runtime? Is this behavior documented? I'd like to understand if the proposed fix is in the right direction or if we should disallow this kind of change at all.

Thanks!

[gaelreyrol]

Hi @phansys,

If you follow the implementation for this call $this->objectManager->getClassMetadata($parentClass) implemented by doctrine/persistence package, it leads to the abstract class AbstractClassMetadataFactory and its method getMetdataFor which returns the class metadata if cached and if not loads it directly from the file. So I guess if it should not be updated at runtime, doctrine maintainers would not have implemented it that way.

What do you thing?

[phansys]

IIUC, what you are trying to fix is the missing metadata returned by ExtensionMetadataFactory::getExtensionMetadata(), when there is no existing cache.
If it is the case, I think the test should organized this way:

• A test covering the behavior without cache;
• A test covering the behavior with cache.

What is confusing to me, is that in your current fix you seem to be changing the cache driver instead of performing the comparison with and without cache. Please, correct me if I am wrong in my interpretation.

[X-Coder264]

I've opened an issue on StofDoctrineExtensionsBundle package (stof/StofDoctrineExtensionsBundle#455), turns out the root cause is actually this bug. I've tested the fix that the OP provided and it's working.

@gaelreyrol Could you please open a PR with that fix? If you don't have time for it I can do it instead. Thanks.

[yakobe]

I seem to be having this issue too. @gaelreyrol could you PR that fix if it's suitable? Or are there still problems?

[yakobe]

@phansys I have tested the proposed fix locally and it solves the problems with missing metadata for classes with #[ORM\MappedSuperclass]. Without the change the metadata is empty 😿

[gaelreyrol]

Oh I completely forgot this issue, I am sorry! Thanks for bringing it up again @X-Coder264 @yakobe.

I will submit a PR soon, I just need to refresh my memory to answer @phansys 😁.

[gaelreyrol]

IIUC, what you are trying to fix is the missing metadata returned by ExtensionMetadataFactory::getExtensionMetadata(), when there is no existing cache. If it is the case, I think the test should organized this way:

* A test covering the behavior without cache;

* A test covering the behavior with cache.

What is confusing to me, is that in your current fix you seem to be changing the cache driver instead of performing the comparison with and without cache. Please, correct me if I am wrong in my interpretation.

@phansys I hope my answer will be enough because it's been a long time I've ran into this issue. The fact that the test is not testing if the metadata is properly loaded with and without the cache is because it is not what's causing the issue in the first place.

The metadata is loaded properly by Symfony in the first place, which is fine if you always use the same cache driver but in production you might change or reload your cache driver in specific condition which in that case triggers the issue.

Because the cache has changed, the hasMetadataFor method is returning false, justified by the fact that the cache is new and as explained in the first place:

This method does not return false if the entity has no metadata, it returns false if the cache does not contain any metadata for the given class which in our case can occur for mapped super classes / inherited entities.

Therefore it should use isTransient in combination to load the metadata from Inherited/Mapped classes and cache it.

It is the only way to trigger this issue and verify that Inherited/Mapped classes metadata are always loaded in every case.

[joanna-bak-sourceability]

We also have an issue with this. @franmomu is there any status update on this?

[franmomu]

hey, sorry for taking so long, please send a PR, the fix looks fine, is the $cmf->hasMetadataFor($parentClass) part necessary?

[X-Coder264]

@franmomu The PR has already been opened -> #2651