Fail more gracefully when exceptions happen, eg. Uncaught PHPStan\Broker\ClassAutoloadingException with Redirect 8.x-1.3

Retitled. The module theoretically has error recovery for fatal PHP errors, but it does not work for this case. We should up our recovery potential if we can recover from this issue in PHP itself (it. catch the exception and save it instead of failing entirely).

@aspilicious also reproduced the same issue independently.

All right. So there is a problem with Redirect module here. That was fixed on June 22 and released in v1.4, so that should run fine. See https://git.drupalcode.org/project/redirect/commit/a4d287869a4f782fe9019... for the commit that fixed it in redirect.

That said, Upgrade Status should fail more gracefully and not whitescreen.

I tried to reproduce this also with drupal-check, but it uses such a recent (dev) phpstan now that it had a PHP issue within phpstan, haha. This one: Fatal error:

Class PHPStan\Type\MixedType contains 2 abstract methods and must therefore be declared abstract or implement the remaining methods (PHPStan\Type\Type::inferTemplateTypes, PHPStan\Type\Type::traverse) in ....drupal/vendor/phpstan/phpstan/src/Type/MixedType.php on line 18

That said, that is totally unrelated to this issue. I submitted a pull request to drupal-check to fix THAT on their side: https://github.com/mglaman/drupal-check/issues/93

I will look into reproducing this issue with Upgrade Status and Redirect 1.3 now and see if we can do something about catching the errors.

This exception is coming from Command\AnalyseApplication->analyse( ).../DeprecationAnalyser.php:345 as per the above stack trace and also my reproducing. This indeed we are not catching. I thought this should do it, but it does not seem to actually catch it. Anyone spots where am I mislead?

(I also removed a lower level try/catch, so we can collect all the exceptions at one place and store more info about them, but that is not where this exception originates from, so it should not change the behaviour either way).

Also needs tests once it works.

As per the call stack, the try/catch block is not even in this call path for some reason. I guess when PHPStan Broker does spl_autoload_register() that closure is called out of band from our call path, so we cannot catch that exception in a call path based catcher.

So I also tried to set a global exception handler, but no luck there either. Drupal already sets the _drupal_exception_handler() but I tried to set_exception_handler(null); to reset that and also set_exception_handler('Drupal\upgrade_status\DeprecationAnalyser::exceptionHandler'); (after I added the static method) but neither helped.

¯\_(ツ)_/¯

Added steps to reproduce.

Well this is broken code no - it seems this has been fixed by #3048310: Fatal error analysing code with phpstan. Symfony\Component\Validator\ExecutionContextInterface never existed so there's not much we can do right? Do we need to tell users to be on the latest version of the contrib module? That would fix this.

The problem is that \Drupal\redirect\Plugin\Validation\Constraint\SourceLinkTypeConstraint::initialize() breaks the interface it implements. So whilst doing the autoload we go straight into the shutdown functions. There's no error to catch. If we want to prevent this type error I think we need to do AST parsing and validate the PHP prior to including it.

try {
  include 'modules/redirect/src/Plugin/Validation/Constraint/SourceLinkTypeConstraint.php';
}
catch (\Throwable $e) {
  var_dump("here");
}

If I run the above script in Drush the error you get is

PHP Fatal error:  Declaration of Drupal\redirect\Plugin\Validation\Constraint\SourceLinkTypeConstraint::initialize(Symfony\Component\Validator\ExecutionContextInterface $context) must be compatible with Symfony\Component\Validator\ConstraintValidatorInterface::initialize(Symfony\Component\Validator\Context\ExecutionContextInterface $context) in modules/redirect/src/Plugin/Validation/Constraint/SourceLinkTypeConstraint.php on line 24

Yes redirect did fix their error in 1.4, but according to @mixologic, around 900 contrib modules have some kind of PHP parse error when analysed with phpstan. There was a similar one in ctools we found when testing Upgrade Status. If these high profile modules have them, we better try and catch them if we can and not just say "yeah its broken modules". So I was trying to do that :)

I don't know what happens once we catch the exception from phpstan, but that is our primary symptom here. In this case the autoloading fail will halt the processing, so the interface mismatch will not be reached AFAIS. So it could still be useful to catch these cases. @chx proposed this fix to phpstan to make it catchable: https://github.com/phpstan/phpstan/compare/master...chx:chx?expand=1 I will try this locally and see if that helps catch this and gracefully recover for this issue. If the autoloading would have worked but there would still be a type mismatch, that is something we cannot catch apparently, thanks for looking at that @alexpott.

Sidenote: two types of PHP fatal errors @mixologic found that would fall under the same "cannot catch" umbrella that @alexpott uncovered:

PHP Fatal error:  Declaration of Drupal\date_recur_ss\Plugin\DateRecurInterpreter\SsInterpreter::interpret(array $rules, $language): string must be compatible with Drupal\date_recur\Plugin\DateRecurInterpreterPluginInterface::interpret(array $rules, string $language): string in /var/lib/drupalci/workspace/drupal-checkouts/drupal27/modules/contrib/date_recur_ss/src/Plugin/DateRecurInterpreter/SsInterpreter.php on line 23

PHP Fatal error:  Declaration of Drupal\paragraphs_enhancements\Plugin\Field\FieldWidget\ParagraphsEnhancementsWidget::create($pluginId, $pluginDefinition, array $configuration, Symfony\Component\DependencyInjection\ContainerInterface $container) must be compatible with Drupal\Core\Plugin\ContainerFactoryPluginInterface::create(Symfony\Component\DependencyInjection\ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) in /var/lib/drupalci/workspace/drupal-checkouts/drupal2/modules/contrib/paragraphs_enhancements/src/Plugin/Field/FieldWidget/ParagraphsEnhancementsWidget.php on line 22

(These were results of running drupal-check on contrib modules).

Here's an idea for how to fail a bit more gracefully. We can do another request in the batch to do the scan. This way if it breaks in a way that causes PHP fatal error we can do something a bit nicer for the user because the fail has not happened in the user's request.

The code is a bit janky at the moment but it can be cleaned up.

Thanks @alexpott, that made me think of even more options :) @chx noting that the execution will continue made me think that we can use the existing shutdown function to gracefully recover. In the prior UI implementation we did have a similar recovery method where we stored which extension failed in state and then cleaned up later. That did not have test coverage (oops) so got refactored out of the module (oops) when it was simplified.

The culprit is the batch JS which bails out when there was a non-JSON response. But we can make Drupal not emit the error and log it instead and also return a sensible JSON response that keeps the batch running. The only problem then is the batch item is not marked complete, so it will kept being invoked again and again and again. So I opted to store a "unique" scanid identified (based on time and random generated at form submit time for this batch). So when we get to a batch item and we find there were already results, we can check if said results were from this same batch and if so, we can ignore trying to do the futile task of parsing again.

This way we can store more useful fatal error details like file name, line number, etc. which @alexpott's solution did not provide. It is true that there is no sandboxing of the parsing in a different HTTP request here, but phpstan itself is quite heavy already so not wrapping it in one more layer of HTTP request may help not run so slow.

Now this definitely needs test coverage to ensure we don't regress the functionality again.

The progress bar BTW tells the user there was a fatal error and we recovered from it triumphantly :D

Hm, it does not exactly work in a no-JS environment where the response is not AJAX and therefore there is already some response. As this sample proves. We can still look at output buffering and emptying the buffer in the error case without outputing it, but generating JSON then is useless to carry on processing. Duh. So probably @alexpott's approach is the workable one.

Hum, I don't even know how to test this on d.o as there is multiple instances of the fatal PHP file failing on testbot before it ever has a chance to run the tests. It does not seem like sacrificing some verification steps would avoid that as it fails right in composer.

From discussion at #3067356: Fail gracefully for HTTP result 500, patch attached to catch HTTP result code 500 .

Instead of working forward from my patch, I decided to work forward from @alexpott's from #13. Cleaned that up and can now log exceptions in the HTTP request part as well. Let's see how tests will behave here. Fixed the analyse stuff to work with profiles and fixed docs.

Cannot reproduce fails locally. Not sure the above was running with beta1.

There are even more changes since beta1 in error/warning logic. Retest on dev.

Putting back the fresh service retrieval because it does matter now given the separate HTTP request, good point @alexpott. Also removing the injected service, since that does not get used anymore. Also fixing the error classification as warning, since that is how it will end up being displayed (with a note to check manually).

Still cannot figure out why the PHP fatal testing works differently locally and on the testbot. Locally passes. So let's try without that for this issue.

Ok this now shows that the actual problem is this:

1) Drupal\Tests\upgrade_status\Functional\UpgradeStatusAnalyseTest::testAnalyser
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Call to deprecated function menu_cache_clear_all(). Deprecated in Drupal 8.6.0, will be removed before Drupal 9.0.0. Use\n
-\Drupal::cache('menu')->invalidateAll() instead.'
+'Client error: `POST http://php-apache-jenkins-drupal8-contrib-patches-3352/subdirectory/admin/reports/upgrade/analyze/module/upgrade_status_test_error` resulted in a `403 Forbidden` response:\n
+\n
+\n
+  \n
+    \n
+

This did not show up in previous reports due to the PHP fatal testing masking it. So ehm. I don't know why testbot would not work the same way for doing HTTP requests. We are passing over the cookie after all to the same domain. The http://php-apache-jenkins-drupal8-contrib-patches-3352 domain is the same as in the results HTML files, so it is the right domain for this testbot job.

¯\_(ツ)_/¯

I asked for advice in drupal.org/slack's #testing channel. @mixologic suggested I try on my local testbot. Which I did. And it did fail the same way. Let's try with making the session info debug appear in our fail then. When I fiddled with these details, my local failed as well, so it is down to the session cookie not being passed over properly for some reason on testbots.

Also minor path typofix but that does not make it or break it.

Ok so this is strange. Based on the debug code I put in:

json_encode($cookie_jar->toArray())

produces [], so this snippet produces an empty array:

    $session_config = \Drupal::service('session_configuration');
    $request = \Drupal::request();
    $session_options = $session_config->getOptions($request);
    $cookie_jar = new CookieJar();
    $cookie = new SetCookie([
      'Name' => $session_options['name'],
      'Value' => $request->cookies->get($session_options['name']),
      'Domain' => $session_options['cookie_domain'],
      'Secure' => $session_options['cookie_secure'],
    ]);
    $cookie_jar->setCookie($cookie);

Let's check session options.

So that $session_options comes down to this on testbot (in json):

{"gc_probability":1,"gc_divisor":100,"gc_maxlifetime":200000,"cookie_lifetime":2000000,"cookie_domain":"","cookie_secure":false,"name":"SESSb3e03831c0a5fae43a08856e1e929898"}

This other than having an empty cookie domain matches the rough values locally. Let's check the actual cookie value.

Ok that got us "etsrx0yynahPYLbtfU7aLS5QU1AwL21h60gyy4b0dAA" so I blame it on the lack of cookie domain being set on testbot. Based on #38. See https://github.com/guzzle/guzzle/blob/master/src/Cookie/CookieJar.php#L166 for how CookieJar skips cookies with empty domains.

Let's try a cookie setting workaround.

With testbot worked around, we can restore the basic PHP fatal testing that was not actually causing the fail at all.

Finally remove our debug code.

Adjusting credits. #drumroll

Thanks all! I added #3073644: Add test coverage for uncatchable PHP fatal error testing for adding test coverage as it proved to be challenging in #16 as well. The existing test coverage does prove that there is no regression whatsoever. It does not prove that we improved. I think its fine adding that test coverage in #3073644: Add test coverage for uncatchable PHP fatal error testing and unleash the feature on people already as this was a recurring pain for some testers and is quite a jarring experience without a fix.