Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Weird conditional skipping of mapping information in attribute/annotations driver #10417

Closed
mpdude opened this issue Jan 17, 2023 · 4 comments · Fixed by #10455
Closed

Weird conditional skipping of mapping information in attribute/annotations driver #10417

mpdude opened this issue Jan 17, 2023 · 4 comments · Fixed by #10455

Comments

@mpdude
Copy link
Contributor

mpdude commented Jan 17, 2023

I am looking into an issue where an invalid association definition on a mapped superclass is not rejected as it should be.

It turns out the assocation is not even reported by the mapping driver, due to the following lines of code.

// Evaluate annotations on properties/fields
foreach ($class->getProperties() as $property) {
if (
$metadata->isMappedSuperclass && ! $property->isPrivate()
||
$metadata->isInheritedField($property->name)
||
$metadata->isInheritedAssociation($property->name)
||
$metadata->isInheritedEmbeddedClass($property->name)
) {
continue;
}

This is present in the attribute driver as well. To add insult to injury, the thing I am trying to chase depends on wheter an association is mapped in a public or private property.

I have seen this exact piece of code has been questioned in #5744 before, but neither that nor the reference to #4198 help.

Can anybody help reasoning about this – what's the purpose of these continuations, and what does it have to do with private in mapped superclasses?

Maybe it has to do with that the Reflection API also reports non-private properties inherited from base classes, but I don't get it.

How do we want to treat such inherited properties, and/or can we tell whether a property was just inherited or inhertied + redeclared?

@mpdude
Copy link
Contributor Author

mpdude commented Jan 19, 2023

X-Ref #9702

mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 24, 2023
…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 added a commit to mpdude/doctrine2 that referenced this issue Jan 24, 2023
This test case demonstrates a situation where two entities inherit from each other. Both entity classes have a private property named `field`. (Inheritance is set up correctly, but that is irrelevant here.)

Technically, that's two properties on two separate classes. Also, the mapping configuration given suggests to put both properties into separate database columns.

I think this violates fundamental design assumptions, since `ClassMetadata::$fieldMappings` is indexed by "field name" – there is no room for juggling with duplicate private fields being declared in different classes. My guess is this is a design limitation that would be very hard to change, should be documented as a limitation and be rejected when it is noticed.

The same applies to association mappings and embeddables.

The current situation, however, is that the property and mapping configuration will be picked up from the first (parent) entity class.

At least the annotation and attribute mapping driver will then silently skip the property on the subclass (see doctrine#10417).

Mapping configuration for `GH10450ChildClass::$field` will never be considered. This property will (silently) remain unmapped.
@mpdude
Copy link
Contributor Author

mpdude commented Jan 24, 2023

#10449, #10450 and #10454 show different ways how the conditions from the OP fail.

@mpdude
Copy link
Contributor Author

mpdude commented Jan 25, 2023

#10455 tries to fix the mapping driver.

mpdude added a commit to mpdude/doctrine2 that referenced this issue Feb 5, 2023
…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 added a commit to mpdude/doctrine2 that referenced this issue Feb 5, 2023
This test case demonstrates a situation where two entities inherit from each other. Both entity classes have a private property named `field`. (Inheritance is set up correctly, but that is irrelevant here.)

Technically, that's two properties on two separate classes. Also, the mapping configuration given suggests to put both properties into separate database columns.

I think this violates fundamental design assumptions, since `ClassMetadata::$fieldMappings` is indexed by "field name" – there is no room for juggling with duplicate private fields being declared in different classes. My guess is this is a design limitation that would be very hard to change, should be documented as a limitation and be rejected when it is noticed.

The same applies to association mappings and embeddables.

The current situation, however, is that the property and mapping configuration will be picked up from the first (parent) entity class.

At least the annotation and attribute mapping driver will then silently skip the property on the subclass (see doctrine#10417).

Mapping configuration for `GH10450ChildClass::$field` will never be considered. This property will (silently) remain unmapped.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Feb 5, 2023
…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 added a commit to mpdude/doctrine2 that referenced this issue Feb 5, 2023
This test case demonstrates a situation where two entities inherit from each other. Both entity classes have a private property named `field`. (Inheritance is set up correctly, but that is irrelevant here.)

Technically, that's two properties on two separate classes. Also, the mapping configuration given suggests to put both properties into separate database columns.

I think this violates fundamental design assumptions, since `ClassMetadata::$fieldMappings` is indexed by "field name" – there is no room for juggling with duplicate private fields being declared in different classes. My guess is this is a design limitation that would be very hard to change, should be documented as a limitation and be rejected when it is noticed.

The same applies to association mappings and embeddables.

The current situation, however, is that the property and mapping configuration will be picked up from the first (parent) entity class.

At least the annotation and attribute mapping driver will then silently skip the property on the subclass (see doctrine#10417).

Mapping configuration for `GH10450ChildClass::$field` will never be considered. This property will (silently) remain unmapped.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Feb 14, 2023
…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 added a commit to mpdude/doctrine2 that referenced this issue Feb 14, 2023
This test case demonstrates a situation where two entities inherit from each other. Both entity classes have a private property named `field`. (Inheritance is set up correctly, but that is irrelevant here.)

Technically, that's two properties on two separate classes. Also, the mapping configuration given suggests to put both properties into separate database columns.

I think this violates fundamental design assumptions, since `ClassMetadata::$fieldMappings` is indexed by "field name" – there is no room for juggling with duplicate private fields being declared in different classes. My guess is this is a design limitation that would be very hard to change, should be documented as a limitation and be rejected when it is noticed.

The same applies to association mappings and embeddables.

The current situation, however, is that the property and mapping configuration will be picked up from the first (parent) entity class.

At least the annotation and attribute mapping driver will then silently skip the property on the subclass (see doctrine#10417).

Mapping configuration for `GH10450ChildClass::$field` will never be considered. This property will (silently) remain unmapped.
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
This test case demonstrates a situation where two entities inherit from each other. Both entity classes have a private property named `field`. (Inheritance is set up correctly, but that is irrelevant here.)

Technically, that's two properties on two separate classes. Also, the mapping configuration given suggests to put both properties into separate database columns.

I think this violates fundamental design assumptions, since `ClassMetadata::$fieldMappings` is indexed by "field name" – there is no room for juggling with duplicate private fields being declared in different classes. My guess is this is a design limitation that would be very hard to change, should be documented as a limitation and be rejected when it is noticed.

The same applies to association mappings and embeddables.

The current situation, however, is that the property and mapping configuration will be picked up from the first (parent) entity class.

At least the annotation and attribute mapping driver will then silently skip the property on the subclass (see doctrine#10417).

Mapping configuration for `GH10450ChildClass::$field` will never be considered. This property will (silently) remain unmapped.
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.
@mpdude
Copy link
Contributor Author

mpdude commented May 8, 2023

Closing, since #10455 has been merged and fixes this.

@mpdude mpdude closed this as completed May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant