Conversation
swissspidy
added
Type: Bug
|
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
The reason for this I recall is to guard against a situation where the repo fork gets deleted or the branch is removed. |
Ah, interesting. Not sure about the fork-getting-deleted part, a URL like |
|
Is the thinking that using a remote patch file will facilitate keeping it updated? cc @pierlon |
That, and it would help work around cweagans/composer-patches#315 because we'd no longer have to copy the patches from the amp-wp repo to this one. |
|
Got it. @pierlon What do you think about going back to using remote patches? |
|
The patches currently employed by the amp-wp are from currently open PRs, so if we were to use remote patches there is a possibility of them being remotely updated if someone were to push to the relevant forked repo. Alternatively, maybe we could create a PR on amp-wp with those patches and then use that as the remote patch. Thoughts @swissspidy @westonruter? |
@pierlon apparently the patch URLs persist even after the PR is closed and even when the fork is deleted, per Pascal above. |
|
Right but it would be possible to push commits to the PR while it's open, thus updating the referenced remote patch. The example Pascal referred to ( |
|
On one hand that is a good thing since it means it would be easy to update the patch (as in the case of a conflict). On the other hand, this means that the version is not explicitly indicated in the We could mitigate this by linking to the patch for a specific commit, rather than a PR. For example: https://github.com/wp-media/wp-rocket/commit/1b47bb97f9e7acdaaf8b66f44391fe2481a7293e.patch For example, for your PR: MyIntervals/PHP-CSS-Parser#193. Instead of linking to https://github.com/sabberworm/PHP-CSS-Parser/pull/193.patch, we could instead link to: https://github.com/sabberworm/PHP-CSS-Parser/compare/d9d0190c2c29946aac93375ca05df2d48329d6d7%5E...0ff0e38629b5ba96ae3b8368385974b1ebee9acb.patch This includes the same commits in your PR: MyIntervals/PHP-CSS-Parser@d9d0190c2c29946aac93375ca05df2d48329d6d7^...0ff0e38 So then if the PR gets updated, then we'd need to update the commit hashes in the |
Hmm yea that makes sense. In that case I'm on board with using remote patches then. |
|
@swissspidy |
swissspidy
force-pushed
the
fix/composer-patches
branch
from
10d526b to
40372fc
Compare
Codecov Report
@@ Coverage Diff @@
## main #5082 +/- ##
==========================================
+ Coverage 76.36% 77.17% +0.81%
==========================================
Files 932 932
Lines 16485 16485
==========================================
+ Hits 12589 12723 +134
+ Misses 3896 3762 -134
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
As ampproject/amp-wp#5556 has been merged, I think we are safe to merge. I will approve after merge conflict is resolved. |
|
@spacedmonkey conflicts resolved! |
4 participants
Summary
Fixes broken CSS gradients.
Relevant Technical Choices
Originally, I simply wanted to use
"enable-patching": true, but that doesn't work because of cweagans/composer-patches#315(cc @westonruter maybe the Composer patches in the plugin could use patches from GitHub instead of locally provided ones?)
Then, I wanted to do
"enable-patching": falseand simply provide"patches": { ... }myself, but that didn't work -enable-patchingis ignored when patches are provided.So the correct way is to provide
patches-ignoreto ignore the AMP plugin's patches in favor of our own patches declaration.Next, I learned that
patches/php-css-parser-pull-185.patchcannot be applied for some reason (Could not apply patch! Skipping. The error was: Cannot apply patch patches/php-css-parser-pull-185.patch).Since that patch doesn't seem critical for us, I simply removed it from the patches list.
Aside:
I wanted to use version 1.7 of the
cweagans/composer-patchesas it has a different structure and Composer 2 support, but the AMP plugin requires 1.6.x, so that doesn't work.To-do
User-facing changes
Gradients are back!
Testing Instructions
Fixes #5081