bug #25255 [Console][DI] Fail gracefully (nicolas-grekas)

This PR was merged into the 3.4 branch. Discussion ---------- [Console][DI] Fail gracefully | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | https://github.com/symfony/flex/issues/212, #25280 | License | MIT | Doc PR | - I already experienced this issue a few times without spending time digging it: sometimes, you call `cache:clear`, and the command quits without any output, and with 255 status code. The reason is the `@include` in `Kernel`, which makes everything silent, especially fatal errors (thanks PHP...) So if the to-be-removed container is broken for some fatal reason, the failure is really bad. To fix that, here are two measures: - use `include_once` instead of `require_once` in the dumped container: that's OK there to actually not immediately load the file, any hard failure will happen later anyway, and any soft failure will allow the `cache:clear` command to complete (like when you remove a package) - register `Application::renderException()` as the main PHP exception handler, via `Debug::ErrorHandler` when it's available End result when it fails: ![image](https://user-images.githubusercontent.com/243674/33494543-e1d07202-d6c3-11e7-9677-bc2ae72fbba9.png) instead of a blank output. Commits ------- 4a5a3f52ab [Console][DI] Fail gracefully
2017-12-02 15:04:03 -08:00 · 2017-12-02 15:04:03 -08:00 · c927c481aa
commit c927c481aa
parent 9f1acc1b04 4a5a3f52ab
8 changed files with 74 additions and 38 deletions
--- a/src/Symfony/Component/Console/Application.php
+++ b/src/Symfony/Component/Console/Application.php
@ -40,6 +40,7 @@ use Symfony\Component\Console\Event\ConsoleExceptionEvent;
 use Symfony\Component\Console\Event\ConsoleTerminateEvent;
 use Symfony\Component\Console\Exception\CommandNotFoundException;
 use Symfony\Component\Console\Exception\LogicException;
+use Symfony\Component\Debug\ErrorHandler;
 use Symfony\Component\Debug\Exception\FatalThrowableError;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;

@ -118,6 +119,25 @@ class Application
            $output = new ConsoleOutput();
        }

+        $renderException = function ($e) use ($output) {
+            if (!$e instanceof \Exception) {
+                $e = class_exists(FatalThrowableError::class) ? new FatalThrowableError($e) : new \ErrorException($e->getMessage(), $e->getCode(), E_ERROR, $e->getFile(), $e->getLine());
+            }
+            if ($output instanceof ConsoleOutputInterface) {
+                $this->renderException($e, $output->getErrorOutput());
+            } else {
+                $this->renderException($e, $output);
+            }
+        };
+        if ($phpHandler = set_exception_handler($renderException)) {
+            restore_exception_handler();
+            if (!is_array($phpHandler) || !$phpHandler[0] instanceof ErrorHandler) {
+                $debugHandler = true;
+            } elseif ($debugHandler = $phpHandler[0]->setExceptionHandler($renderException)) {
+                $phpHandler[0]->setExceptionHandler($debugHandler);
+            }
+        }
+
        if (null !== $this->dispatcher && $this->dispatcher->hasListeners(ConsoleEvents::EXCEPTION)) {
            @trigger_error(sprintf('The "ConsoleEvents::EXCEPTION" event is deprecated since Symfony 3.3 and will be removed in 4.0. Listen to the "ConsoleEvents::ERROR" event instead.'), E_USER_DEPRECATED);
        }
@ -125,21 +145,13 @@ class Application
        $this->configureIO($input, $output);

        try {
-            $e = null;
            $exitCode = $this->doRun($input, $output);
        } catch (\Exception $e) {
-        }
-
-        if (null !== $e) {
            if (!$this->catchExceptions) {
                throw $e;
            }

-            if ($output instanceof ConsoleOutputInterface) {
-                $this->renderException($e, $output->getErrorOutput());
-            } else {
-                $this->renderException($e, $output);
-            }
+            $renderException($e);

            $exitCode = $e->getCode();
            if (is_numeric($exitCode)) {
@ -150,6 +162,12 @@ class Application
            } else {
                $exitCode = 1;
            }
+        } finally {
+            if (!$phpHandler) {
+                restore_exception_handler();
+            } elseif (!$debugHandler) {
+                $phpHandler[0]->setExceptionHandler(null);
+            }
        }

        if ($this->autoExit) {
--- a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
+++ b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
@ -217,12 +217,16 @@ EOF;
 {$namespaceLine}
 // This file has been auto-generated by the Symfony Dependency Injection Component for internal use.

-if (!class_exists(\\Container{$hash}\\{$options['class']}::class, false)) {
-    require __DIR__.'/Container{$hash}/{$options['class']}.php';
+if (\\class_exists(\\Container{$hash}\\{$options['class']}::class, false)) {
+    // no-op
+} elseif (!include __DIR__.'/Container{$hash}/{$options['class']}.php') {
+    touch(__DIR__.'/Container{$hash}.legacy');
+
+    return;
 }

-if (!class_exists({$options['class']}::class, false)) {
-    class_alias(\\Container{$hash}\\{$options['class']}::class, {$options['class']}::class, false);
+if (!\\class_exists({$options['class']}::class, false)) {
+    \\class_alias(\\Container{$hash}\\{$options['class']}::class, {$options['class']}::class, false);
 }

 return new \\Container{$hash}\\{$options['class']}();
@ -428,13 +432,13 @@ EOTXT;
            }

            foreach (array_diff_key(array_flip($lineage), $this->inlinedRequires) as $file => $class) {
-                $code .= sprintf("        require_once %s;\n", $file);
+                $code .= sprintf("        include_once %s;\n", $file);
            }
        }

        foreach ($inlinedDefinitions as $def) {
            if ($file = $def->getFile()) {
-                $code .= sprintf("        require_once %s;\n", $this->dumpValue($file));
+                $code .= sprintf("        include_once %s;\n", $this->dumpValue($file));
            }
        }

@ -1233,7 +1237,7 @@ EOF;
        foreach ($lineage as $file) {
            if (!isset($this->inlinedRequires[$file])) {
                $this->inlinedRequires[$file] = true;
-                $code .= sprintf("        require_once %s;\n", $file);
+                $code .= sprintf("        include_once %s;\n", $file);
            }
        }

--- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9.php
@ -297,7 +297,7 @@ class ProjectServiceContainer extends Container
     */
    protected function getMethodCall1Service()
    {
-        require_once '%path%foo.php';
+        include_once '%path%foo.php';

        $this->services['method_call1'] = $instance = new \Bar\FooClass();

--- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_as_files.txt
+++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_as_files.txt
@ -200,7 +200,7 @@ use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
 // This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
 // Returns the public 'method_call1' shared service.

-require_once ($this->targetDirs[0].'/Fixtures/includes/foo.php');
+include_once ($this->targetDirs[0].'/Fixtures/includes/foo.php');

 $this->services['method_call1'] = $instance = new \Bar\FooClass();

@ -473,12 +473,16 @@ class ProjectServiceContainer extends Container

 // This file has been auto-generated by the Symfony Dependency Injection Component for internal use.

-if (!class_exists(\Container%s\ProjectServiceContainer::class, false)) {
-    require __DIR__.'/Container%s/ProjectServiceContainer.php';
+if (\class_exists(\Container%s\ProjectServiceContainer::class, false)) {
+    // no-op
+} elseif (!include __DIR__.'/Container%s/ProjectServiceContainer.php') {
+    touch(__DIR__.'/Container%s.legacy');
+
+    return;
 }

-if (!class_exists(ProjectServiceContainer::class, false)) {
-    class_alias(\Container%s\ProjectServiceContainer::class, ProjectServiceContainer::class, false);
+if (!\class_exists(ProjectServiceContainer::class, false)) {
+    \class_alias(\Container%s\ProjectServiceContainer::class, ProjectServiceContainer::class, false);
 }

 return new \Container%s\ProjectServiceContainer();
--- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php
@ -309,7 +309,7 @@ class ProjectServiceContainer extends Container
     */
    protected function getMethodCall1Service()
    {
-        require_once '%path%foo.php';
+        include_once '%path%foo.php';

        $this->services['method_call1'] = $instance = new \Bar\FooClass();

--- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_inline_requires.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_inline_requires.php
@ -46,10 +46,10 @@ class ProjectServiceContainer extends Container

        $this->aliases = array();

-        require_once $this->targetDirs[1].'/includes/HotPath/I1.php';
-        require_once $this->targetDirs[1].'/includes/HotPath/P1.php';
-        require_once $this->targetDirs[1].'/includes/HotPath/T1.php';
-        require_once $this->targetDirs[1].'/includes/HotPath/C1.php';
+        include_once $this->targetDirs[1].'/includes/HotPath/I1.php';
+        include_once $this->targetDirs[1].'/includes/HotPath/P1.php';
+        include_once $this->targetDirs[1].'/includes/HotPath/T1.php';
+        include_once $this->targetDirs[1].'/includes/HotPath/C1.php';
    }

    public function getRemovedIds()
@ -105,8 +105,8 @@ class ProjectServiceContainer extends Container
     */
    protected function getC2Service()
    {
-        require_once $this->targetDirs[1].'/includes/HotPath/C2.php';
-        require_once $this->targetDirs[1].'/includes/HotPath/C3.php';
+        include_once $this->targetDirs[1].'/includes/HotPath/C2.php';
+        include_once $this->targetDirs[1].'/includes/HotPath/C3.php';

        return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2(${($_ = isset($this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3']) ? $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3'] : $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3()) && false ?: '_'});
    }
@ -118,7 +118,7 @@ class ProjectServiceContainer extends Container
     */
    protected function getC3Service()
    {
-        require_once $this->targetDirs[1].'/includes/HotPath/C3.php';
+        include_once $this->targetDirs[1].'/includes/HotPath/C3.php';

        return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3();
    }
--- a/src/Symfony/Component/HttpKernel/Kernel.php
+++ b/src/Symfony/Component/HttpKernel/Kernel.php
@ -582,7 +582,13 @@ abstract class Kernel implements KernelInterface, RebootableInterface, Terminabl
        $cacheDir = $this->warmupDir ?: $this->getCacheDir();
        $cache = new ConfigCache($cacheDir.'/'.$class.'.php', $this->debug);
        if ($fresh = $cache->isFresh()) {
-            $this->container = require $cache->getPath();
+            // Silence E_WARNING to ignore "include" failures - don't use "@" to prevent silencing fatal errors
+            $errorLevel = error_reporting(\E_ALL ^ \E_WARNING);
+            try {
+                $this->container = include $cache->getPath();
+            } finally {
+                error_reporting($errorLevel);
+            }
            $fresh = \is_object($this->container);
        }
        if (!$fresh) {
@ -590,7 +596,7 @@ abstract class Kernel implements KernelInterface, RebootableInterface, Terminabl
                $collectedLogs = array();
                $previousHandler = set_error_handler(function ($type, $message, $file, $line) use (&$collectedLogs, &$previousHandler) {
                    if (E_USER_DEPRECATED !== $type && E_DEPRECATED !== $type) {
-                        return $previousHandler ? $previousHandler($type, $message, $file, $line) : false;
+                        return $previousHandler ? $previousHandler($type & ~E_WARNING, $message, $file, $line) : E_WARNING === $type;
                    }

                    if (isset($collectedLogs[$message])) {
@ -617,23 +623,27 @@ abstract class Kernel implements KernelInterface, RebootableInterface, Terminabl
                        'count' => 1,
                    );
                });
+            } else {
+                $errorLevel = error_reporting(\E_ALL ^ \E_WARNING);
            }

            try {
                $container = null;
                $container = $this->buildContainer();
                $container->compile();
+
+                $oldContainer = file_exists($cache->getPath()) && is_object($oldContainer = include $cache->getPath()) ? new \ReflectionClass($oldContainer) : false;
            } finally {
                if ($this->debug) {
                    restore_error_handler();

                    file_put_contents($cacheDir.'/'.$class.'Deprecations.log', serialize(array_values($collectedLogs)));
                    file_put_contents($cacheDir.'/'.$class.'Compiler.log', null !== $container ? implode("\n", $container->getCompiler()->getLog()) : '');
+                } else {
+                    error_reporting($errorLevel);
                }
            }

-            $oldContainer = file_exists($cache->getPath()) && is_object($oldContainer = @include $cache->getPath()) ? new \ReflectionClass($oldContainer) : false;
-
            $this->dumpContainer($cache, $container, $class, $this->getContainerBaseClass());
            $this->container = require $cache->getPath();
        }
@ -649,13 +659,13 @@ abstract class Kernel implements KernelInterface, RebootableInterface, Terminabl
            // old container files are not removed immediately,
            // but on a next dump of the container.
            $oldContainerDir = dirname($oldContainer->getFileName());
-            foreach (glob(dirname($oldContainerDir).'/*.legacyContainer') as $legacyContainer) {
-                if ($oldContainerDir.'.legacyContainer' !== $legacyContainer && @unlink($legacyContainer)) {
+            foreach (glob(dirname($oldContainerDir).'/*.legacy') as $legacyContainer) {
+                if ($oldContainerDir.'.legacy' !== $legacyContainer && @unlink($legacyContainer)) {
                    (new Filesystem())->remove(substr($legacyContainer, 0, -16));
                }
            }

-            touch($oldContainerDir.'.legacyContainer');
+            touch($oldContainerDir.'.legacy');
        }

        if ($this->container->has('cache_warmer')) {
--- a/src/Symfony/Component/HttpKernel/Tests/KernelTest.php
+++ b/src/Symfony/Component/HttpKernel/Tests/KernelTest.php
@ -833,7 +833,7 @@ EOF;

        $this->assertTrue(get_class($kernel->getContainer()) !== $containerClass);
        $this->assertFileExists($containerFile);
-        $this->assertFileExists(dirname($containerFile).'.legacyContainer');
+        $this->assertFileExists(dirname($containerFile).'.legacy');
    }

    public function testKernelPass()