Make Annotations/Attribute mapping drivers report fields for the classes where they are declared

[mpdude]

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.

⚠️ 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 Failing test: Duplicate private fields not detected #10450)
• Redeclaring/overwriting mapped properties inherited from mapped superclasses and/or other entities (see Failing test: Re-configured protected property not rejected #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:

orm/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php

Lines 345 to 357 in 69c7791

	// 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 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 #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 #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, 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.

#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 $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]

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.

[DemoniacDeath]

Please see BC break #10927

[mpdude]

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.

[mpdude]

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.

[greg0ire]

stab in the dark worked 👍

[greg0ire]

Potential breakage

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.

[mpdude]

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.

[mpdude]

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.

[mpdude]

Attempt to add the new config setting in doctrine/DoctrineBundle#1661

[janklan]

It's worth noting that the fix for the incorrect mapping is fairly straightforward, don't panic!

In a scenario of Something extends Base where Base defines a property that Something re-defines, all you have to do is removing the property from Something and override the association instead.

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.