ComposerPatchesValidator should use ComposerInspector instead of ComposerUtility

My take on the issue,

1. The first thing that pops up is the check empty($extra['composer-exit-on-patch-failure']) and this is checking the extra which is inside package and not config (added a screenshot) so we need a new function something like getRootPackageInfo in package_manager/src/ComposerInspector.php
2. So I tried this
   

       $this->runner->run(['--self', "--working-dir=$working_dir", '--json'], $this->jsonCallback);
       return $this->jsonCallback->getOutputData();

   but it failed with an error

       In PluginManager.php line 169:                                                                           
       Plugin cweagans/composer-patches is missing a require statement for a version of the composer-plugin-api package.                               

3. So next thing was to add "composer-plugin-api" => "^1.0 || ^2.0" to the fixture being created but it still failed, @wim.leers had a hunch that it might be caused because we are not updating the composer and just touching the installed.json and installed .php in our manipulator.

Thoughts @ted.bowman ?

@omkar.podey thanks for investigating this.

My guess is we might need 🐛 The 'fake_site' fixture cannot be using with `composer show` because the packages are not installed Fixed so our fake_site fixture is not so fake.

I think in that issue we would make a Composer path repo for cweagans/composer-patches and then do an actual composer require cweagans/composer-patches to bring in a mocked version of the package. The we won't have to worry about which fails we need to update

The closest thing i could find, so now i think we might need to create a class which parses the composer for us and return specific values that we request for.

Or

I might have missed something and we might be able to do it with cli somehow.

@wim.leers thoughts ?

I did a lot digging and trial and error. One thing is certain: composer's docs on this front suck.

👆 composer config works not just for the top-level config key in composer.json, but also for extra… discovered this thanks to:

$ composer help config | grep extra
  -j, --json                     JSON decode the setting value, to be used with extra.* keys
  -m, --merge                    Merge the setting value with the current value, to be used with extra.* keys in combination with --json
  To add or edit extra properties you can use:
      /opt/homebrew/bin/composer config extra.property value
      /opt/homebrew/bin/composer config extra.property --json '{"foo":true, "bar": []}'

Just discussed with @omkar.podey, he's +1 — and he did try composer config but the docs suggested that it was for writing only — like I said: the docs suck 🙈

So now postponed on 🐛 The 'fake_site' fixture cannot be using with `composer show` because the packages are not installed Fixed .

🐛 The 'fake_site' fixture cannot be using with `composer show` because the packages are not installed Fixed committed

Per #10.

Discussed with @omkar.podey — this should wait for 📌 Reliably support cweagans/composer-patches in Package Manager & Automatic Updates: validate stage Fixed : that issue is 90% done and will heavily conflict with this.

Blocker is in!

in \Drupal\package_manager\Validator\ComposerPatchesValidator::computePatcherStatus to calculate $is_installed we need list of installed packages, so this should wait for 📌 Add new InstalledPackagesList which does not rely on Composer API to get package info Fixed

#18: good catch!

Blocker committed!

The changes I made here:

• The patches validator needs to know whether the plugin is a root requirement. As it happens, composer show returns a direct-dependency field which tells us exactly that. So I added that to InstalledPackage.
• I added the ability for ComposerInspector::getConfig() to return a default value. It's not clear that https://github.com/composer/composer/issues/11302 will ever be merged, and I don't think we should be adding more special-casing into ComposerInspector when we need a default value. This also allows the logic in computePatcherStatus() to be a little more straightforward.

Yay, since 📌 Stop reusing core's commit-code-check.sh in favor of 4 simple commands Fixed this is no longer resulting in aborted CI runs, but instead it's reporting/failing on the actual quality problems 👍

I will fix the quality problems.

Discussed in detail during meeting just now. Conclusion: this is hard-blocked on direction from @tedbow in 📌 Update FixtureManipulator to work with InstalledPackagesList, real composer show command Fixed to avoid wasted work.

📌 Update FixtureManipulator to work with InstalledPackagesList, real composer show command Fixed landed, this is unblocked!

Wim Leers → closed merge request !704

I think this looks great.

However, the one thing I really don't like is the special-casing of extra.composer-exit-on-patch-failure in ComposerInspector::getConfig(). That feels boundary-violating to me and I don't think we should be establishing that as a precedent.

That said, I won't block commit on it, but I want to know what @tedbow thinks.

Restoring RTBC on green. I don't think my changes are too controversial.

Excellent catch, and 100% agreed with the solution 😊👍

Great! Looks good. will merge on green

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