Add a validate() method to ComposerInspector to ensure that Composer is usable

🤔

I don't think I agree with this proposal. 📌 Create an API for base requirement validators which run before other validators and stop event propagation if they fail Fixed is already solving this at the root, without the need for every validator to do a ComposerInspector::validate() call?

will comment on 📌 Create an API for base requirement validators which run before other validators and stop event propagation if they fail Fixed I think that issue will cause other problems

I don't think I agree with this proposal. 📌 Create an API for base requirement validators which run before other validators and stop event propagation if they fail Fixed is already solving this at the root, without the need for every validator to do a ComposerInspector::validate() call?

The whole idea here is that validators won't need to call ComposerInspector::validate(). They will only want to do it if they need to invoke Composer directly, without going through ComposerInspector's domain methods.

#6 confuses me thoroughly.

#6 says:

The whole idea here is that validators won't need to call ComposerInspector::validate(). They will only want to do it if they need to invoke Composer directly, without going through ComposerInspector's domain methods.

But the issue summary says:

Any other validators that use Composer inspector to get Composer info or make their own calls to it can do something like this beforehand:

try {
$this->composerInspector->validate($dir);
…

Is that not a contradiction? 🤔

Right you are. I adjusted the IS to clarify.

I … don't know if that change is actually accurate.

But either way — I'm looking forward to feedback from you both at #3312960-25: Certain validators should be higher priority and stop further validation → thanks to @tedbow's excellent #3312960-24: Certain validators should be higher priority and stop further validation → he mentioned in #4.

I think this issue ought to be closed as a duplicate in favor of 📌 Create an API for base requirement validators which run before other validators and stop event propagation if they fail Fixed for the reasons cited there.

Assigning this to myself because we need to figure out this one and 📌 Create an API for base requirement validators which run before other validators and stop event propagation if they fail Fixed now both assigned to me so hopefully nobody will work on them till I can respond

I'm taking this on to keep @tedbow's plate slightly less crowded.

To make this easier to review: the last commit in patch form with diff -M1%, to force it to detect renames.

Looks good! Glad we scaled this back for now

So the "scaling back" is why the package_manager/src/Validator/ComposerDependentValidatorTrait.php was removed, and 📌 ComposerInspector::validate() should run `composer validate` Fixed is now supposed to do that, is that right?

Ah, no, it seems that those things were moved to 📌 Create an API for base requirement validators which run before other validators and stop event propagation if they fail Fixed ? 🤔

A comment indicating in which way this was scaled back and where (which issues) to find where that work was shifted to, would've been very helpful 🙏

This MR in the way it was committed removed ComposerJsonExistsValidator, and the stopPropagation() in it.

But it did not remove ComposerExecutableValidator like the issue summary proposed.

It also did not add a ComposerValidator like the issue summary stated.

It also did not update package_manager.api.php, AGAIN, which is why it still says

 *  be stopped to prevent further event subscribers from breaking. For example,
 *  Package Manager stops event propagation if:
…
 * - No composer.json file exists in active directory. 👈👈👈

Regressions like this keep making 📌 Define the Package Manager API (package_manager.api.php is outdated) Fixed impossible to finish. I'll fix this too there, but please still either add a change record or update the issue summary, so that everybody working on Automatic Updates is clear on the direction without having to re-read a lot of code (and the same for the folks working on Project Browser).

Also, why was ComposerExecutableValidatorTest removed if we still have that validator? 🤔 At minimum, the assertResultsWithHelp() coverage is not included in ComposerInspectorTest?

I added new merge request to rename the class and the service which we should have done here

RE #20: by reviewing 📌 Create an API for base requirement validators which run before other validators and stop event propagation if they fail Fixed , I discovered that MR731 over there is restoring ComposerJsonExistsValidator(Test)? 😳

Added https://www.drupal.org/node/3344691 → .

re #24 that was just a mistake. probably I was working on both at near the same time

Needs review for the validator class name change. I think this is more correct because it is not simply just testing executable anymore.

There are now merge conflicts that need to be resolved.

Thanks for the CR! 😊

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