Project

General

Profile

Actions

Bug #100871

closed

Common symfony command exceptions should not be thrown

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

Status:
Closed
Priority:
Should have
Assignee:
Category:
-
Target version:
-
Start date:
2023-05-13
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
12
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 open1 closed)

Related to TYPO3 Core - Bug #99912: ProductionExceptionHandler does not handle CLI ExceptionsClosed2023-02-09

Actions
Actions #1

Updated by Torben Hansen over 1 year ago

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

Updated by Torben Hansen over 1 year ago

  • Description updated (diff)
Actions #3

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 #4

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 #5

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 #6

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 #7

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 #8

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 #9

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 #10

Updated by Torben Hansen over 1 year ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
Actions #11

Updated by Benni Mack 4 months ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF