why #211
why #211
Conversation
| @@ -213,7 +213,7 @@ public function boot($options = array()) { | |||
|
|
|||
| $this->log->debug("Load settings file \"" . $settings . "\""); | |||
| define('CIVICRM_SETTINGS_PATH', $settings); | |||
| $error = @include_once $settings; | |||
| $error = include_once $settings; | |||
There was a problem hiding this comment.
For this one, my guess: inertia
It's a little hard to see now, but Bootstrap.php arose from reorganizing/consolidating various copies of civicrm.config.php.XXX -- and some copies had suppression on this line. My guess is that someone felt dissatisfied that include_once didn't provide enough recoverability, and no one had the mix of incentive/time/expertise to question it.
But I share your view that this line probably isn't doing much.
| if (PHP_SAPI === "cli") { | ||
| error_reporting(1); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is more likely to be doing something. Different CMS's are likely to switch error-reporting in different ways. (And if they have primary responsibility for processing the request, then power to them.) But from cv-cli pov, we just want access to their APIs -- and then the CLI UX should be consistent wrt to error-handling.
Looking at this again, I'm a little on the fence. Perhaps this value should be tuned-in more to the verbosity level...
There was a problem hiding this comment.
On a live site, it's probably set to 1 to begin with. On a dev site, it's probably E_ALL. I'm not clear on why it can't just be left at whatever the surrounding environment has. But yes it could work to set error_reporting(1) here, but if you specify enough 'v's then skip setting it (and/or maybe force E_ALL with 3 v's?)
| $result = $application->run(); | ||
| ErrorHandler::popHandler(); |
There was a problem hiding this comment.
I believe that this was a response to shifts in how "Symfony Console" approached error handling. Cv used symfony/console 2.x for a long time, and this wasn't needed. (Maybe 2.x had a more "batteries included" philosophy? Or just didn't have the bug) Somewhere around 3.x or 4.x, the default error-handling became... worse ("less opinionated"? "more vanilla"?)... e.g. it would exit(0) even if there was an error.
Of course, now we've switched to 5.x... maybe it's changed again...
There was a problem hiding this comment.
Ok. I have had silence before this one existed, so it's probably not the biggest one, although there must be some error handler somewhere because normally @ doesn't silence fatals. But removing the @ is maybe the best bang for buck.
Trimmed down version of #211 - just remove the `@` suppression
I'm curious about: