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:

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Component: taxonomy.module » base system
Berdir’s picture

Issue summary: View changes

Updated issue summary.

Berdir’s picture

Issue summary: View changes

Updated issue summary.

Berdir’s picture

Issue summary: View changes

Updated issue summary.

Berdir’s picture

Issue summary: View changes

Updated issue summary.

Berdir’s picture

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

In 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?

Berdir’s picture

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

function the_wrapper($url, $headers = array()) {
  retun drupal_container()->get('http_default_client')->get($url, $headers)->send();
}

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

Berdir’s picture

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

Crell’s picture

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

rbayliss’s picture

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

Berdir’s picture

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.

Right. 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?

Berdir’s picture

@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... ?

rbayliss’s picture

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

effulgentsia’s picture

we have to add it in CoreBundle

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

Crell’s picture

Berdir: 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.

Crell’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue tags: +Novice

Just 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):

  • (Novice to) open the remaining issues needed (see the issue summary bullet points that dont have issue links yet). Look at the other issues for a pattern to follow (tips might be helpful in contributor task for issue summary: http://drupal.org/node/1427826)
  • Work on the individual issues in the issue summary
Berdir’s picture

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

tstoeckler’s picture

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

Crell’s picture

Sutharsan’s picture

@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)

Sutharsan’s picture

Issue summary: View changes

Included issue to convert to Guzzle in locale.batch.inc

gabriel.achille’s picture

Assigned: Unassigned » gabriel.achille

creating separate issues...

gabriel.achille’s picture

Issue summary: View changes

Added sub-issue #1887046

gabriel.achille’s picture

gabriel.achille’s picture

Assigned: gabriel.achille » Unassigned

For me all remaining issues have been created. (issue summary updated)

YesCT’s picture

@gabriel.achille thanks!

chx’s picture

So, what's the equivalent of request headers in Guzzle?

Edit: https://github.com/guzzle/guzzle-docs/pull/28 grrr.

mikeytown2’s picture

chx’s picture

That does not cover the post() call...

Berdir’s picture

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

Berdir’s picture

Issue summary: View changes

adding sub-issues 1912590 and 1912594

Berdir’s picture

Issue tags: -Novice

Bump. Re-rolled most of the patches linked in the issue summary and reviewed two that should be easy to re-roll. Please review :)

Berdir’s picture

Status: Active » Needs review
FileSize
26.08 KB

This should pass once everything is converted.

Berdir’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, remove-drupal-http-request-1862398-25.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, remove-drupal-http-request-1862398-25.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

webchick’s picture

Ok, AFAIK all Guzzle conversions are complete. Re-testing #25.

webchick’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, remove-drupal-http-request-1862398-25.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, remove-drupal-http-request-1862398-25.patch, failed testing.

Berdir’s picture

Looks like statistics views integration added a new call to drupal_http_request() :(

I'll fix that in here.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, remove-drupal-http-request-1862398-35.patch, failed testing.

Berdir’s picture

Testbot, you no like me?!

Also noticed this in the log:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'line' at row 3: INSERT INTO {simpletest} (test_id, test_class, status, message, message_group, function, line, file) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7), (:db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14, :db_insert_placeholder_15), (:db_insert_placeholder_16, :db_insert_placeholder_17, :db_insert_placeholder_18, :db_insert_placeholder_19, :db_insert_placeholder_20, :db_insert_placeholder_21, :db_insert_placeholder_22, :db_insert_placeholder_23), (:db_insert_placeholder_24, :db_insert_placeholder_25, :db_insert_placeholder_26, :db_insert_placeholder_27, :db_insert_placeholder_28, :db_insert_placeholder_29, :db_insert_placeholder_30, :db_insert_placeholder_31); Array
(
    [:db_insert_placeholder_0] => 828
    [:db_insert_placeholder_1] => Drupal\Tests\Component\PhpStorage\FileStorageTest
    [:db_insert_placeholder_2] => pass
    [:db_insert_placeholder_3] =>
    [:db_insert_placeholder_4] => Other
    [:db_insert_placeholder_5] => Drupal\Tests\Component\PhpStorage\FileStorageTest->testCRUD()
    [:db_insert_placeholder_6] => 42
    [:db_insert_placeholder_7] => /var/lib/drupaltestbot/sites/default/files/checkout/core/tests/Drupal/Tests/Component/PhpStorage/FileStorageTest.php
    [:db_insert_placeholder_8] => 828
    [:db_insert_placeholder_9] => Drupal\Tests\Component\PhpStorage\FileStorageTest
    [:db_insert_placeholder_10] => pass
    [:db_insert_placeholder_11] =>
    [:db_insert_placeholder_12] => Other
    [:db_insert_placeholder_13] => Drupal\Tests\Component\PhpStorage\FileStorageTest->testReadOnly()
    [:db_insert_placeholder_14] => 51
    [:db_insert_placeholder_15] => /var/lib/drupaltestbot/sites/default/files/checkout/core/tests/Drupal/Tests/Component/PhpStorage/FileStorageTest.php
    [:db_insert_placeholder_16] => 828
    [:db_insert_placeholder_17] =>
    [:db_insert_placeholder_18] => pass
    [:db_insert_placeholder_19] =>
    [:db_insert_placeholder_20] => Other
    [:db_insert_placeholder_21] => ->Drupal\Tests\Component\Utility\StringTest::testCheckPlain()
    [:db_insert_placeholder_22] =>
    [:db_insert_placeholder_23] =>
    [:db_insert_placeholder_24] => 828
    [:db_insert_placeholder_25] =>
    [:db_insert_placeholder_26] => pass
    [:db_insert_placeholder_27] =>
    [:db_insert_placeholder_28] => Other
    [:db_insert_placeholder_29] => ->Drupal\Tests\Component\Utility\StringTest::testFormat()
    [:db_insert_placeholder_30] =>
    [:db_insert_placeholder_31] =>
)
 in Drupal\Core\Database\Connection->query() (line 554 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php).].

Looks like our PHPUnit Tests are so broken that we don't even get informed about it ;)

Berdir’s picture

Status: Needs work » Needs review
ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

good riddance

Crell’s picture

Status: Reviewed & tested by the community » Needs work

rootawc: The PHPUnit fail above is still in the log. :-(

Related: #1963022: Problem with PHPUnit test results when using @dataProvider

Berdir’s picture

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

Crell’s picture

Status: Needs work » Reviewed & tested by the community

Ahso! Well, in that case, please carry on!

webchick’s picture

Title: [meta] Replace drupal_http_request() with Guzzle » Change notice: [meta] Replace drupal_http_request() with Guzzle
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

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

Berdir’s picture

Title: Change notice: [meta] Replace drupal_http_request() with Guzzle » [meta] Replace drupal_http_request() with Guzzle
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs change record

Yay!

This issue just completd the job, we already have http://drupal.org/node/1862446, added this issue to the references.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.