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

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

I closed 📌 Use Composer's internal API to access ::getInstalledPackages() Closed: outdated

I should have postponed this issue on 🐛 The 'fake_site' fixture cannot be using with `composer show` because the packages are not installed Fixed but too late now because it is fixed. This is ready to be worked on again

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

core-mvp, also critical because 📌 Remove our runtime dependency on composer/composer: remove ComposerUtility Fixed is critical

tedbow → closed merge request !663

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 📌 Tighten ComposerPluginsValidator: support only specified version constraint Fixed .

I thing I am starting to realize is that with this new `InstalledPackagesList` using `composer show` we need to have real packages installed for it to work.

So all our tests that use our FixtureManipulators are not going to work because they don't actually install packages.

We could update them to use real composer operations. This though would really slow down our tests. I notice our kernel tests already when from 1:30 to over 2 minutes. My suspicion is that it is because all of the new Composer calls for ComposerInspector.

If we had even more calls to install packages and many `composer show` calls I think this is going very slow.

We could put a caching layer in for the InstalledPackagesList. Probably if the composer.lock file has not changes we could used a cached list.

Another option to speed up the tests/mock versions of the InstalledPackagesList objects in most of our tests. This way we would not need to make composer show calls every where. We could add extensive test for InstalledPackagesList itself to ensure that returns back the correct results but then in most of our tests just mock the results from InstalledPackagesList.

Unassigning adding 3) to proposed solution which is replace calls to certain methods in ComposerUtility with calls to InstalledPackagesList class.

These replacements will not mostly not need test change because it is just the source of the list that is changing

The error i am running into most times 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 .

1. re #15 yes this is because the package_manager.composer_inspector service has public: false which means it is only available via dependency injection so \Drupal\Tests\package_manager\Kernel\InstalledPackagesListTest::register for how to get around this in tests.

   But specifically where you were going us this in \Drupal\Tests\package_manager\Kernel\FakeSiteFixtureTest::testExpectedPackages I think this is test is outdated and we should have removed it in 🐛 The 'fake_site' fixture cannot be using with `composer show` because the packages are not installed Fixed because the purpose of this test was to make sure our installed.json and installed.php files were kept in sync because we just to make them manually now they we make them via the script \Drupal\automatic_updates\Development\ComposerFixtureCreator::createFixture with an actual composer install I think we need this test.

👍 working on it

We can continue working on this but I think it should land in 3.x not 2.x see 🌱 Tag 8.x-2.7, and move development to new 3.x branch to focus on Core inclusion Fixed for reasons

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

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 📌 ScaffoldFilePermissionsValidator should use ComposerInspector instead of ComposerUtility Fixed 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 being 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 is doing 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 because of this:

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.

Here's one way to fix it.

Remove this entire piece:

      // The installation paths in $data will have been interpreted by the PHP
      // runtime, so make them all relative again by stripping $this->dir out.
      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']);
        }
      });

Then, replace this line:

$data = str_replace("'install_path' => '../", "'install_path' => __DIR__ . '/../", $data);

...with this line:

$data = str_replace("'$composer_folder", "__DIR__ . '", $data);

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 monstrousness.) 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 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 📌 ComposerPatchesValidator should use ComposerInspector instead of ComposerUtility Fixed .

see MR comments

Looks good!

Onward and upward!

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

This did fail to update the documentation and it did not provide a change record. I'll take care of the documentation in 📌 Define the Package Manager API (package_manager.api.php is outdated) Fixed . 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!

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