Failing test: Illegal to-many association on MappedSuperclass not rejected #10449
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
mpdude
force-pushed
the
demo-fail-reject-invalid-association
branch
from
bb78319 to
87f85ab
Compare
…ected This test case demonstrates a situation where an illegal to-many association on a mapped superclass is not being detected. Since at least the annotations and attribute drivers currently skip non-private properties from mapped superclasses (see doctrine#10417), the `ClassMetadataFactory` is not aware of the association on the mapped superclass at all and can not reject it. IMHO, we should bail out as soon as we detect this on the mapped superclass. I don't see how we could proceed at that time and try to rescue/fixup the situation afterwards. More details on what happens down the road, just for the sake of completeness: When the mapping driver processes the entity class with the inherited association, PHP reflection will report the association property. Since the association was not reported for the mapped superclass, the driver thinks (see doctrine#10417) this is a "new" (non-inherited) field and reports to the `ClassMetadataFactory`. The association will show up as being from `GH10449Entity` to `GH10449ToManyAssociationTarget`. On `GH10449ToManyAssociationTarget`, it will refer back to `GH10449MappedSuperclass`.
mpdude
force-pushed
the
demo-fail-reject-invalid-association
branch
from
87f85ab to
ac33652
Compare
mpdude
commented
Jan 24, 2023
| public $id; | ||
|
|
||
| /** | ||
| * @ORM\ManyToOne(targetEntity="GH10449MappedSuperclass", inversedBy="targets") |
There was a problem hiding this comment.
A mapped superclass cannot be a target entity at all, can it?
mpdude
commented
Jan 24, 2023
| public $id; | ||
|
|
||
| /** | ||
| * @ORM\OneToMany(targetEntity="GH10449ToManyAssociationTarget", mappedBy="base") |
There was a problem hiding this comment.
Not allowed on a mapped superclass, since the mapped superclass has no table that the foreign key from the other side could refer to.
This was referenced Jan 24, 2023
Closed
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Jan 25, 2023
…hopefully they all pass now
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
May 5, 2023
…ses where they are declared This PR will make the annotations and attribute mapping drivers more consistently report mapping configuration for the classes where it is declared. This is necessary to be able to catch mis-configurations in `ClassMetadataFactory`. Fixes doctrine#10417, closes doctrine#10449, closes doctrine#10450, closes doctrine#10454. #### Current situation The annotations mapping driver has the following condition to skip properties that are reported by the PHP reflection API: https://github.com/doctrine/orm/blob/69c7791ba256d947ddb1aafe5f2439ab31704937/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php#L345-L357 This code has been there basically unchanged since the initial 2.0 release. The same condition can be found in the attribute driver, probably it has been copied when attributes were added. I _think_ what the driver tries to do here is to deal with the fact that Reflection will also report `public`/`protected` properties inherited from parent classes. This is supported by the observation (see doctrine#5744) that e. g. YAML and XML drivers do not contain this logic. The conditions are not precise enough for edge cases. They lead to some fields and configuration values not even being reported by the driver. Only since the fields would be "discovered" again when reflecting on subclasses, they eventually end up in class metadata structures for the subclasses. In one case of inherited ID generator mappings, the `ClassMetadataFactory` would also rely on this behaviour. A few of the bugs that can result from this are demonstrated in doctrine#10449, doctrine#10450 and doctrine#10454. #### Suggested solution In order to find a more reliable way of separating properties that are merely reported again in subclasses from those that are actual re-declarations, use the information already available in `ClassMetadata`. In particular, `declared` tells us in which non-transient class a "field" was first seen. Make the mapping driver skip only those properties for which we already know that they have been declared in parent classes, and skip them only when the observed declaring class matches the expectation. For all other properties, report them to `ClassMetadataFactory` and let that deal with consistency checking/error handling. doctrine#10449, doctrine#10450 and doctrine#10454 are merged into this PR to show that they all pass now. #### Soft deprecation strategy When users re-declare (overwrite) mapped properties inherited from mapped superclasses and/or other entities, the new behaviour may cause their mapping configuration to be rejected as invalid. This applies only to configurations that were never allowed as per the documentation. For some cases, we missed the opportunity to reject the configuration with an exception early on. In other cases, we had the exception-throwing code in place, but due to the driver's behaviour, it was never reached. To avoid throwing new/surprising exceptions (even for misconfigurations) during a minor version upgrade, the new driver mode is opt-in. Users will have to set the `$reportFieldsWhereDeclared` constructor parameters to `true` for the `AnnotationDriver` and/or `AttributesDriver`. Unless they do so, a deprecation warning will be raised. In 3.0, the "new" mode will become the default. The constructor parameter can be deprecated (as of ORM 3.1, probably) and is a no-op. We should follow up in other places (DoctrineBundle, ... – what else?) to make this driver parameter an easy-to-change configuration setting. #### Relationship to doctrine#10473 Update: doctrine#10473 was merged ~In doctrine#10473, an example is given where code that currently works will break because the CMF will now see and reject an association mapping that it missed before. I think that the example given is valid, and that the check which will suddenly become effective is, in fact, too strict. So, my recommendation is to treat both PRs as closely related, even though both are valid and can be reviewed in their own right.~
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.
Closing since this is no longer an error condition we need to check for.
#10473 relaxed the runtime check that mapped superclasses must not use one-to-many associations. It turned the check the other way round, so that no mapped superclass may be used as the targetEntity of an association. (In addition, #10554 also made it a "lazy" (non-runtime) check in the schema validator.)
This test case demonstrates a situation where an invalid to-many association on a mapped superclass is not being detected.
Since at least the annotations and attribute drivers currently skip non-private properties from mapped superclasses (see #10417), the
ClassMetadataFactoryis not even aware of the association on the mapped superclass and so cannot reject it.IMHO, we should bail out as soon as we detect this on the mapped superclass. I don't see how we could proceed at that time and try to rescue/fixup the situation afterwards.
In addition to that, maybe an association with a
targetthat is a mapped superclass should be rejected in its own right?More details on what happens down the road, just for the sake of completeness:
When the mapping driver processes the entity class with the inherited association, PHP reflection will report the association property. Since the association was not reported for the mapped superclass, the driver thinks (see #10417) this is a "new" (non-inherited) field and reports to the
ClassMetadataFactory.The association will show up as being from
GH10449EntitytoGH10449ToManyAssociationTarget. OnGH10449ToManyAssociationTarget, it will refer back toGH10449MappedSuperclass.