mirror of
https://github.com/doctrine/orm.git
synced 2026-03-24 06:52:09 +01:00
Resolve Target Entity Listener not longer working like expected #7115
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @alexander-schranz on GitHub (Mar 1, 2023).
Bug Report
4fad7a1(stable 2.14.1 and 2.14@dev works)Summary
Our tests against
@devversions are currently failing with the dev version of doctrine/orm and it ssems like the EntityTargetResolver is not longer working like expected in current@devversion.We have some special handling for resolved targets and they are not configured via the doctrine bundle instead we are adding the resolved targets inside an own compilerpass not sure if that could cause an issue now:
57bf8c4dcc/src/Sulu/Bundle/PersistenceBundle/DependencyInjection/Compiler/ResolveTargetEntitiesPass.php (L50-L51). There is some special handling behind which not only @sulu but also @sylius use to make things easier overwritable.The last commit where the CI did run was
979b3dcb8df6815caef022d78262bcfa232238fcso it must be some of this changes:979b3dcb8d...2.15.xThe changes from @mpdude looks releated to the EntityResolveListener: https://github.com/doctrine/orm/pull/10473
Current behavior
Stack Trace:
How to reproduce
Expected behavior
Entities which are mapped via the ResolvedTargetListener should befound like before.
@mpdude commented on GitHub (Mar 1, 2023):
The target entity class mentioned in the error message:
Sulu\Bundle\SecurityBundle\Entity\UserIs that the result of running the ResolveTargetEntityListener? Or is that the (abstract) class that should have been resolved?
@mpdude commented on GitHub (Mar 1, 2023):
If it is the resolved class, is it an
@Entity? It should be, otherwise the association makes no sense, right?@alexander-schranz commented on GitHub (Mar 1, 2023):
@mpdude the
Sulu\Bundle\SecurityBundle\Entity\Useris as I know anEntityafter the Metadata is loaded via metadata listener. This is handled in PHP Metadata listener not via annotation or xml mapping.So it could maybe be that now doctrine/orm accesses the metadata before the
MetadataSubscriberis called? Was a new Subscriber added for the a new feature? Maybe it is just a priority thing and the a new subscriber need just handle things later?@mpdude commented on GitHub (Mar 1, 2023):
Let's see – #10473 caused this.
That change moved a configuration consistency check to another place and relaxed the requirements a little.
Until now, when the ClassMetadataFactory loads the mapping configuration for a class, it will check whether the class is a mapped superclass (MS) and contains a one-to-many association. If so, an exception would be thrown. The reason: A MS cannot declare a one-to-many association, since that means the referred-to entity would have to contain the foreign key pointing back to it. But since a MS does not have a database table, that's not possible.
What I did in #10473 is to postpone this check and do it the other way round:
I have described with an example in #10473 why this is useful.
Now that leads to the ClassMetadataFactory (CMF) asking a new question – is a class given as a
targetEntityin an association mapping a "real" entity or not ("not" = it's a "transient" class or a mapped superclass)?Since this question is asked during the runtime metadata validation (while the CMF loads a particular class), we cannot use the CMF itself to get the full data about the referred-to class. It does not support this kind of re-entrant loading. But, we also don't need the full data, we just need to know whether it's an entity or not.
We already have
\Doctrine\Persistence\Mapping\Driver\MappingDriver::isTransientfor one part of the answer. For the other half, the new\Doctrine\ORM\Mapping\ClassMetadataFactory::peekIfIsMappedSuperclassmethod uses the metadata driver directly, and asks that to load mapping configuration for thetargetEntity. (Yes, it throws away most of the data immediately, it just wants to know if it's a MS.)If I get you right, in your case there is no driver involved that could be used here. You're using a custom metadata event listener that will hook into the CMF and provide mapping configuratoin. Is that correct?
@alexander-schranz commented on GitHub (Mar 2, 2023):
Our mapped super classes are in some kind converted into normal entities by our Persistence Bundle Metadatalistener, sadly I'm not very deep in that logic but it is used in our case used to allow a project overwrite our entities, when the project does not overwrite it its just a normal entity, for referencing that entities which are overwritable a Interface is used via Doctrine ResolveTargetEntityListener.
So in our case a MetadataListener with a very high priority is registered here which is handling the mapped superclasses relations:
57bf8c4dcc/src/Sulu/Bundle/PersistenceBundle/Resources/config/services.xml (L24-L28C19)57bf8c4dcc/src/Sulu/Component/Persistence/EventSubscriber/ORM/MetadataSubscriber.php (L25)Yes the one listenered above is used to set our Metadata correctly for that special overwritable entities, we also have a lot other listeners doing things like Blameable, Timestampable, ... (little bit similar to the Doctrine Gedmo Extensions), so the Metadata is only correct after all Listenes are called.
In our case there are a lot
Interfacesuse like here:57bf8c4dcc/src/Sulu/Component/Persistence/EventSubscriber/ORM/UserBlameSubscriber.php (L69-L80)or here:57bf8c4dcc/src/Sulu/Bundle/ContactBundle/Resources/config/doctrine/Contact.orm.xml (L58). Which are then resolved via DoctrinesResolveTargetEntityListener.#10554 seems to fix the issue the CI runs without errors using that branch: https://github.com/sulu/sulu/actions/runs/4311605614/jobs/7521179740
@mpdude commented on GitHub (Mar 2, 2023):
Glad to hear #10554 works for now 👍.
I am terrified to hear about the tricks you seem to employ in Sulu. Not to say that what you're doing is wrong or not allowed – you only stress the mapping-related components of the ORM even more than I do, and I thought I'd be an exceptional example.
So, be aware that the
\Doctrine\ORM\Mapping\ClassMetadataFactory::peekIfIsMappedSuperclassmethod introduced in #10411 still has to be called in one particular situation when dealing with entity inheritance hierarchies, and itwill notcan not?does currently not support all those tricks.This is only about discovering entity subclasses that for good reasons need not be declared in a discriminator map. I am reasonably sure this does not cause regressions for you. But maybe some of the problems I thought were fixed with #10411 still exist when mapping configuration is loaded "the Sulu way".
@alexander-schranz commented on GitHub (Mar 2, 2023):
Want to mention that this logic isn't something implemented newly and used since more then 8 years inside @sulu CMS and as I know also in the @sylius Ecommerce System to allow overwriting of Entities inside projects, I'm not aware of current state in @sylius but I will ping a developer there to test it also. I'm aware that its a specific usecase normal Projects never have and only packages like us will have, but for Systems like @sulu and @sylius it are some requirement to be here flexible enough and decide things inside the
Metadatalistener. But as I didn't review the code I'm also not sure what changes were required here to support our usecase. But really thank you here for your fast responses and fixes 👍@alexander-schranz commented on GitHub (Mar 2, 2023):
I also tested @sylius and they currently run into the same issue with
2.15@dev: https://github.com/Sylius/Sylius/pull/14834#issuecomment-1451768467Was not yet able to run there tests again your pull request as the CI does use github token for authentication and so composer currently fails to test against a fork. Will try to ask @sylius core team member for help so we can also test @sylius against your changes to check it also fixes there problem. Pinged @lchrusciel about it.
@alexander-schranz commented on GitHub (Mar 26, 2023):
Good news first @sylius seems also not longer run into the error with merged #10554.
Strangely I'm running now into an other error with Gedmo Extensions:
https://github.com/sulu/sulu/blob/2.5/src/Sulu/Bundle/MediaBundle/Resources/config/doctrine/Collection.orm.xml
Not yet sure if the issue is on gedmo side or orm side. Need to debug it deeper but if somebody has a hint let me know.
@mpdude commented on GitHub (Mar 26, 2023):
This seems not to be related with this issue, is it?
@alexander-schranz commented on GitHub (Apr 12, 2023):
It seems not related to this issue but currently blocks to test if all works. Today it also failed on the stable branch so it seems like a change in
2.14.2seems making the gedmo extensions failing.@eisberg commented on GitHub (Apr 12, 2023):
Yes, I confirm, in version 2.14.2,
get a similar error:
However, in version 2.14.1 - everything works
@greg0ire commented on GitHub (Apr 12, 2023):
I guess next step would be using
git bisectto figure out which commit breaks this.@eisberg commented on GitHub (Apr 12, 2023):
maybe: https://github.com/doctrine/orm/pull/10579
@greg0ire commented on GitHub (Apr 12, 2023):
You're right, the correct move would have been to expose it with a default value offalse, like in the parent class.I don't know where I saw that value of
true… the code shows it is already set tofalse.Can you try that and report back? If that works, please send a PR.
@eisberg commented on GitHub (Apr 12, 2023):
yes, i try and confirm.
Problem in line 23 (lib/Doctrine/ORM/Mapping/Driver/SimplifiedXmlDriver.php):
The version before this commit works:
parent::__construct($locator, $fileExtension );@mpdude commented on GitHub (Apr 13, 2023):
This is getting off track, what you're discussing has nothing to do with the initial report. Just sayin'.
@mpdude commented on GitHub (Apr 13, 2023):
Oh, and #10554 which addresses the initial report has been merged, so maybe this issue here should be closed/marked as fixed?