Use dry-run to test patch in order to avoid empty folders being generated #178

ericchenshine · 2017-12-13T02:31:40Z

Currently, I am trying to upgrade Drupal from 8.3.7 to 8.4.3 including BLT, Lightning, and Drush upgrade. I have found there are some bizarre files have been generated on the air after each build.

As you can see the core dir appears in 3 places as below:

docroot/core
docroot/core/core
docroot/core/b/core

This does not smell right and after investigation, I have found that those unwanted files are generated after patches defined in lightning has applied to drupal/core. I have checked this issue on our dev, test, and prod environments on Acquia Cloud and have found that this problem also happens on those servers. So this issue happens prior to my upgrade, I just happen to notice that during the upgrade work.

Here are the details of my local environment:

drush: 8.1.15
patch: 2.5.8
composer: 1.5.5
cweagans/composer-patches: 1.6.4

Here is the log with my debug messages:

Gathering patches for root package.
Warning: The value for extras.installer-paths in composer.json differs from BLT's recommended values.
See https://github.com/acquia/blt/blob/8.x/template/composer.json
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Package operations: 1 install, 0 updates, 0 removals
Gathering patches for root package.
Gathering patches for dependencies. This might take a minute.
  - Installing drupal/core (8.4.3): Loading from cache
  - Applying patches for drupal/core
    https://www.drupal.org/files/issues/2752961-90.patch (Clear Twig caches on deploys)
$install_path: docroot/core
  - Applying patches for https://www.drupal.org/files/issues/2752961-90.patch
$patch_level: -p1
$patch_level: -p0
$patch_level: -p2
    https://www.drupal.org/files/issues/ignore_front_end_vendor-2329453-116.patch (Ignore front end vendor folders to improve directory search performance)
$install_path: docroot/core
  - Applying patches for https://www.drupal.org/files/issues/ignore_front_end_vendor-2329453-116.patch
$patch_level: -p1
$patch_level: -p0
$patch_level: -p2
    https://www.drupal.org/files/issues/1356276-408--8.4.3.patch (1356276 - Allow profiles to provide a base/parent profile and load them in the correct order)
$install_path: docroot/core
  - Applying patches for https://www.drupal.org/files/issues/1356276-408--8.4.3.patch
$patch_level: -p1
$patch_level: -p0
$patch_level: -p2
    https://www.drupal.org/files/issues/2880374-remove-experimental-warnings-6.patch (2880374 - Experimental modules should not have warnings after being installed)
$install_path: docroot/core
  - Applying patches for https://www.drupal.org/files/issues/2880374-remove-experimental-warnings-6.patch
$patch_level: -p1
$patch_level: -p0
$patch_level: -p2
    https://www.drupal.org/files/issues/2880445-remove-config-write-warning-2.patch (2880445 - Config sync should not throw a warning when not being writable)
$install_path: docroot/core
  - Applying patches for https://www.drupal.org/files/issues/2880445-remove-config-write-warning-2.patch
$patch_level: -p1
$patch_level: -p0
$patch_level: -p2
    https://www.drupal.org/files/issues/2869592-remove-update-warning-7.patch (2869592 - Disabled update module shouldn't produce a status report warning)
$install_path: docroot/core
  - Applying patches for https://www.drupal.org/files/issues/2869592-remove-update-warning-7.patch
$patch_level: -p1
$patch_level: -p0
$patch_level: -p2
    https://www.drupal.org/files/issues/2885441-2.patch (2885441 - EntityReferenceAutocompleteWidget should define its size setting as an integer)
$install_path: docroot/core
  - Applying patches for https://www.drupal.org/files/issues/2885441-2.patch
$patch_level: -p1
$patch_level: -p0
$patch_level: -p2
    https://www.drupal.org/files/issues/2883813-27.patch (2883813 - Move File/Image media type into Standard once Media is stable)
$install_path: docroot/core
  - Applying patches for https://www.drupal.org/files/issues/2883813-27.patch
$patch_level: -p1
$patch_level: -p0
$patch_level: -p2
    https://www.drupal.org/files/issues/2877383-23.patch (2877383 - Add action support to Media module)
$install_path: docroot/core
  - Applying patches for https://www.drupal.org/files/issues/2877383-23.patch
$patch_level: -p1
$patch_level: -p0
$patch_level: -p2
    https://www.drupal.org/files/issues/2862041-229.patch (2862041 - Provide useful Views filters for Content Moderation State fields)
$install_path: docroot/core
  - Applying patches for https://www.drupal.org/files/issues/2862041-229.patch
$patch_level: -p1
$patch_level: -p0
$patch_level: -p2
    https://www.drupal.org/files/issues/2815221-69-8.4.x.patch (2815221 - Add quickedit to the latest-revision route)
$install_path: docroot/core
  - Applying patches for https://www.drupal.org/files/issues/2815221-69-8.4.x.patch
$patch_level: -p1
$patch_level: -p0
$patch_level: -p2

Here are my observations
As you can see in a lightning project when patches apply to drupal/core, only p2 works. This is because the original patches are created in drupal/drupal in the root directory. But those patches target to drupal/core in the Lightning composer.json. As a result, the install_path becomes root directory/core instead of root directory.

This is totally fine to apply the patch with the parameter -p2. However wrong files and directories are created even when patch attempts with p1 and p0 fail.

The proposal
We can fix this by adding a dry run test before applying that patch.

-Here is the PR #179-

The patch is originally created when I was using Patch version 2.5.8 on Mac OS. However, after I installed GNU Patch version 2.7.1, this PR does not work anymore. Even using dry-run, empty folders will be still created.

Further questions
The PR should fix the wrong files issue. However, I am still wondering why these files are created by patch command. I have randomly picked some patches and try to produce this issue by manually applying some of those patches. But there is no avail. Maybe something related to composer? Here are the details of wrong files created.

-But I think this PR should be enough to avoid the issue.-

The text was updated successfully, but these errors were encountered:

potterme · 2017-12-26T23:32:14Z

I am having this same problem. Doing a "composer install" to update to the latest Lightning that uses the 1.6.4 version of composer-patches causes the creation of the "core/b" and "core/core" folders. Not sure what to try, but going to try going back to 1.6.3.

bkosborne · 2018-02-02T20:27:02Z

Can this issue title be updated? The two extra sets of folders that are generated are not empty, they contain the patched files.

Anyway this is affecting me too. I guess it's not really a problem that the files exist in multiple places since the autoloader would only load the ones in the correct path, but this a weird/ugly problem!

johnzzon · 2018-02-06T17:47:53Z

As noted in #43, here's why this is happening:

The issue is that as drupal/core is a subtree split of only the core/ directory. All patches posted on DO contains the core/ prefix, while that subdirectory doesn't exist in the composer package

diff --git a/core/modules/book/book.views.inc b/core/modules/book/book.views.inc
This means that if a patch modifies a file p0 and p1 will always fail and p2 will be used. If the patch only introduces new code p1 will pass and create the file in the incorrect location.

Figured I'd post that in here so we know the issue behind it.

balsama · 2018-02-06T21:10:55Z

To prove @johnzzon point, I used patch 229 from issue 2862041:
https://www.drupal.org/files/issues/2862041-229.patch

(Note that this patch adds files. This issue isn't reproducible with a patch that doesn't add files)

To test, I added the following to the extra section of the composer.json file of a new project created with drupal-composer/drupal-project:

        "patches": {
            "drupal/core": {
                "2862041 - Provide useful Views filters for Content Moderation State fields":
                "https://www.drupal.org/files/issues/2862041-229.patch"
            }
        }

This resulted with a b directory created in /web/core/ with /core/modules/content_moderation inside it.

Then I replaced all instances of "a/core/" with "a/" and "b/core/ with "b/" in the patch (attached) and ran `composer update drupal/core. The /web/core/b directory was not present after updating.

2862041-229-relative.patch.txt

bkosborne · 2018-02-08T16:57:41Z

So one workaround is to store the patch locally in your repo and strip out the /core/ from the paths.

balsama · 2018-02-08T17:36:27Z

@bkosborne exactly my thought. I'm thinking that when Lightning rerolls our core patches for 8.5.x, we'll make two versions. One for D.O and one without core that we're reference in our composer.json file. So at least vanilla Lightning 3.1.0 - without any additional core patches - won't be affected.

On second thought, we can't commit to maintaining that. We should work to solve the problem here instead.

…/folders.

pescetti · 2018-03-30T18:20:46Z

I found this issue after creating independent patches for it, but my findings are largely similar to what people already found.

The issue affects both master (pull request: #202) and 1.x (pull request: #203). In master, though, it's rarer to see it due to the extended usage of git apply.

The following composer.json file would simply apply the patch for https://www.drupal.org/sa-core-2018-002 to Drupal 8.5.0 (just an example, as in real life one would just upgrade to 8.5.1):

{
  "name": "patched-drupal",
  "type": "project",
  "require": {
    "composer/installers": "dev-master",
    "cweagans/composer-patches": "1.x",
    "drupal/core": "8.5.0"
  },
  "extra": {
    "installer-paths": {
      "web/core": ["type:drupal-core"]
    },
    "patches": {
      "drupal/core": {
        "SA-CORE-2018-002": "https://cgit.drupalcode.org/drupal/rawdiff/?h=8.5.x&id=5ac8738fa69df34a0635f0907d661b509ff9a28f"
      }
    }
  }
}

What happens instead is that, upon composer update, one has three files named RequestSanitizer.php:

$ find . -name RequestSanitizer.php 
./web/core/lib/Drupal/Core/Security/RequestSanitizer.php
./web/core/b/core/lib/Drupal/Core/Security/RequestSanitizer.php
./web/core/core/lib/Drupal/Core/Security/RequestSanitizer.php

The core/b and core/core directories are created by unsuccessful calls to patch, that still leave these "leftover" files/folders behind due to partial success on the new file.

I concur with @johnzzon that the title should be updated - this would have helped me in finding the issue before going and fixing it in a slightly different way than the one already posted here.

cweagans · 2018-06-02T06:40:31Z

There are many other issues about this. 2.0.0 will solve it by allowing you to define the patch depth on a per-patch basis - no more guessing + incorrect results.

pescetti · 2018-06-03T22:07:47Z

I see. The option to define patch level in 2.x is definitely welcome. Still, without a stable 2.x release yet, this patch would have allowed to prevent having duplicate files in wrong places, a bug that I considered bad enough to warrant inclusion in a stable 1.x release.

cweagans · 2018-06-03T22:25:57Z

Sorry. I'm not spending more time on 1.x. It's too fragile to change major things at this point - a problem that I'm focusing on rectifying in 2.0.0.

Which patch are you talking about? The one referenced in the OP was closed by the author because it didn't work.

pescetti · 2018-06-21T19:41:22Z

I was referring to #203 but the 1.x backport of the "define patch level" feature #189 in the new 1.6.5 release is indeed a more or less equivalent solution, even though it requires a little bit of extra work in composer.json. This issue can probably be closed. Thanks for still maintaining 1.x!

cweagans · 2023-02-07T20:25:27Z

main is now very flexible wrt patch depth. You can define it in a number of ways:

Per-patch in the patch definition
(coming soon) Per-package in your composer.json
Per-package in Util::getDefaultPackagePatchDepth() (this is global for all composer-patches users -- just trying to have good defaults for ecosystems with different workflows like Drupal)
Global default as a fallback (which defaults to -p1).

cweagans · 2023-02-07T20:26:16Z

Also, patch depth is no longer guessed: either use the defaults or explicitly say what you want. :)

ericchenshine mentioned this issue Dec 13, 2017

Use 'dry run' to test patch first (against branch 1.x) #179

Closed

ericchenshine changed the title ~~Use dry-run to test patch in order to avoid wrong files are generated~~ Use dry-run to test patch in order to avoid empty folders being generated Dec 14, 2017

bkosborne mentioned this issue Feb 2, 2018

Patch for new file applied to incorrect location #43

Closed

pescetti added a commit to pescetti/composer-patches that referenced this issue Mar 30, 2018

Fixes cweagans#178: Use dry-run to test patch to avoid leftover files…

beef85c

…/folders.

pescetti added a commit to pescetti/composer-patches that referenced this issue Mar 30, 2018

Fixes cweagans#178: Use dry-run to test patch to avoid leftover files…

d391da1

…/folders.

This was referenced Mar 30, 2018

Fixes #178: Use dry-run to test patch to avoid leftover files/folders. #202

Closed

Fixes #178: Use dry-run to test patch to avoid leftover files/folders. #203

Closed

danepowell mentioned this issue May 8, 2018

Workaround for composer patches bug. acquia/blt#2809

Closed

boobaa mentioned this issue May 15, 2018

Replace cweagans/composer-patches with vaimo/composer-patches drupal-composer/drupal-project#393

Closed

This was referenced Apr 7, 2021

Patch to drupal/core applied twice #60

Closed

Drupal core patches apply incorrectly if they only add new files #234

Closed

cweagans closed this as completed Feb 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dry-run to test patch in order to avoid empty folders being generated #178

Use dry-run to test patch in order to avoid empty folders being generated #178

ericchenshine commented Dec 13, 2017 •

edited

Loading

potterme commented Dec 26, 2017

bkosborne commented Feb 2, 2018

johnzzon commented Feb 6, 2018

balsama commented Feb 6, 2018

bkosborne commented Feb 8, 2018

balsama commented Feb 8, 2018 •

edited

Loading

pescetti commented Mar 30, 2018 •

edited

Loading

cweagans commented Jun 2, 2018

pescetti commented Jun 3, 2018

cweagans commented Jun 3, 2018

pescetti commented Jun 21, 2018

cweagans commented Feb 7, 2023

cweagans commented Feb 7, 2023

Use dry-run to test patch in order to avoid empty folders being generated #178

Use dry-run to test patch in order to avoid empty folders being generated #178

Comments

ericchenshine commented Dec 13, 2017 • edited Loading

potterme commented Dec 26, 2017

bkosborne commented Feb 2, 2018

johnzzon commented Feb 6, 2018

balsama commented Feb 6, 2018

bkosborne commented Feb 8, 2018

balsama commented Feb 8, 2018 • edited Loading

pescetti commented Mar 30, 2018 • edited Loading

cweagans commented Jun 2, 2018

pescetti commented Jun 3, 2018

cweagans commented Jun 3, 2018

pescetti commented Jun 21, 2018

cweagans commented Feb 7, 2023

cweagans commented Feb 7, 2023

ericchenshine commented Dec 13, 2017 •

edited

Loading

balsama commented Feb 8, 2018 •

edited

Loading

pescetti commented Mar 30, 2018 •

edited

Loading