-
-
Notifications
You must be signed in to change notification settings - Fork 242
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
Comments
No, there's not. Presumably, those files are committed to your repository. Why not just change them directly? |
@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 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. |
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. |
This will be a part of 2.0, FWIW. |
@cweagans Fantastic news! Any idea of a timeline for 2.0? |
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. |
@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. |
+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. |
@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. |
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 ! |
@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. I can't see how this can be automated, unless I'm missing something obvious here? 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: |
@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. |
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. |
@cweagans Ok, good to know. Thanks for letting me know. |
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? |
In my head it seemed pretty easy as I though of creating a new field like this
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 👍 |
I agree with @cweagans here, just keep it simple and let patches apply per package and not apply for the entire project. 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 |
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. |
@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:
So I'd vote to close this issue and let you focus on more important 2.0.0 features. |
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. |
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). |
I can still see a use case for this when used along side of |
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?
The text was updated successfully, but these errors were encountered: