Always catch \Throwable, not \Exception, and pass the old exception when re-throwing.

I changed the code to catch \Throwable instead of \Exception (\Throwable covers both php errors and exceptions)

1. First, I think we don't our code execution to stop abruptly on an error and allow it to cleanup (like destroying the stage) and catching \Throwable allows us to do that.
2. Another reason i cloud think of is if we catch the errors too then we can probably format and display them better which might make it easier to debug.

For #3.1 to be actually true, we should apply this pattern far more widely than only when trying $ignored_paths = $this->getIgnoredPaths();.

Assigning to @phenaproxima for review on both #3 and the preceding paragraph.

Also, isn't this basically proposing to do a superset of 🐛 Unhandled Composer Stager exceptions leave the update process in an indeterminate state Fixed ?

First, I think we don't want our code execution to stop abruptly on an error and allow it to cleanup (like destroying the stage and other things) and catching errors using \Throwable allows us to do that.

This is what I'm thinking. I don't see any reason, generally, for us to treat errors and exceptions any differently in Package Manager and AU. (If there is such a case, it should be one-off and heavily commented.) To me, being resilient means we should catch and handle as many problems as we can, and that means catching \Throwable.

Okay, based on #7, @phenaproxima clearly agrees with what I wrote in #5.

However … it looks like that is already the case!

But is not yet consistently implemented by Stage. Marking for that.

Since this affects reliability, tagging core-mvp.

I don't see any more instances to be changed, setting to needs review

The MR is still targeting 8.x-2.x 😅 We need a new MR, that targets 3.0.x.

@omkar.podey: please also add diff /var/www/html/core/scripts/dev/commit-code-check.sh modules/contrib/automatic_updates/commit-code-check.sh like I suggested during the call — that's going to make comparing this simpler.

Also: ls -al /var/www/html/core and ls -al modules/contrib/automatic_updates.

All just prior to executing modules/contrib/automatic_updates/commit-code-check.sh --drupalci.

This is an absolute nightmare to figure out. This approach is also a nightmare to debug. Enough. The alternative approach I've started at #3343430-8: [PP-1] Commit our copied and edited version of commit-code-check.sh → looks promising.

For this issue: just reverting all code quality checks temporarily, so @omkar.podey can continue 👍

Looks good!

🚢

Question.

Do we have any other places where we are directly re-throwing a caught exception? Something like:

try {
  // Do an exceptional thing
}
catch (\Throwable $e) {
  // Do stuff...
  throw $e;
}

...because, in those cases, we are also losing the backtrace of $e, and should be wrapping those exceptions too in most cases.

Ideally we'd have test coverage of this. It wouldn't be too hard to add, but I'm not sure it would be worth it at this point.

This.

The point is that it's not knowable what might fail, but that we have a big net, catch it, and surface it in an appropriate way. That is already the case here! 👍

Do we have any other places where we are directly re-throwing a caught exception? Something like:

I specifically checked that. See #9.

(Method used: search for throw new in all *.php files in both src and package_manager/src.)

phenaproxima → closed merge request !696

(Method used: search for throw new in all *.php files in both src and package_manager/src.)

🤔 Well...throw new would not be the thing. It'd be more like throw $ (i.e., throwing some already-caught exception).

And grep -irn 'throw \$' . does find some stuff:

./package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php:136:      throw $results;
./package_manager/tests/modules/package_manager_bypass/src/LoggingCommitter.php:49:      throw $exception;
./package_manager/tests/src/Kernel/ComposerExecutableValidatorTest.php:300:      throw $output;
./package_manager/tests/src/Kernel/FixtureManipulatorTest.php:390:        throw $exception;
./package_manager/src/ComposerInspector.php:106:          throw $e;
./package_manager/src/Stage.php:540:      throw $error;
./automatic_updates_extensions/src/BatchProcessor.php:67:    throw $error;
./src/Validator/VersionPolicyValidator.php:198:        throw $unknown_target;
./src/Validator/VersionPolicyValidator.php:211:    throw $unknown_target;
./src/BatchProcessor.php:77:    throw $error;

Most of these seem fine. The only one that's maybe a little questionable is the one in ComposerInspector; we're re-throwing an exception from Composer Stager. IMHO that one should be wrapped. Thoughts?

Oh!

Interesting. But that's not the pattern this issue was aiming to address, that's why I didn't check for that.

No, the one in ComposerInspector is 100% intentional. (That happened in 🐛 Validate compliance with composer minimum stability during PreRequireEvent Fixed after very careful deliberation.)

We are not in the business of wrapping composer-stager's exceptions in ComposerInspector because validators ought to handle exceptions of composer-stager themselves.

Otherwise, \Drupal\package_manager\Validator\ComposerSettingsValidator::validate() wouldn't have to do this for example:

    try {
      $setting = (int) $this->inspector->getConfig('secure-http', $dir);
    }
    catch (\Exception $exception) {
      $event->addErrorFromThrowable($exception, $this->t('Unable to determine Composer <code>secure-http

setting.'));
return;
}

If you're arguing that ComposerInspector should wrap all \PhpTuf\ComposerStager\Domain\Service\ProcessRunner\ComposerRunnerInterface::run() exceptions, that'd be a different concern/discussion, and IMHO out of scope.

You've convinced me. Merged!

Automatically closed - issue fixed for 2 weeks with no activity.