Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw a new exception after catching Cannot apply patch , so that Com… #31

Closed
wants to merge 2 commits into from

Conversation

pivulic
Copy link
Contributor

@pivulic pivulic commented Feb 23, 2016

Problem

When a patch fails, an exception is thrown from: https://github.com/cweagans/composer-patches/blob/master/src/Patches.php#L349

The exception is then catched in https://github.com/cweagans/composer-patches/blob/master/src/Patches.php#L264

During the Composer install, the patch fails to be applied and the user is informed about the error, but the install process continues.

Expected Result

If a patch fails to be applied, an exception should be thrown so that Composer fails the install and the user is notified about the error. Today, if the Composer installs several packages, the user might not wait for the entire install to finish and might therefor miss that a patch failed.

@cweagans
Copy link
Owner

I agree with the intent of this patch, but I think it needs a bit of work first. Some people may just want to continue, some may want it to stop. For now, let's leave the default behavior as-is, but add a way for users to opt-in to throwing another exception so that the composer install/update stops.

I'm thinking something like this:

  "extra": {
    "patches": {
      "vendor/package": {
        "My patch": "http://example.com/mychanges.patch"
      },
      "_config": {
        "exit-on-patch-failure": true,
      }
    }
  }

@pivulic Do you want to do this?

@cweagans cweagans mentioned this pull request Feb 23, 2016
2 tasks
@moleman
Copy link

moleman commented Feb 24, 2016

One suggestion would be to be able to set "exit-on-patch-failure" as an environment variable. Then you can fail or not fail "composer install / update" depending on the environment. In a build environment it's important to fail the build if a patch is not correctly applied.

Example:

export CP_EXIT_ON_PATCH_FAILURE=1
composer install

@pivulic
Copy link
Contributor Author

pivulic commented Feb 24, 2016

👍 Agree with @moleman

@cweagans
Copy link
Owner

@moleman, @pivulic 👍 from me. That's even better. The main point is that it should be opt-in. If you're going the environment variable route, please prefix with COMPOSER_ to be consistent with other Composer environment vars (so maybe COMPOSER_EXIT_ON_PATCH_FAILURE).

@pivulic
Copy link
Contributor Author

pivulic commented Feb 28, 2016

What about now? :)

@cweagans
Copy link
Owner

Committed in 0d75c63 -- added a space, changed some copy, and added some detail to the commit message, but it's all still credited to you. Thanks!

@cweagans cweagans closed this Feb 29, 2016
@pivulic
Copy link
Contributor Author

pivulic commented Feb 29, 2016

Thanks! By the way, when will this be included in a new release? :)

@cweagans
Copy link
Owner

When there's enough to justify a new release - 1-2 months tops.

@pivulic
Copy link
Contributor Author

pivulic commented Mar 1, 2016

Would it be possible to release a minor version at least, i.e. 1.3.1? :) That way, we can use your package from https://packagist.org/ instead of our fork in our projects.

@cweagans
Copy link
Owner

cweagans commented Mar 1, 2016

Actually, now that I look at it, there hasn't been a release in a long time (July 2015). I think it's time for a 1.4.0 release. I'll do that in the next day or two.

@cweagans
Copy link
Owner

cweagans commented Mar 3, 2016

@pivulic FYI, there is now a 1.4.0 release. Thanks again!

@pivulic
Copy link
Contributor Author

pivulic commented Mar 3, 2016

GR8!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants