Restore composer.json after failed drush installation

Ready for review.

Hmm, not sure what's going on. The composer require fails with a PHP stack trace and so does the composer remove afterwards as well. As if something got into the code base which conflicts with something in composer.

See https://git.drupalcode.org/project/sqrl/-/jobs/3069951 where I have used issue/gitlab_templates-3481194 and 3481194-revert-failed-drush-changes as variables when running the pipeline.

Thanks for the quick test. It was a quick MR to see if that'd be enough, but it's not.

We can probably copy the composer.json/lock files before trying the operation and if it fails, then put those files back and run "composer install" again, as if nothing would have happened.

I'm not sure I'll have the time to work on it today, but I'll make notes in the MR right now so I won't forget or if anybody wants to give it a go.

I went ahead and did the change directly as I was almost typing full commands 😅
Ready for testing again.

Just a note: this was a composer crash.

Normally composer will revert the composer.json back if it is unable to require a new package due to resolution issues.

Likely it is this Composer bug:
https://github.com/composer/composer/issues/12151

Especially since the module has the following in its .gitlab-ci.yml:

COMPOSER_EXTRA: '--ignore-platform-req=ext-sodium'

Arguably this failure may be correct as the situation implies composer had a serious problem and the system is in an inconsistent state, composer could have written files into vendor/ and than crashed corrupting the rest of the results (I’m not sure when the lock file is written, before or after file writing).

Would suggest considering this works as designed.

Side note: the drush job needs to ignore extensions as well for it to ever work in this scenario.

Especially since the module has the following in its .gitlab-ci.yml: COMPOSER_EXTRA

That's why I suggested, that this variable should also be used for all subsequent composer commands. That's why extra arguments are provided and if so, they need to be used all the time.

@fjgarlin would you mind if we added that variable to this composer command as well? I bet, the problem will then disappear.

The problem is that the options are not the same for install/update/require I think. There are a couple of options in each one not compatible in the other ones.

Note that if Drush is really required as dependency, it should be included in the project's "composer.json" file and that way COMPOSER_EXTRA will actually be taken into account.

What we are trying in this block of code is a nice-to-have (ie: we've detected that you don't have drush but it might be useful to have it, so let's try, otherwise continue).

So, for me, the current MR solves the issue of leaving an inconsistent composer.json/lock + vendor files. It should just try, otherwise continue as if nothing would have happened. Hard dependencies (even if they are test dependencies) should be on composer.json in my opinion.

That's correct. However, the issue is not about drush, it's that composer fails because of a system requirement on libsodium in this project, which we don't have in our PHP images, and we can happily ignore as we don't need it for the pipelines. That said, any composer command would fail that didn't ignore that extension.

So, I can confirm that the current MR fixes the issue in the sqrl project.

My concern is, we only avoid the symptom, while the underlying issue is still there and could hit us elsewhere anytime again.

But I take your point, that composer extras can be different for each command. Not so for ignoring requirements. They are the same everywhere. Maybe we should start a follow-up issue to propose that we provide separate support for ignoring requirements and then use that for all composer commands when not empty?

Happy for a follow-up, but I also quote myself from before:

Note that if Drush is really required as dependency, it should be included in the project's "composer.json" file and that way COMPOSER_EXTRA will actually be taken into account.

Thanks for the review

Sure, but we don't need drush.

Oh cool. In that case, I don't think we are calling any other composer update/require anywhere else in the codebase. The drush one is a special case, and this code will fix any quirks, and the "upgrade status" one has a dedicated variable, so I think we're good when it comes to variables.

That also means, that a hard failure in composer install should provoke an error exist in the composer jobs I guess.

But I might be missing something (foggy brain, long week), so if you feel that we need a follow-up for a case not covered, just create it and we can discuss further in there.

Merged. Thanks!

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