Ensure composer patches fail the build if a patch fails

[grayside]

cweagans/composer-patches has a setting to ensure the build fails if any patch fails: COMPOSER_EXIT_ON_PATCH_FAILURE=1.

We would like this to be handled via a composer.json config value. We do not know if the composer-patches library can receive it that way, though it seems that proper use of the composer config API would allow it to be injected either way.

Action Items

1. Check to see if composer.json config can be used to activate this setting.
2. If not PR upstream and see if it will have a release quickly enough to leverage.
3. If a release with a fix is fast enough, add that setting to our composer.json config.
4. If a release is not fast enough, add the environment variable to our build.yml and build.devcloud.yml templates.

This ticket emerged from conversation with @jhedstrom and @scottalan

[scottalan]

I set this environmental variable in my container and then ran an install. I got output in the CLI but the build did not fail:

https://www.drupal.org/files/issues/duplicate_ajax_wrapper-2346893-194.patch (2346893 - Duplicate AJAX wrapper around a file field)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/duplicate_ajax_wrapper-2346893-194.patch

It did not create the lock file but there was nothing "in your face" that the patch didn't apply. Maybe this is expected?

[jhedstrom]

No, when the variable is detected (must be set to 1, or non-zero anyway), composer install/update will definitely fail rather than just moving on...

[jhedstrom]

This is the code from \cweagans\Composer\Patches::postInstall() that checks for the variable. I don't think that second bit checking $extras would find a value in composer.json's config section...

    $localPackage = $localRepository->findPackage($package_name, $package->getVersion());
    $extra = $localPackage->getExtra();
    $extra['patches_applied'] = array();

    foreach ($this->patches[$package_name] as $description => $url) {
      $this->io->write('    <info>' . $url . '</info> (<comment>' . $description. '</comment>)');
      try {
        $this->eventDispatcher->dispatch(NULL, new PatchEvent(PatchEvents::PRE_PATCH_APPLY, $package, $url, $description));
        $this->getAndApplyPatch($downloader, $install_path, $url);
        $this->eventDispatcher->dispatch(NULL, new PatchEvent(PatchEvents::POST_PATCH_APPLY, $package, $url, $description));
        $extra['patches_applied'][$description] = $url;
      }
      catch (\Exception $e) {
        $this->io->write('   <error>Could not apply patch! Skipping. The error was: ' . $e->getMessage() . '</error>');
        if (getenv('COMPOSER_EXIT_ON_PATCH_FAILURE') || !empty($extra['composer-exit-on-patch-failure'])) {
          throw new \Exception("Cannot apply patch $description ($url)!");
        }
      }
    }

[scottalan]

RTBC :)

I placed the variable in the wrong container. It should live in the base service. I have tested this and adding COMPOSER_EXIT_ON_PATCH_FAILURE: 1 does in fact work.

Running "composer:install" (composer) task
Gathering patches for root package.
Removing package drupal/core so that it can be re-installed and re-patched.
Deleting build/html/core - deleted
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Package operations: 1 install, 1 update, 0 removals
Gathering patches for root package.
Gathering patches for dependencies. This might take a minute.
  - Installing drupal/core (8.3.7): Loading from cache
  - Applying patches for drupal/core
    https://www.drupal.org/files/issues/2699157-23.drupal.Plugin-Lazy-loading-can-cause-usort-warning.patch (2699157 - Plugin Lazy loading can cause usort warning)
    https://www.drupal.org/files/issues/2759397-1-entity_reference_recursion.patch (2759397 - Patch EntityReferenceItemNormalizer to prevent recursion)
    https://www.drupal.org/files/issues/2679775-11-inline-labels.patch (2679775 - Fixes float issue with inline label fields (entity references on most cases).)
    https://www.drupal.org/files/issues/duplicate_ajax_wrapper-2346893-194.patch (2346893 - Duplicate AJAX wrapper around a file field)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/duplicate_ajax_wrapper-2346893-194.patch

  [Exception]
  Cannot apply patch 2346893 - Duplicate AJAX wrapper around a file field (https://www.drupal.org/files/issues/duplicate_ajax_wrapper-2346893-194.patch)!

install [--prefer-source] [--prefer-dist] [--dry-run] [--dev] [--no-dev] [--no-custom-installers] [--no-autoloader] [--no-scripts] [--no-progress] [--no-suggest] [-v|vv|vvv|--verbose] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--ignore-platform-reqs] [--] [<packages>]...

Warning: Task "composer:install" failed. Use --force to continue.

Aborted due to warnings.
[ERROR] Error running project script 'install': 3

[grayside]

Here's an example of how to improve this to use Composer configuration.

From my review of the Config handling, it looks like custom config env vars need to be separately handled, but the example above illustrates how it could be expanded to include checking the composer.json.

[jhedstrom]

I've added https://github.com/cweagans/composer-patches/pull/158.

[grayside]

In looking at the upstream solution to use composer.json config, it looks to me like !empty($extra['composer-exit-on-patch-failure']) is already in place to facilitate doing this in composer.json along the lines of how the external patchfile is documented.

Could you test that as well @scottalan? That would be a better immediate solution.

[grayside]

Given my further testing (noted in https://github.com/cweagans/composer-patches/pull/158) it seems like the composer.json setting does not exist. As such, I'll go ahead and merge Jonathan's PR. We'll hold this issue open to circle back and move the change to the composer.json once we have a path forward.