Fix check for associations declared in MappedSuperclasses

[DanielCausebrook]

Doctrine does not allow toMany associations to be declared on MappedSuperclasses. The check for this occurs on the child class, which correctly checks whether the parent is a MappedSuperclass.
However, it does not check that the field in question was actually declared on that class. This causes an incorrect exception if there is an inheritance structure like so:

  Root:        Joined/Single Table Inheritance Entity
  Child:       Mapped Superclass
  Grandchild:  Entity

When there is a toMany association on Root, the Grandchild throws an exception, assuming that the association has come from Child.

This fix updates the check to also include whether the association has come from the parent MappedSuperclass in question.

I have added two tests. The first test is simply to check that an exception is not thrown in the case described above. The second check was to confirm that this change does not break the following case:

  Root:             Joined/Single Table Inheritance Entity
  Child:            Mapped Superclass
  Grandchild:       Mapped Superclass
  GreatGrandChild:  Entity

If the toMany association is on the Child, an exception should be thrown. I was worried my changes might prevent this from happening, but that is not the case and the test passes.

[DanielCausebrook]

I read that bugfixes are merged into the "lowest supported branch". I'm not sure exactly what this means, but if it helps, I'm running this fix successfully on Doctrine 2.4 with no issues. The only change is that the type declarations in the tests are replaced with docblock annotations. If you do merge fixes like this into old versions, I can provide that code as well.

[greg0ire]

I read that bugfixes are merged into the "lowest supported branch".

You did great, 2.12.x is the lowest supported branch. Please let me know where you read that, so that I can contribute a sentence helping people figure out what the lowest supported branch is.

[DanielCausebrook]

I read that bugfixes are merged into the "lowest supported branch".

You did great, 2.12.x is the lowest supported branch. Please let me know where you read that, so that I can contribute a sentence helping people figure out what the lowest supported branch is.

I read that on this page: https://www.doctrine-project.org/contribute/maintainer/. Admittedly, it's for maintainers - and I assume that maintainers would know more about these things in the first place.

[greg0ire]

Oh ok. There's also the contributor workflow, with this paragraph:

New pull requests are created with the repository's default branch as base branch. The default branch is the branch you see when you enter the repository page on GitHub.
In this DBAL example, it's the branch with the name 2.11.x. The branch name reflects the current lowest supported version of a repository.

[DanielCausebrook]

The workflow tests failed on php 7.2, which puzzles me since locally I can't even get compser to install with that php version. Regardless, I have attempted to resolve the issue by removing the int type declarations on $id fields and replacing them with @var int docblocks.

I'll see if the tests now pass and adjust further if necessary.

[DanielCausebrook]

I should mention that I'm not sure whether this line should be changed, since I'm unfamiliar with the ['inherited'] property.

If this change is correct, I see that there are similar lines at L356 and L405 that should probably be updated in the same way.

I will look into this more, sorry for missing it. Feel free to advise if someone knows about this.

[DanielCausebrook]

If I understand correctly, the ['inherited'] value is set when a property is inherited from something other than a MappedSuperclass.

Looking at this scenario:

  Root:             Joined/Single Table Inheritance Entity
  Child:            Mapped Superclass
  Grandchild:       Mapped Superclass
  GreatGrandChild:  Entity

If I understand correctly, putting a toOne association on Child should not set the ['inherited'] key of the association. However, this change does set that key: when the inherited associations are added to GreatGrandChild, $declaredInMappedSuperclass will be false. I think therefore that this line should be reverted?

This also indicates to me that the variable I added $declaredInMappedSuperclass is misleading, and the code should probably be adjusted to the following:

// L384
$declaredInParent =  && $mapping['declared'] === $parentClass->name;
if ($parentClass->isMappedSuperclass && $declaredInParent) {
    if ($mapping['type'] & ClassMetadata::TO_MANY && ! $mapping['isOwningSide']) {
        throw MappingException::illegalToManyAssociationOnMappedSuperclass($parentClass->name, $field);
    }

    $mapping['sourceEntity'] = $subClass->name;
}

//$subclassMapping = $mapping;
if (! isset($mapping['inherited']) && ! $parentClass->isMappedSuperclass) {
    $mapping['inherited'] = $parentClass->name;
}

If my analysis is correct, I can fix the code and add a relevant test to catch this issue in the future. Can someone confirm this?

[DanielCausebrook]

I've pushed the changes mentioned above as commit 4095ff6, including an extra test to cover this case.

[mpdude]

Hey @DanielCausebrook,

I've worked on that part during the last days.

Could you please check if your problem persists on the 2.14.x (dev) branch?

Also, I suspect there are some issues with that mapping validation check, but I was unable to get deeper into it. See #10398 and #10417.

[mpdude]

I think the conditions for detecting the mis-configuration and now right in the ClassMetadataFactory on 2.14. Still, the mapping driver won't report the problematic association in the first place, so the error is not detected.

#10455 might solve that.

[github-actions]

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

[github-actions]

This pull request was closed due to inactivity.