symfony / recipes

Symfony Recipes Repository
https://github.com/symfony/recipes/blob/flex/main/RECIPES.md
MIT License
964 stars 476 forks source link

chore: Always catch errors when calling `console` #1296

Closed llupa closed 7 months ago

llupa commented 7 months ago
Q A
License MIT
Doc issue/PR #53946

Pinging @wouterj @greg0ire

This is my first time doing "recipe" work and I am not sure if this is the correct place to open this PR. Let me know for any change needed!

In short: when executing console commands in production mode/env commands have to fail with exit 1, and not render the \Error and exit 0.

github-actions[bot] commented 7 months ago

Thanks for the PR 😍

How to test these changes in your application

  1. Define the SYMFONY_ENDPOINT environment variable:

    # On Unix-like (BSD, Linux and macOS)
    export SYMFONY_ENDPOINT=https://raw.githubusercontent.com/symfony/recipes/flex/pull-1296/index.json
    # On Windows
    SET SYMFONY_ENDPOINT=https://raw.githubusercontent.com/symfony/recipes/flex/pull-1296/index.json
  2. Install the package(s) related to this recipe:

    composer req 'symfony/flex:^1.16'
    composer req 'symfony/console:^6.4'
  3. Don't forget to unset the SYMFONY_ENDPOINT environment variable when done:

    # On Unix-like (BSD, Linux and macOS)
    unset SYMFONY_ENDPOINT
    # On Windows
    SET SYMFONY_ENDPOINT=

Diff between recipe versions

In order to help with the review stage, I'm in charge of computing the diff between the various versions of patched recipes. I'm going keep this comment up to date with any updates of the attached patch.

symfony/console

3.3 vs 4.2 ```diff diff --git a/symfony/console/3.3/config/bootstrap.php b/symfony/console/4.2/config/bootstrap.php index 2a47186..55560fb 100644 --- a/symfony/console/3.3/config/bootstrap.php +++ b/symfony/console/4.2/config/bootstrap.php @@ -13,38 +13,8 @@ if (!class_exists(Dotenv::class)) { if (is_array($env = @include dirname(__DIR__).'/.env.local.php') && (!isset($env['APP_ENV']) || ($_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? $env['APP_ENV']) === $env['APP_ENV'])) { (new Dotenv(false))->populate($env); } else { - $path = dirname(__DIR__).'/.env'; - $dotenv = new Dotenv(false); - // load all the .env files - if (method_exists($dotenv, 'loadEnv')) { - $dotenv->loadEnv($path); - } else { - // fallback code in case your Dotenv component is not 4.2 or higher (when loadEnv() was added) - - if (file_exists($path) || !file_exists($p = "$path.dist")) { - $dotenv->load($path); - } else { - $dotenv->load($p); - } - - if (null === $env = $_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? null) { - $dotenv->populate(array('APP_ENV' => $env = 'dev')); - } - - if ('test' !== $env && file_exists($p = "$path.local")) { - $dotenv->load($p); - $env = $_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? $env; - } - - if (file_exists($p = "$path.$env")) { - $dotenv->load($p); - } - - if (file_exists($p = "$path.$env.local")) { - $dotenv->load($p); - } - } + (new Dotenv(false))->loadEnv(dirname(__DIR__).'/.env'); } $_SERVER += $_ENV; ```
4.2 vs 4.4 ```diff diff --git a/symfony/console/4.2/bin/console b/symfony/console/4.4/bin/console index 5cef879..5de0e1c 100755 --- a/symfony/console/4.2/bin/console +++ b/symfony/console/4.4/bin/console @@ -4,7 +4,7 @@ use App\Kernel; use Symfony\Bundle\FrameworkBundle\Console\Application; use Symfony\Component\Console\Input\ArgvInput; -use Symfony\Component\Debug\Debug; +use Symfony\Component\ErrorHandler\Debug; if (!in_array(PHP_SAPI, ['cli', 'phpdbg', 'embed'], true)) { echo 'Warning: The console should be invoked via the CLI version of PHP, not the '.PHP_SAPI.' SAPI'.PHP_EOL; ```
4.4 vs 5.1 ```diff diff --git a/symfony/console/4.4/bin/console b/symfony/console/5.1/bin/console index 5de0e1c..8fe9d49 100755 --- a/symfony/console/4.4/bin/console +++ b/symfony/console/5.1/bin/console @@ -4,6 +4,7 @@ use App\Kernel; use Symfony\Bundle\FrameworkBundle\Console\Application; use Symfony\Component\Console\Input\ArgvInput; +use Symfony\Component\Dotenv\Dotenv; use Symfony\Component\ErrorHandler\Debug; if (!in_array(PHP_SAPI, ['cli', 'phpdbg', 'embed'], true)) { @@ -14,8 +15,8 @@ set_time_limit(0); require dirname(__DIR__).'/vendor/autoload.php'; -if (!class_exists(Application::class)) { - throw new LogicException('You need to add "symfony/framework-bundle" as a Composer dependency.'); +if (!class_exists(Application::class) || !class_exists(Dotenv::class)) { + throw new LogicException('You need to add "symfony/framework-bundle" and "symfony/dotenv" as Composer dependencies.'); } $input = new ArgvInput(); @@ -27,7 +28,7 @@ if ($input->hasParameterOption('--no-debug', true)) { putenv('APP_DEBUG='.$_SERVER['APP_DEBUG'] = $_ENV['APP_DEBUG'] = '0'); } -require dirname(__DIR__).'/config/bootstrap.php'; +(new Dotenv())->bootEnv(dirname(__DIR__).'/.env'); if ($_SERVER['APP_DEBUG']) { umask(0000); diff --git a/symfony/console/4.4/config/bootstrap.php b/symfony/console/4.4/config/bootstrap.php deleted file mode 100644 index 55560fb..0000000 --- a/symfony/console/4.4/config/bootstrap.php +++ /dev/null @@ -1,23 +0,0 @@ -=1.2) -if (is_array($env = @include dirname(__DIR__).'/.env.local.php') && (!isset($env['APP_ENV']) || ($_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? $env['APP_ENV']) === $env['APP_ENV'])) { - (new Dotenv(false))->populate($env); -} else { - // load all the .env files - (new Dotenv(false))->loadEnv(dirname(__DIR__).'/.env'); -} - -$_SERVER += $_ENV; -$_SERVER['APP_ENV'] = $_ENV['APP_ENV'] = ($_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? null) ?: 'dev'; -$_SERVER['APP_DEBUG'] = $_SERVER['APP_DEBUG'] ?? $_ENV['APP_DEBUG'] ?? 'prod' !== $_SERVER['APP_ENV']; -$_SERVER['APP_DEBUG'] = $_ENV['APP_DEBUG'] = (int) $_SERVER['APP_DEBUG'] || filter_var($_SERVER['APP_DEBUG'], FILTER_VALIDATE_BOOLEAN) ? '1' : '0'; diff --git a/symfony/console/4.4/manifest.json b/symfony/console/5.1/manifest.json index c00421d..b0e9e1d 100644 --- a/symfony/console/4.4/manifest.json +++ b/symfony/console/5.1/manifest.json @@ -1,7 +1,6 @@ { "copy-from-recipe": { - "bin/": "%BIN_DIR%/", - "config/": "%CONFIG_DIR%/" + "bin/": "%BIN_DIR%/" }, "aliases": ["cli"] } ```
5.1 vs 5.3 ```diff diff --git a/symfony/console/5.1/bin/console b/symfony/console/5.3/bin/console index 8fe9d49..c933dc5 100755 --- a/symfony/console/5.1/bin/console +++ b/symfony/console/5.3/bin/console @@ -3,41 +3,15 @@ use App\Kernel; use Symfony\Bundle\FrameworkBundle\Console\Application; -use Symfony\Component\Console\Input\ArgvInput; -use Symfony\Component\Dotenv\Dotenv; -use Symfony\Component\ErrorHandler\Debug; -if (!in_array(PHP_SAPI, ['cli', 'phpdbg', 'embed'], true)) { - echo 'Warning: The console should be invoked via the CLI version of PHP, not the '.PHP_SAPI.' SAPI'.PHP_EOL; +if (!is_file(dirname(__DIR__).'/vendor/autoload_runtime.php')) { + throw new LogicException('Symfony Runtime is missing. Try running "composer require symfony/runtime".'); } -set_time_limit(0); +require_once dirname(__DIR__).'/vendor/autoload_runtime.php'; -require dirname(__DIR__).'/vendor/autoload.php'; +return function (array $context) { + $kernel = new Kernel($context['APP_ENV'], (bool) $context['APP_DEBUG']); -if (!class_exists(Application::class) || !class_exists(Dotenv::class)) { - throw new LogicException('You need to add "symfony/framework-bundle" and "symfony/dotenv" as Composer dependencies.'); -} - -$input = new ArgvInput(); -if (null !== $env = $input->getParameterOption(['--env', '-e'], null, true)) { - putenv('APP_ENV='.$_SERVER['APP_ENV'] = $_ENV['APP_ENV'] = $env); -} - -if ($input->hasParameterOption('--no-debug', true)) { - putenv('APP_DEBUG='.$_SERVER['APP_DEBUG'] = $_ENV['APP_DEBUG'] = '0'); -} - -(new Dotenv())->bootEnv(dirname(__DIR__).'/.env'); - -if ($_SERVER['APP_DEBUG']) { - umask(0000); - - if (class_exists(Debug::class)) { - Debug::enable(); - } -} - -$kernel = new Kernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG']); -$application = new Application($kernel); -$application->run($input); + return new Application($kernel); +}; ```
5.3 vs 6.4 ```diff diff --git a/symfony/console/5.3/bin/console b/symfony/console/6.4/bin/console index c933dc5..5573910 100755 --- a/symfony/console/5.3/bin/console +++ b/symfony/console/6.4/bin/console @@ -13,5 +13,8 @@ require_once dirname(__DIR__).'/vendor/autoload_runtime.php'; return function (array $context) { $kernel = new Kernel($context['APP_ENV'], (bool) $context['APP_DEBUG']); - return new Application($kernel); + $app = new Application($kernel); + $app->setCatchErrors(true); + + return $app; }; ```
nicolas-grekas commented 7 months ago

This would be a work around. Let's fix the root issue instead, see https://github.com/symfony/symfony/pull/54189

wouterj commented 7 months ago

@nicolas-grekas I think aside of this issue that triggered this PR, we might want to still do this.

The console app now defaults to catchExceptions: true and catchErrors: false. I think it makes sense to make both true in Symfony 8, meaning the console terminate event is dispatched for both exceptions and errors. Thus we can do the traditional BC cycle here: deprecate not setting catchErrors, change recipe to always use new default (this PR), and in 8.0, swap default around, undeprecate not setting catchErrors and update recipe to not override the default. What do you think?

(btw, thanks for providing a patch in the error handler!)

llupa commented 7 months ago

Not as detailed as Wouter's comment, but I also will have to set it to true for my projects because I cannot really think of a use case (DX) where I want Exceptions to be caught and rendered one way, but Errors not caught (probably will be after your patch) but rendered a different way.

Maybe it boils down to personal preference, but it is odd that it works differently for one from the other.

nicolas-grekas commented 7 months ago

I think we should minimize those flip-flap with deprecations so I'd better not change anything personally. Also the current logic is perfectly fine to me. Error are split from Exception because they're not the same beast.