Add new InstalledPackagesList which does not rely on Composer API to get package info

Actually, it looks like this is a duplicate of #3307365: Use Composer's internal API to access ::getInstalledPackages()…

@tedbow: I think this is core-mvp because it blocks #3316368: Remove our runtime dependency on composer/composer: remove ComposerUtility? Please confirm 😊

Regarding

Right now, we have a method in ComposerUtility called getInstalledPackages(), which uses Composer's API to return a keyed array of PackageInterface objects.

As surfaced at https://drupal.slack.com/archives/C7QJNEY3E/p1675885515350319?thread_ts=..., the current code:

  public function getInstalledPackages(): array {
    $installed_packages = $this->getComposer()
      ->getRepositoryManager()
      ->getLocalRepository()
      ->getPackages();

… is wrong. It doesn't do what it documents nor what this issue claims.

That is not looking at installed packages. That is looking at packages available in local (aka path) repositories…

So any FixtureManipulator::addPackage() calls that specified a version will not be returned. Because those packages do not exist in a local repository. Unless install_path is specified, in which case the fixture manipulator generates a local repository — meaning this AFAICT only works by accident?! 😳 I feel like I'm missing something still though…

In any case, this is what I just spent a few hours pulling my hair out over. This is now for sure a hard blocker for #3340022: Tighten ComposerPluginsValidator: support only specified version constraint.

The error i am running into is
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException : The "package_manager.composer_inspector" service or alias has been removed or inlined when the container was compiled. You should either make it public, or stop using the container directly and use dependency injection instead.

i saw that we have \Drupal\package_manager_bypass\TestComposerInspector but that doesn't help either. So i'm blocked .

👍 working on it

Back to you, @omkar.podey!

As far as I know, what's still needed here is to replace all calls to ComposerUtility::getInstalledPackages() and ComposerUtility::getInstalledPackagesData() with calls to ComposerInspector::getInstalledPackagesList().

Feel free to use any PHP 8.1 features you've been itching to try ;)

The two places outside of automatic_updates_extensions that i didn't update

1. src/Validator/ScaffoldFilePermissionsValidator.php because // We don't have a getExtra() function yet .
2. package_manager/src/Validator/ComposerPatchesValidator.php needs extra logic to switch between active and stage.

I think we need getProjectForPackage and something similar to getPrettyVersion in package_manager/src/InstalledPackagesList.php , the commit i just pushed above should fail as we don't have the functions yet.

src/Validator/ScaffoldFilePermissionsValidator.php because // We don't have a getExtra() function yet .

You don't need it :) You can call $this->composerInspector->getConfig('extra') and parse the output as JSON.

I think we need getProjectForPackage

Right you are. I've added InstalledPackage::getProjectName() for this purpose, and InstalledPackagesList::getPackageByProjectName() for the inverse. (Note that the latter now returns an InstalledPackage or null, but never a string.)

Whoa. Didn't mean to turn this completely green, but hey...just goes to show that sometimes a few small changes have outsized effects.

I guess this is reviewable now...?

So #3342227: ScaffoldFilePermissionsValidator should use ComposerInspector instead of ComposerUtility is what we decided to go with i have pushed the change there which needs @travis.carden's input.

Epic progress here! 🤩 Left a range of suggestions & questions.

Interesting - I think the addition of strict_types = 1 has revealed an existing problem here!

xdebug tells me that, in certain cases, the realpath being computed for a (fake) installed package is wildly wrong. Example: in CronUpdaterTest::testUpdaterCalled(), at some point InstalledPackage tries to get a realpath for /private/tmp/package_manager_testing_roottest46267339/stage/gypHULuVrCAuBshO10VGa-1tjDkH1ZMWgWDzSQQzWYk/vendor/composer/../drupal/core-dev...but /private/tmp/package_manager_testing_roottest46267339/stage doesn't even exist yet.

I'm happy for anyone else to try and fix this, because I believe it's a pre-existing condition that our code simply wasn't strict enough to find until now.

Found the problem: FixtureManipulator::setPackage() is faulty. It's towards the end, when it rewrites installed.php.

Here's the thing: when Composer generates installed.php, it uses the __DIR__ constant quite liberally because it knows that the package paths are always relative to some vendor directory. But that's not what FixtureManipulator is doing. It's just stripping off the full path of the vendor directory, effectively converting the install path to an absolute path.

But that's no good, because a) it's not realistic, and b) if that rewritten installed.php is moved to some other place, the paths break because they may no longer be accurate. InstalledPackage.php's strict typing is detecting this because realpath() is producing a FALSE, rather than a string.

I think the right approach here is for FixtureManipulator to do what Composer would do and write __DIR__ into installed.php, keeping the paths relative. That's previously what I programmed it to do, and indeed that code is still there, at the end of setPackage():

    $data = var_export($data, TRUE);
    $data = str_replace("'install_path' => '../", "'install_path' => __DIR__ . '/../", $data);
    file_put_contents($file, "<?php\nreturn $data;");

But that's not having an effect anymore. I presume that's because of this, but the code is hard to understand, so I'm not sure. This feels really haphazard.

array_walk($data['versions'], function (array &$install_php_package) use ($composer_folder) : void {
        if (array_key_exists('install_path', $install_php_package)) {
          $install_php_package['install_path'] = str_replace("$composer_folder/", '', $install_php_package['install_path']);
        }
      });

We should open another issue to fix that, since it may break some tests (which, it should be clear, are probably having erroneous expectations now). That issue will have to block this one, I'm afraid.

EDIT: This is gonna take some more involved work to get it to a sane place.

This comment from Wim scared the hell out of me:

😱
Why?! package_manager_bypass installing this means we're still doing something else in tests than in reality?!

So I did what I always do: I got into a Zoom with Ted, who is basically my code therapist at this point, to talk about it.

What we concluded is that the scope of this issue is starting to balloon. We are constantly having to adjust fixtures, and the way we manipulate fixtures, as we change our validators to use this new API. I think the reason that is such a painful and omnipresent task is because, due to ComposerUtility and the way it works, we are tightly coupled to Composer's internals. (This has happened over time; it started as a good idea, but has mutated into something much more complicated and sinister.) This is the reason we're having to add things like TestComposerInspector -- because if we tried to always directly use composer show to read fixtures, we might suffer from performance issues, and we'd need the fixtures to always be 100% complete and correct. (Although, to be clear, they are significantly better than they used to be -- but as #32 demonstrates, there are still problems.)

The addition of InstalledPackage and InstalledPackagesList is really meant to help us move away, in many cases, from fixture manipulation, rather than further entrench the need for it. For example, if a validator's test coverage needs a list of installed packages, it should be possible for the test code to simply define an array of installed packages, rather than have to prepare a fixture in the exact right way. What's not totally clear to Ted and I is whether it's "better" to rely on fixture manipulation, or adopt the use of light mocking (i.e., by directly creating InstalledPackageList objects in test code, and telling ComposerInspector to return them). We need to discuss that with Wim.

But what is clear is that such a decision is only going to come into play when we actually start adopting InstalledPackageList in our validators. That, we think, should happen in separate issues, so that the decision can be made case-by-case whether to test any given validator (or, really, any given code path) by manipulating a fixture, or by mocking an InstalledPackageList.

But, either way, we need to add these new classes, with solid unit-level test coverage. So I've opened another MR to do just that. It only adds the stuff we need, and doesn't bother trying to convert any validators to use it. I think we can comfortably commit that -- as far as I know, all of Wim's feedback is addressed -- and then adopt the new system piecemeal in follow-ups.

This now also blocks #3337760: ComposerPatchesValidator should use ComposerInspector instead of ComposerUtility.

Onward and upward!

🥳 This unblocked #3340022: Tighten ComposerPluginsValidator: support only specified version constraint and #3342227: ScaffoldFilePermissionsValidator should use ComposerInspector instead of ComposerUtility!

This did fail to update the documentation and it did not provide a change record. I'll take care of the documentation in #3318306: Define the Package Manager API (package_manager.api.php is outdated). Please ensure that every infrastructure change comes with explicit CRs + docs from now on.

Will do. Sorry about that. I feel appropriately chastised. 🥺

Here's the change record: https://www.drupal.org/node/3343718

Will do. Sorry about that. I feel appropriately chastised. 🥺

Not the intent!

The intent is just to stop making the docs outdated, and then taking weeks/months to get them up-to-date. Now that we're close to core, we need to start treating the docs equally important as the code 😊

We just collectively need to change our habits!