Merge ComposerSettingsValidator into ComposerValidator

I found out that in 📌 Add a validate() method to ComposerInspector to ensure that Composer is usable Fixed package_manager/src/Validator/ComposerJsonExistsValidator.php was merged in package_manager/src/Validator/ComposerExecutableValidator.php (now package_manager/src/Validator/ComposerValidator.php) but we add help document and in help doc we only have doc for the case if composer executable not found so should we add doc for the case if composer.lock or composer.json not found i.e. project is not valid.

And also now that we are going to merge ComposerSettingsValidator and ComposerValidator should we also add help doc for error in ComposerSettingsValidator because we don't have it currently.

When I merged ComposerSettingsValidator( checking both active and staged directory) in ComposerValidator(checking only active directory) , I thought it seems logically wrong that some part of the validator checks only for active directory and some part is checking for both active and staged directory, so I made it check both active and staged directories for the whole validator after merging.
Now the problem is that the ComposerPluginsValidatorTest::testAddDisallowedPlugin() is failing way before on ComposerValidator as it now also checks staged directory and the error message is coming with some random token on every line because of which it fails to assert.

At @kunal.sachdev's request, I traced into this. I found a bit of a mess!

But first, let me say that @kunal.sachdev did everything right. I think the changes in this MR are absolutely correct and good.

It turns out, though, that ComposerPluginsValidatorTest::testAddDisallowedPlugin is currently passing by coincidence in HEAD!

Here's the thing: we assert that we get an exception that contains a particular string. So far, so good. But are we sure we know which validator, exactly, is causing that exception to be thrown? 😈 We're assuming it's ComposerPluginsValidator, but during my trace, it was actually coming from EnabledExtensionsValidator.

That's because the exception will be raised by the first validator that happens to do a ComposerInspector operation on the stage directory -- which might not be ComposerPluginsValidator at all! So that particular test has never actually tested ComposerPluginsValidator. It might not actually be needed at all, ultimately, since it's really just testing that...we call ComposerInspector::validate() on the stage directory. Not all that useful. But we can figure that out in another issue.

So that leaves us with the question: why the hell is this test failing? I have an answer for that too.

In most validators, when we do ComposerInspector operations, it calls validate() implicitly. If that raises an exception, we don't catch it -- we just let it bubble up to the calling code. Somehow, that results in the control characters (what Kunal referred to as a "random token", but it is actually the way that the Symfony Console component creates a red background under white characters) being removed from the exception message.

But ComposerValidator is different -- it calls validate(), and catches the exception, and converts it to a validation error. That ends up preserving the control characters, which causes our assertion to fail!

I found a workaround, but I'm not sure it's what we necessarily want to do.

If you change ComposerInspector::validateProject so that it passes the --no-ansi flag to Composer (rather than --ansi), it passes.

I need to research what this option does; I'm not sure why exactly we passed the --ansi flag in the first place.

I have asked @TravisCarden to weigh in on this, but I think that ComposerInspector::validate() should probably NOT be calling composer validate in ANSI mode. ANSI mode allows for the colored text, which is useful on the terminal, but we are fundamentally displaying the output of the command on the web, not a terminal.

Not only this, but why the hell would we call composer validate --no-cache --no-interaction? It's not an interactive command, and it doesn't install anything (so the cache should be irrelevant). The presence of those flags feels a bit like a copypasta job.

We should probably adjust the flags in another issue, which should block this one.

Postponed on 🐛 ComposerInspector::validateProject() passes unnecessary flags to `composer validate` Fixed .

Affirming what @phenaproxima said: the --ansi and --no-ansi options control whether the command output uses ANSI escape codes or not. Such codes are only for visual effects like colors in the terminal where they have special meaning and get parsed out. In any other context they'll be taken raw, making output effectively useless in scripts. If you're seeing apparent nonsense like \e[0;37m in your command output, that's what it is. You definitely don't want to use the --ansi option if you're parsing command output. And while --no-ansi should be the default behavior, it wouldn't hurt to always use it.

There is one conflicting change with the other issue other than that this looks good!

I need to open a follow-up here to make the test coverage correct, as I described in #8.

Shouldn't we also have added hook_help() entries for these cases, matching what's already in \Drupal\package_manager\Validator\ComposerValidator::validate() prior to this commit?

I discussed this with @kunal.sachdev on Monday (see #6).

We need a follow-up for that too.

Opened 🐛 Improve hook_help() to explain the actual Composer requirements Fixed .

Added 🐛 Explain why ComposerPluginsValidatorTest::testAddDisallowedPlugin() exists, even if it is not directly testing ComposerPluginsValidator Fixed .

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