Follow-up of #1447736: Adopt Guzzle library to replace drupal_http_request().
We should create separate issues for each module/thing that uses drupal_http_request() I think and then finally remove it. Filling as major, but it needs to happen unless we want to ship with both drupal_http_request() and the guzzle library.
Suggested list of issues, based on grep -Rl drupal_http_request
:
- #1866124: Convert drupal_http_request usage in openid.module to Guzzle
- #1862512: Convert drupal_http_request usage in xmlrpc.module to Guzzle
- #1912590: Convert drupal_http_request usage in WebTestBase.php to Guzzle
- #1912594: Convert drupal_http_request usage in system.module to Guzzle
- #1867382: Convert drupal_http_request usage in statistics.module to Guzzle
- #1862524: Convert drupal_http_request usage in aggregator.module to Guzzle
- #1862538: Convert drupal_http_request usage in update.fetch.inc to Guzzle
- #1912568: Convert drupal_http_request usage in SearchRankingTest.php to Guzzle
- #1875614: Convert drupal_http_request usage in locale.batch.inc to Guzzle
- #1887046: Convert drupal_http_request usage in install.core.inc to Guzzle.
and then finally the removal of the function, including the complete removal of HttpRequestTest.php I guess, unless we want to rename and keep that to ensure that we're not introducing any bugs? (Maybe just port it to make sure and report any issues upstream including fixes and test coverage).
Comment | File | Size | Author |
---|---|---|---|
#35 | remove-drupal-http-request-1862398-35.patch | 27.61 KB | Berdir |
#35 | remove-drupal-http-request-1862398-35-interdiff.txt | 1.48 KB | Berdir |
#25 | remove-drupal-http-request-1862398-25.patch | 26.08 KB | Berdir |
Comments
Comment #1
BerdirComment #1.0
BerdirUpdated issue summary.
Comment #1.1
BerdirUpdated issue summary.
Comment #1.2
BerdirUpdated issue summary.
Comment #1.3
BerdirUpdated issue summary.
Comment #1.4
BerdirUpdated issue summary.
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedIn http://drupal.org/node/1862446#comment-6828664, JeremyFrench asked whether we can keep drupal_http_request() as a convenient wrapper function to Guzzle, for cases where a synchronous request is ok. I think this was somewhat discussed in #1447736: Adopt Guzzle library to replace drupal_http_request() and spin-off issues, but I forget the key reasons why this was considered problematic. Can someone summarize those?
Comment #3
BerdirI'm not sure:
- A question is how simple it should be. Should it support support different http methods, for example? I would suggest no. Even if we would, I don't think we want to keep it backwards compatible with all the possible options that we currently support there.
- I assume that function should return a guzzle response object, which also means exceptions instead of $response->error, which means that all usage of drupal_http_request() would need to be changed/updated anyway.
- Given the above two points, we might want to add a completely new function instead.
- The function shouldn't be used by services which get their stuff injected I assume, using the http client is easy enough for them anyway.
The most simple wrapper function would do this:
Which means a single-line function. Maybe even without send() as this means that there is no way to do something special with the request object. And calls to this aren't countless and equally simple as state() for example.
Such a wrapper function would solve exactly two problems, discoverability for developers and IDE integration (method autocomplete for the client, request and response objects), both are drupal_container() problems*, not guzzle problems. Guzzle itself is easy enough to use, once you know where to find it.
* For which we should find a solution...
Comment #4
BerdirOne thing that I don't get is why the service is called http_*default*_client. Nothing else uses default_something. I'd suggest to rename that to http_client, which would already help with finding it I guess.
Comment #5
Crell CreditAttribution: Crell commentedI agree with http_client. As for keeping drupal_http_request(), I think Berdir already laid out why it's not worthwhile. The API would have to change anyway, and all it does is shorten one line of code while making people have to keep track of two API calls instead of one. But we're trying to eliminate those wrapper functions anyway, because they're a hard-dependency and not injectable and break testing, so really, let's just bite the bullet and use Guzzle directly.
The solution to the "drupal_container() problem" above is to not use it, and get injected objects in the first place from the DIC. Then everything type hints nicely.
Comment #6
rbayliss CreditAttribution: rbayliss commentedI think the reason I had called it http_default_client was because Guzzle clients have some pretty powerful features on their own, and we might want to have additional clients in the future. As an example, an update client could have a URL template for the Drupal.org project update urls, as well as response caching and parsing features. But I guess it still makes sense to rename to http_client for the 90% use case.
Comment #7
BerdirRight. But that's not going to happen for 8.x. We already have ~300 references to that function, 60 of them in .module files. These calls will not go away, we will add more of them. And even if you define services and pass the arguments to them through a MyModuleBundle, you still need to know what services exist and how they are named. So for example, the very least thing we need is a documentation page that lists what kind of services are provided by core, with a reference to the interface. Maybe that can even be auto-generated somehow?
Comment #8
Berdir@rbayliss: Yes, that's a problem I've thought about too, see for example what I've done in #1599622: Run poormanscron via a non-blocking HTTP request, to execute a asyonchronous request, I had to add a plugin and then remove it again, to not influence other calls. Please comment in there if there is a better way but not that this is core/system functionality, so adding a special client would mean that we have to add it in CoreBundle, just for this feature... ?
Comment #9
rbayliss CreditAttribution: rbayliss commentedI just commented on that issue. Basically, you can add plugins at the client or the request level. Each request gets a cloned version of the client's event dispatcher. The only reason I can see that you'd need a client level plugin is if you want that plugin active for every request the client makes (Simpletest for example). WRT multiple specialized clients, I realized I'm talking about service clients, which have been split into a separate composer project that we aren't pulling in, so maybe the possibility of multiple clients in core is a moot point right now.
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedIf that were needed, it wouldn't be so bad. Since we compile the DIC, service definitions are pretty cheap. Getting into the thousands might start introducing noticeable overhead, but until then, we should be ok. For things only needed by system.module, we can add it to SystemBundle instead for better organization.
Comment #11
Crell CreditAttribution: Crell commentedBerdir: Sure, I'm not suggesting that drupal_container() can die in Drupal 8. It must live at least as long as hooks do. But the problem of not having a type hinted return value is, basically, the cost of using procedural code. (There are other documentation ways to let some IDEs know about those sort of return values, but that's OT here.)
The Symfony DIC is fully introspectable so we can absolutely build an index of registered services. Symfony full stack even has a CLI utility that does so for you.
Comment #11.0
Crell CreditAttribution: Crell commentedUpdated issue summary.
Comment #12
YesCT CreditAttribution: YesCT commentedJust checking to make sure that the issues in the issue summary can continue.
When they are done, the course of action is still to then remove the function (and file) in this issue?
Next steps (can be done concurrently):
Comment #13
BerdirMakes sense to wait for #1875792: Standardize Guzzle exception handling first (which in turn needs #1875818: Update composer.json to use the latest stable 3.0.x version of Guzzle, which needs a new guzzle release) or we have to update all patches again anyway.
Also not sure if the remaining parts need separate issues. SearchRank test probably, WebTestBase is just a reference and system_file_retrieval() is a single use, can be done here as well I think.
Comment #14
tstoecklerI didn't open a new sub-issue for that yet, but #1848490: Import translations automatically during installation just added a couple usages of drupal_http_request() to install.core.inc.
Comment #15
Crell CreditAttribution: Crell commentedAnd we now have a clear case for why #1876842: [policy, no patch] Use @deprecated to indicate a deprecated method or function is necessary. :-)
Comment #16
Sutharsan CreditAttribution: Sutharsan commented@tstoeckler, I admit, it was my issue. I think I overlooked it, although I can't deny I was aware of the Guzzle migration ;) Find the issue at #1887046: Convert drupal_http_request usage in install.core.inc to Guzzle. (added it to the issue summary as well)
Comment #16.0
Sutharsan CreditAttribution: Sutharsan commentedIncluded issue to convert to Guzzle in locale.batch.inc
Comment #17
gabriel.achille CreditAttribution: gabriel.achille commentedcreating separate issues...
Comment #17.0
gabriel.achille CreditAttribution: gabriel.achille commentedAdded sub-issue #1887046
Comment #17.1
gabriel.achille CreditAttribution: gabriel.achille commentedadding sub-issue #1912568: Convert drupal_http_request usage in SearchRankingTest.php to Guzzle
Comment #18
gabriel.achille CreditAttribution: gabriel.achille commentedFor me all remaining issues have been created. (issue summary updated)
Comment #19
YesCT CreditAttribution: YesCT commented@gabriel.achille thanks!
Comment #20
chx CreditAttribution: chx commentedSo, what's the equivalent of request headers in Guzzle?
Edit: https://github.com/guzzle/guzzle-docs/pull/28 grrr.
Comment #21
mikeytown2 CreditAttribution: mikeytown2 commented@chx
http://guzzlephp.org/tour/http.html#request-and-response-headers
Comment #22
chx CreditAttribution: chx commentedThat does not cover the post() call...
Comment #23
Berdir#1875792: Standardize Guzzle exception handling finally happened. I want to push in a Drupal::httpClient() first in #1937600: Determine what services to register in the new Drupal class then I'll try to get back to those issues here.
Comment #23.0
Berdiradding sub-issues 1912590 and 1912594
Comment #24
BerdirBump. Re-rolled most of the patches linked in the issue summary and reviewed two that should be easy to re-roll. Please review :)
Comment #25
BerdirThis should pass once everything is converted.
Comment #25.0
BerdirUpdated issue summary.
Comment #27
Berdir#25: remove-drupal-http-request-1862398-25.patch queued for re-testing.
Comment #28.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #29
webchickOk, AFAIK all Guzzle conversions are complete. Re-testing #25.
Comment #30
webchick#25: remove-drupal-http-request-1862398-25.patch queued for re-testing.
Comment #32
Berdir#25: remove-drupal-http-request-1862398-25.patch queued for re-testing.
Comment #34
BerdirLooks like statistics views integration added a new call to drupal_http_request() :(
I'll fix that in here.
Comment #35
BerdirComment #37
BerdirTestbot, you no like me?!
Also noticed this in the log:
Looks like our PHPUnit Tests are so broken that we don't even get informed about it ;)
Comment #38
Berdir#35: remove-drupal-http-request-1862398-35.patch queued for re-testing.
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentedgood riddance
Comment #40
Crell CreditAttribution: Crell commentedrootawc: The PHPUnit fail above is still in the log. :-(
Related: #1963022: Problem with PHPUnit test results when using @dataProvider
Comment #41
BerdirThat fail has nothing to do with this issue, I it seems to be in every test run, you can also see it on http://qa.drupal.org/8.x-status, opened #1969406: PHPUnit Test asserting logging broken.
Comment #42
Crell CreditAttribution: Crell commentedAhso! Well, in that case, please carry on!
Comment #43
webchickAll right! :D
Committed and pushed to 8.x. Thanks!
This'll need a change notice. So long, drupal_http_request(). Sniff. We knew thee well...
Comment #44
BerdirYay!
This issue just completd the job, we already have http://drupal.org/node/1862446, added this issue to the references.
Comment #45.0
(not verified) CreditAttribution: commentedUpdated issue summary.