Optimize UrlHelper::buildQuery() to use http_build_query()

1: 2248257-1.patch queued for re-testing.

1. +++ b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php
   @@ -54,8 +55,8 @@ public function providerTestBuildQuery() {
   +    $this->assertEquals(http_build_query($query, '', '&', PHP_QUERY_RFC3986), UrlHelper::buildQuery($query), $message);
   

   How is this asserting *anything*? It's just the implementation compared to the method using that implementation. The $expected should not be removed, IMO

2. +++ b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
   @@ -297,16 +297,16 @@ public function testPathBasedURLGeneration() {
   -        $url = $base . 'node/123?foo';
   ...
   +        $url = $base . 'node/123?foo=';
   ...
   -        $url = $base . 'node/123?foo#bar';
   ...
   +        $url = $base . 'node/123?foo=#bar';
   

   Isn't this the entire reason why we have this implementation? Those are ugly.

1) Yeah, so I mentioned we should remove it since we don't want to be in the business of testing php's code. That's the job of php's test suite. However if we're concerned about coverage then we could do that. It basically just confirms we're calling it correctly I guess... I'm not really sure how to test this to be honest.

2) Yeah... those are ugly. Cleaning them up seems likely to be a beast though since we get back a string.

Looking at the changes it seems we're also introducing a "regression" in not supporting array('foo' => NULL) and requiring array('foo' => ''). I'm actually OK with this unless there are some deeper implications I can't think of.

1. +++ b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php
   @@ -34,11 +34,12 @@ public static function getInfo() {
   -      array(array('a' => ' &#//+%20@۞'), 'a=%20%26%23//%2B%2520%40%DB%9E', 'Value was properly encoded.'),
   -      array(array(' &#//+%20@۞' => 'a'), '%20%26%23%2F%2F%2B%2520%40%DB%9E=a', 'Key was properly encoded.'),
   -      array(array('a' => '1', 'b' => '2', 'c' => '3'), 'a=1&b=2&c=3', 'Multiple values were properly concatenated.'),
   -      array(array('a' => array('b' => '2', 'c' => '3'), 'd' => 'foo'), 'a[b]=2&a[c]=3&d=foo', 'Nested array was properly encoded.'),
   -      array(array('foo' => NULL), 'foo', 'Simple parameters are properly added.'),
   +      array(array('a' => ' &#//+%20@۞'), 'Value was properly encoded.'),
   +      array(array(' &#//+%20@۞' => 'a'), 'Key was properly encoded.'),
   +      array(array('a' => '1', 'b' => '2', 'c' => '3'), 'Multiple values were properly concatenated.'),
   +      array(array('a' => array('b' => '2', 'c' => '3'), 'd' => 'foo'), 'Nested array was properly encoded.'),
   +      array(array('foo' => NULL), 'NULL parameters are omitted.'),
   +      array(array('foo' => ''), 'Simple parameters are properly added.'),
   
   @@ -54,8 +55,8 @@ public function providerTestBuildQuery() {
   +    $this->assertEquals(http_build_query($query, '', '&', PHP_QUERY_RFC3986), UrlHelper::buildQuery($query), $message);
   

   This change is not ideal. We're now essentially testing that calling a method two different times will give the same results. Please revert!

2. +++ b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
   @@ -297,16 +297,16 @@ public function testPathBasedURLGeneration() {
   -        $url = $base . 'node/123?foo';
   -        $result = $this->generator->generateFromPath('node/123', array('query' => array('foo' => NULL), 'absolute' => $absolute));
   +        $url = $base . 'node/123?foo=';
   +        $result = $this->generator->generateFromPath('node/123', array('query' => array('foo' => ''), 'absolute' => $absolute));
   ...
   -        $url = $base . 'node/123?foo#bar';
   -        $result = $this->generator->generateFromPath('node/123', array('query' => array('foo' => NULL), 'fragment' => 'bar', 'absolute' => $absolute));
   +        $url = $base . 'node/123?foo=#bar';
   +        $result = $this->generator->generateFromPath('node/123', array('query' => array('foo' => ''), 'fragment' => 'bar', 'absolute' => $absolute));
   

   We shouldn't have to change our tests :(

Re-rolled, doesn't applied anymore. Let's start again from #1 I can't see the results from the logger two years after this patch was tested.

Oh goodness we have that much code still doing string matches :(

This will still fail but it fixes UrlHelperTest. Similar to pwolanin's approach but slightly different. It looks like it changed more but it actually changed less I think. Every case is covered as it was before. By comparing that the result of the method decodes to the same value we're testing the intent of the method without testing the exact string output. I attached the test change alone to confirm its testing and passing the same without the optimization.

The gotcha here is parse_str(), which decodes query strings, drops leading spaces from keys for some crazy reason. As best I can tell we have to string compare on this one but luckily the output of the 2 implementation is identical for this case.

I also hacked the ['foo' => NULL]; logic in as you'll see from the interdiff so that test case is also unchanged. I'd prefer we stick with http_build_query() directly since we documented that as the goal of the method but we should also keep with our API promise and be consistent across minor releases. Especially if we have a test that caught the change. We can change that in 9 (or remove entirely).

Undoubtedly the other string matches like in bigpipe are going to still fail though. :(

+++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
@@ -42,27 +36,16 @@ class UrlHelper {
-        // For better readability of paths in query strings, we decode slashes.
-        $params[] = $key . '=' . str_replace('%2F', '/', rawurlencode($value));

Neat fact, this was casting objects. http_build_query() doesn't do that. Turns out by accident we had coverage for this "functionality" because of t() and safestring. Another back ported functionality added. Added explicit unit test since that's probably an important usability improvement.

The bigpipe method that builds the ids is protected so we can't call it in the test without modifying the class :(

This uses our helpers though instead of hardcoding the strings though so its a bit better off.

Doing some benchmarks I realized the if statements where unnecessary and its a bit faster to just cast everything. string casting a string is significantly cheaper then applying the 2 if statements.

The benchmarks are a bit of a mixed bag though. We see huge improvements in 5.x php but in 7 we are just a smidge slower in most cases. The exception being nested data which is always a lot faster. Seems the array walk is a bit slower then the loop for some reason that might be worth investigating.

The bigpipe method that builds the ids is protected so we can't call it in the test without modifying the class :(

@neclimdul I assume you already know this but, have you considered use reflection to test private methods?

I'd avoid using reflection for that since its pretty heavy. Much easier to create a stub and expose the method through it. Definitely possible though. I went with the hacky solution since I planned on making a follow up to discuss making it public.

1. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -42,27 +36,16 @@ class UrlHelper {
   +    // Do some conversions to match previous implementations.
   +    array_walk_recursive($query, function (&$value, $key) {
   +      // Previous versions of Drupal created query arguments with NULL values
   +      // but http_build_query throws them away. It also throws away objects.
   +      // As a convenience and backward compatibility we convert all values to a
   +      // string.
   +      $value = (string) $value;
   +    });
   

   I am not sure if this is correct.
   I have tested the http_build_query with booleans and it converts them to numbers. 1 for true and 0 for false.
   This will convert the false to an empty string.
   This will keep the ::buildQuery being different that the http_build_query itself.

2. +++ b/core/modules/system/src/Tests/Database/SelectTableSortDefaultTest.php
   @@ -32,8 +32,8 @@ function testTableSortQuery() {
   +      $this->assertEqual($first->task, $sort['first'], format_string('Items appear in the correct order sorting by @field @sort.', array('@field' => $sort['field'], '@sort' => $sort['sort'])));
   +      $this->assertEqual($last->task, $sort['last'], format_string('Items appear in the correct order sorting by @field @sort.', array('@field' => $sort['field'], '@sort' => $sort['sort'])));
   

   format_string is now deprecated. This should be updated to use the FormattableMarkup class.

3. +++ b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php
   @@ -18,13 +19,12 @@ class UrlHelperTest extends UnitTestCase {
   +    return [
   +      [['a' => ' &#//+%20@۞'], 'Value was properly encoded.'],
   +      [['a' => '1', 'b' => '2', 'c' => '3'], 'Multiple values were properly concatenated.'],
   +      [['a' => ['b' => '2', 'c' => '3'], 'd' => 'foo'], 'Nested array was properly encoded.'],
   +      [['foo' => NULL], 'Simple parameters are properly added.'],
   +    ];
   

   We need to also test the boolean values here.
   I will add one more case.

4. +++ b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php
   @@ -35,13 +35,25 @@ public function providerTestBuildQuery() {
   +    parse_str(UrlHelper::buildQuery($query), $result);
   +    $this->assertEquals($query, $result, $message);
   ...
   +   */
   

   I have an issue with the boolean values here.
   While the other cases work fine, using ['autoplay' => FALSE, 'fullscreen' => TRUE].

   The UrlHelper::buildQuery($query) returns autoplay=&fullscreen=1.
   The http_build_query returns autoplay=0&fullscreen=1.

   The second problem is the parse_str. It will also convert the query into the following array:

   Array
   (
       [autoplay] => 
       [fullscreen] => 1
   )
   

   Even if this is not the same as the initial array, the $this->assertEquals is considering them equals.
   In addition, if I try to work around it and end up having autoplay = 0, the assertion will fail because string 0 is not equals to false.

   IMO we should not test the query encoding by parsing the string. The query built is not obliged to be decoded since multiple values might be converted to the same result (e.g. both 0 and false are converted to "0" by http_build_query).

I am re-rolling the patch fixing also the issues detected here. I will follow the previous philosophy of keeping the array_walk before doing the http_build_query but I think we should not workaround in order to provide results different that what php would do.

I would like to, however, see what failures are there if we consider a proper solutions that query parameters with NULL are removed (just like http_build_query does).

@neclimdul, @idimopoulos instead of adapting this function that will be dropped anyway, why not replacing all occurrences from core with http_build_query()? Of course, on each usage we'll have to account, if case, the different output. Then we deprecate UrlHelper::buildQuery().

(deleted)

@claudiu tbh, I also like that idea. I am attaching a small patch to check how tests will behave. I replaced the usage of the ::buildQuery and cleaned up the test. I also deprecated the method.

It seems that was a decision or, at least, a discussion to keep this method as it is because of the different output. See #2322059-50: Keep the UrlHelper::buildQuery method and remove @todo. It's likely that both issues will be "won't fix". Let's put on hold the work here till something is decided.

It sounds like its being overlooked still that buildQuery does object casting which http_build_query fails to do. This means if you have a stringable object in your query like a markup string or as the new test I added demonstrates an Attribute it fails to produce the correct output.

Granted this is super easy to miss and undocumented but this was a breaking change to core I documented in my patch and in the early comments. Getting rid of this method is likely to be a strangely breaking change I'm not sure makes sense.

Here's a re-roll of the earlier functioning patch. Also adding back my credit... not sure why that was removed.

Lets try this again now that this method's fate is decided.

With the recent release of Drupal 10 are there any other maintainers of core considering this for an upcoming minor version release?

Reference https://www.drupal.org/project/commerce_usps/issues/3326069#comment-1483...

With the holidays in full swing, this has become a hot topic.

Thank you everyone for your time.