NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

leofeyer wants to merge2 commits intosymfony:masterfromleofeyer:feature/file-locator-exception

Closed

Use a dedicated exception in the file locator#19511

leofeyer wants to merge2 commits intosymfony:masterfromleofeyer:feature/file-locator-exception

Conversation

Copy link

Contributor

leofeyer commentedAug 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 dedicatedFileLocatorFileNotFoundException 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}

Use a dedicated exception in the file locator.

5dce9f3

carsonbot added Status: Needs Review Feature labels

Aug 2, 2016

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

dostenAug 2, 2016

Choose a reason for hiding this comment

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

Why not simplyFileNotFoundException? The current name is verbose

Copy link

ContributorAuthor

leofeyerAug 2, 2016

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.

Copy link

Contributor

dosten commentedAug 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'?

Copy link

ContributorAuthor

leofeyer commentedAug 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 reviewed

Aug 3, 2016

View reviewed changes

src/Symfony/Component/Config/Tests/FileLocatorTest.php Outdated


		/**
		* @expectedException\InvalidArgumentException
		* @expectedExceptionFileLocatorFileNotFoundException

Copy link

Member

nicolas-grekasAug 3, 2016

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 beSymfony\Component\Config\Exception\FileLocatorFileNotFoundException (same below)

Copy link

ContributorAuthor

leofeyerAug 3, 2016

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

nicolas-grekasAug 3, 2016

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.* handlesuse, but since we still support PHP 5.3, we can't use them.

Copy link

ContributorAuthor

leofeyerAug 3, 2016

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 --versionPHPUnit 4.8.27 by Sebastian Bergmann and contributors.leofeyer:core-bundle$ php -vPHP 7.0.9 (cli) (built: Jul 21 2016 14:50:47) ( NTS )Copyright (c) 1997-2016 The PHP GroupZend 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.