Page MenuHomePhabricator

Create GuzzleHttpRequest class as new default for HttpRequestFactory
Closed, ResolvedPublic

Description

Create a GuzzleHttpRequest class using the external Guzzle (docs.guzzlephp.org) library. This will be the new default request type, but CurlHttpRequest and PhpHttpRequest will remain available and accessible via Http::$httpEngine

This has several positives, including:

  1. increases parity and consistency between mediawiki installations with and without curl installed (Guzzle automatically falls back to php streams if curl is unavailable)
  1. brings Guzzle into the codebase, making it available to other areas that might benefit from it (ex. MultiHttpClient)
  1. provides a path to reducing the amount of custom code we must maintain (we could deprecate and eventually remove CurlHttpRequest and PhpHttpRequest)

It also has a few negatives, including:

  1. at least temporarily, adds additional code without removing any
  1. ties us to external code over which we have less control

This task flows from discussion in T110022 and T139169 and is hopefully a big step toward addressing T137926. (MultiHttpClient still needs addressed, presumably also via Guzzle. I intend to do that under a separate task.)

Details

Event Timeline

BPirkle created this task.Aug 17 2018, 3:57 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 17 2018, 3:57 AM

I have a complete implementation of GuzzleHttpRequest coded and tested locally, including a phpunit test. (Wanted to make sure it was going to work out well before I created the official task.)

Does https://www.mediawiki.org/wiki/Manual:External_libraries describe the steps I need to take before adding a new external library to mediawiki? In particular, do I need to request a security review before posting a Gerrit change?

I have a complete implementation of GuzzleHttpRequest coded and tested locally, including a phpunit test. (Wanted to make sure it was going to work out well before I created the official task.)
Does https://www.mediawiki.org/wiki/Manual:External_libraries describe the steps I need to take before adding a new external library to mediawiki? In particular, do I need to request a security review before posting a Gerrit change?

Feel free to upload whatever you want to Gerrit, we just can't merge anything that depends upon the new library until the security review is finished.

Change 453408 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Create GuzzleHttpRequest class as new default for HttpRequestFactory

https://gerrit.wikimedia.org/r/453408

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptAug 20 2018, 8:18 PM
Reedy added a subscriber: Reedy.Dec 4 2018, 1:25 PM

So doing some poking around here... We end up with a few extra other libraries, which is fine, just a bit more poking around needed.

Vendor patching incoming as I'm going through stuff

(we might also want to pin some more of these versions in mediawikis composer.json too)

reedy@ubuntu64-web-esxi:/var/www/wiki/mediawiki/vendor$ composer update --no-dev
Loading composer repositories with package information
Updating dependencies
Package operations: 5 installs, 0 updates, 0 removals
  - Installing guzzlehttp/promises (v1.3.1): Downloading (100%)         
  - Installing ralouphie/getallheaders (2.0.5): Downloading (100%)         
  - Installing psr/http-message (1.0.1): Downloading (100%)         
  - Installing guzzlehttp/psr7 (1.5.0): Downloading (100%)         
  - Installing guzzlehttp/guzzle (6.3.3): Downloading (100%)         
Writing lock file
Generating optimized autoload files
reedy@ubuntu64-web-esxi:/var/www/wiki/mediawiki/vendor$

Change 477538 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/vendor@wmf/1.33.0-wmf.6] Add guzzlehttp/guzzle 6.3.3 and dependancies

https://gerrit.wikimedia.org/r/477538

My main question about bringing in Guzzle is how we're going to apply the deprecation policy to this. Typically we've said that extensions can depend upon any library that MediaWiki core depends upon, since we're going to keep back-compat or cautiously update external libraries. But the size of Guzzle is much much larger than any other library that core depends upon to date. If Guzzle makes a breaking change, I'm imagining a mess if we need to update every thing that makes HTTP requests.

My thinking right now is that we somehow/somewhere declare that Guzzle isn't part of the stable interface, and extensions need to use the MW abstraction layer instead (this seems like a good idea regardless given T202143#4802539). Does that seem reasonable?

I also wanted to say that I found the Guzzle codebase relatively well-written. My full comments/thoughts are:

  • It would be nice if we didn't have any autoload files and everything was class loaded. E.g. for Guzzle there's no reason the default_user_agent() function couldn't be static on a class.
  • Having CodeSniffer run upstream (https://github.com/guzzle/guzzle/pull/2194) would be nice too, would make it easy for some minor things I noticed like php_sapi_name() instead of PHP_SAPI.
  • .gitattributes exclusions, but Reedy already sent PRs to everyone
  • PHP only has the getallheaders function in the Apache and FPM (since 7.3) SAPIs so it needs a polyfill. Maybe we can ask PHP to provide the function on all SAPIs where it does nothing so it doesn't need a userland polyfill?
Reedy added a comment.Dec 6 2018, 10:37 AM

My thinking right now is that we somehow/somewhere declare that Guzzle isn't part of the stable interface, and extensions need to use the MW abstraction layer instead (this seems like a good idea regardless given T202143#4802539). Does that seem reasonable?

I think so. Or if they use it directly, MW isn't responsible for breaking changes and therefore won't accept bug reports

Reedy added a comment.Dec 6 2018, 10:41 AM

And for your bullet points, we can hopefully grab the little improvements in point releases in the near future

There's definitely some annoyances, but I don't see anything too major to not continue with doing this

TheDJ added a subscriber: TheDJ.Dec 6 2018, 12:55 PM
TheDJ added a comment.Dec 6 2018, 1:17 PM

@Legoktm on the deprecation policy and Guzzle support.. I point at PSR-17 and PSR-18 and I think that that is what we need to keep in mind in terms of any breaking changes and support.

So maybe have one new interface that is compatible with upcoming php interfaces and those psr's should be the only contract we promise to deliver to extensions. Either use that and be supported or use guzzle directly yourself and be responsible for any of your breakages.

Change 477538 merged by jenkins-bot:
[mediawiki/vendor@master] Add guzzlehttp/guzzle 6.3.3 and dependancies

https://gerrit.wikimedia.org/r/477538

Change 453408 merged by jenkins-bot:
[mediawiki/core@master] Create GuzzleHttpRequest class as new default for HttpRequestFactory

https://gerrit.wikimedia.org/r/453408

BPirkle closed this task as Resolved.Dec 10 2018, 3:57 PM