Bug #100871: Common symfony command exceptions should not be thrown - TYPO3 Core - TYPO3 Forge

Actions

Copy link

Bug #100871

closed

Common symfony command exceptions should not be thrown

Added by Torben Hansen over 1 year ago. Updated 5 months ago.

Status:

Closed

Priority:

Should have

Assignee:

Torben Hansen

Category:

Target version:

Start date:

2023-05-13

Due date:

% Done:

100%

Estimated time:

TYPO3 Version:

PHP Version:

Tags:

Complexity:

Is Regression:

Sprint Focus:

Description

With #99912, setCatchExceptions(false) has been implemented to TYPO3\CMS\Core\Console\CommandApplication, so that exceptions from symfony console are handled by TYPO3 exception handlers. That change made it possible to log CLI errors in TYPO3 logs, but after thinking more about the change, I see some downsides in #99912 as pointed out below:

CLI now throws all symfony console exceptions, which does not look pretty
Possible user errors (e.g. unknown command or invalid amount of arguments) are now logged to the TYPO3 log

Example: ./typo3/sysext/core/bin/typo3 site:show

Uncaught TYPO3 Exception Not enough arguments (missing: "identifier").
thrown in file /path/to/typo3/vendor/symfony/console/Input/Input.php
in line 70

Also note, that #100059 fixes the described problem about the not registered listener in #99912. So it is now possible to register event listeners, who actually catch all CLI errors.

I'm currently not really sure, what is best to resolve the described downsides. We could either catch common/known symfony console exceptions and print the message in a "pretty" way or we could revert #99912 and implement an event listener for ConsoleErrorEvent events, which logs those errors to the TYPO3 log.

Related issues 1 (0 open — 1 closed)

Actions

Copy link

Updated by Torben Hansen over 1 year ago

Related to Bug #99912: ProductionExceptionHandler does not handle CLI Exceptions added

Actions

Copy link

Updated by Torben Hansen over 1 year ago

Description updated (diff)

Actions

Copy link

Updated by Christoph Lehmann over 1 year ago

If it's now possible to use a ConsoleErrorEventListener, then we could react on interactive mode (-n flag) and "forward" Exceptions to the ProductionExceptionHandler in non-interactive mode only. With the downside users possibly need to configure the flag for e.g. typo3/sysext/core/bin/typo3 scheduler:run (?)

A more beautiful formatting would be nice to have.

I have no opinion or rather don't care about the superfluous log messages in error log due to console errors like it is at the moment, because when something faulty happens in prod it's better to know it, even if it's due to wrong user input.

Actions

Copy link

Updated by Torben Hansen over 1 year ago

I created an event listener which listens on ConsoleErrorEvent events and logs exceptions using AbstractExceptionHandler::writeLogEntries(), but this also has some downsides:

All exceptions will be logged to sys_log and TYPO3 log (this includes possible user input failures)
The event listener is not really pretty, because in order to execute TYPO3 exception handling functionality for CLI (@AbstractExceptionHandler::handleException()), the current exception handler must be set/restored

I therefore suggest to revert #99912. If logging of CLI exceptions is required by 3rd party code (e.g. Sentry extension), it is recommended that the 3rd party application listens on ConsoleErrorEvent to process logging of thrown console exceptions.

Actions

Copy link

Updated by Gerrit Code Review over 1 year ago

Status changed from New to Under Review

Patch set 1 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/79060

Actions

Copy link

Updated by Gerrit Code Review over 1 year ago

Patch set 2 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/79060

Actions

Copy link

Updated by Gerrit Code Review over 1 year ago

Patch set 3 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/79060

Actions

Copy link

Updated by Gerrit Code Review over 1 year ago

Patch set 4 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/79060

Actions

Copy link

Updated by Gerrit Code Review over 1 year ago

Patch set 1 for branch 12.4 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/79074

Actions

Copy link

#10