AnnotationDriver handles field mappings differently #5744
Assignees
Comments
|
See referenced issue DDC-34 = #4198 |
|
👀 needed regarding this in #10455 |
mpdude
added a commit
to mpdude/doctrine2
that referenced
this issue
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.~
mpdude
added a commit
to mpdude/doctrine2
that referenced
this issue
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#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. Two potential bugs that can result from this are demonstrated in 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#10450 and doctrine#10454 are merged into this PR to show that they 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.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this issue
May 8, 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#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. Two potential bugs that can result from this are demonstrated in 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#10450 and doctrine#10454 are merged into this PR to show that they 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.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this issue
May 8, 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#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. Two potential bugs that can result from this are demonstrated in 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#10450 and doctrine#10454 are merged into this PR to show that they 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. # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # Date: Tue Jan 24 20:28:32 2023 +0000 # # On branch fix-mapping-driver-load # Your branch is up-to-date with 'mpdude/fix-mapping-driver-load'. # # Changes to be committed: # modified: UPGRADE.md # modified: lib/Doctrine/ORM/Configuration.php # modified: lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php # modified: lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php # modified: lib/Doctrine/ORM/Mapping/Driver/AttributeDriver.php # new file: lib/Doctrine/ORM/Mapping/Driver/ReflectionBasedDriver.php # modified: lib/Doctrine/ORM/ORMSetup.php # modified: tests/Doctrine/Tests/DoctrineTestCase.php # modified: tests/Doctrine/Tests/Models/Company/CompanyFlexContract.php # modified: tests/Doctrine/Tests/ORM/ConfigurationTest.php # modified: tests/Doctrine/Tests/ORM/Functional/EnumTest.php # modified: tests/Doctrine/Tests/ORM/Functional/MergeProxiesTest.php # modified: tests/Doctrine/Tests/ORM/Functional/ReadonlyPropertiesTest.php # modified: tests/Doctrine/Tests/ORM/Functional/Ticket/DDC719Test.php # new file: tests/Doctrine/Tests/ORM/Functional/Ticket/GH10450Test.php # new file: tests/Doctrine/Tests/ORM/Functional/Ticket/GH10454Test.php # modified: tests/Doctrine/Tests/ORM/Functional/Ticket/GH5998Test.php # modified: tests/Doctrine/Tests/ORM/Mapping/AttributeDriverTest.php # modified: tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php # modified: tests/Doctrine/Tests/OrmFunctionalTestCase.php # modified: tests/Doctrine/Tests/OrmTestCase.php # # Untracked files: # phpunit.xml #
mpdude
added a commit
to mpdude/doctrine2
that referenced
this issue
May 8, 2023
…ses where they are declared This PR will make the annotations and attribute mapping drivers report mapping configuration for the classes where it is declared, instead of omitting it and reporting it for subclasses only. This is necessary to be able to catch mis-configurations in `ClassMetadataFactory`. Fixes doctrine#10417, closes doctrine#10450, closes doctrine#10454. ####⚠️ Summary for users getting `MappingExceptions` with the new mode When you set the `$reportFieldsWhereDeclared` constructor parameters to `true` for the AnnotationDriver and/or AttributesDriver and get `MappingExceptions`, you may be doing one of the following: * Using `private` fields with the same name in different classes of an entity inheritance hierarchy (see doctrine#10450) * Redeclaring/overwriting mapped properties inherited from mapped superclasses and/or other entities (see doctrine#10454) As explained in these two PRs, the ORM cannot (or at least, was not designed to) support such configurations. Unfortunately, due to the old – now deprecated – driver behaviour, the misconfigurations could not be detected, and due to previously missing tests, this in turn was not noticed. #### 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. Two potential bugs that can result from this are demonstrated in 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#10450 and doctrine#10454 are merged into this PR to show that they pass now. #### Soft deprecation strategy 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 need to follow up in other places (DoctrineBundle, ... – what else?) to make this driver parameter an easy-to-change configuration setting.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
A problem with a mapped superclass led me to this piece of code that conditionally maps fields in the
loadMetadataForClassmethod in the AnnotationDriver. See commit 09a48a8Other drivers like yaml or xml don't have these conditions, or not that I can see. Does anyone have more information about this or has encountered the same problems with overriding field mappings with annotations?
The text was updated successfully, but these errors were encountered: