\Drupal\Component\Utility\UrlHelper::buildQuery() adds extra ampersands

I've verified that this is a bug. The patch looks promising, please attach it for testing!

Nice patch. I'm merely converting the code to patch format and queuing for testing. Also confirming this works but I won't RTBC it since I'm the one submitting it.

Have reviewed the code and tested the implementation, it works as expected.

Also good to see that it doesn't strip out empty variables like

<?php
$q = array(
  'action' => 'search',
  'filter' => array(
    'name' => array(),
    'city' => array(),
    'location' => array(),
  ),
  'test' => '',
  'page' => 10,
);
?>

Reason for this is that some web services require that you pass all parameters even if they are empty, taking an example "p1=&q=123213" is valid and is in some cases required by the receiving server.

Thanks for the fix! Let's add a test here to prove the fix works.

Also, patches go into D8 first, then are backported to D7, so bumping to D8.

Right, right. Forgot about that piece. :) I haven't the slightest idea how to code a test. Sounds like I have a project to do. I'll work on it.

Ok, so I am attaching this here so I don't lose it but here's the D8 version. Still needs a test. Working on that now.

So far the following test has failed but i am continuing to check into it:

$this->assertEqual(drupal_http_build_query(array('a' => '1', 'b' => NULL, 'c' => '3')), 'a=1&c=3', t('Multiple values were properly concatenated when null values were passed.'));

I have to assume this is just my newness to writing tests. I'll post back when I have something.

Your test assertion assumes that it should be empty correct output would be a=1&b=&c=3

Updating tags per http://drupal.org/node/1517250

Spoke with chx in IRC and he confirmed this is the desired behavior. See the attached test and fix files.

Reading http://en.wikipedia.org/wiki/Query_string:

Within each pair, the field name and value are separated by an equals sign. The equals sign may be omitted if the value is an empty string.

So it looks like we got this one wrong. Either ?a=1&b=&c=3 or ?a=1&b&c=3 are valid, but ?a=1&c=3 is not valid.

So in the test that is in the newest patch for

$query = array(
      'a' => '1',
      'b' => array(
        'c' => array(),
        'd' => NULL,
      ),
      'e' => '2',
      'f' => '',

the result should be a=1&b[c]&b[d]&e=2&f?

Both having an equals sign and not having an equals sign for empty string parameters seem to be valid. Uploading both variations of the patch to determine which will break the least amount of code in core.

http://example.com/node/123?foo= is VERY weird.

Comparing a=1&e&f=2 and a=1&e=&f=2, I think we definitely want the former.

Unfortunately, that means debugging why _openid_get_params() is blowing up.

chx demonstrated that a pure HTML form includes the = sign, and it's required for OpenID.

Alright. I think I have the test created and bug fixed. I have attached the test for this bug and also the bug fix as well as the test fixes.

Alright. I think I have the test created and bug fixed. I have attached the test for this bug and also the bug fix as well as the test fixes.

This had chx's input at MWDS, and I think Chris nailed it here.

Just a note...

I'm actually working on a link bug http://drupal.org/node/1710578 where the user has a url he's entering - 'http://www.example.com/?something' and I'm outputting 'http://www.example.com/?something=', the latter of which breaks what he's trying to point at. I'm currently able to fix this by setting the query array so that it's array('something' => NULL), but this change would break that. I realize that for PHP, the querystrings are equivalent, but that may not be true in all languages, would it?

Then he can parse it differently. They are equivalent in HTTP.

He doesn't have any control over the target's processing in this case, but if they're equivalent in HTTP, then that's fine. Just pointing out that it could cause trouble for someone else.

-  return implode('&', $params);
+  $params = array_filter($params);
+  return !empty($params) ? implode('&', $params) : NULL;
 }

This array_filter() feels sloppy (it removes any falsish value by default, aka NULL, '', 0, '0', etc.). If I understand this correctly, we are just removing the special case elseif (!isset($value)) and checking that we don't add an entry if the sub-query is empty. So the whole function could read:

function drupal_http_build_query(array $query, $parent = '') {
  $params = array();

  foreach ($query as $key => $value) {
    $key = ($parent ? $parent . '[' . rawurlencode($key) . ']' : rawurlencode($key));

    // Recurse into children.
    if (is_array($value)) {
      $children_query = drupal_http_build_query($value, $key);
      if ($children_query !== '') {
        $params[] = $children_query;
      }
    }
    else {
      // For better readability of paths in query strings, we decode slashes.
      $params[] = $key . '=' . str_replace('%2F', '/', rawurlencode($value));
    }
  }

  return implode('&', $params);
}

     // Verify that the current path is used as destination.
-    $this->drupalGet('common-test/destination', array('query' => array($query => NULL)));
+    $this->drupalGet('common-test/destination', array('query' => array($query => '')));
     $url = 'common-test/destination?' . $query;
     $this->assertText('The destination: ' . $url, t('The current path is determined as destination.'));

That cannot possibly be the intention of the test? This clearly only passes by chance (because it does a assertText() which is a substring match).

We likely want something like:

    // Verify that the current path is used as destination.
    $this->drupalGet('common-test/destination', array('query' => array('additional_argument' => $query)));
    $url = 'common-test/destination?additional_argument=' . $query;
    $this->assertText('The destination: ' . $url, t('The current path is determined as destination.'));

But can be done without auxiliary variable:

No, because the sub-array could contain empty sub-sub-arrays.

But can be done without auxiliary variable:

No, because the sub-array could contain empty sub-sub-arrays.

Yes, the test written by cgreaten contains an array with an empty array and I don't think it would pass without the auxiliary variable.

This evening I applied the test I originally wrote to prove there was a bug and then applied Damien's proposed code. When I ran the test, it failed. The reason it failed is that '&d=' was added to the query string for the d => NULL portion of the array in the test. In the patch I submitted, that portion of the query string is left out (and I think correctly).

I am happy to re-roll my patches again or make changes, if needed. Sorry I'm not quite sure what the process is from here (it's my first attempt at contributing to the community).

Alright, I took a bit of time tonight and I tweaked Damien's code so it passes the test. I also used his feedback to modify the test in the GotoTest to be better. With the tests that are in UrlTest tests, I added a test for '' along side the NULL so that both are there instead of just changing the NULLs to '' so that the tests pass.

This patch can be reviewed and then compared to my first one to see which is preferable (or whether we want some kind of hybrid of the two).

GotoTest.php is no longer there and patch no longer applies.

Hi,

I don't know how to create a patch, but here is my code suggestion:

function drupal_http_build_query(array $query, $parent = '') {
  $params = array();
  
  foreach ($query as $key => $value) {
    $key = ($parent ? $parent . '[' . rawurlencode($key) . ']' : rawurlencode($key));

    // Recurse into children.
    if (is_array($value)) {
      $children_query = drupal_http_build_query($value, $key);
      if ($children_query) {
        $params[] = $children_query;
      }
    } elseif ($value === NULL ) {
        
    } elseif (!isset($value) || $value === FALSE || $value === '') {
      $params[] = $key;
    }
    else {
      // For better readability of paths in query strings, we decode slashes.
      $params[] = $key . '=' . str_replace('%2F', '/', rawurlencode($value));
    }
  }
  return implode('&', $params);
}

In my tests this function returns the expected Results.

Creating a patch built on #36 just to show which unit tests fail.

@Sawascwoolf, https://www.drupal.org/patch should help with patch creation if you'd like to learn!

The phpunit test that failed might have more information with this patch - its identical to #37 with the exception to the change to the simpletest module, but probably with more information

How come everybody but me remembers to mark it needs work review?

A problem is that

array(array('a' => '1', 'b' => array('c' => array()), 'd' => NULL, 'e' => '', 'f' => '2'), 'a=1&e=&f=2', 'Empty query parameter values are properly ignored.'),

and
array(array('foo' => NULL), 'foo', 'Simple parameters are properly added.')

this two tests are not right.

see:
'd' => NULL = ''
'foo' => NULL = foo

and this can not work.

We need a clear behavior for this function .

http_build_query(array('a' => '1', 'b' => array('c' => array()), 'd' => NULL, 'e' => '', 'f' => '2'));
=> "a=1&e=&f=2"

http_build_query(array('foo' => NULL));
=> ""

So there should be no 'd'

@marvin_B8, it's really hard to see what changed from one patch to the other without an interdiff

I think we should consider the testcases a bit more precisely.

We should use http_build_query to generate test cases!

With the following code I've created the test cases:

//TestCase Array
$array = array('a' => array('b' => '2', 'c' => '3'), 'd' => 'foo');

$phpFunctionQuery = http_build_query($array,'','&',PHP_QUERY_RFC3986);
// Decode [ and ]
$result = str_replace(array('%5B','%5D'),array('[',']'), $phpFunctionQuery);

The test-cases

• array(' &#//+%20@۞' => 'a') ---> '%20%26%23%2F%2F%2B%2520%40%DB%9E=a'
• array('a' => '1', 'b' => '2', 'c' => '3') ---> 'a=1&b=2&c=3'
• array('a' => array('b' => '2', 'c' => '3')) ---> 'a[b]=2&a[c]=3'
• array('a' => '1', 'b' => array('c' => array()), 'd' => NULL, 'e' => '', 'f' => '2') ---> 'a=1&e=&f=2'
• array('foo' => NULL) ---> ''

And here is my current version:

function drupal_http_build_query(array $query, $parent = '') {
  $params = array();
  
  foreach ($query as $key => $value) {
    $key = ($parent ? $parent . '[' . rawurlencode($key) . ']' : rawurlencode($key));
    
    if (!$value && $value !== '') {
     
    } elseif (is_array($value)) {
      // Recurse into children.
      $children_query = drupal_http_build_query($value, $key);
      if ($children_query) {
        $params[] = $children_query;
      }
    } else {
      // For better readability of paths in query strings, we decode slashes.
      $params[] = $key . '=' . str_replace('%2F', '/', rawurlencode($value));
    }
  }
  return implode('&', $params);
}

I don't have much time right now to learn the patch-system, if I'm going on to contribute I will learn it.

I'm not sure right now if it is really desired to use the http_build_query() as reference.

Is there a need for passing:

• (bool) FALSE values?
• empty strings
• empty array

Assume a form with a huge amount of checkboxes, with the current version every checkbox-value is added to the query string with value=0, this results in really long query-strings which don't hold information.

I suggest to ignore the values (see list above) and don't append anything at all for this case.

As this could cause notices of type Undefined index: it must be discussed if it should be backported to D7 in this way.

Updated IS and rerolled patch.

Landing here from Bug smash initiative.

Few updates from end:

1. I am still able to replicate this issue on D11.
2. Added patch compatible with 11.x-dev version with interdiff from #57
3. Adding Needs Tests tag for help in solving failures in test
4. Did initial level of testing and works as expected.
5. Updated IS by adding sample for for Drupal 11

Updating status to Needs Review after test run:

Need some advice or suggestion for following doubts.

1. Wondering why phpstan is throwing error elseif (!isset($value)) { i didnt change line for this and its giving error
2. Its weird that 1806 test run failed and at the same time it also shows core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php passed

Will create MR, once above issue is addressed.

Think it's saying your change caused that check to be redundant. So may need to look at the solution and make sure.

I added #65 to the MR.