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 how namespace matching happens in SymfonyFileLocator #367

Closed
wants to merge 2 commits into from

Conversation

ptlis
Copy link
Contributor

@ptlis ptlis commented May 8, 2015

The problem was that namespace prefixes can overlap, and for classes
that would match more than one prefix only the first prefix would be
tested for. This meant that the findMappingFile and fileExists functions
would return different results depending on the order of the stored
prefixes.

For these methods to return the correct values the prefixes must be
listed in order of most to least specific. For example

['Vendor\Package\Entities', 'Vendor\Package\Entities\Blog']

would return the wrong result whereas

['Vendor\Package\Entities\Blog', 'Vendor\Package\Entities']

would return the correct one.

This changeset simply allows the class to be compared to more than one prefix, and updates one test as an exception message has changed.

The problem was that namespace prefixes can overlap, and for classes
that would match more than one prefix only the first prefix would be
tested for. This meant that the findMappingFile and fileExists functions
would return different results depending on the order of the stored
prefixes.

For these methods to return the correct values the prefixes must be
listed in order of most to least specific. For example

    ['Vendor\Package\Entities', 'Vendor\Package\Entities\Blog']

would return the wrong result whereas

    ['Vendor\Package\Entities\Blog', 'Vendor\Package\Entities']

would return the correct one.
@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DCOM-286

We use Jira to track the state of pull requests and the versions they got
included in.

@Ocramius Ocramius self-assigned this Aug 31, 2015
@ptlis
Copy link
Contributor Author

ptlis commented Sep 29, 2015

Has anybody had a chance to look at this?

This is a very simple PR and it's very disheartening to see it languish for 4 months without any feedback either way.

@Ocramius
Copy link
Member

@ptlis didn't check so far, sorry. Thanks for the ping though.

What I see is that a test is missing here.

@Ocramius
Copy link
Member

Ocramius commented Dec 3, 2015

Just re-checked this: it needs an additional test case for the scenario where multiple paths are mapped.

@ptlis
Copy link
Contributor Author

ptlis commented Dec 3, 2015

👍 thanks @Ocramius , I'll try and find time to look at this over the weekend.

Ensures that the prefix to path mappings can be provided in an arbitrary order. Previously these had to be in most to least specific ordering.

For example with the prefix mapping ['\Foo\Bar' => '.', '\Foo' => '../'] classes in the \Foo\Bar namespace would be found, but on providing the prefix mapping  ['\Foo' => '../', '\Foo\Bar' => '.'] these classes could not be found.
@ptlis
Copy link
Contributor Author

ptlis commented Dec 21, 2015

I've added tests to ensure that the prefix ordering no longer makes a difference, is there anything else I need to do to get this into a mergeable state?

@Ocramius Ocramius added the Bug label Dec 25, 2015
@Ocramius Ocramius added this to the 2.5.3 milestone Dec 25, 2015
Ocramius added a commit that referenced this pull request Dec 25, 2015
Ocramius added a commit that referenced this pull request Dec 25, 2015
@Ocramius Ocramius closed this in 14fd1b0 Dec 25, 2015
@Ocramius
Copy link
Member

@ptlis merged, thanks!

Backported to 2.5.x in e8768f6
Backported to 2.6.x in 50b2308

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants