When phpstan is not found, the error is too subtle, does not support custom binary path

Sending this for a test. I don't honestly know how to find the site's composer.json file. Is it where the global bin dir would then be defined?

I think #3126174: Warning: Invalid argument supplied for foreach() was related which I think was likely also due to a missing phpstan. We don't do a very good job of letting people know if phpstan was not found.

Making title more explicit. Fixing the too subtle error would be really useful for everyone.

@fgm also suggested hardening right where we get the json results from phpstan, which would also be invalid if the file is not there or we cannot run it or there was some other error with it. See interdiff in #3129903-26: Remove git deploy as a requirement from upgrade status. I did not commit those there because it was unrelated. Incorporating that here would be great.

Re how do we read composer information, following #3129903: Remove git deploy as a requirement from upgrade status we don't rely on git_deploy anymore and suggest either composer_deploy or git_deploy for more accurate categorisation but have some basic heuristics ourselves. It is probably worth a look at how composer_deploy finds the composer information because going along those lines would probably be the most proven so far :)

I just ran into this same issue.

We could use weblo/drupal-finder to locate the root composer.json file. The project is already required by mglaman/phpstan-drupal which this project depends on so we could make it an explicit dependency and then do something like:

$finder = new DrupalFinder\DrupalFinder();
$finder->locateRoot(DRUPAL_ROOT);
$composerRootFile = $finder->getComposerRoot() . '/' . $finder->getComposerFileName();

Then read the contents of the file and figure out what the bin directory setting is.

Here's a patch that uses webflo/drupal-finder.

I wasn't sure what additional code you wanted added in #8. It looks like the referenced code in the interdiff was already added. But, if you can provide a little more direction as to what you're looking for I can try and update this patch as needed.

Oops, that's not going to work. I removed the findVendorPath() function, but then didn't remove uses of it. This patch should actually work. :)

Re #8 I did notice I commit it already. That said it still just more silently swallows the error if phpstan did not return a JSON. If it was not executable (permission issues or got removed or bad bin path or whatnot) then it will just behave like there was no error and everyone is happy :) we should log an error into the project's error list (not to a logger channel IMHO because if you got green results you will not think to go check your Drupal logs for errors).

This is starting to look great, thanks!

+++ b/src/DeprecationAnalyzer.php
@@ -150,22 +169,41 @@ final class DeprecationAnalyzer {
+    if (is_null($json)) {
+      throw new \Exception('Unable to decode ' . $rootComposer);
     }
...
+    // Bail here as continuing makes no sense.
+    throw new \Exception('Binary path not found.');

I notice we don't catch these and make it visible to users that it did not work. So it will fail in some ugly way for users on the UI. I did not resolve this yet, as we need to figure out how to resolve this.

The second exception thrown does not explain what's binary path is not found either. I added some code for that and also to list what we tried in the message which should help a lot in debugging.

I also added logging code from @DamienMcKenna from #3132908: Do not fail entire scan if drupal.org cannot be reached to log errors when phpstan did not return a valid result (which may also be because there was no permission to run it or it was moved from there or some other massive fail).

Setting needs review for testbot sake but actually needs work for exposing the problem to the user rather than failing out of an exception entirely.

Ok I took a stab at surfacing the issues to the user. So I separated the environment specific parts of the constructor to its own method, which can now freely throw exceptions right and left :D Made the existing error conditions for temporary directories, etc. exceptions as well for consistency. Then made analyze() call that but return silently if there was a problem, since we now assume the integration code invokes it first and reports any environment errors first before any analyzation would take place. Then added a bit of a dsm() to the form, so the error appears to the user and disabled the form since the error needs fixing first. I also looked into disabling the checkboxes but #2895352: Allow tableselect element options to be disabled is still outstanding for that and I did not want to do all that workaround to disable them.

Here is how it look like now, which I think is good :)

Tests also like it, so let's get this in and improve further later as needed. Thanks all for working on this! It will make life much easier for people setting up the module. It will also catch mistakes like not installing the module with composer and thus not having phpstan installed at all. Running phpstan will not be attempted in this case and the failure will be surfaced to the user. (I already did a quick followup commit here to expand on the error message in the screenshot and add a Did you install Upgrade Status with composer? hint to the error message displayed.