Skip to content

Fix regression with an edge case for the CMF::peekIfIsMappedSuperclass() method#10557

Closed
mpdude wants to merge 1 commit intodoctrine:2.15.xfrom
mpdude:peek-superclass-with-override
Closed

Fix regression with an edge case for the CMF::peekIfIsMappedSuperclass() method#10557
mpdude wants to merge 1 commit intodoctrine:2.15.xfrom
mpdude:peek-superclass-with-override

Conversation

@mpdude
Copy link
Copy Markdown
Contributor

@mpdude mpdude commented Mar 2, 2023

This comment hints to a case where the ClassMetadataFactory::peekIfIsMappedSuperclass() method introduced in #10411 causes a failure.

CMF::peekIfIsMappedSuperclass() has to perform improvised metadata loading in a situation where the CMF is currently loading a class. So, we cannot use the full/real ClassMetadataFactory mechanisms, since it would require a re-entry for a subclass of the current class, causing an infinite loop (loads parent classes first, and that's what we're currently doing).

The problem is that the improvised call to $driver->loadMetadataForClass() cannot provide a pre-filled ClassMetadata instance populated with all parent class fields and associations. But, when attribute or association overrides are used, a check is made to see if the overridden field/association actually exists, and this information is missing in that situation.

This PR suggests to override the methods to get around this. In fact, we do not care about all these details, we only want to ask the driver if the class is a mapped superclass or not.

A much better fix would be to have a dedicated method on the driver to ask it just that particular question (also better performance-wise). But I do not see how we could get that done in a BC way – ideas? 💡

A few things that need to come together to make the bug surface:

  • Load an entity declaring an inheritance tree
  • There must be a mapped superclass in the inheritance tree to provide the field that shall be overriden
  • An entity class must inherit from the mapped superclass and override the field
  • That entity class must be an abstract, intermediate class not be declared in the discriminator map so we can "discover" it
  • The overriden property must be private so the mapping drivers (using reflection) do not see it when looking at the overriding entity class.

@mpdude mpdude force-pushed the peek-superclass-with-override branch from 70fc74b to 5408ffd Compare March 2, 2023 08:53
@mpdude mpdude changed the title Fix an edge case in the CMF::peekIfIsMappedSuperclass() method Fix regression with an edge case for the CMF::peekIfIsMappedSuperclass() method Mar 2, 2023
@mpdude mpdude force-pushed the peek-superclass-with-override branch from 5408ffd to a36868d Compare March 2, 2023 09:20
@mpdude mpdude marked this pull request as ready for review March 2, 2023 09:20
@mpdude
Copy link
Copy Markdown
Contributor Author

mpdude commented Mar 7, 2023

@derrabus Can you tell me how to proceed with Psalm errors here?

@derrabus
Copy link
Copy Markdown
Member

derrabus commented Mar 7, 2023

Your class extends ClassMetadata which is generic. You need to add a @extends annotation that specifies the template parameters for ClassMetadata.

@mpdude
Copy link
Copy Markdown
Contributor Author

mpdude commented Mar 7, 2023

Thank you, will 👀

@mpdude mpdude force-pushed the peek-superclass-with-override branch 3 times, most recently from 72a8f3e to 205a50a Compare March 8, 2023 09:04
@mpdude
Copy link
Copy Markdown
Contributor Author

mpdude commented Mar 8, 2023

@derrabus The remaining Psalm error seems to be unrelated to my changes...?

Ah, I get it!

I am consuming one of the baseline entries with the change I made, so we're out of baseline entries at the end of the file and the error pops up there.

…lassMetadataFactory::peekIfIsMappedSuperclass()` method introduced in doctrine#10411 causes a failure.

`CMF::peekIfIsMappedSuperclass()` has to perform improvised metadata loading in a situation where the CMF is currently loading a class. So, we cannot use the full/real `ClassMetadataFactory` mechanisms, since it would require a re-entry for a subclass of the current class, causing an infinite loop (loads parent classes first, and that's what we're currently doing).

The problem is that the improvised call to `$driver->loadMetadataForClass()` cannot provide a pre-filled `ClassMetadata` instance populated with all parent class fields and associations. But, when attribute or association overrides are used, a check is made to see if the overridden field/association actually exists, and this information is missing in that situation.

This PR suggests to override the methods to get around this. In fact, we do not care about all these details, we only want to ask the driver if the class is a mapped superclass or not.

A much better fix would be to have a dedicated method on the driver to ask it just that particular question (also better performance-wise). But I do not see how we could get that done in a BC way – ideas? 💡

A few things that need to come together to make the bug surface:

* Load an entity declaring an inheritance tree
* There must be a mapped superclass in the inheritance tree to provide the field that shall be overriden
* An entity class must inherit from the mapped superclass and override the field
* That entity class must be an abstract, intermediate class not be declared in the discriminator map so we can "discover" it
* The overriden property must be private so the mapping drivers (using reflection) do not see it when looking at the overriding entity class.
@mpdude mpdude force-pushed the peek-superclass-with-override branch from 205a50a to 50d6451 Compare March 8, 2023 09:10
@greg0ire greg0ire changed the base branch from 2.14.x to 2.15.x June 25, 2023 16:16
@greg0ire
Copy link
Copy Markdown
Member

But I do not see how we could get that done in a BC way – ideas?

Add the method to all existing drivers, and deprecate not adding it to a driver (and fallback to the old behavior when that's the case).

You can also add an @method annotation to the interface of drivers so that static analysis lets people know that they should implement the new method.

@github-actions
Copy link
Copy Markdown
Contributor

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Oct 29, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 6, 2024

This pull request was closed due to inactivity.

@github-actions github-actions bot closed this Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants