Updated: Comment #9
Problem/Motivation
Guzzle development was very active since we last updated, the most important change for us right now is a fix that allows us to deal with #1991298: Fatal error when installing Drupal on servers that don't have the cURL extension (critical bug).
Less urgent but very important in the long-term is that we need a version of Guzzle that is going to be supported long enough for us.
The downside is that this introduces a number of BC breaks, see comment #5 for the detailed explanation and interdiff for the changes that affect our current usage.
Proposed resolution
Update to the latest stable 3.7.* version. @mtdowling, the Guzzle maintainer, confirmed in #3 that 3.7 is the much better version to lock into and that he can support it as a LTS release (although it has yet to be defined what that exactly means).
Remaining tasks
Get an OK from a core maintainer about the API change. Possibly define with the security team how long this needs to be supported (Necessary anyway, also for other vendor-code, so can be a separate issue).
User interface changes
None.
API changes
See comment #5 and Guzzle's upgrading documentation.
Related Issues
#1991298: Fatal error when installing Drupal on servers that don't have the cURL extension
Original report by Yaron Tal
In order to solve #1991298: Fatal error when installing Drupal on servers that don't have the cURL extension we need to update Guzzle. I think the impact of this might be, quite big, so if someone with more knowledge of this could jump in?
The patch for Guzzle that solves our problem:
https://github.com/guzzle/guzzle/commit/e3470f68606041172b08d7eab828bb92...
The oldest release which includes this patch is 3.6.0. We currently use 3.1.* according to the composer.json in the Drupal root.
Comment | File | Size | Author |
---|---|---|---|
#5 | guzzle-update-2052923-5.patch | 387.28 KB | Berdir |
#5 | guzzle-update-2052923-5-interdiff.txt | 8.61 KB | Berdir |
#1 | guzzle-update-2052923-1.patch | 378.67 KB | Berdir |
Comments
Comment #1
BerdirWoah, that guy is moving fast ;)
Two possibilities.
- We test an upgrade, check for API changes and if there are none that affect us, we should be able to upgrade.
- If there's a problem, I'm quite sure that it wouldn't be a problem to coordinate a new 3.1.x release with that patch applied.
So, here's an attempt at the first option, updating to 3.7.x.
From looking at the changelog, the biggest change for us is that getPreviousResponse() is deprecated, which actually means no longer implemented. We use that in at least two places. However, there's a nice new getEffectiveUrl() method now, which seems to be much easier to use, so that might be an acceptable change?
Expecting some tests fails in aggregator and possibly locale.
Comment #2
BerdirAs mentioned to @alexpott in IRC, we might want to check with @mtdowling that we use a Guzzle version in core that is at least minimally maintained (security fixes) while 8.x is supported, which is going to be a looong time.
See http://symfony.com/doc/current/contributing/community/releases.html for references (especially the long term support releases part).
I'll see if I can get @mtdowling in here, that worked fine in the past...
Comment #3
mtdowling CreditAttribution: mtdowling commentedHow long will Drupal 8 depend on third-party libraries to maintain a specific version? It looks like Symfony is only supporting LTS releases for 3 years with 4 years of security fixes. Is that what is expected of a Guzzle LTS release?
Locking into the 3.7 version of Guzzle is a much better idea than anything before 3.7. I can mark that as the LTS branch for Guzzle that Drupal can depend on.
Comment #5
BerdirOk, this should fix the test fails.
Here's what I had to change:
- POST headers don't seem to default to application/x-www-form-urlencoded when you pass in a string. It however does when you pass in an array. Only one test failed that was missing the explicit header, but I unified all of them.
- It looks like the if-modified-since handling now actually works and responds with an 304 Not Modified and empty body, so I had to catch that.
- The biggest change is how we detect redirects. Previously, we had to get the previous response and check the status there and get the location from it. Now there's a new getEffectiveUrl() and we can just get that. So much easier, but it is an API change.
Comment #6
BerdirEdit: Wrong issue. (Posted something in here that doesn't belong here)
Comment #7
BerdirTagging.
Comment #8
BerdirUpdating to base system and setting to major, as this is currently blocking a critical bug.
Comment #8.0
Berdirmade the issue id a link
Comment #9
alexpottI think we have to do this. We don't want Drupal 8 to be stuck on outdated versions of software before it's even released. Also there are no Drupal API changes in this only Guzzle changes so I'm not sure this even comes under own API freeze process. This is an interesting question to which I'm not sure I know the answer but my gut feeling is that if we do not have to change a Drupal API then it should not be subject to our API freeze process.
The changes look good to me. Assigning to catch for a second opinion on API changing with respect to vendor.
Comment #10
catchAs far as I'm concerned we should update external libraries to the latest stable right up until 8.0 so we're not shipping with something that's already out of date.
Post 8.0 I'd still like to update if possible - but that depends on the BC policy of the library.
Haven't reviewed the patch yet so unassigning.
Comment #11
Yaron Tal CreditAttribution: Yaron Tal commentedI don't know about always upgrading to the latest stable.
mtdowling said he can make 3.7 an LTS release (which is awesome), we shouldn't jump on 3.8 if it releases before 8.0 is out. Unless Guzzle 3.8 could become the LTS version, I think we should stick to 3.7 to make sure we don't have an unmaintained version of an external library in core.
Comment #12
webchickSeems like this still needs discussion.
Comment #13
BerdirI'm not sure what kind of discussion :)
I think everyone agrees that updating to 3.7 is the correct thing to do. The only question above was if we should continue to update. And I agree with Yaron Tal that we should stick with 3.7 given that it becomes an LTS version.
But that this should go in doesn't need discussion IMHO.
Comment #14
webchickOops, sorry! I misread the latest comments.
Committed and pushed to 8.x. Thanks!
Comment #15
BerdirCreated a change notice: https://drupal.org/node/2061915
Comment #16.0
(not verified) CreditAttribution: commentedUpdated issue summary