[GH-8229] Prevent Illegal Inheritance Override #8348
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…only allowed for MappedSuperclass
…, only allowed for MappedSuperclass
…octrine#8329)" This reverts commit f4ebded.
greg0ire
reviewed
Nov 24, 2020
| // TODO: Deprecate overriding the fetch mode via association override for 3.0, | ||
| // users should do this with a listener and a custom attribute/annotation | ||
| throw MappingException::illegalOverrideOfInheritedProperty($this->name, $fieldName); | ||
| } |
There was a problem hiding this comment.
Should the parts that are about throwing be contributed to 2.8 instead? If we think there are people currently using this, we should probably not have it blow up in their face in a patch
There was a problem hiding this comment.
You are right, i should remove them for 2.7, and re-introduce them for 2.8
greg0ire
previously approved these changes
Nov 25, 2020
greg0ire
approved these changes
Nov 25, 2020
I think this update just severely broke my application. I'm still debugging, however, I use a ClassMetadataSubscriber to override the [name] field on the Entities to match a prefix. ie. You know how wordpess prefixes based on Anyway, I have a MappedSuperclass called.. "Building", and then I have on entity which extends it, called "Room". It appears now, as though the MappedSuperClass "Building" does not existing within the Is this intended? It loosk like all MapperSuperClasses have been removed, so any of my superclasses now do not exist with the vendor prefix. What ended up happening on my development machine was 20 table deletions.. so it's quite a big issue which I didn't expect from a minor patch? |
|
Please open up a new issue and improve the issue description, I don't understand mapped superclasses do not have tables, and should not have table prefixes. What means "not existing within LoadClassMetadataEventArgs"? |
|
#10431 suggests adding a runtime check for missing inheritance declarations |
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Jan 20, 2023
Inheritance has to be declared as soon as one entity class extends (directly or through middle classes) another one. This is also pointed out in the opening comment for doctrine#8348: > Entities are not allowed to extend from entities without an inheritence mapping relationship (Single Table or Joined Table inheritance). [...] While Doctrine so far allowed these things, they are fragile and will break on certain scenarios. Throwing an exception in case of this misconfiguration is nothing we should do light-heartedly, given that it may surprise users in a bugfix or feature release. So, we should start with a deprecation notice and make this an exception in 3.0. The documentation is updated accordingly at doctrine#10429. Catching missing inheritance declarations early on is important to avoid weird errors further down the road, giving users a clear indication of the root cause. In case you are affected by this, please understand that although things "previously worked" for you, you have been using the ORM outside of what it was designed to do. That may have worked in simple cases, but may also have caused invalid results (wrong or missing data after hydration?) that possibly went unnoticed in subtle cases.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Jan 23, 2023
Inheritance has to be declared as soon as one entity class extends (directly or through middle classes) another one. This is also pointed out in the opening comment for doctrine#8348: > Entities are not allowed to extend from entities without an inheritence mapping relationship (Single Table or Joined Table inheritance). [...] While Doctrine so far allowed these things, they are fragile and will break on certain scenarios. Throwing an exception in case of this misconfiguration is nothing we should do light-heartedly, given that it may surprise users in a bugfix or feature release. So, we should start with a deprecation notice and make this an exception in 3.0. The documentation is updated accordingly at doctrine#10429. Catching missing inheritance declarations early on is important to avoid weird errors further down the road, giving users a clear indication of the root cause. In case you are affected by this, please understand that although things "previously worked" for you, you have been using the ORM outside of what it was designed to do. That may have worked in simple cases, but may also have caused invalid results (wrong or missing data after hydration?) that possibly went unnoticed in subtle cases.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Jan 23, 2023
Inheritance has to be declared as soon as one entity class extends (directly or through middle classes) another one. This is also pointed out in the opening comment for doctrine#8348: > Entities are not allowed to extend from entities without an inheritence mapping relationship (Single Table or Joined Table inheritance). [...] While Doctrine so far allowed these things, they are fragile and will break on certain scenarios. Throwing an exception in case of this misconfiguration is nothing we should do light-heartedly, given that it may surprise users in a bugfix or feature release. So, we should start with a deprecation notice and make this an exception in 3.0. The documentation is updated accordingly at doctrine#10429. Catching missing inheritance declarations early on is important to avoid weird errors further down the road, giving users a clear indication of the root cause. In case you are affected by this, please understand that although things "previously worked" for you, you have been using the ORM outside of what it was designed to do. That may have worked in simple cases, but may also have caused invalid results (wrong or missing data after hydration?) that possibly went unnoticed in subtle cases.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Jan 24, 2023
Inheritance has to be declared as soon as one entity class extends (directly or through middle classes) another one. This is also pointed out in the opening comment for doctrine#8348: > Entities are not allowed to extend from entities without an inheritence mapping relationship (Single Table or Joined Table inheritance). [...] While Doctrine so far allowed these things, they are fragile and will break on certain scenarios. Throwing an exception in case of this misconfiguration is nothing we should do light-heartedly, given that it may surprise users in a bugfix or feature release. So, we should start with a deprecation notice and make this an exception in 3.0. The documentation is updated accordingly at doctrine#10429. Catching missing inheritance declarations early on is important to avoid weird errors further down the road, giving users a clear indication of the root cause. In case you are affected by this, please understand that although things "previously worked" for you, you have been using the ORM outside of what it was designed to do. That may have worked in simple cases, but may also have caused invalid results (wrong or missing data after hydration?) that possibly went unnoticed in subtle cases.
derrabus
pushed a commit
that referenced
this pull request
Jan 24, 2023
Inheritance has to be declared as soon as one entity class extends (directly or through middle classes) another one. This is also pointed out in the opening comment for #8348: > Entities are not allowed to extend from entities without an inheritence mapping relationship (Single Table or Joined Table inheritance). [...] While Doctrine so far allowed these things, they are fragile and will break on certain scenarios. Throwing an exception in case of this misconfiguration is nothing we should do light-heartedly, given that it may surprise users in a bugfix or feature release. So, we should start with a deprecation notice and make this an exception in 3.0. The documentation is updated accordingly at #10429. Catching missing inheritance declarations early on is important to avoid weird errors further down the road, giving users a clear indication of the root cause. In case you are affected by this, please understand that although things "previously worked" for you, you have been using the ORM outside of what it was designed to do. That may have worked in simple cases, but may also have caused invalid results (wrong or missing data after hydration?) that possibly went unnoticed in subtle cases.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Jan 26, 2023
…ridden This was brought up in doctrine#8348, but seemingly forgotten to be implenented in later versions. Closes doctrine#10289.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Feb 9, 2023
…ridden This was brought up in doctrine#8348, but seemingly forgotten to be implenented in later versions. Closes doctrine#10289.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PRs #8234 and #8329 are based on a flawed understanding of inheritance in Doctrine and need to be reverted:
Attribute/Association Overrides are only allowed to be used for MappedSuperclass and Traits. This is documented, albeit not phrased strongly enough: https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/reference/inheritance-mapping.html#overrides
Entities are not allowed to extend from entities without an inheritence mapping relationship (Single Table or Joined Table inheritance). If you have this use case you should instead have two entities and they each extend a common
@MappedSuperclass.One excemption was made to use
@AssocationOVerrideonly for changing the fetch strategy. Soft-deprecated this with a TODO.While Doctrine so far allowed these things, they are fragile and will break on certain scenarios. Please give feedback here or in new issues on the impact of this change on your projects.