RESTTestBase::httpRequest() should work with HEAD requests so that one don't need to override it just to test custom REST services.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dixon_’s picture

Title: Undefined variable causes test failures in RESTTestBase » Make RESTTestBase::httpRequest() work with HEAD requests
Category: Bug report » Feature request
Issue summary: View changes
FileSize
1.17 KB

Turns out there are more things missing to make HEAD requests work with RESTTestBase::httpRequest().

Updated issue summary and attaching new patch.

Status: Needs review » Needs work

The last submitted patch, 1: 2274153-resttestbase-head-request-1.patch, failed testing.

dixon_’s picture

Status: Needs work » Needs review
FileSize
1.22 KB

Rerolled the patch and added the possibility of passing in query parameters (the same way as when using RESTTestBase::httpRequest() with GET requests).

dixon_’s picture

Status: Needs review » Needs work

The last submitted patch, 3: 2274153-resttestbase-head-request-2.patch, failed testing.

dixon_’s picture

Status: Needs work » Needs review
dawehner’s picture

RESTTestBase::httpRequest() should work with HEAD requests so that one don't need to override it just to test custom REST services.

Are there usecases for that in core, like when testing the entity services?

jhedstrom’s picture

There is a tiny bit of reference to HEAD requests in RequestBase and CSRFAccessCheck, and without this patch, that code is presumably untestable?

klausi’s picture

Status: Needs review » Needs work
+++ b/core/modules/rest/src/Tests/RESTTestBase.php
@@ -92,6 +93,17 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
+        case 'HEAD':
+          $options = isset($body) ? array('absolute' => TRUE, 'query' => $body) : array('absolute' => TRUE);

hm, we are not doing that for GET requests, putting the $body on the URL as query parameters. So this should have at least a comment?

And this does not apply anymore.

Wim Leers’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
FileSize
1.12 KB
913 bytes

#8: those references are no longer there.

Wim Leers’s picture

dawehner’s picture

Seams reasonable. On the longrun it would be nice to just use guzzle here. I think this would be simply more readable.

Wim Leers’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

.

  • alexpott committed ac0150a on 8.2.x
    Issue #2274153 by dixon_, Wim Leers: Make RESTTestBase::httpRequest()...

  • alexpott committed 5971c4b on 8.1.x
    Issue #2274153 by dixon_, Wim Leers: Make RESTTestBase::httpRequest()...

  • alexpott committed 13b4525 on 8.0.x
    Issue #2274153 by dixon_, Wim Leers: Make RESTTestBase::httpRequest()...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 13b4525 and pushed to 8.0.x, 8.1.x, 8.2.x (as this is a test-only change). Thanks!

Status: Fixed » Closed (fixed)

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