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.

#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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
378.67 KB

Woah, 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.

Berdir’s picture

As 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...

mtdowling’s picture

How 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.

Status: Needs review » Needs work

The last submitted patch, guzzle-update-2052923-1.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.61 KB
387.28 KB

Ok, 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.

Berdir’s picture

Issue tags: -API change

Edit: Wrong issue. (Posted something in here that doesn't belong here)

Berdir’s picture

Issue tags: +API change

Tagging.

Berdir’s picture

Component: other » base system
Priority: Normal » Major
Issue tags: +API change

Updating to base system and setting to major, as this is currently blocking a critical bug.

Berdir’s picture

Issue summary: View changes

made the issue id a link

alexpott’s picture

Assigned: Unassigned » catch
Status: Needs review » Reviewed & tested by the community
Issue tags: +Approved API change

I 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.

catch’s picture

Assigned: catch » Unassigned

As 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.

Yaron Tal’s picture

I 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Seems like this still needs discussion.

Berdir’s picture

I'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.

webchick’s picture

Status: Needs review » Fixed

Oops, sorry! I misread the latest comments.

Committed and pushed to 8.x. Thanks!

Berdir’s picture

Created a change notice: https://drupal.org/node/2061915

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary