Skip to content

use try-finally when possible #15949

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

Merged
merged 1 commit into from
Sep 28, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public function load($resource, $type = null)
// Here is the scenario:
// - while routes are being loaded by parent::load() below, a fatal error
// occurs (e.g. parse error in a controller while loading annotations);
// - PHP abruptly empties the stack trace, bypassing all catch blocks;
// - PHP abruptly empties the stack trace, bypassing all catch/finally blocks;
// it then calls the registered shutdown functions;
// - the ErrorHandler catches the fatal error and re-injects it for rendering
// thanks to HttpKernel->terminateWithException() (that calls handleException());
Expand All @@ -74,13 +74,10 @@ public function load($resource, $type = null)

try {
$collection = parent::load($resource, $type);
} catch (\Exception $e) {
} finally {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolas-grekas I assume this hack also works with finally, i.e. fatal errors are skipping it. I tried to test it as described in #14342. And it had the same result. So this code change didn't change the outcome of the exception page. But it was kinda strange anyway as there was a second error below the real one:

FatalErrorException in TraceableEventDispatcher.php line 357:
Error: Cannot use object of type Symfony\Component\EventDispatcher\Debug\WrappedListener as array

in vendor\symfony\symfony\src\Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher.php at line 357

$this->loading = false;
throw $e;
}

$this->loading = false;

foreach ($collection->all() as $route) {
if ($controller = $route->getDefault('_controller')) {
try {
Expand Down
5 changes: 1 addition & 4 deletions src/Symfony/Component/Config/Loader/FileLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,10 @@ public function import($resource, $type = null, $ignoreErrors = false, $sourceRe

try {
$ret = $loader->load($resource, $type);
} catch (\Exception $e) {
} finally {
unset(self::$loading[$resource]);
throw $e;
}

unset(self::$loading[$resource]);

return $ret;
} catch (FileLoaderImportCircularReferenceException $e) {
throw $e;
Expand Down
7 changes: 1 addition & 6 deletions src/Symfony/Component/Config/Tests/Util/XmlUtilsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -169,16 +169,11 @@ public function testLoadWrongEmptyXMLWithErrorHandler()
} catch (\InvalidArgumentException $e) {
$this->assertEquals(sprintf('File %s does not contain valid XML, it is empty.', $file), $e->getMessage());
}
} catch (\Exception $e) {
} finally {
restore_error_handler();
error_reporting($errorReporting);

throw $e;
}

restore_error_handler();
error_reporting($errorReporting);

$disableEntities = libxml_disable_entity_loader(true);
libxml_disable_entity_loader($disableEntities);

Expand Down
6 changes: 1 addition & 5 deletions src/Symfony/Component/Debug/DebugClassLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -130,14 +130,10 @@ public function loadClass($class)
call_user_func($this->classLoader, $class);
$file = false;
}
} catch (\Exception $e) {
} finally {
ErrorHandler::unstackErrors();

throw $e;
}

ErrorHandler::unstackErrors();

$exists = class_exists($class, false) || interface_exists($class, false) || trait_exists($class, false);

if ('\\' === $class[0]) {
Expand Down
5 changes: 1 addition & 4 deletions src/Symfony/Component/Debug/ErrorHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -468,11 +468,8 @@ public function handleError($type, $message, $file, $line, array $context, array
try {
$this->isRecursive = true;
$this->loggers[$type][0]->log(($type & $level) ? $this->loggers[$type][1] : LogLevel::DEBUG, $message, $e);
} finally {
$this->isRecursive = false;
} catch (\Exception $e) {
$this->isRecursive = false;

throw $e;
}
}

Expand Down
6 changes: 1 addition & 5 deletions src/Symfony/Component/Debug/Tests/DebugClassLoaderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,6 @@ class ChildTestingStacking extends TestingStacking { function foo($bar) {} }
$this->fail('ContextErrorException expected');
} catch (\ErrorException $exception) {
// if an exception is thrown, the test passed
restore_error_handler();
restore_exception_handler();
$this->assertStringStartsWith(__FILE__, $exception->getFile());
if (PHP_VERSION_ID < 70000) {
$this->assertRegExp('/^Runtime Notice: Declaration/', $exception->getMessage());
Expand All @@ -118,11 +116,9 @@ class ChildTestingStacking extends TestingStacking { function foo($bar) {} }
$this->assertRegExp('/^Warning: Declaration/', $exception->getMessage());
$this->assertEquals(E_WARNING, $exception->getSeverity());
}
} catch (\Exception $exception) {
} finally {
restore_error_handler();
restore_exception_handler();

throw $exception;
}
}

Expand Down
49 changes: 7 additions & 42 deletions src/Symfony/Component/Debug/Tests/ErrorHandlerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,6 @@ public function testNotice()
$this->fail('ContextErrorException expected');
} catch (ContextErrorException $exception) {
// if an exception is thrown, the test passed
restore_error_handler();
restore_exception_handler();

$this->assertEquals(E_NOTICE, $exception->getSeverity());
$this->assertEquals(__FILE__, $exception->getFile());
$this->assertRegExp('/^Notice: Undefined variable: (foo|bar)/', $exception->getMessage());
Expand All @@ -96,11 +93,9 @@ public function testNotice()
$this->assertEquals(__CLASS__, $trace[2]['class']);
$this->assertEquals(__FUNCTION__, $trace[2]['function']);
$this->assertEquals('->', $trace[2]['type']);
} catch (\Exception $e) {
} finally {
restore_error_handler();
restore_exception_handler();

throw $e;
}
}

Expand All @@ -118,14 +113,9 @@ public function testConstruct()
$handler = ErrorHandler::register();
$handler->throwAt(3, true);
$this->assertEquals(3 | E_RECOVERABLE_ERROR | E_USER_ERROR, $handler->throwAt(0));

} finally {
restore_error_handler();
restore_exception_handler();
} catch (\Exception $e) {
restore_error_handler();
restore_exception_handler();

throw $e;
}
}

Expand Down Expand Up @@ -157,14 +147,9 @@ public function testDefaultLogger()
E_CORE_ERROR => array(null, LogLevel::CRITICAL),
);
$this->assertSame($loggers, $handler->setLoggers(array()));

restore_error_handler();
restore_exception_handler();
} catch (\Exception $e) {
} finally {
restore_error_handler();
restore_exception_handler();

throw $e;
}
}

Expand Down Expand Up @@ -283,14 +268,9 @@ public function testHandleUserError()
}

$this->assertSame($x, $e);

restore_error_handler();
restore_exception_handler();
} catch (\Exception $e) {
} finally {
restore_error_handler();
restore_exception_handler();

throw $e;
}
}

Expand Down Expand Up @@ -350,14 +330,9 @@ public function testHandleException()
});

$handler->handleException($exception);

} finally {
restore_error_handler();
restore_exception_handler();
} catch (\Exception $e) {
restore_error_handler();
restore_exception_handler();

throw $e;
}
}

Expand All @@ -384,14 +359,9 @@ public function testErrorStacking()
@trigger_error('Silenced warning', E_USER_WARNING);
$logger->log(LogLevel::WARNING, 'Dummy log');
ErrorHandler::unstackErrors();

} finally {
restore_error_handler();
restore_exception_handler();
} catch (\Exception $e) {
restore_error_handler();
restore_exception_handler();

throw $e;
}
}

Expand Down Expand Up @@ -513,14 +483,9 @@ public function testHandleFatalErrorOnHHVM()

call_user_func_array(array($handler, 'handleError'), $error);
$handler->handleFatalError($error);

} finally {
restore_error_handler();
restore_exception_handler();
} catch (\Exception $e) {
restore_error_handler();
restore_exception_handler();

throw $e;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -457,14 +457,10 @@ public function get($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INV

try {
$service = $this->createService($definition, $id);
} catch (\Exception $e) {
} finally {
unset($this->loading[$id]);

throw $e;
}

unset($this->loading[$id]);

return $service;
}

Expand Down
8 changes: 2 additions & 6 deletions src/Symfony/Component/OptionsResolver/OptionsResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -783,11 +783,9 @@ public function offsetGet($option)
foreach ($this->lazy[$option] as $closure) {
$value = $closure($this, $value);
}
} catch (\Exception $e) {
} finally {
unset($this->calling[$option]);
throw $e;
}
unset($this->calling[$option]);
// END
}

Expand Down Expand Up @@ -885,11 +883,9 @@ public function offsetGet($option)
$this->calling[$option] = true;
try {
$value = $normalizer($this, $value);
} catch (\Exception $e) {
} finally {
unset($this->calling[$option]);
throw $e;
}
unset($this->calling[$option]);
// END
}

Expand Down
4 changes: 1 addition & 3 deletions src/Symfony/Component/Yaml/Tests/InlineTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,8 @@ public function testDumpNumericValueWithLocale()

$this->assertEquals('1.2', Inline::dump(1.2));
$this->assertContains('fr', strtolower(setlocale(LC_NUMERIC, 0)));
} finally {
setlocale(LC_NUMERIC, $locale);
} catch (\Exception $e) {
setlocale(LC_NUMERIC, $locale);
throw $e;
}
}

Expand Down