Fix detectMappingType if the entity contain a package attribute #2115

jtattevin · 2025-10-16T15:57:47Z

Update the regex used to discover if the class is using annotations or attribute to fix the case

stof · 2025-10-16T16:19:40Z

src/DependencyInjection/DoctrineExtension.php

            if (
-                preg_match('/^(?: \*|\/\*\*) @.*' . $quotedMappingObjectName . '\b/m', $content)
-                || preg_match('/^(?: \*|\/\*\*) @.*Embeddable\b/m', $content)
+                preg_match('/^(?: \*|\/\*\*) @[a-zA-Z\\\\]*' . $quotedMappingObjectName . '\b/m', $content)


a-zA-Z is not the range of characters allowed in PHP namespaces. Your current regex will reject some valid class names.

Yes, my bad and thank for the feedback

greg0ire

Please add tests.

greg0ire · 2025-10-16T20:10:09Z

src/DependencyInjection/DoctrineExtension.php

-                preg_match('/^(?: \*|\/\*\*) @[a-zA-Z\\\\]*' . $quotedMappingObjectName . '\b/m', $content)
-                || preg_match('/^(?: \*|\/\*\*) @[a-zA-Z\\\\]*Embeddable\b/m', $content)
+                preg_match('/^(?: \*|\/\*\*) @\\\\?([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*\\\\?)*' . $quotedMappingObjectName . '\b/m', $content)
+                || preg_match('/^(?: \*|\/\*\*) @\\\\?([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*\\\\?)*Embeddable\b/m', $content)


This regex is becoming hard to read. Please use the x modifier and add comments.

I'm not familiar with the x modifier, so I'll take a look and update the code.

Note: I'm away for a few day, so I'm happy to update the pr but it will only be tuesday.

Don't worry, I'm not in a hurry 😉

jtattevin · 2025-10-16T20:40:29Z

Please add tests.

Currently I need to create a bundle for each positive test case (@ORM\Entity, @\Doctrine\ORM\Mapping\Entity, @ORMEntity), it may lead to a lot of bundle, is that ok for you ? (That said, i'm not sure if there is many more case except those three that need to be checked ?)
All the cases that should not return annotations can be together, but the cases that match should be isolated to be certain that the right line is matched.

greg0ire · 2025-10-16T20:47:00Z

it may lead to a lot of bundle, is that ok for you ?

That seems overkill when what you truly want to do is test a regex against a string.
Maybe you should extract the regex logic inside a public static method marked as @internal, so that it's possible to test it without making it part of the public API of the bundle?

jtattevin · 2025-10-16T20:53:12Z

Maybe you should extract the regex logic inside a public static method

Ok for me, was thinking about that but afraid to change too much the structure :)

greg0ire · 2025-10-16T21:14:45Z

The regex-based detection logic is removed in 3.0.x so don't worry too much about the structure, it will eventually be OK.

greg0ire · 2025-10-20T11:05:04Z

src/DependencyInjection/DoctrineExtension.php

+    {
+        return preg_match('/^(?:[ ]\*|\/\*\*)[ ]@                # Match phpdoc start or line with an at
+            \\\\?                                               # Can start with antislash
+            ([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*\\\\?)*    # Match 0-n namespace, the antislash is optionnal as it may be a prefix if an alias is used


Suggested change

([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*\\\\?)* # Match 0-n namespace, the antislash is optionnal as it may be a prefix if an alias is used

([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*\\\\?)* # Match 0-n namespace, the antislash is optional as it may be a prefix if an alias is used

Also, what do you mean by "0-n namespace"? It seems you're saying that in Doctrine\DoctrineBundle, DoctruneBundle is a namespace, which it isn't, only Doctrine and Doctrine\DoctrineBundle are.

Yes, that it, to say that it can be of namespace component like Doctrine or DoctrineBundle.
I can replace by Match namespace, the antislash is optional as it may be a prefix if an alias is used if you prefer ?

Or "Match namespace components", just like you wrote above. That's fine.

greg0ire · 2025-10-20T11:07:49Z

src/DependencyInjection/DoctrineExtension.php

+    public static function textContainsAnnotation(string $quotedMappingObjectName, string $content): bool
+    {
+        return preg_match('/^(?:[ ]\*|\/\*\*)[ ]@                # Match phpdoc start or line with an at
+            \\\\?                                               # Can start with antislash


The regex comments are misaligned

stof · 2025-10-20T11:58:12Z

src/DependencyInjection/DoctrineExtension.php

+            \\\\?                                               # Can start with antislash
+            ([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*\\\\?)*    # Match 0-n namespace, the antislash is optionnal as it may be a prefix if an alias is used
+            ' . $quotedMappingObjectName . '                    # The target class
+            [a-zA-Z0-9_\x80-\xff]*                              # Match a suffix if the class is aliased


this does not make sense. Aliasing a class does not restrict it to apply a suffix anyway, and it would match unrelated things. This part should be reverted.

The idea was to match also if you have :

use Doctrine\ORM\Mapping\Entity as ORMEntity; use Doctrine\ORM\Mapping\Entity as EntityORM; /** @EntityORM */ /** @ORMEntity */ class Something { ... }

But yes, if you alias to DatabaseObject it would not match like the previous version.
Do you still want me to revert this part ?

if we want to matching these, we would have to match it only when there is no namespace part (we cannot have a namespace reference before an aliased name). And making it reliable would require inspecting use statements to detect such aliases, which is overkill IMO (if the detection fails, the dev can always provide a type explicitly)

stof · 2025-10-20T11:59:40Z

src/DependencyInjection/DoctrineExtension.php

+    {
+        return preg_match('/^(?:[ ]\*|\/\*\*)[ ]@                # Match phpdoc start or line with an at
+            \\\\?                                               # Can start with antislash
+            ([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*\\\\?)*    # Match 0-n namespace, the antislash is optionnal as it may be a prefix if an alias is used


-1 for the part about the prefix thing. This leads to matching anyway annotation that has Entity somewhere in its name).

We should keep checking for the Entity word

jtattevin force-pushed the fix-2111-detect-mapping-type-with-package branch 2 times, most recently from 3fa388d to 092e5f7 Compare October 16, 2025 16:10

Fix detectMappingType if the entity contain a package attribute

1e2cb74

jtattevin force-pushed the fix-2111-detect-mapping-type-with-package branch from 092e5f7 to 1e2cb74 Compare October 16, 2025 16:14

stof reviewed Oct 16, 2025

View reviewed changes

Fix regex to match php allowed char in namespace

440ac19

greg0ire reviewed Oct 16, 2025

View reviewed changes

Add tests and comments on the regex

6eb59c2

greg0ire reviewed Oct 20, 2025

View reviewed changes

stof reviewed Oct 20, 2025

View reviewed changes

Remove prefix/suffix detection ; Adjust comments

0109efc

	([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]\\\\?) # Match 0-n namespace, the antislash is optionnal as it may be a prefix if an alias is used
	([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]\\\\?) # Match 0-n namespace, the antislash is optional as it may be a prefix if an alias is used

Uh oh!

Fix detectMappingType if the entity contain a package attribute #2115

Are you sure you want to change the base?

Fix detectMappingType if the entity contain a package attribute #2115

Uh oh!

Conversation

jtattevin commented Oct 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtattevin commented Oct 16, 2025

Uh oh!

greg0ire commented Oct 16, 2025

Uh oh!

jtattevin commented Oct 16, 2025

Uh oh!

greg0ire commented Oct 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants