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

Apply patch on root folder #76

Closed
tomasnagy opened this issue Oct 5, 2016 · 22 comments
Closed

Apply patch on root folder #76

tomasnagy opened this issue Oct 5, 2016 · 22 comments

Comments

@tomasnagy
Copy link

Is there any way to apply a patch the root of the project, not specifically targeting a package.
Currently I couldn't any documentation about this.
Is this supported in any form or way?

@cweagans
Copy link
Owner

cweagans commented Oct 5, 2016

No, there's not. Presumably, those files are committed to your repository. Why not just change them directly?

@erikhansen
Copy link

@cweagans I too would love to be able to apply patches at the root of a project. My use case for this is as follows: I work on a lot of Magento 2 projects. Magento 2 projects use Composer to install its packages. Unlike applications like Drupal (which seems to use a single Composer package for the entire application) Magento 2 has separate composer packages for its dozens of packages.

There are many situations where a patch would need to be applied to multiple Magento 2 packages simultaneously. With the current state of this plugin, the patch would need to be split between multiple .patch files, which would be very clunky. It would be great to be able to apply a patch to multiple packages simultaneously, and it seems like the simplest way to enable this would be to allow patches to be applied to the root of the project. Although allowing patches to be applied to the vendor directory would also work, although it seems less elegant.

I'm a contributor to the capistrano-magento2 Gem and we're currently discussing the best way to patch Magento when deploying it to stage/production environments. In my opinion, using this plugin would be better than building a specific patching mechanism into the capistrano-magento2 Gem. We would just need root-patching support in order for the compser-patches plugin to fully satisfy our requirements.

@cweagans
Copy link
Owner

If you want to code something up to show what this would look like, I'd take a look. I can't promise it'll go in as-is, but I'm certainly not opposed to looking at it.

@cweagans
Copy link
Owner

This will be a part of 2.0, FWIW.

@erikhansen
Copy link

@cweagans Fantastic news! Any idea of a timeline for 2.0?

@cweagans
Copy link
Owner

When it's done :) I have less than 1 page in my notebook for remaining tasks. This week is crazy, but I have Monday off, so I should be able to take a big bite out of it. I may put that list up on GH later today.

@erikhansen
Copy link

@cweagans Completely understood. I appreciate you putting so much of your personal time into this very useful plugin.

Once 2.0.0 is released, I expect even more Magento 2 developers will start using this plugin as it solves a very real problem with Magento 2 development and deployment.

@Vedrillan
Copy link

+1

How is the 2.x going ? :)

Trying to apply Magento patches but does not work out of the box if the patch applies to several package at the same time.

@erikhansen
Copy link

@Vedrillan While it's a bit of work, it is possible to split a Magento patch into multiple files so that it can be applied to each of the relevant Composer packages. Someone on my team recently did this with a patch supplied from Magento.

@Vedrillan
Copy link

Yep indeed it is still possible, chopping down the patches to target only 1 package at a time, it is also necessary to adapt the files path, but if we can avoid this fastidious part thanks to the 2.x, I'd be happier !

@hostep
Copy link

hostep commented Mar 28, 2017

@erikhansen & @Vedrillan: I fail to see how this can be solved, well for Magento2 patches I mean.

One Magento2 patch file can contain fixes for multiple modules at once. Without any mapping to say which part of the file should be applied to which module, there is no way this is ever going to work.
Next to that, the patches always use paths like app/code/Magento/SomeModule/SomeDirectory/SomeFile.php but in the vendor/magento/module-somemodule directory, we only have SomeDirectory/SomeFile.php. So that's already something else which needs manual labour.

I can't see how this can be automated, unless I'm missing something obvious here?
The only thing I can think of is searching through composers autoloader files and try to find matches in there, but that is very specific to what type of project which is being used I guess.

So yeah, we'll have to manually keep splitting the patches into multiple patch files for each submodule and change the paths in the patch files I'm afraid, but I'm already used to it by now:

Example

screen shot 2017-03-28 at 21 48 47

sigh ;)

@erikhansen
Copy link

@hostep You are correct that if you generate a patch directly from a Github commit or PR, you would have to manually change the paths to Composer-based paths. However this could still be done in the context of a single file and applied using a “root patching” method and in my mind would be preferable because it would keep a patch contained within a single file.

Also, for Magento Enterprise Merchants that get patches directly from Magento, those patches are in a “Composer friendly” format that can be applied directly to the root of a Magento install.

Another limitation I’ve run into with composer-patches is that it doesn’t appear possible to apply two separate patches to the same Composer package. There’s probably an issue about that somewhere on this repo.

@cweagans
Copy link
Owner

cweagans commented Apr 2, 2017

Another limitation I’ve run into with composer-patches is that it doesn’t appear possible to apply two separate patches to the same Composer package.

There is no such limitation in this plugin. If you're encountering errors, it's likely that the patches conflict with each other, so you'll have to manually apply them both and generate a combined patch to get around the conflicts.

@erikhansen
Copy link

@cweagans Ok, good to know. Thanks for letting me know.

@cweagans
Copy link
Owner

cweagans commented Jun 2, 2018

Sheesh. I was very optimistic about how long 2.0.0 would take.

Bad news though: in other issues, this has been discussed repeatedly, and I think the best thing for this plugin is to stay focused on apply patches to composer packages. I'm not sure that there's a way to do this in a way that doesn't add all kinds of special cases across the plugin, so I'm leaning toward not doing this, and instead documenting how to use custom composer scripts to apply patches outside of this plugin.

Thoughts?

@cweagans cweagans mentioned this issue Jun 2, 2018
36 tasks
@Vedrillan
Copy link

In my head it seemed pretty easy as I though of creating a new field like this

"extra": {
    "root-patches": {
        "some_patch": "local/path/to/your/patch.patch"
  }

and then a special case in the code to apply these patches from the dependencies parent folder, which should be project root, and that is all, should do the trick without affecting the other regular cases.

But if you say it is not as trivial as I thought then I'll trust you, I haven't dived much into the code, and as a result you can de-prioritize this task and keep it for later 👍

@hostep
Copy link

hostep commented Jun 2, 2018

I agree with @cweagans here, just keep it simple and let patches apply per package and not apply for the entire project.
It only takes a few minutes of manual work to split a big patch covering multiple packages out over a multiple files and let them apply separately as patches. If someone is clever enough, this could potentially been done with some sort of script.

If people still want this, maybe they can look at another composer patches module, which (according to its documentation - section "Patches: bundled patches") is capable of doing such a thing: https://github.com/vaimo/composer-patches#patches-bundled-patches

@cweagans
Copy link
Owner

cweagans commented Jun 2, 2018

After sleeping on this, I think I have a decent way to handle this. I'm planning on converting the patch application logic into a Composer capability, which would be user-extensible. If I can drop another Capability plugin in that can apply patches to arbitrary paths, that might be a reasonable way to approach this.

I'm not 100% sure it'll work (and I'm still of the opinion that per-package patches are the best way to go), but if it does work and it doesn't break anything or cause major refactors, I'm willing to talk about it.

@erikhansen
Copy link

@cweagans Back when I commented on this issue in November of 2016, it seemed like it was going to be a lot of work to split big patches into separate patches for each composer package. However after having done it for the past year and a half, I would agree with @hostep:

It only takes a few minutes of manual work to split a big patch covering multiple packages out over a multiple files and let them apply separately as patches.

So I'd vote to close this issue and let you focus on more important 2.0.0 features.

@Sanfam
Copy link

Sanfam commented Jun 11, 2018

I agree with @erikhansen and @hostep on this one. Another vote for close.

While breaking apart a vendor-provided patch can seem daunting for someone new to the process, it becomes extremely easy after tackling it even once and finding that it is largely a matter of bulk find/replace and cut/paste. I haven't encountered a patch which required more than a 4-way split. It'd be a nice convenience to have but is not worth the time right now.

Some official instructions for identifying and correcting patches requiring rework would be sufficient.

@cweagans
Copy link
Owner

Ask and ye shall receive :)

If it's not a priority anymore, I'm happy to not implement it. If it would still be useful, I think I have a good way to do it. Maybe this would be a decent thing to look at for a 2.1.0 release (or later).

@Jaesin
Copy link

Jaesin commented Sep 21, 2018

I can still see a use case for this when used along side of replace and using an upstream you have little control over.

@cweagans cweagans mentioned this issue Feb 7, 2023
31 tasks
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

No branches or pull requests

7 participants