Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions src/DependencyInjection/DoctrineExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -418,8 +418,8 @@ private function detectMappingType(string $directory, ContainerBuilder $containe
}

if (
preg_match('/^(?: \*|\/\*\*) @.*' . $quotedMappingObjectName . '\b/m', $content)
|| preg_match('/^(?: \*|\/\*\*) @.*Embeddable\b/m', $content)
self::textContainsAnnotation($quotedMappingObjectName, $content)
|| self::textContainsAnnotation('Embeddable', $content)
) {
$type = 'annotation';
break;
Expand All @@ -429,6 +429,21 @@ private function detectMappingType(string $directory, ContainerBuilder $containe
return $type;
}

/**
* Check if the file content contains a class-like annotation
*
* @internal
*/
public static function textContainsAnnotation(string $quotedMappingObjectName, string $content): bool
{
return preg_match('/^(?:[ ]\*|\/\*\*)[ ]@ # Match phpdoc start or line with an at
\\\\? # Can start with antislash
Copy link
Member

Choose a reason for hiding this comment

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

The regex comments are misaligned

([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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

-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

' . $quotedMappingObjectName . ' # The target class
[a-zA-Z0-9_\x80-\xff]* # Match a suffix if the class is aliased
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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)

\b/mx', $content) === 1;
}

/**
* Returns a modified version of $managerConfigs.
*
Expand Down
63 changes: 63 additions & 0 deletions tests/DependencyInjection/DoctrineExtensionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1507,6 +1507,69 @@ public function testControllerResolver(bool $simpleEntityManagerConfig): void
$this->assertEquals(new MapEntity(null, null, null, [], null, null, null, true, true), $container->get('controller_resolver_defaults'));
}

#[TestWith(['AnnotationsBundle', 'attribute', 'Vendor'], 'Bundle without anything')]
#[TestWith(['AttributesBundle', 'attribute'], 'Bundle with attributes')]
#[TestWith(['RepositoryServiceBundle', 'attribute'], 'Bundle with both')]
#[TestWith(['AnnotationsBundle', 'annotation'], 'Bundle with annotations')]
#[TestWith(['AttributesWithPackageBundle', 'attribute'], 'Bundle with attributes and @package')]
public function testDetectMappingType(string $bundle, string $expectedType, string $vendor = '')
{
if (! interface_exists(EntityManagerInterface::class)) {
self::markTestSkipped('This test requires ORM');
}

$container = $this->getContainer([$bundle], $vendor);
$extension = new DoctrineExtension();

$config = BundleConfigurationBuilder::createBuilder()
->addBaseConnection()
->addEntityManager([
'default_entity_manager' => 'default',
'entity_managers' => [
'default' => [
'mappings' => [
$bundle => [],
],
],
],
])
->build();

if (! class_exists(AnnotationDriver::class) && $expectedType === 'annotation') {
$this->expectException(LogicException::class);
$this->expectExceptionMessage('The annotation driver is only available in doctrine/orm v2.');
}

$extension->load([$config], $container);

$calls = $container->getDefinition('doctrine.orm.default_metadata_driver')->getMethodCalls();
$this->assertEquals(
sprintf('doctrine.orm.default_%s_metadata_driver', $expectedType),
(string) $calls[0][1][0],
);
}

#[TestWith([' * @Mapping\\Entity', true], 'Using the namespace without alias')]
#[TestWith([' * @ORM\\Entity', true], 'Using the namespace with alias')]
#[TestWith([' * @\\Doctrine\\ORM\\Mapping\\Entity', true], 'Complete namespace with starting slash')]
#[TestWith([' * @Doctrine\\ORM\\Mapping\\Entity', true], 'Complete namespace without starting slash')]
#[TestWith([' * @Entity', true], 'Use of the class')]
#[TestWith(['/** @Entity */', true], 'Comment start')]
#[TestWith([' * @ORMEntity', true], 'Use of the class with alias prefixing')]
#[TestWith([' * @EntityORM', true], 'Use of the class with alias suffixing')]
#[TestWith([' * @ormEntity', true], 'namespace can start with lowercase')]
#[TestWith([' * @_ORMEntity', true], 'namespace can start with underscore')]
#[TestWith([" * @\x80ORMEntity", true], 'namespace can start with char from x80-Xff')]
#[TestWith([" * @orm0_\x80Entity", true], 'namespace can contain number, underscore and char from x80-Xff')]
#[TestWith([' * @package testEntity', false], 'Annotation with Entity as value')]
#[TestWith([' * @entity', false], 'Lowercase use of the class')]
#[TestWith([' * @1ORMEntity', false], 'namespace can\'t start with number')]
#[TestWith([' * @extend<Entity>', false], 'The Entity is used inside < and >')]
public function testTextContainsAnnotation(string $input, bool $expected): void
{
self::assertEquals($expected, DoctrineExtension::textContainsAnnotation('Entity', $input));
}

/** @param list<string> $bundles */
private static function getContainer(array $bundles = ['XmlBundle'], string $vendor = ''): ContainerBuilder
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

declare(strict_types=1);

namespace Fixtures\Bundles\AnnotationsBundle;

use Symfony\Component\HttpKernel\Bundle\Bundle;

class AnnotationsBundle extends Bundle
{
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php

declare(strict_types=1);

namespace Fixtures\Bundles\AnnotationsBundle\Entity;

/** @ORM\Entity() */
class TestAnnotationEntity
{
/**
* @ORM\Id
* @ORM\GeneratedValue(strategy="AUTO")
* @ORM\Column(type="integer")
*/
public int|null $id = null;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

declare(strict_types=1);

namespace Fixtures\Bundles\AttributesWithPackageBundle;

use Symfony\Component\HttpKernel\Bundle\Bundle;

class AttributesWithPackageBundle extends Bundle
{
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

declare(strict_types=1);

namespace Fixtures\Bundles\AttributesWithPackageBundle\Entity;

/** @testUnrelatedAnnotation Fixtures\Bundles\AttributesWithPackageBundle\Entity */
interface TestInterface
{
}