Skip to content

Fix fields of transient classes being considered duplicate with reportFieldsWhereDeclared#11769

Merged
greg0ire merged 1 commit intodoctrine:2.20.xfrom
HypeMC:fix-reportfieldswheredeclared
Jan 20, 2025
Merged

Fix fields of transient classes being considered duplicate with reportFieldsWhereDeclared#11769
greg0ire merged 1 commit intodoctrine:2.20.xfrom
HypeMC:fix-reportfieldswheredeclared

Conversation

@HypeMC
Copy link
Copy Markdown
Contributor

@HypeMC HypeMC commented Dec 18, 2024

When I enabled the reportFieldsWhereDeclared option introduced in #10455, I encountered a problem when using transient classes. For example, the following code produces an error:

abstract class AbstractEntity
{
    #[ORM\Column]
    #[ORM\Id]
    #[ORM\GeneratedValue]
    protected int $id;
}

#[ORM\Entity]
#[ORM\InheritanceType('SINGLE_TABLE')]
#[ORM\DiscriminatorMap(['cat' => Cat::class])]
#[ORM\DiscriminatorColumn(name: 'type')]
abstract class Animal extends AbstractEntity
{
    #[ORM\Column]
    private string $field;
}

#[ORM\Entity]
class Cat extends Animal
{
}
Doctrine\ORM\Mapping\MappingException: Duplicate definition of column 'id' on entity 'Cat' 
in a field or discriminator column mapping.

To my understanding, this shouldn't happen since AbstractEntity is a transient class. Perhaps @mpdude, as the original author, could shed some light on this.

@HypeMC
Copy link
Copy Markdown
Contributor Author

HypeMC commented Jan 20, 2025

@greg0ire @mpdude Sorry to bother you, but could you possibly take a look at this PR? I'm fairly certain this is a bug.

@greg0ire greg0ire added the Bug label Jan 20, 2025
@greg0ire greg0ire added this to the 2.20.2 milestone Jan 20, 2025
@greg0ire greg0ire merged commit 5ad5b11 into doctrine:2.20.x Jan 20, 2025
@greg0ire
Copy link
Copy Markdown
Member

greg0ire commented Jan 20, 2025

Thanks @HypeMC !

@HypeMC HypeMC deleted the fix-reportfieldswheredeclared branch January 21, 2025 08:01
@github-actions github-actions bot mentioned this pull request Feb 4, 2025
Merged
@mpdude
Copy link
Copy Markdown
Contributor

mpdude commented Sep 4, 2025

I know I remained silent back in January 😉, so I am late to the party.

Now while investigating #11488, I came across the change here and wonder whether it was the right thing to do.

I think a transient class that is neither a MappedSuperclass nor an Entity – like AbstractEntity in the example given above – should not be considered a valid source of mapping configuration in the first place.

This is clearly stated here:

At least when using attributes or annotations to specify your mapping, it seems as if you could inherit from a base class that is neither an entity nor a mapped superclass, but has properties with mapping configuration on them that would also be used in the inheriting class.
This, however, is due to how the corresponding mapping drivers work and what the PHP reflection API reports for inherited fields. Such a configuration is explicitly not supported.

https://www.doctrine-project.org/projects/doctrine-orm/en/2.21/reference/inheritance-mapping.html#:~:text=the%20owning%20sides.-,At%20least%20when%20using,-attributes%20or%20annotations

The code added here in parts defeated the intention to make sure we can clearly trace back every field to an entity or mapped superclass. My feeling is that we should instead fail with a clear error message stating that a field was found that does not come from a mapped class, instead of somehow running into seemingly duplicate definitions.

mpdude
mpdude reviewed Sep 4, 2025
View reviewed changes
src/Mapping/Driver/ReflectionBasedDriver.php
/** @var class-string $declaringClass */
$declaringClass = $property->class;

if ($this->isTransient($declaringClass)) {
Copy link
Copy Markdown
Contributor

@mpdude mpdude Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more issue here: This assumes that the same mapping driver that is working on $property can also judge whether $declaringClass is transient.

This may not work out e. g. when using different mapping drivers, for example mixing XML and attribute based configuration in different namespaces.

Example: The ReflectionBasedDriver will falsely assume that $declaringClass is transient when it is in fact mapped through XML.

@beberlei
Copy link
Copy Markdown
Member

beberlei commented Sep 4, 2025

I think a transient class that is neither a MappedSuperclass nor an Entity – like AbstractEntity in the example given above – should not be considered a valid source of mapping configuration in the first place.

Yes, this is what I would say as well.

mpdude added a commit to mpdude/doctrine2 that referenced this pull request Oct 9, 2025
@mpdude
Revert "Fix fields of transient classes being considered duplicate wi…
2ad720b 
…th `reportFieldsWhereDeclared` (doctrine#11769)"

This reverts commit 4feaa47.
@mpdude mpdude mentioned this pull request Oct 9, 2025
Merged
@mpdude
Copy link
Copy Markdown
Contributor

mpdude commented Oct 9, 2025

I am suggesting to revert this change in #12212.

greg0ire added a commit that referenced this pull request Oct 10, 2025
@greg0ire
Merge pull request #12212 from mpdude/revert-11769
930a790 
Revert "Fix fields of transient classes being considered duplicate with `reportFieldsWhereDeclared` (#11769)"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mpdude mpdude mpdude left review comments

@greg0ire greg0ire greg0ire approved these changes

@SenseException SenseException SenseException approved these changes

@derrabus derrabus Awaiting requested review from derrabus

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

2.20.2

Development

Successfully merging this pull request may close these issues.

5 participants

@HypeMC @greg0ire @mpdude @beberlei @SenseException