Use drupal_http_build_query() in drupal_http_request()

Hi Liam,

Do you mind explaining why we would need drupal_http_build_query() instead of http_build_query() ?

I see in the documentation that the difference are:

This (drupal_http_build_query()) differs from http_build_query() as we need to rawurlencode() (instead of urlencode()) all query parameters.

Do you mind let us know why we should change ? It worked like this since ages... why this sudden change now ?

Fair enough :-)

I will move this things forward.

As this is a bug and tests are changed, this will need a test.

Hi Liam,

I had a talk with Fabian.

Fabian meant that you should provide a patch only with a test that is failing, and then, another patch with your changes and tests should pass.

Thanks Liam !

I think it would have been nice to create a new tests that is failing without any code modification.
Then, include that new test in the final patch that will contain the code update AND the tests.

Hi Liam,

Any news on this ?

@Liam Morland your final patch should include both the fix and the tests.

The first tests-only patch is only needed to verify that the tests will catch the problem. The final patch (which includes the tests) is the one that will be committed.

Hiding and reordering files to bring #2 to the top.

#2 is pending Drupal 7 commit.

I missed that we just recently introduced http_build_query().

That changes the scope definitely.

To increase our confidence in the test results for the above issue, these two should go in first so we can re-queue php 7.3 and php 5.3 tests.
#3047844: [Regression] Tests fail on PHP 5.3
#3025335: session_id() cannot be changed after session is started

This info is already in this issue, but I also didn't pick up the full context / backstory initially.

To make it really explicit; we introduced support for passing data as an array to drupal_http_request() fairly recently in #2814031: In drupal_http_request(), allow passing data as array (which is already marked as the parent of this issue).

However, as @Liam Morland pointed out in #19 of that issue:

[the new code] probably should have [used] drupal_http_build_query() instead of http_build_query().

So what we're doing in this issue is fixing that oversight.

The patch (#2) looks good to me, but one thing I wasn't sure about in the changes to the test:

     // Fetch page and check that the data parameter works with both array and string.
-    $data_array = array($this->randomName() => $this->randomName());
-    $data_string = http_build_query($data_array);
+    $data_array = array($this->randomName() => $this->randomString() . ' "\'');
+    $data_string = drupal_http_build_query($data_array);

I can see we're adding a space character because the two different functions will encode that differently, but why are we adding the two types of quotes to the end of the value here?

/path/to/drupal-7.x$ drush php
Psy Shell v0.9.9 (PHP 7.2.24-0ubuntu0.18.04.1 — cli) by Justin Hileman
>>> $test = array('foo' => 'bar "\'');
=> [
     "foo" => "bar "'",
   ]
>>> http_build_query($test);
=> "foo=bar+%22%27"
>>> drupal_http_build_query($test);
=> "foo=bar%20%22%27"

Doesn't look like they do any harm, I'm just curious whether I'm missing a reason why we need to add them?

Also, PHP 7.3 tests should pass now, so I'll queue a retest.

RTBM - let’s get #2 in.

I agree that we expand the test scope here for the sole reason to see that the new function does not Regress something with quotes.

Test hardening.

Thank you contributors!