Skip to content

Use a dedicated exception in the file locator #19511

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Use a dedicated exception in the file locator #19511

wants to merge 2 commits into from

Conversation

leofeyer
Copy link
Contributor

@leofeyer leofeyer commented Aug 2, 2016

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

This PR adds a dedicated FileLocatorFileNotFoundException class to the file locator, so it is possible to catch file locator exceptions separately from invalid argument exceptions.

try {
    foreach ($container->get('file_locator')->locate('file.php', null, false) as $file) {
        include $file;
    }
} catch (\InvalidArgumentException $e) {
    // this will catch both file locator exceptions as well as
    // any invalid argument exception thrown in an included file
}

With the dedicated exceptions, we could do this:

try {
    foreach ($container->get('file_locator')->locate('file.php', null, false) as $file) {
        include $file;
    }
} catch (FileLocatorFileNotFoundException $e) {
    // this will only ignore file locator exceptions
}

dosten
dosten reviewed Aug 2, 2016
View reviewed changes
src/Symfony/Component/Config/Exception/FileLocatorFileNotFoundException.php
*
* @author Leo Feyer <https://github.com/leofeyer>
*/
class FileLocatorFileNotFoundException extends \InvalidArgumentException
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply FileNotFoundException? The current name is verbose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the other exception classes also use a prefix: https://github.com/symfony/symfony/tree/master/src/Symfony/Component/Config/Exception

Just wanted to keep things consistent.

@dosten
Copy link
Contributor

dosten commented Aug 2, 2016

In some way this can be a BC break, while your example works fine, but what happens if someone do something like this: get_class($e) === 'InvalidArgumentException'?

@leofeyer
Copy link
Contributor Author

leofeyer commented Aug 2, 2016 •
edited
Loading

get_class($e) === 'InvalidArgumentException' is wrong in the first place.

You would normally check $e instanceof \InvalidArgumentException and because the class extends from \InvalidArgumentException, there is no BC break implied.

nicolas-grekas
nicolas-grekas reviewed Aug 3, 2016
View reviewed changes
src/Symfony/Component/Config/Tests/FileLocatorTest.php
@@ -86,7 +87,7 @@ public function testLocate()
}

/**
* @expectedException \InvalidArgumentException
* @expectedException FileLocatorFileNotFoundException
Copy link
Member

Choose a reason for hiding this comment

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

phpunit doesn't handle use statements, should be Symfony\Component\Config\Exception\FileLocatorFileNotFoundException (same below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the PR accordingly. However, use statements seem to work in phpunit under PHP 5.6 and PHP 7. Is this a known issue?

Copy link
Member

Choose a reason for hiding this comment

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

PHP 7 requires a phpunit 5.* whereas phpunit 4.8 is the only option for PHP5.3 to 5.5.
Maybe phpunit 5.* handles use, but since we still support PHP 5.3, we can't use them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That cannot be true. I am using PhpUnit 4.8 with PHP 7.0.

leofeyer:core-bundle$ vendor/bin/phpunit --version
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

leofeyer:core-bundle$ php -v
PHP 7.0.9 (cli) (built: Jul 21 2016 14:50:47) ( NTS )
Copyright (c) 1997-2016 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2016 Zend Technologies
    with Zend OPcache v7.0.9, Copyright (c) 1999-2016, by Zend Technologies
    with blackfire v1.11.1, https://blackfire.io, by Blackfireio Inc.

@Jean85
Copy link
Contributor

Jean85 commented Aug 5, 2016

Status: reviewed

@nicolas-grekas
Copy link
Member

👍

@fabpot
Copy link
Member

fabpot commented Aug 9, 2016

Thank you @leofeyer.

@fabpot fabpot closed this in ccb684a Aug 9, 2016
@leofeyer leofeyer deleted the feature/file-locator-exception branch August 9, 2016 14:26
@fabpot fabpot mentioned this pull request Oct 27, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@leofeyer @dosten @Jean85 @nicolas-grekas @fabpot @carsonbot