EnabledSourceHandler's query result caching should also consider the contents of composer.lock

Created on 18 February 2025, about 2 months ago

Problem/Motivation

This was found by @fjgarlin while manually testing ✨ Feature: Include option in drupalorg_jsonapi source to show only locally installed modules Active :

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.

✨ Feature request
Status

Active

Version

2.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @phenaproxima
  • Pipeline finished with Failed
    about 2 months ago
    Total: 390s
    #427654
  • πŸ‡ͺπŸ‡ΈSpain fjgarlin

    The code looks good. We'll need to test it once ✨ Feature: Include option in drupalorg_jsonapi source to show only locally installed modules Active is merged.

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

  • Pipeline finished with Failed
    about 2 months ago
    Total: 342s
    #427660
  • Pipeline finished with Failed
    about 2 months ago
    Total: 388s
    #427664
  • Pipeline finished with Failed
    about 2 months ago
    Total: 947s
    #427711
  • Pipeline finished with Failed
    about 2 months ago
    Total: 425s
    #427783
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 198s
    #427793
  • Pipeline finished with Failed
    about 2 months ago
    Total: 509s
    #427795
  • Pipeline finished with Failed
    about 2 months ago
    Total: 433s
    #427807
  • Pipeline finished with Failed
    about 1 month ago
    Total: 537s
    #430776
  • Pipeline finished with Failed
    about 1 month ago
    Total: 525s
    #430812
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    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.

  • Pipeline finished with Success
    about 1 month ago
    Total: 1132s
    #430827
  • πŸ‡ͺπŸ‡ΈSpain fjgarlin

    Setting to Needs work as there is a merge conflict.

  • Pipeline finished with Canceled
    about 1 month ago
    Total: 135s
    #433885
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 36s
    #433887
  • Pipeline finished with Failed
    about 1 month ago
    Total: 461s
    #433889
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 322s
    #433921
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • Pipeline finished with Success
    about 1 month ago
    Total: 743s
    #433925
  • πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

    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)

  • Pipeline finished with Skipped
    about 1 month ago
    #433977
  • First commit to issue fork.
  • πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

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

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

Production build 0.71.5 2024