Remove our runtime dependency on composer/composer: remove ComposerUtility

+1 to this plan. I'd recommend creating a service for executing a composer command that can be used by #3, #4, #5, and other places. That way, if a contrib module wants to make things work for sites without proc_open, it would only need to override that one service, rather than a separate one for each of those. Such a contrib module could then use composer/composer to perform the command execution in process. I don't think there's going to be overlap between sites that disable proc_open and sites where a composer/composer dependency is problematic, so I think an approach where core doesn't depend on composer/composer but uses proc_open, and a separate contrib module is available for sites without proc_open but that can depend on composer/composer instead, is a good one.

I'd recommend creating a service for executing a composer command

In fact, Composer Stager has such a ComposerRunnerInterface service, so might even make sense to use that one.

Based on Drupal core release manager @catch confirming that this should indeed happen (if at all possible) on November 3 over at #3243899-14: [policy] Move composer/composer from a dev dependency to a production dependency, bumping this to Critical.

Assigning to @tedbow to confirm that @phenaproxima's proposed plan is still the plan to execute. Also: should we convert this from a Plan to a Task?

Assigning to @tedbow to confirm that @phenaproxima's proposed plan is still the plan to execute.

Yes I think it is the way to go though obviously we will have see if it actually works out as planned as we try it.

Also: should we convert this from a Plan to a Task?

Should it still be plan because the work will be done in child issues?

Should it still be plan because the work will be done in child issues?

Because this was titled [meta] …, which indeed makes it sound like this needs child issues. If it can/should all be done in a single issue, we should change it to Task and remove the title prefix.

Adding this to the sprint since it is core-mvp we should work on it

@tedbow: this is a meta/plan issue. Can you please make this more actionable?

Especially now that you pointed me here over at https://git.drupalcode.org/project/automatic_updates/-/merge_requests/54..., where I flagged \ComposerUtility::getInstalledPackagesData() not using the official composer API as problematic and you pointed to this issue as having a scope that includes getting rid of that.

But I don't see that mentioned here explicitly, only implicitly: I think we can instead rely on two things to get the information we need: […] The installed.php that Composer generates when packages are installed or updated. We already rely on this quite a bit in ComposerUtility. → this actually suggest to double down on the thing I called out as problematic 😱
Next, the issue summary says Eventually, I think we might even be able to drop the first item, since installed.php is technically internal to Composer, and rely solely on the output of Composer commands. But that could be done later. → this is the thing I flagged as risky.

This tells me that this plan needs refining for it to become executable. With the list of remaining core-mvp issues shrinking, this is quickly becoming very important. Let's get this issue to an actionable state?

The MR #10 links to is #3315834: GitExcluder should not ignore .git directories that belong to packages installed by Composer.

#10 @Wim Leers sorry I just noticed the issue was not what I thought it was. Not sure if this from discussions with @phenaproxima that didn't get updated here. I will update the summary and get feedback from @phenaproxima

#3334994: Add new InstalledPackagesList which does not rely on Composer API to get package info landed.

#3337760: ComposerPatchesValidator should use ComposerInspector instead of ComposerUtility needs many more sibling issues. #3342227: ScaffoldFilePermissionsValidator should use ComposerInspector instead of ComposerUtility is the only one that exists so far — we need those for all validators that currently use ComposerUtility — as mentioned in #autoupdates Drupal Slack yesterday.

The issues in #16 look like they're relevant to removing a composer/composer dependency entirely. However, is there anything left unresolved before we can move composer/composer from require to require-dev? If not, it would be great to do that, close this issue, and open a separate meta for tracking all of the improvements to tests (if our other meta issues aren't already sufficient for that).

Oh, sorry. I see now that #3337760: ComposerPatchesValidator should use ComposerInspector instead of ComposerUtility is runtime code, not just test code.

Per #3343827-29: Update FixtureManipulator to work with InstalledPackagesList, real composer show command, #3337760: ComposerPatchesValidator should use ComposerInspector instead of ComposerUtility and #3342227: ScaffoldFilePermissionsValidator should use ComposerInspector instead of ComposerUtility are now unblocked.

While working on #3343827: Update FixtureManipulator to work with InstalledPackagesList, real composer show command, I got pretty confident that we should be able to remove all things ComposerInspector after those land. Let's see how well-founded that confidence was…

That confidence seems to have been pretty well-founded! But as I indicated in #16, this needs many more blocking issues, because we need to refactor away the many remaining uses of Stage::getActiveComposer() and Stage::getStageComposer(), as well as 2 remaining direct uses of ComposerUtility:

ComposerUtility

Stage::getActiveComposer()

Stage::getStageComposer()

The good news is that that should now be easy 😊🤞

This MR is currently +37, -1113, but won't be able to land until the following are done, to address #22:

1. ScaffoldFilePermissionsValidator → #3342227: ScaffoldFilePermissionsValidator should use ComposerInspector instead of ComposerUtility
2. ComposerPatchesValidator → #3337760: ComposerPatchesValidator should use ComposerInspector instead of ComposerUtility
3. Updater → #3345754: Updater should use ComposerInspector instead of ComposerUtility
4. ExtensionUpdater → #3345755: ExtensionUpdater should use ComposerInspector instead of ComposerUtility
5. \Drupal\automatic_updates_extensions\Form\UpdateReady → #3345757: \Drupal\automatic_updates_extensions\Form\UpdateReady should use ComposerInspector instead of ComposerUtility
6. \Drupal\automatic_updates_extensions\Form\UpdaterForm → #3345760: \Drupal\automatic_updates_extensions\Form\UpdaterForm should use ComposerInspector instead of ComposerUtility
7. UpdateReleaseValidator → #3345761: UpdateReleaseValidator should use ComposerInspector instead of ComposerUtility
8. GitExcluder → #3345762: GitExcluder should use ComposerInspector instead of ComposerUtility
9. UnknownPathExcluder → #3345763: UnknownPathExcluder should use ComposerInspector instead of ComposerUtility
10. OverwriteExistingPackagesValidator → #3345764: OverwriteExistingPackagesValidator should use ComposerInspector instead of ComposerUtility
11. SupportedReleaseValidator → #3345765: SupportedReleaseValidator should use ComposerInspector instead of ComposerUtility
12. CollectIgnoredPathsFailValidator → #3345766: CollectIgnoredPathsFailValidator should use ComposerInspector instead of ComposerUtility
13. FakeSiteFixtureTest → #3345767: FakeSiteFixtureTest should use ComposerInspector instead of ComposerUtility
14. StatusCheckTraitTest → #3345768: StatusCheckTraitTest should use ComposerInspector instead of ComposerUtility
15. RequestedUpdateValidator → #3345769: RequestedUpdateValidator should use ComposerInspector instead of ComposerUtility
16. VersionPolicyValidator → #3345771: VersionPolicyValidator should use ComposerInspector instead of ComposerUtility
17. \Drupal\automatic_updates\Form\UpdateReady → #3345772: \Drupal\automatic_updates\Form\UpdateReady should use ComposerInspector instead of ComposerUtility

I think any issue not in package_manager but in automatic_updates can be marked as core-post-mvp
Worst case scenario we could move ComposerUtility into automatic_updates as this will not be going into core first
so please work on package_manager issues first

Sure, but … until the ones in AU/AUE are removed … we cannot actually remove the composer/composer dependency 🙈 So not impossible, but definitely would leave things in a confusing state, although #3341974: Finalize \Drupal\automatic_updates\Development\Converter script to update core MR could compensate for it for the package_manager-only MR.

Landed:

• #3345765: SupportedReleaseValidator should use ComposerInspector instead of ComposerUtility
• #3345764: OverwriteExistingPackagesValidator should use ComposerInspector instead of ComposerUtility
• #3345768: StatusCheckTraitTest should use ComposerInspector instead of ComposerUtility

That leaves 14. Reflecting that in the title.

> We currently have a runtime dependency on composer/composer. This is mainly so we can invoke Composer's API to get information about what packages are currently installed (in either the active directory or staging area). The bulk of that takes place in Package Manager's ComposerUtility class.

We could depend on https://github.com/joachim-n/composer-manifest to know which packages are currently installed.

Would maybe want to add an API to that so that the list of Composer packages gets stored somewhere under Drupal's control.

@joachim very interesting!

What if someone runs a command from the terminal with `--no-plugins`? I don't think we could stop that. Won't this list then be out of date because the yml file not be updated? I guess maybe to get around that you could store a hash of the `composer.lock` in the yml file from that last time the .yml was updated and then anyone reading the file could check it is up-to-date. But still it would be out of date whether you detect I am not sure helps

Landed:

• #3345771: VersionPolicyValidator should use ComposerInspector instead of ComposerUtility
• #3345757: \Drupal\automatic_updates_extensions\Form\UpdateReady should use ComposerInspector instead of ComposerUtility

#3345761: UpdateReleaseValidator should use ComposerInspector instead of ComposerUtility landed.

#3345760: \Drupal\automatic_updates_extensions\Form\UpdaterForm should use ComposerInspector instead of ComposerUtility landed.

#3345762: GitExcluder should use ComposerInspector instead of ComposerUtility landed.

#3345763: UnknownPathExcluder should use ComposerInspector instead of ComposerUtility landed.

There are a bunch of mentions to installed.json and installed.php left after this MR:

• 99% of them occur in FixtureManipulatorTest (for example to verify that the original fixture's installed.json and installed.php are unchanged)
• 1% in TemplateProjectTestBase (to help generate a composer project from scratch for Build tests)

I think these can all be factored away, but … that doesn't block removing the composer/composer dependency, so that's out of scope for this issue. So created follow-up issues for both:

• #3347165: Remove FixtureManipulatorTest's reliance on installed.json and installed.php
• #3347164: Use Composer commands instead of parsing installed.json in TemplateProjectTestBase

#3347031: Stage::validatePackageNames() should not use the Composer API is in, updating this MR… 🥳

Turns out that those failures are being triggered by @phenaproxima's nice simplification in \Drupal\fixture_manipulator\FixtureManipulator::removePackage() … but that simplification was right!

Apparently somehow composer require some/package --no-update works even for require-dev packages, but only if there's a separate subsequent composer update!

@phenaproxima changed it to do composer require some/package (without the subsequent composer update), and that now fails. This is simple enough to fix though: make sure we pass TRUE to FixtureManipulator::removePackage() calls if they're for require-dev packages.

Fixed in the commit I just pushed!

There is not a single contentious thing in here. It's deleting code that's effectively unused in HEAD.

The diffstat speaks for itself:

+25 -967

The only "change" that is introduced here is described in #46. And adding docs. Those are the last 2 commits. They're tiny in the grand scheme of things.

@phenaproxima did not spot problems in his reviews last night (see #40–#43).

Therefore, going ahead and merging this, so we can get a green core merge request going at #3346707: Add Alpha level Experimental Package Manager module! 🚀

🚢