Workaround for composer patches bug.

[danepowell]

Composer patches has a bug that causes extraneous directories to get created in docroot/core such as docroot/core/core and docroot/core/b: cweagans/composer-patches#178

This creates a never-ending stream of issues and support noise. BLT could work around this issue and cut down on the noise by simply removing those extraneous directories after composer updates.

I'm not generally a fan of workarounds like this, but:

1. It's low risk (I can't see it being a problem unless Drupal legitimately adds a core/core or core/b directory, which seems unlikely)
2. It doesn't introduce significant additional complexity.
3. It solves a pretty immediate problem and actually decreases the overall maintenance burden for BLT.

[danepowell]

I guess we'd just want a way of tracking that upstream issue so we can remove this workaround when it's no longer necessary.

[mikemadison13]

do you have steps to reproduce this issue? does it "always" happen to just sometimes?

i do see a similar issue in my local for one of my projects (where my core has a b and core directory as you describe).

the code looks simple enough, happy to give it a whirl...

[danepowell]

I think it's triggered by certain core patches with a specific structure. I know that Lightning contains at least one of these patches:

• The problematic patch: https://www.drupal.org/project/drupal/issues/2862041
• Description of problem: Use dry-run to test patch in order to avoid empty folders being generated cweagans/composer-patches#178 (comment)

[effulgentsia]

BLT could work around this issue and cut down on the noise by simply removing those extraneous directories after composer updates.

Unfortunately, that's not enough. The problem is that if there's a patch that only adds new files (e.g., https://www.drupal.org/project/drupal/issues/2835825), then those files only get added to docroot/core/core and not to docroot/core. A post-update command that simply removes the former directory would then leave no trace that the patch failed to apply (to the intended directory) entirely.

What's needed is a backport of cweagans/composer-patches#101 (which fixes the underlying cause of patches being applied to incorrect directories) to the 1.x branch. Unfortunately, @cweagans commented on that issue that he does not want to do that. Fortunately, @jhedstrom has a fork that does it (https://github.com/jhedstrom/composer-patches/tree/patch-level-1-x).

So, a possible fix might be something like the following...

To BLT's composer.required.json, add (merge) the following:

    "repositories": {
        "composer-patches": {
            "type": "vcs",
            "url": "https://github.com/jhedstrom/composer-patches"
        }
    }

    "require": {
        "cweagans/composer-patches": "dev-patch-level-1-x#49dbb8f618b6f3d25e67f127dd400d487a6afd67 as 1.6.4"
    }

    "extra": {
        "patchLevel": {
            "drupal/core": "-p2"
        }
    }

1. The first bit says to get composer-patches from @jhedstrom's fork.
2. The second bit says to get that particular commit from that fork and to then treat that as version 1.6.4 (that commit basically is 1.6.4 plus the backport for specifying patch level).
3. The last bit then instructs composer-patches to patch Drupal core with -p2 rather than trying -p1 (which adds files to docroot/core/core) and -p0 (which adds files to docroot/core/b/core) first.

Note that this is fragile in that if we solve it this way, then if/when @cweagans releases composer-patches 1.6.5 or 1.7.0, BLT will still be forcing this fork of 1.6.4. It would be much nicer for the backport to be committed upstream. cweagans/composer-patches#189 is the pull request advocating for that.

[cweagans]

I went ahead and merged cweagans/composer-patches#189. Hopefully that helps.

[danepowell]

Okay, so the existing BLT workaround PR is probably no good.

It seems with cweagans/composer-patches#189 being merged we can now instead work around this by specifying the patch level for drupal core.

Longer-term, wouldn't cweagans/composer-patches#203 be the best fix for this? It seems to fix the problem universally, without having to specify a patch level.

[effulgentsia]

Longer-term, wouldn't cweagans/composer-patches#203 be the best fix for this? It seems to fix the problem universally, without having to specify a patch level.

When a patch level isn't specified, and so composer-patches tries an incorrect level, there's 2 cases to consider: the patch fails at the incorrect level, or the patch succeeds at the incorrect level.

If the patch is deleting or modifying any files, then it will fail when attempted at the incorrect level (since it won't find the file to delete or modify), and then try the next level, until it eventually succeeds. For this case, cweagans/composer-patches#203 is indeed a good solution to make sure there aren't stray folders/files created during a failed patch attempt.

However, if the patch only adds new files, and doesn't delete or modify any existing files, then it will succeed even when applied at an incorrect level, since it simply creates the files in the location specified by the patch level. By succeeding at the incorrect level, it never applies the patch at the correct level, so as far as the Drupal site is concerned, it's as if the patch didn't get applied at all. For this case, it's necessary to be able to explicitly set the level, which cweagans/composer-patches#189 being merged in now enables.

[effulgentsia]

I went ahead and merged cweagans/composer-patches#189. Hopefully that helps.

Yay! Thank you.

It seems with cweagans/composer-patches#189 being merged we can now instead work around this by specifying the patch level for drupal core.

Yes. But I think there's still a requirement for either the project's root composer.json or some dependency of it to specify:

    "require": {
        "cweagans/composer-patches": "1.x-dev"
    }

    "extra": {
        "patchLevel": {
            "drupal/core": "-p2"
        }
    }

Note in particular that it's "1.x-dev", not "dev-1.x" (that tripped me up for the last half hour).

So maybe that's something worth adding to BLT's composer.required.json?

[danepowell]

Thanks a lot for the tips everyone. It seems like any project that uses Drupal Core will want this config, so I've added it in PRs for both BLT and Lightning:

• Fix extraneous Drupal core patch directories lightning#581
• Fix composer patches creating extraneous core directories. #2816

I doubt stable releases of those libraries want to be using the 1.x-dev branch of Composer Patches, so we may just have to roll the config and wait for the next stable Composer Patches release.