8000 Catch \Throwable by fprochazka · Pull Request #18812 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Catch \Throwable #18812

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

Closed
wants to merge 3 commits into from
Closed

Catch \Throwable #18812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
970bcbe
Catch \Throwable
fprochazka May 12, 2016
55bab99
WebProfilerBundle: catch RouteNotFoundException instead of generic \E…
fprochazka May 12, 2016
8fab1b1
FrameworkBundle: catch InvalidArgumentException instead of generic \E…
fprochazka May 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Symfony/Bundle/FrameworkBundle/Routing/DelegatingLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ public function load($resource, $type = null)
} catch (\Exception $e) {
$this->loading = false;
throw $e;
} catch (\Throwable $e) {
$this->loading = false;
throw $e;
Copy link
@AyrtonRicardo AyrtonRicardo May 20, 2016

Choose a reason for hiding this comment

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

in php7 isn't possible implements \Throwable interface only extends \Exception class, so why not use only one catch with \Throwable?

Copy link
Member
@nicolas-grekas nicolas-grekas May 20, 2016

Choose a reason for hiding this comment

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

because we support PHP5 also

Copy link
@AyrtonRicardo AyrtonRicardo May 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I got it

}

$this->loading = false;
Expand All @@ -85,7 +88,7 @@ public function load($resource, $type = null)
if ($controller = $route->getDefault('_controller')) {
try {
$controller = $this->parser->parse($controller);
} catch (\Exception $e) {
} catch (\InvalidArgumentException $e) {
// unable to optimize unknown notation
}

Expand Down
5 changes: 3 additions & 2 deletions src/Symfony/Bundle/WebProfilerBundle/Controller/ProfilerController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use Symfony\Component\HttpFoundation\Session\Flash\AutoExpireFlashBag;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Bundle\WebProfilerBundle\Profiler\TemplateManager;
use Symfony\Component\Routing\Exception\RouteNotFoundException;
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;

/**
Expand Down Expand Up @@ -193,8 +194,8 @@ public function toolbarAction(Request $request, $token)
$url = null;
try {
$url = $this->generator->generate('_profiler', array('token' => $token));
} catch (\Exception $e) {
// the profiler is not enabled
} catch (RouteNotFoundException $e) {
// the named route doesn't exist => the profiler is not enabled
Copy link
Member
@xabbuh xabbuh May 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

We now don't catch other exceptions (MissingMandatoryParametersException or InvalidParameterException) anymore (which might happen when the _profiler route is overridden).

Copy link
Contributor Author
@fprochazka fprochazka May 23, 2016

Choose a reason for hiding this comment

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

True, but does that happen?

Copy link
Member
@nicolas-grekas nicolas-grekas May 27, 2016

Choose a reason for hiding this comment

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

@fprochazka let's stay on the safe side

Copy link
Contributor Author
@fprochazka fprochazka May 27, 2016

Choose a reason for hiding this comment

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

Ok, so I'll add all 3 of them? Or do I revert this for now?

Copy link
Member
@nicolas-grekas nicolas-grekas May 29, 2016

Choose a reason for hiding this comment

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

let's revert

Copy link
Contributor
@patrick-mcdougle patrick-mcdougle Oct 31, 2016

Choose a reason for hiding this comment

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

@nicolas-grekas @fabpot It doesn't look like this change was reverted before merging... just thought you'd want to know.

Copy link
Member
@fabpot fabpot Oct 31, 2016

Choose a reason for hiding this comment

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

ping @nicolas-grekas You merged this one.

Copy link
Contributor
@patrick-mcdougle patrick-mcdougle Oct 31, 2016

Choose a reason for hiding this comment

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

Nvm, false alarm. Unless the DelegatingLoader.php wasn't supposed to be updated.

}

return new Response($this->twig->render('@WebProfiler/Profiler/toolbar.html.twig', array(
Expand Down
3 changes: 3 additions & 0 deletions src/Symfony/Component/Config/Loader/FileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ public function import($resource, $type = null, $ignoreErrors = false, $sourceRe
} catch (\Exception $e) {
unset(self::$loading[$resource]);
throw $e;
} catch (\Throwable $e) {
unset(self::$loading[$resource]);
throw $e;
}

unset(self::$loading[$resource]);
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/DependencyInjection/Container.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,11 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
return;
}

throw $e;
} catch (\Throwable $e) {
unset($this->loading[$id]);
unset($this->services[$id]);

throw $e;
}

Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Component/DependencyInjection/ContainerBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,10 @@ public function get($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INV
return;
}

throw $e;
} catch (\Throwable $e) {
unset($this->loading[$id]);

throw $e;
}

Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/HttpKernel/DependencyInjection/ContainerAwareHttpKernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
$this->container->set('request', null, 'request');
$this->container->leaveScope('request');

throw $e;
} catch (\Throwable $e) {
$this->container->set('request', null, 'request');
$this->container->leaveScope('request');

throw $e;
}

Expand Down
0