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

[Routing] Fixes fatal errors with object resources in AnnotationDirectoryLoader::supports #11232

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 25, 2014

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Fixes fatal errors that occur in the supports method with objects that aren't string convertible / don't implement ArrayAccess. This is mostly a problem because some locators try to access a specific character in the resource name.
Since the resource is checked if it's a string either way, it's the most simple solution to just move that check a bit ahead.

if (!is_string($resource)) {
return false;
}

try {
$path = $this->locator->locate($resource);
Copy link
Member

Choose a reason for hiding this comment

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

FileLocatorInterface allows mixed https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Config/FileLocatorInterface.php#L22 . But the actual implementation https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Config/FileLocator.php#L44 doesn't actually work with any parameter. So this issue is there and not really in this class here.

Copy link
Author

Choose a reason for hiding this comment

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

Well so the question is if FileLocator(Interface) shouldn't allow mixed in which case this change is relevant or if it should and appropriate string conversion and or exceptions would've to be implemented there.
In both cases moving is_string upfront removes some overhead or it could be removed of course.

@ghost ghost added the Routing label Jun 29, 2014
@fabpot
Copy link
Member

fabpot commented Jun 29, 2014

👍 as it fixes a fatal error and is only about moving things around.

@Tobion
Copy link
Member

Tobion commented Jun 29, 2014

As I said, the real problem is the locator. And depending on how the locator is fixed, this fix here applies or does not apply. If the locator really works with mixed, then the is_string can be dropped completely.

@ghost
Copy link
Author

ghost commented Jun 29, 2014

So how do i continue? Should I create an issue to discuss the FileLocator?

@ghost
Copy link
Author

ghost commented Feb 5, 2015

Since the FileLocatorInterface has been recently changed to only "allow" strings, it'd be nice if this change could be implemented.

@fabpot
Copy link
Member

fabpot commented Jan 25, 2016

Thank you @Tischoi.

fabpot added a commit that referenced this pull request Jan 25, 2016
…tationDirectoryLoader::supports (Tischoi)

This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #11232).

Discussion
----------

[Routing] Fixes fatal errors with object resources in AnnotationDirectoryLoader::supports

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Fixes fatal errors that occur in the supports method with objects that aren't string convertible / don't implement ArrayAccess. This is mostly a problem because some locators try to access a specific character in the resource name.
Since the resource is checked if it's a string either way, it's the most simple solution to just move that check a bit ahead.

Commits
-------

5e80585 Update AnnotationDirectoryLoader.php
@fabpot fabpot closed this Jan 25, 2016
@fabpot fabpot mentioned this pull request Feb 3, 2016
This was referenced Feb 28, 2016
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.

None yet

2 participants