Resolve Target Entity Listener not longer working like expected #10552

alexander-schranz · 2023-03-01T11:46:41Z

Bug Report

Q	A
BC Break	yes/no
Version	2.15.x-dev `4fad7a1` (stable 2.14.1 and 2.14@dev works)

Summary

Our tests against @dev versions are currently failing with the dev version of doctrine/orm and it ssems like the EntityTargetResolver is not longer working like expected in current @dev version.

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: https://github.com/sulu/sulu/blob/57bf8c4dcc59361033ef6d7acc5898b850f15b5c/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 979b3dcb8df6815caef022d78262bcfa232238fc so it must be some of this changes:

979b3dc...2.15.x

The changes from @mpdude looks releated to the EntityResolveListener: #10473

Current behavior

1) Sulu\Bundle\ActivityBundle\Tests\Functional\Infrastructure\Doctrine\Repository\ActivityRepositoryTest::testCreateForDomainEvent
Doctrine\ORM\Mapping\MappingException: The target entity class Sulu\Bundle\SecurityBundle\Entity\User specified for Sulu\Bundle\TrashBundle\Domain\Model\TrashItem::$user is not an entity class.

Stack Trace:

/Users/alexanderschranz/Documents/Projects/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/MappingException.php:856
/Users/alexanderschranz/Documents/Projects/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:312
/Users/alexanderschranz/Documents/Projects/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:273
/Users/alexanderschranz/Documents/Projects/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:251
/Users/alexanderschranz/Documents/Projects/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/doctrine-bundle/Mapping/ClassMetadataFactory.php:18
/Users/alexanderschranz/Documents/Projects/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/persistence/src/Persistence/Mapping/AbstractClassMetadataFactory.php:415
/Users/alexanderschranz/Documents/Projects/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/persistence/src/Persistence/Mapping/AbstractClassMetadataFactory.php:281
/Users/alexanderschranz/Documents/Projects/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/persistence/src/Persistence/Mapping/AbstractClassMetadataFactory.php:150
/Users/alexanderschranz/Documents/Projects/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/data-fixtures/lib/Doctrine/Common/DataFixtures/Purger/ORMPurger.php:101
/Users/alexanderschranz/Documents/Projects/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/data-fixtures/lib/Doctrine/Common/DataFixtures/Executor/AbstractExecutor.php:147
/Users/alexanderschranz/Documents/Projects/sulu-develop.localhost/vendor/sulu/sulu/src/Sulu/Bundle/TestBundle/Testing/PurgeDatabaseTrait.php:57
/Users/alexanderschranz/Documents/Projects/sulu-develop.localhost/vendor/sulu/sulu/src/Sulu/Bundle/ActivityBundle/Tests/Functional/Infrastructure/Doctrine/Repository/ActivityRepositoryTest.php:42

How to reproduce

git clone [email protected]:sulu/sulu.git
composer require doctrine/orm:"^2.15@dev" --no-update

composer update

# run mysql db

composer bootstrap-test-environment # errors already  here
composer test # here you get a better stack trace

Expected behavior

Entities which are mapped via the ResolvedTargetListener should befound like before.

The text was updated successfully, but these errors were encountered:

mpdude · 2023-03-01T14:03:18Z

The target entity class mentioned in the error message: Sulu\Bundle\SecurityBundle\Entity\User

Is that the result of running the ResolveTargetEntityListener? Or is that the (abstract) class that should have been resolved?

mpdude · 2023-03-01T14:05:15Z

If it is the resolved class, is it an @Entity? It should be, otherwise the association makes no sense, right?

alexander-schranz · 2023-03-01T14:51:45Z

@mpdude the Sulu\Bundle\SecurityBundle\Entity\User is as I know an Entity after 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 MetadataSubscriber is 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 · 2023-03-01T20:19:21Z

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:

Do not throw the execption right away when we find such an association on the MS. Instead, wait until the ResolveTargetEntityListener has run (it might substitute a MS with a "real" entity class).
Do not check for one-to-many on the MS, but check in general (for every mapped class after it has been loaded) that no association points to a MS.

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 targetEntity in 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::isTransient for one part of the answer. For the other half, the new \Doctrine\ORM\Mapping\ClassMetadataFactory::peekIfIsMappedSuperclass method uses the metadata driver directly, and asks that to load mapping configuration for the targetEntity. (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 · 2023-03-02T07:48:08Z

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.

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:

https://github.com/sulu/sulu/blob/57bf8c4dcc59361033ef6d7acc5898b850f15b5c/src/Sulu/Bundle/PersistenceBundle/Resources/config/services.xml#L24-L28C19

https://github.com/sulu/sulu/blob/57bf8c4dcc59361033ef6d7acc5898b850f15b5c/src/Sulu/Component/Persistence/EventSubscriber/ORM/MetadataSubscriber.php#L25

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?

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.

Now that leads to the ClassMetadataFactory (CMF) asking a new question – is a class given as a targetEntity in an association mapping a "real" entity or not ("not" = it's a "transient" class or a mapped superclass)?

In our case there are a lot Interfaces use like here: https://github.com/sulu/sulu/blob/57bf8c4dcc59361033ef6d7acc5898b850f15b5c/src/Sulu/Component/Persistence/EventSubscriber/ORM/UserBlameSubscriber.php#L69-L80 or here: https://github.com/sulu/sulu/blob/57bf8c4dcc59361033ef6d7acc5898b850f15b5c/src/Sulu/Bundle/ContactBundle/Resources/config/doctrine/Contact.orm.xml#L58. Which are then resolved via Doctrines ResolveTargetEntityListener.

#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 · 2023-03-02T10:02:32Z

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::peekIfIsMappedSuperclass method introduced in #10411 still has to be called in one particular situation when dealing with entity inheritance hierarchies, and it ~~will not~~ ~~can 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 · 2023-03-02T11:11:51Z

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 · 2023-03-02T13:12:13Z

I also tested @Sylius and they currently run into the same issue with 2.15@dev: Sylius/Sylius#14834 (comment)

Was 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 · 2023-03-26T16:39:59Z

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:

Doctrine\ORM\Mapping\MappingException: libxml error: Element '{http://gediminasm.org/schemas/orm/doctrine-extensions-mapping}tree-left': No matching global element declaration available, but demanded by the strict wildcard.
 in /home/runner/work/sulu/sulu/src/Sulu/Bundle/MediaBundle/Resources/config/doctrine/Collection.orm.xml at line 13
libxml error: Element '{http://gediminasm.org/schemas/orm/doctrine-extensions-mapping}tree-right': No matching global element declaration available, but demanded by the strict wildcard.

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 · 2023-03-26T19:48:29Z

This seems not to be related with this issue, is it?

alexander-schranz · 2023-04-12T16:59:48Z

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.2 seems making the gedmo extensions failing.

eisberg · 2023-04-12T17:08:15Z

Yes, I confirm, in version 2.14.2,

get a similar error:

libxml error: Element '{http://gediminasm.org/schemas/orm/doctrine-extensions-mapping}timestampable': No matching global element declaration available, but demanded by the strict wildcard.
in src/Resources/config/doctrine/UserGroupRelation.orm.xml at line 21

However, in version 2.14.1 - everything works

greg0ire · 2023-04-12T17:17:02Z

I guess next step would be using git bisect to figure out which commit breaks this.

eisberg · 2023-04-12T17:18:49Z

maybe: #10579

greg0ire · 2023-04-12T17:22:27Z

~~You're right, the correct move would have been to expose it with a default value of false, like in the parent class.~~

I don't know where I saw that value of true… the code shows it is already set to false.

Can you try that and report back? If that works, please send a PR.

eisberg · 2023-04-12T17:35:02Z

yes, i try and confirm.
Problem in line 23 (lib/Doctrine/ORM/Mapping/Driver/SimplifiedXmlDriver.php):

  parent::__construct($locator, $fileExtension, $isXsdValidationEnabled);

The version before this commit works:
parent::__construct($locator, $fileExtension );

mpdude · 2023-04-13T16:50:03Z

This is getting off track, what you're discussing has nothing to do with the initial report. Just sayin'.

mpdude · 2023-04-13T16:52:12Z

Oh, and #10554 which addresses the initial report has been merged, so maybe this issue here should be closed/marked as fixed?

mpdude mentioned this issue Mar 1, 2023

Make "targetEntity must not be a mapped superclass" a lazy check #10554

Merged

derrabus added the Regression label Mar 2, 2023

derrabus added this to the 2.15.0 milestone Mar 2, 2023

mpdude mentioned this issue Mar 2, 2023

Fill in missing subclasses when loading ClassMetadata #10411

Merged

This was referenced Mar 2, 2023

Run test against doctrine 2.15 Sylius/Sylius#14834

Closed

Refactored usages of short namespace aliases of entities to use of FQCN sulu/sulu#7027

Merged

alexander-schranz mentioned this issue Mar 14, 2023

Run tests against php 8.2 sulu/sulu#7034

Merged

eisberg mentioned this issue Apr 12, 2023

Error in XML mapping (gedmo) afer upgrade to 2.9 doctrine/DoctrineBundle#1643

Closed

greg0ire closed this as completed Apr 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve Target Entity Listener not longer working like expected #10552

Resolve Target Entity Listener not longer working like expected #10552

alexander-schranz commented Mar 1, 2023 •

edited

Loading

mpdude commented Mar 1, 2023

mpdude commented Mar 1, 2023

alexander-schranz commented Mar 1, 2023 •

edited

Loading

mpdude commented Mar 1, 2023 •

edited

Loading

alexander-schranz commented Mar 2, 2023

mpdude commented Mar 2, 2023 •

edited

Loading

alexander-schranz commented Mar 2, 2023 •

edited

Loading

alexander-schranz commented Mar 2, 2023 •

edited

Loading

alexander-schranz commented Mar 26, 2023 •

edited

Loading

mpdude commented Mar 26, 2023

alexander-schranz commented Apr 12, 2023 •

edited

Loading

eisberg commented Apr 12, 2023 •

edited

Loading

greg0ire commented Apr 12, 2023

eisberg commented Apr 12, 2023

greg0ire commented Apr 12, 2023 •

edited

Loading

eisberg commented Apr 12, 2023 •

edited

Loading

mpdude commented Apr 13, 2023

mpdude commented Apr 13, 2023

Resolve Target Entity Listener not longer working like expected #10552

Resolve Target Entity Listener not longer working like expected #10552

Comments

alexander-schranz commented Mar 1, 2023 • edited Loading

Bug Report

Summary

Current behavior

How to reproduce

Expected behavior

mpdude commented Mar 1, 2023

mpdude commented Mar 1, 2023

alexander-schranz commented Mar 1, 2023 • edited Loading

mpdude commented Mar 1, 2023 • edited Loading

alexander-schranz commented Mar 2, 2023

mpdude commented Mar 2, 2023 • edited Loading

alexander-schranz commented Mar 2, 2023 • edited Loading

alexander-schranz commented Mar 2, 2023 • edited Loading

alexander-schranz commented Mar 26, 2023 • edited Loading

mpdude commented Mar 26, 2023

alexander-schranz commented Apr 12, 2023 • edited Loading

eisberg commented Apr 12, 2023 • edited Loading

greg0ire commented Apr 12, 2023

eisberg commented Apr 12, 2023

greg0ire commented Apr 12, 2023 • edited Loading

eisberg commented Apr 12, 2023 • edited Loading

mpdude commented Apr 13, 2023

mpdude commented Apr 13, 2023

alexander-schranz commented Mar 1, 2023 •

edited

Loading

alexander-schranz commented Mar 1, 2023 •

edited

Loading

mpdude commented Mar 1, 2023 •

edited

Loading

mpdude commented Mar 2, 2023 •

edited

Loading

alexander-schranz commented Mar 2, 2023 •

edited

Loading

alexander-schranz commented Mar 2, 2023 •

edited

Loading

alexander-schranz commented Mar 26, 2023 •

edited

Loading

alexander-schranz commented Apr 12, 2023 •

edited

Loading

eisberg commented Apr 12, 2023 •

edited

Loading

greg0ire commented Apr 12, 2023 •

edited

Loading

eisberg commented Apr 12, 2023 •

edited

Loading