Allow JsonProcessOutputCallback and other Composer runner callbacks to gracefully handle deprecated command options

we have the 2 related issues in

but this is core-post-mvp so removing from sprint

I think this is the intent? To automatically ignore the error output if it's just complaining about something deprecated?

We had a related problem in 🐛 anonymous ProcessOutputCallbackInterface class ComposerInspector::getConfig() assumes __invoke does receive any data in error buffer Closed: duplicate and now \Drupal\package_manager\ComposerInspector::getConfig is not using JsonProcessOutputCallback but has an anonymous callback that has a similar problem

1. Adding trim to error buffer and output buffer.
2. Adding back validate() at the start of getConfig().

Quoting the IS:

You are using the deprecated option "installed". Only installed packages are shown by default now. The --all option can be used to show all packages.

👆 Not yet addressed!

This handle the error buffer as:
1. If it has output buffer data even if there is also error buffer output return the output buffer
2. If there is error buffer data and no output buffer data throw an exception with the error buffer data

This seems very dangerous.

It is entirely possible that data could have been written to the output buffer for a while, and THEN some kind of show-stopping problem was encountered that ended the process, and data was written to the error buffer.

IMHO the only surefire way to detect an error is the exit code.

That is correct. In fact, in various issues at https://github.com/composer/composer, the confusing nature of Composer writing to both stdout and stderr even when no error occurred has been discussed. This is intentional behavior, because Composer writes things like deprecation errors to stderr even when there is general success ⇒ which is why the exit code is the only way to know an error has occurred

But that's also the reason we can reliably ignore the error output if the exit code is 0!

Still, it's valuable to have explicit handling for deprecation error output, and I think @omkar.podey's approach to detecting & handling deprecations captures the spirit of what @tedbow wrote 👍, even though I think we should have a follow-up issue to allow tests to fail on Composer deprecations (because that's out of scope here).

The shape of the solution looks good to me, but there are a lot of details that are now wrong/outdated.

Please self-review before asking others to review!

Tests looking good, starting to look good overall!

Attempted to RTBC, but ran into language nits and consistency issues — but this is definitely approaching completion!

Note that this cannot be RTBC without that follow-up having been created.

@omkar.podey created that follow-up: 📌 Update ProcessOutputCallback to fail on Composer deprecations, to get early notifications of Composer changes Postponed . 👍

How did I do that? 🙈

This mostly looks great. There are a few small things I'd like to see changed, and honestly I can probably do that myself. But, just making sure nobody commits this right now...

Assigning to Wim to sign off on my changes.

I really wanted to RTBC, but I can't quite agree with all of the changes 😅

Assigning to Wim for a re-review and to accept or reject my explanations for the more questionable changes...

To log errors

Just to keep it clear where we're at:

Ted noticed, while reviewing this issue, that throwing exceptions if there's error output (even if it only has certain words) is too heavy-handed and could interfere with otherwise working Composer commands.

The thing is, a deprecation error should not be a show-stopper. It means the command succeeded, but will fail in the future. After some discussion, we feel the correct thing to do is throw an exception if the Composer process exits with a non-zero exit code -- Composer Stager already does this (and tests it). If there is error output, we should log it as a warning.

This means we'll need to have a logger channel for Package Manager, which is also given to the process callback by ComposerUtility. We should also alter PackageManagerKernelTestBase so that, if any warnings or errors are logged to that channel, the test fails.

Let the negotiations begin...

I pushe 1 comment addition and add note about missing test coverage, otherwise looks good

Expanded test coverage.

I think that is definitely the coverage we want.

I think it could be a little clearer in some parts so I'll do some clean-up, but otherwise I'm happy with it. Nice work!

Back to @tedbow for final review.

Look good! Thanks!

Finally! Great to get this done.

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