Fail when patches cannot be applied about composer-patches HOT 16 CLOSED

I'd like to see the default fail the build. We could add a flag to disable that for BC purposes, but I'd be surprised if there are folks out there that consider patches specified in their composer.json as optional, and would appreciate knowing exactly which patches fail (and knowing immediately, whether locally or on a CI).

from composer-patches.

Is this an appropriate place to offer feedback for the new implementation? (I'll assume so.)

• Failing the build by default is great.
• It would also be helpful to get a list of patches that couldn't be applied all together at the end of the command output. If you have very many patches, it can be easy for failures to get lost in the backscroll where they're tedious to collate and at risk of getting overlooked altogether.
• For that to be useful in the case of more than one failed patch, the command would need to attempt to apply all patches before failing (as opposed to failing on the first problem).

from composer-patches.

I'd be okay with a flag that controls this. This shouldn't be the default though. Not till 2.0.0 anyway.

from composer-patches.

I suppose you want the flag for BC reasons, but that does mean this plugin will continue to do the opposite of what it advertises whenever patches fail to apply (which is to apply patches). I'm afraid this will lead to a lot of time being spent by developers trying to find the causes of bugs in their sites or security issues, caused by patches not being applied.

from composer-patches.

It hasn't been that much of a problem, as the plugin does already add some output saying that a patch can't be applied. The main thing is, I don't want to just change a behavior that has been in place since 1.0.0 and hope for the best. A flag is better here because users can opt-in to functionality that will be in 2.0.0 (and in 2.0.0, it'll be an opt-out). I would also not be opposed to some additional messaging at the end of composer install or composer update if there were any patches that couldn't be applied.

I would also say that before blindly dropping patches into your composer.json (especially if those are security patches), you should be sure that all the patches will apply in the order that you specify. I'd also say that it's probably much safer to just upgrade the release in the event of a security update, rather than just applying the security patch.

from composer-patches.

Actually, looks like we already added the flag as an environment variable: #31

2.0.0 should also clean up the config flags such that they can be specified in composer.json or as an env var, rather than having a weird mix of stuff.

from composer-patches.

I did not say security releases; I gave examples of problems the current behavior can cause, including those that compromise security.

Adding additional messages will still not solve this problem for automated builds. If a CLI tool does not exit with a non-zero code on failure, it basically fails to tell its executing shell about the failure, which means any non-human observer (build tools) will not be able to take the necessary precautions to prevent worse failures, such as deploying a broken system to production.

from composer-patches.

That's fair, and I mostly agree with you. However, this still can't be the default behavior until 2.0.0 because semver. It's a BC break. The best we can do in the 1.x releases is more messaging and a flag to let you opt-in to a non-zero exit code.

from composer-patches.

Thanks for referring #31! I'll give that a try for now. A composer.json config setting may be useful as well, so projects do not have to depend on manual setup or additional build scripts wrapping the composer install command.

from composer-patches.

Agreed. PRs welcome :)

from composer-patches.

Hello! Thanks for the cool plugin!
Please check #46 for the "composer-exit-on-patch-failure" config option.

from composer-patches.

The default in 2.0.0 will be to fail when a patch cannot be applied. 1.x needs to default to success, unfortunately. I'm not willing to change that behavior in a minor release.

from composer-patches.

Fair enough. Is there a milestone (or list of issues) required for 2.0.0? Happy to help if possible :)

from composer-patches.

Well, not really :( There's a 2.x branch which has some pretty aggressive refactoring in progress, but I haven't had time to finish. It's a rewrite, more or less, with the goal of good test coverage + smaller pieces to work with.

from composer-patches.

This is as good of place as any.

Failing the build by default is great.

👍

It would also be helpful to get a list of patches that couldn't be applied all together at the end of the command output. If you have very many patches, it can be easy for failures to get lost in the backscroll where they're tedious to collate and at risk of getting overlooked altogether
For that to be useful in the case of more than one failed patch, the command would need to attempt to apply all patches before failing (as opposed to failing on the first problem).

These would apply when the plugin is configured to not fail when a patch can't be applied. Definitely still valid behaviors, as the plugin can be configured either way. The default will just be different in 2.0.0.

from composer-patches.

Looks like this has been resolved for some time now.

If you're still looking for 2.x issues, see #93. Lots of stuff to tackle there.

from composer-patches.