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

Put up a warning sign that mapping may not be inherited from transient classes #10392

Merged
merged 2 commits into from
Feb 8, 2023

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Jan 13, 2023

This...

class Parent
{
    /**
     * @Id
     * @Column(type="integer")
     * @GeneratedValue
     */
    protected int $id;

    /**
     * @Column
     */
    protected string $name;

    // ...
}

/** @Entity */
class Child extends Parent
{
    // inherit $id including mapping config

    /**
     * @Column(name="name_override_config")
     */
    protected string $name;

    // ...
}

... seems to work, since it results in a single Child entity class with both fields, and also the $name field is configured with name="name_override_config".

The result comes from the (current) annotations/attribute mapping driver implementation: PHP reflection will report protected and public properties also for subclasses. Since there is no other entity or mapped superclass above Child in this example, the mapping driver assumes that all fields were declared on the Child class initially.

It will not work for e. g. class-level annotations like @Table on Parent, since those are not inherited/reported by the Reflection API for the Child subclass. Also, it does not work for private properties on the Parent class, since those don't show up in the ReflectionClass::getProperties() return value for the Child class.

We should explicitly warn users that this approach to configuration (and yes, I've seen this in the wild) is beyond what the ORM was designed for.

Note that using traits to horizontally reuse mapped fields is nothing different – it pulls fields into classes, including their attributes and annotations (= docblocks). The PHP Reflection API will report those fields as if they had been declared in the class using the trait. Traits have the extra side effect that private fields become part of the class where the trait is used, so they are reported as properties as well.

@greg0ire greg0ire requested a review from beberlei January 13, 2023 11:04
@mpdude mpdude force-pushed the warning-unmapped-classes branch from a15a34d to a221168 Compare January 13, 2023 12:52
…t classes

This _seems_ to work, but...

To my understanding, that is only because `ReflectionClass::getProperties` will report `protected` properties also for subclasses, including DocBlocks etc. The mapping drivers are unable to tell where a field comes from, so they pick up the mapping and treat it as originating from the inheriting class.

If that is indeed outside of what the ORM was designed for (sombody please confirm?), then we should explicitly discourage it.

Yes, I have seen examples of this in the wild.
@mpdude mpdude force-pushed the warning-unmapped-classes branch from a221168 to 0b8254d Compare January 13, 2023 12:55
@SenseException SenseException merged commit 31ff969 into doctrine:2.14.x Feb 8, 2023
@mpdude mpdude deleted the warning-unmapped-classes branch February 9, 2023 06:16
Gwemox pushed a commit to Gwemox/orm that referenced this pull request Feb 10, 2023
…t classes (doctrine#10392)

This _seems_ to work, but...

To my understanding, that is only because `ReflectionClass::getProperties` will report `protected` properties also for subclasses, including DocBlocks etc. The mapping drivers are unable to tell where a field comes from, so they pick up the mapping and treat it as originating from the inheriting class.

If that is indeed outside of what the ORM was designed for (sombody please confirm?), then we should explicitly discourage it.

Yes, I have seen examples of this in the wild.
derrabus added a commit to derrabus/orm that referenced this pull request Feb 28, 2023
* 2.14.x:
  Ignore the cache dir of PHPUnit 10 (doctrine#10546)
  Make data providers static (doctrine#10544)
  Bump dev tools (doctrine#10541)
  Mark SqlWalker methods as not deprecated (doctrine#10540)
  docs: consistency order for docblock in association mapping (doctrine#10534)
  Correct use of PHP attribute
  fix typo in faq.rst (doctrine#10526)
  fix: use executeStatement in SchemaTool (doctrine#10516)
  Write a test in a more specific way
  Put up a warning sign that mapping may not be inherited from transient classes (doctrine#10392)
  Avoid unnecessary information in query hints to improve query cache hit ratio
derrabus added a commit that referenced this pull request Feb 28, 2023
* 2.14.x:
  Ignore the cache dir of PHPUnit 10 (#10546)
  Make data providers static (#10544)
  Bump dev tools (#10541)
  Mark SqlWalker methods as not deprecated (#10540)
  docs: consistency order for docblock in association mapping (#10534)
  Correct use of PHP attribute
  fix typo in faq.rst (#10526)
  fix: use executeStatement in SchemaTool (#10516)
  Write a test in a more specific way
  Put up a warning sign that mapping may not be inherited from transient classes (#10392)
  Avoid unnecessary information in query hints to improve query cache hit ratio
derrabus added a commit to derrabus/orm that referenced this pull request Feb 28, 2023
* 2.15.x:
  Ignore the cache dir of PHPUnit 10 (doctrine#10546)
  Make data providers static (doctrine#10545)
  Make data providers static (doctrine#10544)
  Bump dev tools (doctrine#10541)
  Mark SqlWalker methods as not deprecated (doctrine#10540)
  docs: consistency order for docblock in association mapping (doctrine#10534)
  Correct use of PHP attribute
  fix typo in faq.rst (doctrine#10526)
  fix: use executeStatement in SchemaTool (doctrine#10516)
  Write a test in a more specific way
  Put up a warning sign that mapping may not be inherited from transient classes (doctrine#10392)
  Avoid unnecessary information in query hints to improve query cache hit ratio
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 this pull request may close these issues.

3 participants