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

Doctrine ORM 2.9: MappedSuperclass in entity hierarchy #8771

Closed
BenMorel opened this issue Jun 16, 2021 · 16 comments · Fixed by #8903
Closed

Doctrine ORM 2.9: MappedSuperclass in entity hierarchy #8771

BenMorel opened this issue Jun 16, 2021 · 16 comments · Fixed by #8903

Comments

@BenMorel
Copy link
Contributor

BenMorel commented Jun 16, 2021

We have an entity hierarchy similar to:

/**
 * @ORM\Entity
 * @ORM\Table(name="AbstractInvoiceItem")
 *
 * @ORM\InheritanceType("JOINED")
 * @ORM\DiscriminatorColumn(name="type")
 * @ORM\DiscriminatorMap({
 *     "SetupFee" = SetupFee::class
 * })
 */
abstract class AbstractInvoiceItem
{
}

/**
 * @MappedSuperclass
 */
abstract class AbstractProductInvoiceItem extends AbstractInvoiceItem
{
    /**
     * @ORM\ManyToOne(targetEntity=AbstractProduct::class)
     */
    protected ?AbstractProduct $product;
}

/**
 * @ORM\Entity
 * @ORM\Table(name=SetupFee")
 */
class SetupFee extends AbstractProductInvoiceItem
{
}

This was working fine with Doctrine 2.8.

Since we upgraded to Doctrine 2.9 however, bin/console doctrine:schema:validate started failing with:

[FAIL] The entity-class App(...)\AbstractProductInvoiceItem mapping is invalid:

  • Entity class 'App(...)\AbstractProductInvoiceItem' is part of inheritance hierarchy, but is not mapped in the root entity 'App(...)\AbstractInvoiceItem' discriminator map. All subclasses must be listed in the discriminator map.

I tried adding a "fake" entry to the discriminator map for the abstract class:

/*
 * @ORM\DiscriminatorMap({
 *     "AbstractProductInvoiceItem" = AbstractProductInvoiceItem::class,
 *     "SetupFee" = SetupFee::class
 * })
 */

But now Doctrine attempts to JOIN a non-existing table when loading an entity in the hierarchy:

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'abstract_product_invoice_item' doesn't exist

How can I fix this?

A solution would be to make the intermediate abstract class an entity, but this would mean that $product would be moved from leaf entities to a new AbstractProductInvoiceItem table, which adds an extra JOIN to every query, and would mean a rather scary migration to do.

Is there a better solution? Or are MappedSuperclasses not allowed in the middle of an entity hierarchy anymore?

@greg0ire
Copy link
Member

greg0ire commented Jun 17, 2021

This changed in #8378

@BenMorel
Copy link
Contributor Author

Thanks for the pointer, @greg0ire; do you know of any solution for this issue?

@greg0ire
Copy link
Member

greg0ire commented Jun 17, 2021

I don't, I was just adding to this post because it might give you pointers as to who you might want to talk to about this, or whether this was deliberate or not. I think this might be worth a read too: #7825

Also, #8415 looks like it might fix your issue? I don't have a lot of time to help you right now, but you might have some helpful things to read in this repo's issues and PRs

@BenMorel
Copy link
Contributor Author

I tried the fix from #8415, but it does not seem to solve my issue I'm afraid :(

AFAICS, I still have to choose between:

  • not putting the abstract @MappedSuperclass in the discriminator map, and getting an error in doctrine:schema:validate
  • putting a fake entry for this abstract class in the discriminator map, and getting a table not found error at runtime

Thank you again for the pointers, though! I appreciate it.

@beberlei
Copy link
Member

Its a bug this fails, i oversaw the mapped superclass case

@BenMorel
Copy link
Contributor Author

Glad to hear it, @beberlei! If you give me a hint as to how to fix it, maybe I can help?

@beberlei
Copy link
Member

Just check for isMappedSuperclass in SchemaValifator to avoif the error

@BenMorel
Copy link
Contributor Author

@beberlei Thank you, it works fine for @MappedSuperclass by adding the ! $class->isMappedSuperclass check here:

if (! $class->isInheritanceTypeNone() && ! $class->isRootEntity() && ! $class->isMappedSuperclass && array_search($class->name, $class->discriminatorMap) === false) {

I still, however, have a [FAIL] when there is an abstract @Entity class in the middle of the inheritance hierarchy. Why would we have to create an entry in the discriminator map for a non-instantiable class?

Would you be OK with changing this for ! $class->reflClass->isAbstract() instead?

@BenMorel
Copy link
Contributor Author

Ping @beberlei 🙂

@olsavmic
Copy link
Contributor

olsavmic commented Aug 4, 2021

@beberlei We're having the very same issue. It does not make much sense to me to require abstract classes to be a part of the discriminator map.

If you feel like there is a reason to require it, let us know please. Otherwise I can prepare a PR with a fix :)

Thank you!

@olsavmic
Copy link
Contributor

olsavmic commented Aug 5, 2021

@beberlei We're having the very same issue. It does not make much sense to me to require abstract classes to be a part of the discriminator map.

If you feel like there is a reason to require it, let us know please. Otherwise I can prepare a PR with a fix :)

Thank you!

I found your comment (#8736 (comment)) and I even found a case when the behaviour is unexpected if the abstract class in the middle of hierarchy is not present (SchemaTool:183, relationship columns may not be created) so it's not a BC break but rather a bug fix and this issue can be closed so my issue regarding the abstract class is resolved.

Thank you! @beberlei

@BenMorel
Copy link
Contributor Author

BenMorel commented Aug 5, 2021

@olsavmic Adding a fake entry for abstract classes does not fix all issues, please re-read the first post of this thread.
This issue cannot be closed!

@olsavmic
Copy link
Contributor

olsavmic commented Aug 5, 2021

@olsavmic Adding a fake entry for abstract classes does not fix all issues, please re-read the first post of this thread.
This issue cannot be closed!

@BenMorel I'm sorry, I was concerned only with the abstract class case since I somehow thought that the MappedSuperclass case was already resolved&merged.

Anyway, it partially responds to your question:

I still, however, have a [FAIL] when there is an abstract @Entity class in the middle of the inheritance hierarchy. Why > would we have to create an entry in the discriminator map for a non-instantiable class?

Would you be OK with changing this for ! $class->reflClass->isAbstract() instead?

Abstract entity classes (does not apply to @MappedSuperclass) should be left as is and they have to be part of the DiscriminatorMap so that everything works properly.

@BenMorel
Copy link
Contributor Author

BenMorel commented Aug 5, 2021

Abstract classes should be left as is and they have to be part of the DiscriminatorMap so that everything works properly.

Not sure why it should be this way, though I don't really mind, as long as Doctrine does not attempt to join a non-existing class!

@olsavmic
Copy link
Contributor

olsavmic commented Aug 6, 2021

@BenMorel I want to make sure we’re on the same page.

@MappedSuperclass
abstract class Foo extends FooParent

The issue with MappedSuperclass in the middle of hierarchy is still there, Beberlei confirmed it’s a bug and it needs to be fixed. I’d prepare a PR but it’s rather your contribution.

I just tested the case with joined table and:

@Entity
abstract class Foo extends FooParent {

}   

Needs an entry in the discriminator map and a separate table exists (will be generated by schema diff) for this abstract class (which is implied by the JOINED behaviour and is valid from the DB schema point of view).

abstract class Foo extends FooParent {

}

(Without @Entity annotation) does not need an entry in the DiscriminatorMap, a table is not generated yet the class cannot contain any fields. I'd actually say it should be marked as error by the ValidatorSchema as it's very easy to forget that the @Entity annotation is missing while adding new fields.

@BenMorel
Copy link
Contributor Author

BenMorel commented Oct 4, 2021

Hi, there is still a regression in 2.9 and 2.10: we now have to add a fake entry for abstract entity classes in the discriminator map. This is quite an annoyance, can't this be avoided? Should I open a separate issue for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants