Make Annotations/Attribute mapping drivers report fields for the classes where they are declared#10455
Conversation
| // If this class has a parent the id generator strategy is inherited. | ||
| // However this is only true if the hierarchy of parents contains the root entity, | ||
| // if it consists of mapped superclasses these don't necessarily include the id field. | ||
| if ($parent && $rootEntityFound) { | ||
| $this->inheritIdGeneratorMapping($class, $parent); | ||
| } else { | ||
| $this->completeIdGeneratorMapping($class); | ||
| } |
There was a problem hiding this comment.
The code was based on the assumption that we can only inherit the ID generator mapping when we have already seen an entity class above in the hierarchy, because at that time the ID must have been defined.
As long as we're working downwards the hierarchy and have only seen mapped superclasses so far, it would not inherit the generator mapping, but instead fill in the defaults.
This did not make a difference, since the "id" field would be reported again by the mapping driver as soon as we find an entity class, this time initializing the settings correctly.
With the mapping driver now reporting the ID field only once at the time where it occurs in the class hierarchy, we need to inherit the generator settings right away, as all other inherited configuration.
| $this->assertSqlGeneration( | ||
| 'SELECT f FROM Doctrine\Tests\Models\DirectoryTree\File f JOIN f.parentDirectory d WHERE f.id = ?1', | ||
| 'SELECT f0_.id AS id_0, f0_.extension AS extension_1, f0_.name AS name_2 FROM "file" f0_ INNER JOIN Directory d1_ ON f0_.parentDirectory_id = d1_.id WHERE f0_.id = ?' | ||
| 'SELECT f0_.id AS id_0, f0_.name AS name_1, f0_.extension AS extension_2 FROM "file" f0_ INNER JOIN Directory d1_ ON f0_.parentDirectory_id = d1_.id WHERE f0_.id = ?' |
There was a problem hiding this comment.
In this test, there are some fields in an abstract mapped superclass, and an additional field in the entity subclass.
Previously, the mapping driver would not report the fields for the superclass (they were not contained in the class metadata!). Only on the subclass, PHP reflection would provide the properties again, and this time they end up in the subclass metadata.
With the change suggested in this PR, the fields are now (IMHO correctly) associated with the mapped superclass and then inherited as metadata to the child class, whereare the duplicate reports by PHP reflection (for the subclass) are ignored.
That causes the order of fields in the generated SQL statement to change.
| $q = $this->_em->createQuery('SELECT g, c FROM Doctrine\Tests\ORM\Functional\Ticket\DDC719Group g LEFT JOIN g.children c WHERE g.parents IS EMPTY'); | ||
|
|
||
| $referenceSQL = 'SELECT g0_.name AS name_0, g0_.description AS description_1, g0_.id AS id_2, g1_.name AS name_3, g1_.description AS description_4, g1_.id AS id_5 FROM groups g0_ LEFT JOIN groups_groups g2_ ON g0_.id = g2_.parent_id LEFT JOIN groups g1_ ON g1_.id = g2_.child_id WHERE (SELECT COUNT(*) FROM groups_groups g3_ WHERE g3_.child_id = g0_.id) = 0'; | ||
| $referenceSQL = 'SELECT g0_.id AS id_0, g0_.name AS name_1, g0_.description AS description_2, g1_.name AS name_3, g1_.description AS description_4, g1_.id AS id_5 FROM groups g0_ LEFT JOIN groups_groups g2_ ON g0_.id = g2_.parent_id LEFT JOIN groups g1_ ON g1_.id = g2_.child_id WHERE (SELECT COUNT(*) FROM groups_groups g3_ WHERE g3_.child_id = g0_.id) = 0'; |
There was a problem hiding this comment.
With the mapping driver changes, the ID column is seen as coming from the mapped superclass at the root of the hierarchy, not as a field of the child entity.
44e5653 to
c9c7d67
Compare
|
stab in the dark worked 👍 |
03f39f9 to
3363e4b
Compare
Were the bugs you're fixing here introduced in 2.14.x? If not, maybe targeting 2.15.x would be wiser, because otherwise, people with an application that works despite wrong configuration cannot update without a change to their application. If you feel this "bugfix" is going to generate a lot of support, then please target 2.15.x. patch releases should make the software more stable IMO. |
|
You’re right, I will rebase this onto 2.15. It‘s a bug that has been sitting there for a long time, so no need to hurry and surprise users in a bugfix release. |
fe1e574 to
94a4234
Compare
94a4234 to
f939045
Compare
|
All feedback taken care of, also rebased onto 2.15.x and squashed most of the commits. I kept the tests merged from other PRs (where they failed, here they pass) as separate commits. |
This adds a new config setting `report_fields_where_declared` that shall make it easier for users to address a deprecation added in Doctrine ORM 2.16, more specifically in doctrine/orm#10455. I think that since this bundle allows to specify the mapping configuration per entity manager, it would make sense to have this new config setting at the entity manager level as well.
This adds a new config setting `report_fields_where_declared` that shall make it easier for users to address a deprecation added in Doctrine ORM 2.16, more specifically in doctrine/orm#10455. I think that since this bundle allows to specify the mapping configuration per entity manager, it would make sense to have this new config setting at the entity manager level as well.
|
Attempt to add the new config setting in doctrine/DoctrineBundle#1661 |
This adds a new config setting `report_fields_where_declared` that shall make it easier for users to address a deprecation added in Doctrine ORM 2.16, more specifically in doctrine/orm#10455. I think that since this bundle allows to specify the mapping configuration per entity manager, it would make sense to have this new config setting at the entity manager level as well.
This adds a new config setting `report_fields_where_declared` that makes it easy to opt-in to the new mapping driver mode that was added in Doctrine ORM 2.16 and will be mandatory in ORM 3.0. For details, see doctrine/orm#10455. I think that since this bundle allows to specify the mapping configuration per entity manager, it would make sense to have this new config setting at the entity manager level as well.
This adds a new config setting `report_fields_where_declared` that makes it easy to opt-in to the new mapping driver mode that was added in Doctrine ORM 2.16 and will be mandatory in ORM 3.0. For details, see doctrine/orm#10455. I think that since this bundle allows to specify the mapping configuration per entity manager, it would make sense to have this new config setting at the entity manager level as well.
This adds a new config setting `report_fields_where_declared` that makes it easy to opt-in to the new mapping driver mode that was added in Doctrine ORM 2.16 and will be mandatory in ORM 3.0. For details, see doctrine/orm#10455. I think that since this bundle allows to specify the mapping configuration per entity manager, it would make sense to have this new config setting at the entity manager level as well.
|
It's worth noting that the fix for the incorrect mapping is fairly straightforward, don't panic! In a scenario of In my case, I had a base class defining uni-directional ManyToOne association and some classes adding an inverse side to it - which is the invalid mapping this PR is about. It means that this code: use Doctrine\ORM\Mapping as ORM;
#[ORM\MappedSuperClass]
class BaseClass {
#[ORM\ManyToOne]
#[ORM\JoinColumn(name: 'organisation_id', nullable: false, onDelete: 'CASCADE')]
protected Organisation $organisation;
}
#[ORM\Entity]
class ExtendingClass extends BaseClass {
#[ORM\ManyToOne(inversedBy: 'labels')]
#[ORM\JoinColumn(name: 'organisation_id', nullable: false, onDelete: 'CASCADE')]
protected Organisation $organisation;
}simply had to become // only ExtendingClass changed
#[ORM\Entity]
#[ORM\AssociationOverrides([
new ORM\AssociationOverride(name: 'organisation', inversedBy: 'labels'),
])]
class ExtendingClass extends BaseClass {
// no more $organisation
}I've been putting this off for almost a year, thinking it's a big deal, but it's not. |
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 #10417, closes #10450, closes #10454.
DoctrineBundle users: A new config setting to opt-in to the new mode will be implemented in doctrine/DoctrineBundle#1661.
MappingExceptionswith the new modeWhen you set the
$reportFieldsWhereDeclaredconstructor parameters totruefor the AnnotationDriver and/or AttributesDriver and getMappingExceptions, you may be doing one of the following:privatefields with the same name in different classes of an entity inheritance hierarchy (see Failing test: Duplicate private fields not detected #10450)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:
orm/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php
Lines 345 to 357 in 69c7791
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/protectedproperties inherited from parent classes. This is supported by the observation (see #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
ClassMetadataFactorywould also rely on this behaviour.Two potential bugs that can result from this are demonstrated in #10450 and #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,declaredtells 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
ClassMetadataFactoryand let that deal with consistency checking/error handling.#10450 and #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
$reportFieldsWhereDeclaredconstructor parameters totruefor theAnnotationDriverand/orAttributesDriver. 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.