-
Notifications
You must be signed in to change notification settings - Fork 1.7k
HowDoI: Create a tutorial for patching a composer project from github #1956
Conversation
- show the tool for patching into composer, - show how to create a patch file from a github url,
@keharper, please review. I finished my editorial updates and added this topic to the left navigation in the How to guide (2.1, 2.2, and 2.3 only). |
Sometimes it takes a while for us to include a bug fix made on GitHub in a Magento 2 Composer release. In the meantime, you can create a patch from GitHub and use the `composer-patches` plugin to apply it to your Composer-based Magento 2 installation. | ||
|
||
<div class="bs-callout bs-callout-warning" markdown="1"> | ||
Be careful applying an unreleased patch. Do comprehensive testing before deploying it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion for the 2nd sentence: Always perform comprehensive testing before deploying any unreleased patch.
1. Create a `patches/composer` directory. | ||
1. Prepare your patch file so that the paths are relative to the `vendor/<VENDOR>/<PACKAGE>` directory. For example `vendor/magento/module-payment`. | ||
1. Name the patch file appropriately and move it to the `patches/composer` directory. | ||
2. Add the `composer-patches` plugin to the `composer.json` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change composer-patches
to cweagans/composer-patches
? I'm not sure, but I was a bit confused where that cweagans
came from.
|
||
## Create a patch | ||
|
||
To create a patch file from a GitHub commit or pull request, append `.patch` to the url, [https://github.com/magento/magento2/commit/2d31571f1bacd11aa2ec795180abf682e0e9aede.patch](https://github.com/magento/magento2/commit/2d31571f1bacd11aa2ec795180abf682e0e9aede.patch). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This URL shows you the patch file, but doesn't actually create it for your purposes. I think you copy the contents, and paste it into the file described in line 31.
When take the link, I see this at the top:
From 2d31571f1bacd11aa2ec795180abf682e0e9aede Mon Sep 17 00:00:00 2001
From: Ievgen Sentiabov <[email protected]>
Date: Wed, 31 Aug 2016 18:46:44 +0300
Subject: [PATCH] MAGETWO-56934: Checkout page freezes when ordering with
Authorize.net with invalid credit card
- Added fail() method call
---
app/code/Magento/Payment/view/frontend/web/js/view/payment/iframe.js | 1 +
1 file changed, 1 insertion(+)
Does any of this need to be deleted? It's certainly not essential, but maybe the composer-patches
tool knows to ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmanners, deferring to you on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, you can also add .diff
to the url which results in a smaller file then the .patch
variant.
But both work absolutely fine. People should pick the one they like best.
And yes, it's best if you then download that file and put it in the directory mentioned below: patches/composer
(this directory is very arbitrary btw, you can choose which one you want to use yourself, not sure if this needs to be mentioned?)
Some other additions here:
- Sometimes a PR doesn't contain the full fix, because sometimes a member of the magento community engineering team adds some extra code to the PR. If you would then add
.patch
or.diff
to a PR, you won't get the full fix.
An example of this is this PR: declare var to fix scope error magento2#15265, the correct fix is the commit mentioned at the very bottom (magento/magento2@d9dc66c) instead of the changes of the PR.
So maybe we'll need to add some section about how you find the correct commit (or even multiple commits) for a certain fix. Which will probably be pretty complex to write down...
-
If a fix is old, it's hard to find a single merge commit with only a fix for that problem, this is due to how the magento developers used to merge branches, this has improved more in recent months, and you'll usually find a single commit you can use for a patch.
But if you can't find a single commit and you have multiple commits, what I usually do then is:- clone the Magento 2 repo
- checkout the version I want to create a patch for
- cherry-pick all commits you want to apply (and solve merge conflicts if they arise)
- do a
git diff 2.1.4...HEAD > my-patch-file.diff
(where2.1.4
is the version used in the second step)
-
It also will regularly happen that a commit won't apply cleanly to a particular version of Magento, happens most because there was some other code changed in another commit which is missing in that particular version of Magento somewhere around the same lines of that patch.
If this happens, I also follow the 4 steps outlined above to create a new patch, but you will certainly have to solve merge conflicts in that case.
Feel free to ignore these things above if this would make the documentation more complex then it needs to. It might scare users or confuse them ...
|
||
To create a patch file from a GitHub commit or pull request, append `.patch` to the url, [https://github.com/magento/magento2/commit/2d31571f1bacd11aa2ec795180abf682e0e9aede.patch](https://github.com/magento/magento2/commit/2d31571f1bacd11aa2ec795180abf682e0e9aede.patch). | ||
|
||
<div class="bs-callout bs-callout-info" markdown="1"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give an example of what needs to be changed. Would you change every instance of app/code/Magento/
to vendor/Magento
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmanners, deferring to you on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, example would be very useful here.
BTW: the cweagans/composer-patches
package's original intent was to apply patches relative to the root of the package, not to the root of the application. That particular "feature" was added afterwards, but I'm considering it a hack and never used it. So we always change the paths to the relative path of the module itself.
So in this specific case, you would replace all instances (5 times) of app/code/Magento/Payment/view/frontend/web/js/view/payment/iframe.js
with view/frontend/web/js/view/payment/iframe.js
(But if it would be considered easier to understand, then just replace app/code/Magento/Payment/
with vendor/magento/module-payment/
like was mentioned below on line 29)
Btw, maybe good to mention as a warning: you shouldn't make these changes with an actual code editor, since they sometimes strip out trailing whitespace or adding new lines depending on how they are configured, which can sometimes mess up the patch file. I prefer a very old school editor which doesn't do any fancy stuff like that. On macOS I'm using the built-in TextEdit application for this. For Windows, I'm assuming you can use the built-in Notepad? And for Linux, just vim or nano probably?
</div> | ||
|
||
## Apply a patch | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see more of an intro paragraph.
The following example applies the X patch to ....
"composer-exit-on-patch-failure": true, | ||
"patches": { | ||
"magento/module-payment": { | ||
"MAGETWO-56934: Checkout page freezes when ordering with Authorize.net with invalid credit card": "patches/composer/github-issue-6474.patch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, it looks like there are 3 key elements that need to be called out:
Affected module: "magento/module-payment"
PR title: MAGETWO-56934: Checkout page freezes when ordering with Authorize.net with invalid credit card
Path to patch: patches/composer/github-issue-6474.patch
Line 31 should give an indication of what an appropriately named patch would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmanners, deferring to you on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct!
The title is also arbitrary here, you can put in it what makes sense to you.
Also, if multiple modules are involved with a patch file, you need multiple patch files targeting multiple modules.
Maybe an example can be added as well for this case. Here is one which targets both the Quote and the Sales module: magento/magento2#11702
1. Edit the `composer.json` file and add the following section, specifying the Composer package to apply the patch to, as well as a description of the patch and a reference to the file location: | ||
|
||
```json | ||
"extra": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the composer.json
file contains an extra
block already, do you empty it out, create a new block, or merge the info?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmanners, deferring to you on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should merge the lines unless it doesn't already exist, then you have to create one.
Great topic! I'm willing to do some extra reviewing here as well, since creating a patch which can be applied cleanly on a particular version of Magento is sometimes not as easy as @dmanners thinks ;) If I haven't replied here by next week, feel free to ping me. |
Thanks @hostep! |
One last addition I want to make next to the inline comments: There is also an alternative to the I've tried it a half year ago, but had some minor issue with it (I can't remember what), and maybe it is already fixed by now. So it might be a good alternative to suggest or maybe after testing use as the default here, but then you'll have to rewrite most of the current content. |
--- | ||
|
||
Sometimes it takes a while for us to include a bug fix made on GitHub in a Magento 2 Composer release. In the meantime, you can create a patch from GitHub and use the `composer-patches` plugin to apply it to your Composer-based Magento 2 installation. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You sometimes see devs complaining about not wanting to install a patch but prefer to build a custom M2 module which fixes the bug by rewriting the original code.
This is a very personal opinion, but I think that using a patch is better in most cases, because if you upgrade Magento to a new version and it contains a fix for what you patched, composer will start complaining that it can no longer apply the patch. So you'll automatically go looking for a reason why it doesn't apply and you'll find that it was fixed in this particular new version and you can then remove the patch.
You wouldn't get notified about that if you use a custom build module. So you would have to keep that in mind to check all your custom modules with fixes after every Magento update.
Not sure if this makes a good addition to this page though, since it's a very personal opinion.
I would also add a link to the cweagans/composer-patches
module, since its readme file contains useful information which explains all the options you can use: https://github.com/cweagans/composer-patches/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great context. I'm not sure if it's necessary yet, but I may add it later. I'll definitely add a link to the package.
|
||
## Create a patch | ||
|
||
To create a patch file from a GitHub commit or pull request, append `.patch` to the url, [https://github.com/magento/magento2/commit/2d31571f1bacd11aa2ec795180abf682e0e9aede.patch](https://github.com/magento/magento2/commit/2d31571f1bacd11aa2ec795180abf682e0e9aede.patch). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, you can also add .diff
to the url which results in a smaller file then the .patch
variant.
But both work absolutely fine. People should pick the one they like best.
And yes, it's best if you then download that file and put it in the directory mentioned below: patches/composer
(this directory is very arbitrary btw, you can choose which one you want to use yourself, not sure if this needs to be mentioned?)
Some other additions here:
- Sometimes a PR doesn't contain the full fix, because sometimes a member of the magento community engineering team adds some extra code to the PR. If you would then add
.patch
or.diff
to a PR, you won't get the full fix.
An example of this is this PR: declare var to fix scope error magento2#15265, the correct fix is the commit mentioned at the very bottom (magento/magento2@d9dc66c) instead of the changes of the PR.
So maybe we'll need to add some section about how you find the correct commit (or even multiple commits) for a certain fix. Which will probably be pretty complex to write down...
-
If a fix is old, it's hard to find a single merge commit with only a fix for that problem, this is due to how the magento developers used to merge branches, this has improved more in recent months, and you'll usually find a single commit you can use for a patch.
But if you can't find a single commit and you have multiple commits, what I usually do then is:- clone the Magento 2 repo
- checkout the version I want to create a patch for
- cherry-pick all commits you want to apply (and solve merge conflicts if they arise)
- do a
git diff 2.1.4...HEAD > my-patch-file.diff
(where2.1.4
is the version used in the second step)
-
It also will regularly happen that a commit won't apply cleanly to a particular version of Magento, happens most because there was some other code changed in another commit which is missing in that particular version of Magento somewhere around the same lines of that patch.
If this happens, I also follow the 4 steps outlined above to create a new patch, but you will certainly have to solve merge conflicts in that case.
Feel free to ignore these things above if this would make the documentation more complex then it needs to. It might scare users or confuse them ...
|
||
To create a patch file from a GitHub commit or pull request, append `.patch` to the url, [https://github.com/magento/magento2/commit/2d31571f1bacd11aa2ec795180abf682e0e9aede.patch](https://github.com/magento/magento2/commit/2d31571f1bacd11aa2ec795180abf682e0e9aede.patch). | ||
|
||
<div class="bs-callout bs-callout-info" markdown="1"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, example would be very useful here.
BTW: the cweagans/composer-patches
package's original intent was to apply patches relative to the root of the package, not to the root of the application. That particular "feature" was added afterwards, but I'm considering it a hack and never used it. So we always change the paths to the relative path of the module itself.
So in this specific case, you would replace all instances (5 times) of app/code/Magento/Payment/view/frontend/web/js/view/payment/iframe.js
with view/frontend/web/js/view/payment/iframe.js
(But if it would be considered easier to understand, then just replace app/code/Magento/Payment/
with vendor/magento/module-payment/
like was mentioned below on line 29)
Btw, maybe good to mention as a warning: you shouldn't make these changes with an actual code editor, since they sometimes strip out trailing whitespace or adding new lines depending on how they are configured, which can sometimes mess up the patch file. I prefer a very old school editor which doesn't do any fancy stuff like that. On macOS I'm using the built-in TextEdit application for this. For Windows, I'm assuming you can use the built-in Notepad? And for Linux, just vim or nano probably?
## Apply a patch | ||
|
||
1. Create a `patches/composer` directory. | ||
1. Prepare your patch file so that the paths are relative to the `vendor/<VENDOR>/<PACKAGE>` directory. For example `vendor/magento/module-payment`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above about original intent of the cweagans/composer-patches
package.
1. Edit the `composer.json` file and add the following section, specifying the Composer package to apply the patch to, as well as a description of the patch and a reference to the file location: | ||
|
||
```json | ||
"extra": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should merge the lines unless it doesn't already exist, then you have to create one.
|
||
```json | ||
"extra": { | ||
"magento-force": "override", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this magento-force
setting here, it has nothing to do with the patches and it is already added by default in your Magento installation.
"composer-exit-on-patch-failure": true, | ||
"patches": { | ||
"magento/module-payment": { | ||
"MAGETWO-56934: Checkout page freezes when ordering with Authorize.net with invalid credit card": "patches/composer/github-issue-6474.patch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct!
The title is also arbitrary here, you can put in it what makes sense to you.
Also, if multiple modules are involved with a patch file, you need multiple patch files targeting multiple modules.
Maybe an example can be added as well for this case. Here is one which targets both the Quote and the Sales module: magento/magento2#11702
|
||
```bash | ||
composer install | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a patch can't be applied, composer will complain with an error message: Could not apply patch! Skipping. The error was: Cannot apply patch patches/composer/github-issue-6474.patch
This is not very helpful, so sometimes it helps if you could get some more debugging output about why a patch doesn't want to apply.
And you can get more debugging output by adding the -v
flag to composer install
, so like this: composer -v install
This can maybe be added to some troubleshooting section?
1. Update the `composer.lock` file. The lock file tracks which patches have been applied to each Composer package in an `extra > patches_applied` object. | ||
|
||
```bash | ||
composer update <PACKAGE NAME> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer composer update --lock
which updates nothing but just rewrites the composer.lock
file with the correct information. Since adding patches can sometimes contain fixes for multiple modules, this is probably an easier command to use?
@hostep, |
@jeff-matthews would it make sense to explain in the docs how to create a patch file locally? Like;
|
|
||
1. Save the page as a file in the `patches/composer` directory. | ||
|
||
1. Edit the file and change all paths so that they are relative to the `vendor/<VENDOR>/<PACKAGE>` directory. For example, replace all instances of `app/code/Magento/Payment/` with `vendor/magento/module-payment/`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct, it should say something like: "strip all instances of app/code/<VENDOR>/<PACKAGE>/
from the path, so you end up with a relative path to the vendor/<VENDOR>/<PACKAGE>/
directory." So the replacement part is incorrect, that first part of the path should be stripped of and not replaced.
As an addition it might be a good idea to use an example from the patch mentioned above, where you would replace all paths app/code/Magento/Payment/view/frontend/web/js/view/payment/iframe.js
with view/frontend/web/js/view/payment/iframe.js
|
||
[https://github.com/magento/magento2/commit/2d31571f1bacd11aa2ec795180abf682e0e9aede.patch](https://github.com/magento/magento2/commit/2d31571f1bacd11aa2ec795180abf682e0e9aede.patch){:target="\_blank"} | ||
|
||
1. Save the page as a file in the `patches/composer` directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe specify the exact filename, since this filename gets used in the Apply a patch
section.
Also, since the filename in use here is github-issue-6474.patch
it might make sense to also link to the issue 6474 from the Magento2 repo in one of the first steps of the Create a patch
section? So people understand how issues, pull requests and patches are linked together? (in this particular case, there isn't a pull request I think)
If a patch affects multiple modules, you must create multiple patch files targeting multiple modules. | ||
</div> | ||
|
||
1. Apply the patch. Use the `-v` option to show debugging information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe say: "Optionally you can use the -v
flag to show debugging information.", as this flag outputs lots of lines so it shouldn't be recommended to do this by default I think, only when you get an error you can use it to try to figure out what went wrong.
Suggestion from @peterjaap is a good idea, but it might make the documentation a lot more complex. There are a lot of ways to generate a patch:
|
Yes the added complexity was my consideration as well to just leave it out. A more pessimistic view; it might also deincentivize people to create a PR if they generate a patch locally ;) |
Thanks for the suggestion @peterjaap. I think we all agree that since there are many ways to create a patch, it would be best to keep this tutorial simple and focus on the "create from commit" method. |
I think this type of content is better suited to the Magento Knowledge base (KB). If we keep it on devdocs, we should convert it to the tutorial template: https://devdocs.magento.com/guides/v2.2/contributor-guide/templates/tutorial-template-first.html. @dmrachkovskyi, what do you think? |
See the published article on the Magento Knowledge Base |
@jeff-matthews: Can you change the line:
to:
Or something like that? I think that's more correct. Thanks! (Off topic: where should we contribute to these KB articles? Are they also managed on Github somewhere?) |
@dmrachkovskyi, please revise the KB article per @hostep's comments. @hostep, the Magento KB is in Zendesk, so contributions don't work like they do on devdocs. I'll discuss enabling comments below articles on Zendesk with the support manager, but in the meantime, please feel free to open an issue against the devdocs repo. |
Thanks! :) |
This PR is a:
Summary
When this pull request is merged, it will show users how to create a patch of a change in github and apply this to a project create via composer.
It should show the user:
This came about after discussions with @davidalger during Imagine.
Additional information
I am not sure of the following and would be happy for any feedback: