Contrib.social

For performance, use installed.php to read data about installed packages

Open on Drupal.org →
Created on 21 March 2023, over 1 year ago
Updated 15 May 2023, over 1 year ago

Problem/Motivation

In ComposerInspector::getInstalledPackagesList(), we currently use multiple calls to composer show, as well as directly reading and parsing composer.lock, to compile the information we need about installed packages.

This is generally fine, but it's not super comfy to parse composer.lock, since that's an internal Composer file, just to get the package type. We asked Composer to add a type field to the output of composer show, but the maintainers are hesitant to do that:

right now it's serving basic info and that's it, and if we start adding one thing it's then the question why not more until we have everything in there.

After some more discussion, the conclusion was that it should be reasonably safe to rely on parsing files like installed.php directly. The advantage of doing that in ComposerInspector is performance: we would not need to call composer show at all, nor would we need to parse the lock file. The disadvantage is that installed.php is not documented API.

Or is it? Jordi says that he's very unlikely to change installed.php, so it's sort of an API by gentleman's agreement.

So I'm opening this issue for us to consider maybe using installed.php instead of composer show internally. The case for this would be bolstered if Composer agreed to document that installed.php is a reliable source of package data.

To be clear: no urgent action is needed from us. What we have right now works just fine, and our reliance on the lock file to get the package types is in keeping with the "gentleman's agreement that this is basically API" that Jordi seemed to hint at. We statically cache installed package lists, so even if we have to call composer show three times and do a bunch of parsing, the performance impact is mitigated.

And to clarify another point: I am NOT suggesting that our tests should go back to the dark days of directly manipulating installed.php, installed.json, or other internal Composer files. That way lies madness. I'm strictly suggesting that we consider using installed.php when reading back complete information about packages. It is entirely Composer's responsibility to output that data properly to begin with.

✨ Feature request
Status

Postponed: needs info

Version

3.0

Component

Package Manager

Created by

🇺🇸United States phenaproxima Massachusetts

Live updates comments and jobs are added and updated live.
  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

  • needs profiling

    It may affect performance, and thus requires in-depth technical reviews and profiling.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Comments & Activities

  • Issue created by @phenaproxima
  • Comment over 1 year ago →
    🇺🇸United States phenaproxima Massachusetts

    Downgrading priority.

  • Comment over 1 year ago →
    🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    "very unlikely" is not a promise.

    I very much doubt that @xjm and other release managers will be comfortable parsing internal data structures of Composer.

    How much of a performance difference are we talking about? As an alternative to profiling, we could implement it now and observe how much faster tests become.

  • Comment over 1 year ago →
    🇺🇸United States tedbow Ithaca, NY, USA

    Regarding it not being a promise one idea I had was that if we want to do this issue we should first file a issue with Composer to add `@API` to the auto-generated files. If they are really API that shouldn't be a problem

  • Comment over 1 year ago →
    🇺🇸United States tedbow Ithaca, NY, USA

    No proposed solution

  • Comment over 1 year ago →
    🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    #4++

    A comment in an issue somewhere is probably not enough for Drupal core release managers to accept this 😅

  • Status changed to Postponed: needs info over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States tedbow Ithaca, NY, USA

    If we really want to do this we would need a better understanding of whether this file is concerned API? Also how many time request would actually need this information? It seems it would be more of a test performance improvement because this is where we would need to do this very often. I think compared to file copying this probably takes very little time

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024