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

Fix bug in namespace check in MappingDriverChain #341

Closed
wants to merge 5 commits into from

Conversation

ttk
Copy link

@ttk ttk commented Dec 14, 2023

This fix improves the checking if a namespace appears in a className by ensuring that the namespace ends in a \. Prior to this fix, the following would occur:

A className such as Doctrine\Tests\Persistence\Mapping\DriverChainEntity would match the namespace: Doctrine\Tests\Persistence\Map, which isn't correct.

Other considerations:

  • what if the namespace already ends in a \? is this a valid case? Probably not.
  • what if it's a parent namespace. In the above example, this would be Doctrine\Tests\Persistence. Should this match against the className Doctrine\Tests\Persistence\Mapping\DriverChainEntity or not? The current logic would match, but I'm not sure if it should....

@@ -73,7 +73,7 @@ public function getDrivers()
public function loadMetadataForClass(string $className, ClassMetadata $metadata)
{
foreach ($this->drivers as $namespace => $driver) {
if (strpos($className, $namespace) === 0) {
if (strpos($className, $namespace . '\\') === 0) {
Copy link
Member

@malarzm malarzm Dec 17, 2023

Choose a reason for hiding this comment

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

The $namespace can already contain a \ at the end. It'd be better to ensure there's always a single \ in the addDriver method

@derrabus
Copy link
Member

Please add a test.

@malarzm malarzm added the Bug Something isn't working label Dec 18, 2023
@malarzm malarzm added this to the 3.2.1 milestone Dec 18, 2023
@ttk
Copy link
Author

ttk commented Dec 19, 2023

The namespace matching code has been revised and a new unit test added. Note that in the current implementation, the class name will only match the namespace if it's a direct parent namespace. ie.

A\B\Classname matches A\B but not A. I think this is ok because otherwise the wrong driver can be selected if a more global namespace appears first in the list.

@stof
Copy link
Member

stof commented Dec 20, 2023

This is not good IMO. Drivers currently support organizing entities in sub-namespaces. The chain driver should not restrict that more

@@ -37,7 +37,7 @@ public function testDelegateToMatchingNamespaceDriver(): void
->with(self::equalTo($className))
->willReturn(true);

$chain->addDriver($driver1, 'Doctrine\Tests\Models\Company');
$chain->addDriver($driver1, 'Doctrine\Tests\Persistence\Map');
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this change?

@@ -63,7 +63,7 @@ public function testGatherAllClassNames(): void
$driver1 = $this->createMock(MappingDriver::class);
$driver1->expects(self::once())
->method('getAllClassNames')
->will(self::returnValue(['Doctrine\Tests\Models\Company\Foo']));
->will(self::returnValue(['Doctrine\Tests\Models\Company\Foo', 'Doctrine\Tests\Models\Company2\Foo']));
Copy link
Member

Choose a reason for hiding this comment

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

Same question, why did this test need an adjustment?

self::assertTrue($mdc::isMatchingNamespace($className, 'Doctrine\Tests\Persistence\Mapping\\'));
self::assertTrue($mdc::isMatchingNamespace($className, 'Doctrine\Tests\Persistence\Mapping'));
self::assertFalse($mdc::isMatchingNamespace($className, 'Doctrine\Tests\Persistence\Map'));
self::assertFalse($mdc::isMatchingNamespace($className, 'Doctrine\Tests\Persistence'));
Copy link
Member

Choose a reason for hiding this comment

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

Those tests don't tell me what problem you're attempting to solve. You're merely testing an internal function, an implementation detail.

@greg0ire greg0ire removed this from the 3.2.1 milestone Mar 1, 2024
@greg0ire
Copy link
Member

No answer… let's close this I guess.

@greg0ire greg0ire closed this May 17, 2024
@ttk
Copy link
Author

ttk commented May 17, 2024

This is still a issue. Feel free to fork the branch and update it as necessary.

In the meantime, I worked around this bug by changing the name of my directory for the mapping:
Before, which didn't work because the App mapping would always be selected:

        mappings:
            App:
                dir: '%kernel.project_dir%/src/Entity'
                ...
            AppStatic:
                dir: '%kernel.project_dir%/src/EntityStatic'
                ...

to this, which worked, because of the unique prefix:

        mappings:
            App:
                dir: '%kernel.project_dir%/src/Entity'
                ...
            AppStatic:
                dir: '%kernel.project_dir%/src/StaticEntity'
                ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants