[Mapping] Fix inherited classes metadata collection #2651
Conversation
|
Hmm, I was waiting for @phansys feedback on the related issue to submit a PR, I do not want to put pressure on the maintainers of this repository even tho the fix is ready. |
CHANGELOG.md
Outdated
| @@ -45,6 +45,7 @@ a release. | |||
| - Loggable: Remove unfixable deprecation when extending `LoggableListener` | |||
| - Remove unfixable deprecations when extending repository classes | |||
| - Fix error caused by the attempt of "doctrine/annotations" parsing a `@note` annotation | |||
| - Fix bug collecting metadata for inherited mapped classes | |||
There was a problem hiding this comment.
@magikid This should go under Unreleased , currently it's under [3.11.1] - 2023-02-20 which is wrong.
There was a problem hiding this comment.
Done, sorry, I was confused because there weren't any of the subheadings in the Unreleased section.
| @@ -114,7 +114,7 @@ public function getExtensionMetadata($meta) | |||
| if (null !== $meta->reflClass) { | |||
| foreach (array_reverse(class_parents($meta->getName())) as $parentClass) { | |||
| // read only inherited mapped classes | |||
| if ($cmf->hasMetadataFor($parentClass)) { | |||
| if ($cmf->hasMetadataFor($parentClass) || !$cmf->isTransient($parentClass)) { | |||
There was a problem hiding this comment.
I'm wondering if the $cmf->hasMetadataFor($parentClass) || part is necessary since it's confusing what it does and is not reliable.
There was a problem hiding this comment.
IMO it can stay as is because the hasMetadataFor call is doing just an isset check under the hood while the isTransient call is more expensive since it goes to the mapping driver which then has to check whether the class with the specified name should have its metadata loaded or not (for example in the case of the atrribute driver it has to use reflection to check whether the class is mapped as an entity or as a mapped super class or neither).
There was a problem hiding this comment.
yeah, I guess it would save some calls to the driver, looking at the implementation of getParentClasses, they only use isTransient 🤔 :
https://github.com/doctrine/persistence/blob/413eb71a22c31c309b5a1cea9701d723cc0d4ae2/src/Persistence/Mapping/AbstractClassMetadataFactory.php#L278-L292
There was a problem hiding this comment.
From what I'm reading, I'll leave this as-is.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2651 +/- ##
=======================================
Coverage 79.22% 79.22%
=======================================
Files 161 161
Lines 8415 8415
=======================================
Hits 6667 6667
Misses 1748 1748
☔ View full report in Codecov by Sentry. |
|
I tested it with the bug that appeared in https://github.com/ecamp/ecamp3 |
|
Any updates on this? This PR also successfully fixes a lot of issues on our site. |
|
@magikid can you please rebase? I've tried but I couldn't because of permissions 😞 |
|
Just figuring out this bug after hours trying to understand why my timestamps were no longer applied. Thought it was because of doctrine 2.16 (doctrine/orm#10869) but in fact it was this issue. Using a timestampable trait on an Abstract entity + prod mode = boom SQLSTATE Exception Thanks for your PR @magikid |
magikid
force-pushed
the
fix-inherited-classes-metadata-collection
branch
from
113ebec
to
8e8aa9e
Compare
magikid
force-pushed
the
fix-inherited-classes-metadata-collection
branch
from
8e8aa9e
to
c53d200
Compare
|
@franmomu I just rebased it |
|
Thanks, if you can fix the coding standard issues shown in https://github.com/doctrine-extensions/DoctrineExtensions/actions/runs/5869991947/job/15916135479?pr=2651 that would be all I think |
|
Thanks @gaelreyrol and @magikid! |
|
@phansys Would it be possible to get a release with this included? Thanks everyone for helping to solve this 😸 ! |
|
Sure! 3.13.0. |
All the work here was done by @gaelreyrol, I'm just opening the PR.
Fixes #2600