-
-
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
Split out patch resolution into Composer Capabilities #212
Conversation
1c1d287
to
4a946db
Compare
The fact that the tests are currently passing is not a good thing. Shows how inadequate the test coverage is currently 😢 |
…of Drupal that doesn't have .ht.router.php
The coveralls error is interesting. This PR increases coverage from ~7% to ~32% if I'm not mistaken. |
src/Resolvers/PatchesFile.php
Outdated
$patches = json_decode($patches, true); | ||
|
||
// First, check for JSON syntax issues. | ||
$error = json_last_error(); |
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.
Seems like you could use php's json_last_error_msg() and skip this whole switch/case statement below.
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.
Good call. The current plugin has this big switch/case statement because it supports earlier versions of PHP. I'm only planning on supporting 5.6+ for 2.0.0, so json_last_error_msg() sounds great :) Thanks for catching that!
This enables users to extend patch resolution in their own plugin. This would allow someone to essentially define their own logic for finding patches to apply to a package.
A lot of this code was ported out of the 2.x-prototype branch.
Remaining tasks:
checkPatches()
working again. Since we're now resolving patches before the first package is installed/updated, checkPatches() doesn't have any of that data. The obvious solution is to run patch resolution earlier, but that might be tricky.