Prohibits class typo in the discriminator map #8122

gquemener · 2020-04-23T09:02:08Z

When a defined subclass has a case typo, the query builder will be lost
and will generate exotic table alias. This commit fixes the issue at the
root by prohibiting case typo in discriminator map.

See #8112 for the consequence of
such typo.

gquemener · 2020-04-23T09:07:21Z

This PR is better than #8112 in term of DX as it will raise errors way before any sql query will hit the DB. Pointing the user exactly where the issue is.

gquemener · 2020-04-23T09:14:54Z

lib/Doctrine/ORM/Mapping/ClassMetadata.php

+        $refl = new ReflectionClass($className);
+        if ($refl->name !== $className) {
+            throw MappingException::invalidClassInDiscriminatorMap($className, $this->className);
+        }


I'm opened to suggestion if you see a more efficient way to grab actual class name without using reflection.

However, I'm not sure efficiency is really researched here, as metadata are cached.

I don't think the reflection API performance are that bad, and since this is about metadata, there is probably caching after this, right? So it's not a hot path, and it's cached, isn't it?

That's my belief as well 👍

gquemener · 2020-04-23T09:15:21Z

This change could probably be considered as a BC break.

gquemener · 2020-04-23T09:22:55Z

lib/Doctrine/ORM/Mapping/ClassMetadata.php

@@ -1002,6 +1003,11 @@ public function addDiscriminatorMapClass($name, string $className) : void
            throw MappingException::invalidClassInDiscriminatorMap($className, $this->className);
        }

+        $refl = new ReflectionClass($className);


Throws ReflectionException if the class to reflect does not exist. Source

It won't as we check for class existence on line 1001.

greg0ire

A+ commit message, and nice tests! I'm unsure if this should really be considered a BC-break. If it is, we should probably trigger a deprecation but that sounds weird to me. In any case, I would target the bugfix branch (so 2.7)

greg0ire · 2020-04-24T17:36:26Z

lib/Doctrine/ORM/Mapping/ClassMetadata.php

+        $refl = new ReflectionClass($className);
+        if ($refl->name !== $className) {
+            throw MappingException::invalidClassInDiscriminatorMap($className, $this->className);
+        }


I don't think the reflection API performance are that bad, and since this is about metadata, there is probably caching after this, right? So it's not a hot path, and it's cached, isn't it?

The base branch was changed.

greg0ire · 2020-04-24T18:42:36Z

Why did you change it back?

gquemener · 2020-04-24T18:49:35Z

Switching the base branch to 2.7 showed 500+ commits to merge. Let me work on that.

greg0ire · 2020-04-24T18:54:52Z

git rebase --onto origin/2.7 origin/master prevents-sub-class-case-difference && git push --force ;)

gquemener · 2020-04-24T19:27:46Z

Done, thanks for the hint 👍

I'm curious, how would you transpose this fix into master? I've noticed quite a few changes that'll make it non trivial (ClassMetadataInfo has been renamed ClassMetadata and a few metadata drivers have been removed).

greg0ire · 2020-04-24T19:32:22Z

I'm curious about that too to be honest 😅

tests/Doctrine/Tests/ORM/Mapping/AbstractMappingDriverTest.php

greg0ire · 2020-05-03T13:28:27Z

The build fails, please run vendor/bin/phpcbf

gquemener · 2020-05-03T14:47:47Z

Done ✔️

greg0ire

LGTM

In any case, I would target the bugfix branch (so 2.7)

Correction: if someone feels there is a BC-break and this should instead be a deprecation, then 2.8 should be targeted. Otherwise, 2.7 is fine.

SenseException

👍

beberlei · 2020-05-12T18:40:06Z

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

@@ -2816,6 +2816,11 @@ public function addDiscriminatorMapClass($name, $className)
            throw MappingException::invalidClassInDiscriminatorMap($className, $this->name);
        }

+        $refl = new ReflectionClass($className);


Sorry to swoop in so late, but loading ReflectionClass inside ClassMetadataInfo is not allowed from architecture POV. This code doesn't work with static runtime reflection, which will break detached metadata use cases (such as everything with the EntityGenerator).

Instead this check is something you should put either into ClassMetadataFactory if we want this to fail at runtime, or in the SchemaValidator if we want this to fail only in debug/dev mode.

beberlei

The right place for validation is ClassMetadataFactory::validateRuntimeMetadata

gquemener · 2020-05-12T19:25:54Z

The right place for validation is ClassMetadataFactory::validateRuntimeMetadata

Thanks for pointing this out.
I'll check this out, hopefully soon :)

When a defined subclass has a case typo, the query builder will be lost and will generate exotic table alias. This commit fixes the issue at the root by prohibiting case typo in discriminator map. See doctrine#8112 for the consequence of such typo.

The Cube class must be autoloaded with the correct case, otherwise composer autoloader will complain.

See https://github.com/doctrine/orm/pull/8122/files#r423952247

gquemener · 2020-05-15T08:31:58Z

I'm wondering if this case sensitivity check should not be performed more globally as class_exists and interface_exists are used in other places.

The underlying issue is that once the class has been included once (FYI, composer autoload will not autoload class with incorrect case, as it does not respect psr-0/4 AFAIK), class_exists and interface_exists will behave in a case insensitive manner.

In the case of the discriminator map, this will generate incorrect sql statement because the system won't be able to determine the correct table alias. I'm wondering if a similar issue could not appear anywhere where (class|interface)_exists is used 🤔

I'm thinking about associations (targetEntity with incorrect case), entity definition (name with incorrect case), ...

I need to take some more time to investigate, any hint is welcome in the meantime :)

…ctoryTest case

gquemener changed the title ~~Prevents incorrect table aliases to be generated~~ Prohibits class typo in the discriminator map Apr 23, 2020

gquemener mentioned this pull request Apr 23, 2020

Prevents incorrect table aliases #8112

Closed

gquemener force-pushed the prevents-sub-class-case-difference branch from 75ffa10 to 0d2c8d5 Compare April 23, 2020 09:12

gquemener commented Apr 23, 2020

View reviewed changes

greg0ire previously approved these changes Apr 24, 2020

View reviewed changes

gquemener changed the base branch from master to 2.7 April 24, 2020 18:03

gquemener changed the base branch from 2.7 to master April 24, 2020 18:03

gquemener force-pushed the prevents-sub-class-case-difference branch from 0d2c8d5 to cad84ac Compare April 24, 2020 19:24

gquemener changed the base branch from master to 2.7 April 24, 2020 19:25

greg0ire previously approved these changes Apr 24, 2020

View reviewed changes

SenseException requested changes Apr 29, 2020

View reviewed changes

tests/Doctrine/Tests/ORM/Mapping/AbstractMappingDriverTest.php Outdated Show resolved Hide resolved

tests/Doctrine/Tests/ORM/Mapping/AbstractMappingDriverTest.php Outdated Show resolved Hide resolved

gquemener dismissed greg0ire’s stale review via a2badf9 May 3, 2020 12:50

gquemener requested review from SenseException and greg0ire May 3, 2020 12:51

gquemener force-pushed the prevents-sub-class-case-difference branch from a2badf9 to 33a3e2d Compare May 3, 2020 12:54

gquemener force-pushed the prevents-sub-class-case-difference branch from 33a3e2d to 41ef28f Compare May 3, 2020 14:38

greg0ire added the Bug label May 3, 2020

greg0ire previously approved these changes May 3, 2020

View reviewed changes

SenseException previously approved these changes May 4, 2020

View reviewed changes

beberlei reviewed May 12, 2020

View reviewed changes

beberlei requested changes May 12, 2020

View reviewed changes

gquemener dismissed stale reviews from SenseException and greg0ire via f93336e May 13, 2020 19:48

gquemener force-pushed the prevents-sub-class-case-difference branch 2 times, most recently from 476a1c6 to 701b50a Compare May 13, 2020 20:11

gquemener added 4 commits May 13, 2020 22:11

Controls growing rate of the abstract test class

5e0ee52

Fixes incorrect test case

37f1f19

The Cube class must be autoloaded with the correct case, otherwise composer autoloader will complain.

Removes non architecture compliant code

a758d75

See https://github.com/doctrine/orm/pull/8122/files#r423952247

gquemener force-pushed the prevents-sub-class-case-difference branch from 701b50a to 3b85ec8 Compare May 13, 2020 20:13

Ensures discriminator map is case sensitive

2daee11

gquemener force-pushed the prevents-sub-class-case-difference branch from 3b85ec8 to 2daee11 Compare May 15, 2020 08:23

beberlei merged commit 64c3f68 into doctrine:2.7 Jul 5, 2020

beberlei added a commit that referenced this pull request Jul 5, 2020

[GH-8122] Move test from AbstractMappingDriverTest to ClassMetadataFa…

775d91c

…ctoryTest case

beberlei added this to the 2.7.4 milestone Jul 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prohibits class typo in the discriminator map #8122

Prohibits class typo in the discriminator map #8122

gquemener commented Apr 23, 2020

gquemener commented Apr 23, 2020

gquemener Apr 23, 2020

gquemener Apr 23, 2020

greg0ire Apr 24, 2020

gquemener Apr 24, 2020

gquemener commented Apr 23, 2020

gquemener Apr 23, 2020 •

edited

greg0ire left a comment •

edited

greg0ire Apr 24, 2020

greg0ire commented Apr 24, 2020

gquemener commented Apr 24, 2020

greg0ire commented Apr 24, 2020

gquemener commented Apr 24, 2020

greg0ire commented Apr 24, 2020

greg0ire commented May 3, 2020 •

edited

gquemener commented May 3, 2020 •

edited

greg0ire left a comment

SenseException left a comment

beberlei May 12, 2020

beberlei left a comment

gquemener commented May 12, 2020

gquemener commented May 15, 2020 •

edited

Prohibits class typo in the discriminator map #8122

Prohibits class typo in the discriminator map #8122

Conversation

gquemener commented Apr 23, 2020

gquemener commented Apr 23, 2020

gquemener Apr 23, 2020

Choose a reason for hiding this comment

gquemener Apr 23, 2020

Choose a reason for hiding this comment

greg0ire Apr 24, 2020

Choose a reason for hiding this comment

gquemener Apr 24, 2020

Choose a reason for hiding this comment

gquemener commented Apr 23, 2020

gquemener Apr 23, 2020 • edited

Choose a reason for hiding this comment

greg0ire left a comment • edited

Choose a reason for hiding this comment

greg0ire Apr 24, 2020

Choose a reason for hiding this comment

greg0ire commented Apr 24, 2020

gquemener commented Apr 24, 2020

greg0ire commented Apr 24, 2020

gquemener commented Apr 24, 2020

greg0ire commented Apr 24, 2020

greg0ire commented May 3, 2020 • edited

gquemener commented May 3, 2020 • edited

greg0ire left a comment

Choose a reason for hiding this comment

SenseException left a comment

Choose a reason for hiding this comment

beberlei May 12, 2020

Choose a reason for hiding this comment

beberlei left a comment

Choose a reason for hiding this comment

gquemener commented May 12, 2020

gquemener commented May 15, 2020 • edited

gquemener Apr 23, 2020 •

edited

greg0ire left a comment •

edited

greg0ire commented May 3, 2020 •

edited

gquemener commented May 3, 2020 •

edited

gquemener commented May 15, 2020 •

edited