Invoke Package Manager's status check less frequently to improve performance

The basic code is there, and a review would be welcome.

Cache when: If the "package manager validation" check is successful, the code caches this for 30 minutes. This allows the site builder to roam freely through interface, thinking about how fast Drupal is, and not experiencing user frustration.

Don't cache when: If there is a problem with the package manager, we can assume the User *wants* to use it because they enabled it. So the assumption is that the user will be trying to fix their environment and we shouldn't cache or change anything.

Seeking feedback on a couple of things:

1. In which situations should we invalidate that cache?
   1. Module is installed
   2. Module fails to install
   3. Project browser ettings form is saved
2. What tests? I was thinknig a kernel test where i just mock the Automated Updates status check and test the scenarios.

Notes. I feel like we can call the same method that automatic_updates uses (eg during cron run) which includes its own caching. I need to test it.
https://git.drupalcode.org/project/automatic_updates/-/blob/3.0.x/src/Va...

I'm generally in agreement here that it makes sense to cache the status check results for about 30 minutes, and certainly we'd want to clear them when a Project Browser install stage has been applied (PostApplyEvent).

Before i forget, Chris wanted to be sure that this code was aligned with how AU/PM handles caching of this same data. That was going to be something I looked at after 📌 Do not prevent UI install for PM warnings Needs review .

Before i forget, Chris wanted to be sure that this code was aligned with how AU/PM handles caching of this same data.

Package Manager doesn't do any caching; it only provides the infrastructure for doing status checks (StatusCheckTrait).

Automatic Updates has a StatusChecker service which, similar to InstallReadiness, wraps around StatusCheckTrait and caches the results in non-volatile key-value storage.

I propose that InstallReadiness do something like what StatusChecker does, but we should use a volatile cache bin instead of non-volatile storage. I think it's okay if status check results get wiped out on an unpredictable basis. The reason that AU uses non-volatile storage is because it's supposed to run in the background and needs to periodically check if it's still in a usable state. Project Browser, on the other hand, has no background presence; it's always something someone is actively using. So, volatile cache is okay for that.

Thanks for the guidance!

Tagging this for further profiling before commit, so we can be sure we're actually having a positive effect on performance by adding this cache layer.

Also, I need clarification on something: the issue summary says we're running the status check on every page, but I see no evidence of that? It's only run, as far as I can tell, by BrowserController, which is only invoked when you...load the project browser?

Is it possible there is something else causing the performance issues? Not saying it's necessarily bad to cache the results, because they certainly are expensive to compute, but I'm wondering how serious of a problem this actually is, and whether we're actually going to realize big performance gains across multiple routes by doing this. By no means is Project Browser unusable, even with the expensive status checks.

Tagging for an issue summary update; the answers I'm asking for in #17 should be reproduced there.

> the issue summary says we're running the status check on every page

I found that it was running when I was looking at project detail pages, which is most of the routes you can explore - did you not find this?

Btw. my main goal so far was to demonstrate for myself (and prove to others) that the browsing expereince was significantly improved with caching. So I'm not really wedded to *how* the caching should work and I was mainly waiting to see how the message improvemnet issue landed because it was calling the validation check.

> Is it possible there is something else causing the performance issues? ... Project Browser is by no means unusable, even with the slow status checks.

I agree it's within the bounds of being ok in "normal" circumstances. This is off course true, since no one thought it was a problem... One thing that slows things down is slow file I/O - there seems to be a lot of checks of the file system. What do you think that will be like with WebAssembly?

sime → changed the visibility of the branch 3451863-cache-validate-pm to hidden.

Still needs a test to test clearing the cache on PreApplyEvent

I think this is looking good but there are a couple of sketchy things that need cleaning up. Otherwise, though, this looks like it does what's advertised.

Btw. my main goal so far was to demonstrate for myself (and prove to others) that the browsing expereince was significantly improved with caching. So I'm not really wedded to *how* the caching should work and I was mainly waiting to see how the message improvemnet issue landed because it was calling the validation check.

The more I look at this MR, and the more I examine the test you wrote (thank you!), the less sure I am that we're taking the proper approach here.

For example -- if you previously had no errors, but then a condition changed somewhere and now you have a warning...under the proposed solution (as written in the current MR), you won't know it for at least 30 minutes. That's not going to fly. Or, what if you previously had no errors, but then someone else makes a change that does cause an error? You won't know, because the caching will be covering it up. You'll just hit a nasty surprise when you try to do something with Package Manager.

I don't know if we actually should be caching, exactly. What we should probably be doing is just not running the status checks every time BrowserController is invoked. That's the bug here, which I discovered while working on ✨ Use a route enhancer to move away from having project ID hashes in URLs RTBC . That browse route is used in an extremely awkward way by the frontend.

Here's an idea: rather than running the status checks EVERY time the browse route is hit, why don't we only run it when we're not looking at a particular project? In other words:

    if (array_key_exists('drupalorg_mockapi', $current_sources) && empty($id)) {
      $this->messenger()
        ->addStatus($this->t('Project Browser is currently a prototype, and the projects listed may not be up to date with Drupal.org. For the most updated list of projects, visit <a href=":url">:url</a>', [':url' => 'https://www.drupal.org/project/project_module']))
        ->addStatus($this->t('Your feedback and input are welcome at <a href=":url">:url</a>', [':url' => 'https://www.drupal.org/project/issues/project_browser']));

   // DO THE STATUS CHECKS HERE
    }

This will cause the checks to run a lot less, which should improve performance significantly, even without any caching.

Or, another approach -- what if we created a new endpoint that did the status checks, and the frontend could call out to it whenever it felt like that was necessary (i.e., just before clicking the "Install" button)?

I've been discussing with Adam and I was always a little bit leery of caching the checks, even though I agree that the 90%+ use case nothing would change regardless of what cache lifetime we chose.

I think the right "1.0" approach to this issue is to simply stop calling these checks as frequently. Right now, it seems we are calling the status checks for both the main PB route *and the project detail page route* - and that second one is likely unnecessary and was probably only an accidental byproduct anyway. I believe the intent was to run the status checks only when the "main" UI initialized.

The reality is there that something could change in your setup between browsing and trying an install.

So I'm (a) scoping this issue to simply invoking the status checks less often, but then (b) opening a follow-on / child issue where we will try to come up with an *even better* solution, which may do something like background the process by exposing it as an endpoint that the frontend can invoke. We could even potentially cache the results still, behind that endpoint.

This is good incremental improvement, more great stuff to come in the follow-up.

Great progress! I feel very supported on this itch :)

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