Update FixtureManipulator to work with InstalledPackagesList, real composer show command

Rough but I think it proves it is possible

This hardblocks 📌 Tighten ComposerPluginsValidator: support only specified version constraint Fixed . See #3340022-10: [PP-1] Tighten ComposerPluginsValidator: support only specified version constraint → .

This is a critical follow-up to 📌 Add new InstalledPackagesList which does not rely on Composer API to get package info Fixed .

Reviewed. tl;dr: IMHO we should use composer validate --check-lock --with-dependencies --no-interaction --ansi --no-cache --working-dir=PROJECT_ROOT in both the fixture creator and when committing fixture manipulations.

@Wim Leers thanks for the review

This issue has only updated 1 validator to use `InstalledPackageList`

I think we may be able to bring in the changes to validators we made in this merge request we made in 📌 Add new InstalledPackagesList which does not rely on Composer API to get package info Fixed in before reduced the scope of that issue to not update the validators. In that merge request we still had \Drupal\package_manager\ComposerInspector::getInstalledPackagesList and I think it was pretty much the same API we shouldn't have to do all that work over again.

I may look into do that if I address the other feedback I might unassign myself. But I think using the other MR should save alot of time because those changes already got some review

I tried to get the changes from the merge request in #7 but it was not so easy. That was on 2.x and other changes have happened since this. I think we should get this current issue in with 1 validator swapped to prove it works and then do other validators in other isssues

removing "attempt" since it seems we can do this

It's assigned to you, @tedbow, so you're working on this? If you want Kunal, Yash or Omkar to work on this tomorrow, please unassign 😊

Unassigning myself.

relating the 2 related issues I created. 1 is fixed and 📌 Run `composer validate` after FixtureManipulator commits its changes Fixed can be worked on separately

Uploaded patch without the changes to the validator to test performance changes of just the manipulator changes.

Merged in latest 3.0.x because several commits happened, let's see if this still passes tests.

📌 Run `composer validate` after FixtureManipulator commits its changes Fixed is RTBC. I was going to propose to wait with this one until that one is done, but … there is no conflict at all! 👍

#12: no observable performance impact! 👍

Drupal\Tests\package_manager\Kernel\TestStageValidationException: PHP temp directory (/tmp) does not exist or is not writable to Composer. Set sys_temp_dir in your php.ini

Re-testing…

This hard-blocks #3342227-17: [PP-1] ScaffoldFilePermissionsValidator should use ComposerInspector instead of ComposerUtility → and #3337760-25: [PP-1] ComposerPatchesValidator should use ComposerInspector instead of ComposerUtility → , as well as in principle all of the other remaining work falling under 📌 Remove our runtime dependency on composer/composer: remove ComposerUtility Fixed .

@Wim Leers if you want to take on working the new MR when you start your day please do. I am not sure I left it in good enough place where it would be obvious what the next steps are. If it is all nonsense I can take it back when I start tomorrow.

Sorry the code is a bit sloppy(altered drupalci.yml). I wanted to work quickly to see if this approach was feasible at rather than spend days before we figured out it wasn't

But we are down to a few tests failures so I think this approach will work and is better.

Thoughts on some of the remain failures.

1. FixtureManipulatorTest I expected this to fail because we are updating the FixtureManipulator. I fixed a couple but have not look at the others
2. OverwriteExistingPackagesValidatorTest which has edge cases where we set specific package install paths. Hard to do now but I have left a couple ideas on how to test this https://git.drupalcode.org/project/automatic_updates/-/merge_requests/74.... but also open for better ideas.
3. ComposerUtilityTest I think would a similar problem to FixtureManipulatorTest because we were relying setting install_path here also

So probably could be fixed the same way

4. ComposerPatchesValidatorTest I fixed a bunch of test cases but haven't looked at the last one

Create follow-up 📌 Optimize Composer calls in FixtureManipulator Closed: works as designed . We will need a todo in the code

Pushed this forward. AFAICT the failures in both FixtureManipulatorTest and ComposerUtilityTest are caused by the tests checking the install_path functionality, because that's what ComposerUtility uses.

So we're in a chicken-egg situation where we would not have to fix these if install_path were just not used anymore (ComposerInspector does not!).

So I think the easiest solution is to remove package_manager/tests/fixtures/FixtureUtilityTraitTest/existing_correct_fixture and update the test to use fake_site instead?

As discussed, @tedbow is tackling FixtureManipulatorTest right now 😊

Weird, locally ComposerPatchesValidatorTest passes

Testing started at 2:56 PM ...
PHPUnit 9.5.28 by Sebastian Bergmann and contributors.

Testing /Users/wim.leers/core/modules/contrib/automatic_updates/package_manager/tests/src/Kernel
..............                                                    14 / 14 (100%)

Time: 00:31.460, Memory: 10.00 MB

OK (14 tests, 68 assertions)
Process finished with exit code 0

… but not on DrupalCI 😬

I think I found the solution for ComposerPatchesValidatorTest while not strengthening rather than weakening test coverage! 🤞 Calling it a day though, so I really hope I'm right 😅 Otherwise, feel free to revert my commits since bc89d35a, which did make it pass but still has weakened test coverage.

Assigning to @Wim Leers if you want to take a look.

Of the 3 failing testing in https://www.drupal.org/pift-ci-job/2606788 → . (I did not see later results yet)

1. DeleteExistingUpdateTest now passing for me locally. I expect it to pass. It was a problem that I had put an `composer update` call in the fixture we didn't need. This test hit it because it staged an update cancelled and retried.
2. FixtureManipulatorTest : I haven't had chance to look at these.
3. ComposerUtilityTest : I commented here https://git.drupalcode.org/project/automatic_updates/-/merge_requests/74... and started that idea. But I am done for the day.
   If you want to try, you are welcome to otherwise I will give it a try tomorrow.

UPDATE: see results from https://www.drupal.org/pift-ci-job/2607522 → that InstalledPackagesListTest is now failing 🤦🏼
Here is commit with just a comment about this https://git.drupalcode.org/project/automatic_updates/-/merge_requests/74... different tests failed on this 1 line.

👋

The first test run with FixtureManipulatorTest green is a fact! 🚀🚀🚀

Locally, all tests failing on DrupalCI now pass except for: ComposerUtilityTest and ComposerPatchesValidatorTest.

Leaving those to @tedbow.

PRETTY PLEASE HAVE THIS COMMITTED by the time I wake up 🥺🥺🥺🥺

I was wrong in #24 — while I was writing that comment I made ComposerPatchesValidatorTest pass too 👍 😊

Did a quick review of the MR, but definitely don't wait for my +1 to land this monster!

re #24 @Wim Leers

Leaving those to @tedbow.

PRETTY PLEASE HAVE THIS COMMITTED by the time I wake up 🥺🥺🥺🥺

Assuming this passes tests I am going to commit this. But since I am the one also who do the last work
@Wim Leers
PRETTY PLEASE HAVE REVIEW THE CHANGES SINCE YOUR LAST COMMIT 😜😜😜😜😜😜😜

Beside getting the last tests, passing it has been a lot of comment with todos. the major new part is in OverwriteExistingPackagesValidatorTest look for // Confirm that meta-package will have an install path that is the same

@Wim Leers you also have commit access so you could revert it if need be.

Committed. Re #2 back to @Wim Leers

🚀 I reviewed it, all looks fine! In any case, there cannot be any doubt that this MR is a huge leap forward, and the follow-ups you created for refinements ensure it'll get better still 😊

Attached is a follow-up patch with nits fixed — assigning to @tedbow to review since he already knows this MR inside/out.

Since this MR, kernel tests roughly take ~1 min longer (from ~250 seconds to ~310 seconds), that seems totally reasonable for the added confidence! 😊 📌 For test performance: bundle FixtureManipulator's `composer require` + `composer require --dev` calls Closed: duplicate will narrow that gap further.

Unpostponed:

1. 📌 ComposerPatchesValidator should use ComposerInspector instead of ComposerUtility Fixed
2. 📌 Tighten ComposerPluginsValidator: support only specified version constraint Fixed
3. 📌 ScaffoldFilePermissionsValidator should use ComposerInspector instead of ComposerUtility Fixed

This means that we should now be able to completely finish 📌 Remove our runtime dependency on composer/composer: remove ComposerUtility Fixed 👍🚀

@Wim Leers thanks for the clean up patch! Committed

This unblocked 📌 Optimize Composer calls in FixtureManipulator Closed: works as designed .

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