Problem/Motivation

This was found by @fjgarlin while manually testing #3498835: Feature: add plugin or option to show only locally installed/available modules:

So, if I do composer require drupal/webform drupal/address, the only way to see these modules if after clearing the storage (/admin/config/development/project_browser/actions).

That's a great point. EnabledSourceHandler aggressively caches query results (and it should), but if you require new modules into the site, the cache won't get updated. This wasn't previously a problem, but the fact that a query can now be based entirely on the state of the local code base means this needs to be accounted for.

Proposed resolution

When EnabledSourceHandler is caching queries, it should generate a cache key based on the hash of the query and a hash (xxHash is considered very fast and appropriate for this use case) of the project-level composer.lock file.

Issue fork project_browser-3507468

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

phenaproxima created an issue. See original summary.

phenaproxima opened merge request !735

fjgarlin’s picture

fjgarlin commented

The code looks good. We'll need to test it once #3498835: Feature: add plugin or option to show only locally installed/available modules is merged.

Jobs are failing in the CI, but I'm not sure if it's related to this.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review

I ended up having to de-flakify some tests because for some reason, the ordering of projects changes under certain circumstances. Why, I can't say, but I definitely confirmed two things:

  • The tests I changed do not actually care about order; they were just relying on order-sensitive selectors because they didn't know any better.
  • The EnabledSourceHandler is storing the same exact set of results, before and after this change, and returning them to the frontend. Svelte's rendering them in a different order, for some reason.

Given that, I decided that the correct course of action here is to improve the tests and make them more accurate, rather than trying to find out exactly why the ordering is different. If the ordering mattered to these particular tests, then the tests should, like, test that.

fjgarlin’s picture

fjgarlin commented
Status: Needs review » Needs work

Setting to Needs work as there is a merge conflict.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Reviewed this, and even went down a rabbit hole of the safety of hash_file(), but we're fine because of the file_exists() check (and because the only other error condition is if the file being hashed is over 2GB, and if your composer.lock is over 2GB, you are beyond help)

chrisfromredfin made their first commit to this issue’s fork.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Reviewed & tested by the community » Fixed

feels good to me; I reviewed & so did Tim.

Status: Fixed » Closed (fixed)

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