Handle Package Manager errors and warnings more elegantly

Fix in place.
Does this need a test?
Also - do we want to print warnings still? At the moment this is suppressing them.

We want to print warnings

Yes there should be a test to go along with this, if possible. Not sure how to get it to fire a warning, though.

Refactored the way we return messages from package manager to return errors and non-errors (warnings). If there are any errors, prevent installs and show the heading "Unable to download modules via the UI". If there are any warnings, we print them without a header.

I've added some screenshots of the various scenarios we could get if there are multiple messages. Chris informed me that we're unlikely to get both because of the way Package Manager works, regardless, the styles are in there just in case this happens.

Will have to work on tests later - any help in that department is appreciated!

Tested this manually in DrupalPod and is working well. Allows me to install even if XDebug is enabled, for example, which is perfect. (And great DX.)

Kicking back to NW because it needs tests, but it's working well. If anyone knows of a way to write a test for this to (a) cause an error from PM and (b) cause a _warning_ from PM, help appreciated.

I will go through and do a code review also, just to get that started.

Also left a review in the MR. Nothing big, but worth addressing or discussing.

We may want to wait on https://www.drupal.org/project/project_browser/issues/3376202 📌 Error messages should utilize Drupal's messages Needs work to come in.

pfrilling → changed the visibility of the branch 3365180-pm-warnings to hidden.

pfrilling → changed the visibility of the branch 3365180-pm-warnings to active.

Attached is a patch that _should_ add test coverage for both warnings and errors coming from package manager. I also attempted to merge the changes from the MR into this patch. I admin, I'm no Svelte expert, so there is a good chance I didn't do the JS stuff correctly.

Moved Phil's patch into the MR and fixed PHPCS errors

Test failures appear to be aligned with current failures against 1.0.x

Plan to refresh/rebase and review as this touches the code i'm working on here https://www.drupal.org/project/project_browser/issues/3451863 📌 Too slow to validatePackageManager() on every page Needs review

I'm still in progress cleaning up tests on this please message @sime in slack if you want to contribute or take over.

OK. So this was meant to be just a rebase, and it still is a rebase because I didn't really change anything from the work 6 months ago.

Code logic

I haven't really touched the code logic, but i did have to update the message handling in the svelte, since we are using the Drupal.messages() since the last work on this.

I'm hoping to get things back to where the code reviews were at, as I do think there is ab it more work here, but I think it needs a review as well.

Tests

1. There were event listeners which triggered errors when calling ->validatePackageManager(). This allows the code to spend a lot of time in the package_manager code, so i mocked InstallReadiness instead which means we are focussed on testing PB login instead of PM logic.
2. I was able to remove a todo from PackageManagerFixtureUtilityTrait->initPackageManager() removing a fair bit on module installation and move those modules back to the tests's $modules property
3. Note that this currently includes 📌 phpstan.neon.dist fixes - Get phpstan to level 1 RTBC just to help me wade through the errors.

I still have this assigned to myself just because if someone wants to pick it up it would be great to talk through the approach.

Some thoughts on next steps

1. Review the 6 month old unresolved comments
2. I think we should change the wording "messages" to "warnings" the same way that "errors" => "errors"
3. I think I have a plan for removing the need to isntall package manager, thus hopefully further reducing flakiness

sime → changed the visibility of the branch 3365180-pm-warnings to hidden.

Rebased and created new PR on 2.0.x branch.

I mostly resolved @fjgarlin's comments from 1.x MR

Non working state, removing from needs review.

Updated to 2.0.x (new branch). Old branch kept.

Just re-summarising this progress. The previous progress is still here against 1.x.

Testing strategy

Compared to the 1.x progress, I changed the way errors were mocked so that test logic did not need to go deep into Package Manager. So the updated tests are testing scenarios InstallReadiness generates errors, rather than testing how PM generates errors.

The reason i changed the tests was because it was very flaky (intermittent time outs and failures) and slow for me (even without xdebug). I think this remains a bigger problem with a lot of PB tests seem to do full integration not always for obvious reasons. In these case we are flexing the PM code just to see how ProjectBrowser.svelte handles these messages.

Long story short, I can revert how the tests are architected if asked. it was a journey for me to get them to work but i'm not wedded to my approach.

Message strategy

Instead of just setting status messages in PHP, we are turning them into dumb strings and passing them to the svelte app, which then uses the javascript equivalent of setting the messages. We could just set them in PHP. Basically the current 2.x code is still doing the same thing as a the 1.x coee, I just wanted to point that out.

Couple other points:

1. I did change the svelte app so that it clears and current status messages just to avoid the screen getting filled with messages.
2. I aligned variable names so that everything from package_manager is prefixed as such.

Installing test dependencies

A big thing in this MR is that it seems like we no longer need to install package_manager in a weird way via initPackageManager() and we can just use the $modules property. That seems like a good bit of technical debt removed.

@phenaproxima has tightened the scope and it is looking good to me. I like the improvements in variable handling.

I have created a few extra tickets out of the things I learnt in the course of working on it.

📌 Resolve technical debt in initPackageManager() Active
📌 ServiceProviderBase::alter shouldn't call parent::register Active
📌 Identify and action solutions for flaky tests Active

I'm just going to combined these up a little. It would be rare to have multiple errors here, I believe, so we'll just prefix each one and skip the extra ones?

Changes I made result in this:

The worst case scenario is two messages of the same type, thus showing the same prefix, but I think this is acceptable as it's very rare and looks fine.

For anyone who wants to manually test this, a simple way is to install the project_browser_test module.

1. You need this in your settings.php, note that it will start showing test modules/themes in the UI.

$settings['extension_discovery_scan_tests'] = TRUE;

2. Then install project_browser_test, I just use drush.

3. Now in TestInstallerReadiness.php in the statusCheck method, just change the code to manual create whatever messages you want to create.

Ya know, I think this is RTBC. Feels solid to me. Will let Chris have the final say.

With manual testing, prior to applying this MR - all messages show (two status messages and the error). When applied, only the error shows (now correctly as a warning, though). What happened to the other messages?

Oh man just being able to have Xdebug on is such a win. ;)

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