composer-patches icon indicating copy to clipboard operation
composer-patches copied to clipboard

Drupal core patches apply incorrectly if they only add new files

Open deviantintegral opened this issue 7 years ago • 5 comments
trafficstars

The problem is caused by three factors.

  1. Patches that only add new files, like this responsive image migration patch.
  2. Drupal core is special, because the base of the composer project is the "core" directory, but all the patches are against git which is one directory higher. When composer-patches tries to apply the patch, -p1 works because it's only adding new files, even though -p2 is the correct one to apply. 3. There's an undocumented patch-level config, but since the config setting is global, if we change it could break other other patches that only add new files.

One workaround is to reroll patches to have the updated prefix. For the plugin, I think we should consider making the patch-level config be per patch instead of global.

deviantintegral avatar Aug 31 '18 12:08 deviantintegral

This needs to be fixed or at the very least documented better. I've spent hours trying to figure out what was happening and this issue was not easy to find. (Somewhat easier once i figured out the problem had to be related it to it being a new-file-only patch.)

mlncn avatar Sep 11 '19 20:09 mlncn

Documenting the workaround better would help for me also. I couldn't quite gather what i should do to the patch to make it work.

So i added tests to the core patch that wouldn't apply, so now it is changing a file, and will apply, so that's all good. But i still don't think it's composer-patches' place to be enforcing the requirement that core patches have tests :wink:

mlncn avatar Sep 12 '19 16:09 mlncn

@mlncn https://github.com/cweagans/composer-patches/tree/1.x#allowing-to-force-the-patch-level--px should help -- this is a side effect of how composer-patches guesses the patch level. Since patches that add new files will succeed pretty much all the time, you may have to specify that value in your config. I'm hoping to make this smarter in 2.x.

cweagans avatar Sep 12 '19 17:09 cweagans

We ended up going with a forking model to avoid edge cases like this entirely. If you're interested, I wrote up our process over at https://www.lullabot.com/articles/patch-less-composer-workflow-drupal-using-forks.

deviantintegral avatar Sep 15 '19 13:09 deviantintegral

For current 1.x, patchLevel should help with patches for drupal/core.

Also, see #178 , #203

john-herreno-ds avatar Apr 07 '21 16:04 john-herreno-ds

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 avatar Feb 07 '23 20:02 cweagans