Contrib.social

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

Open on Drupal.org →
Created on 2 February 2012, over 12 years ago
Updated 19 December 2023, 6 months ago

Problem/Motivation

Calling drupal_http_build_query() --or another function like url() which calls drupal_http_build_query-- with some empty params adds extra ampersand signs. Take a look at next code:

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

echo drupal_http_build_query($q);

++++++ sample for for Drupal 11

use Drupal\Component\Utility\UrlHelper;
echo UrlHelper::buildQuery($q);

This displays

action=search&&&&page=10

and you notice that 3 extra amps are added. I know this is because name, city and location are empty but this can happen often if you are making some complex filters.
PHP http_build_query() handles well these empty arrays but drupal don't. With PHP http_build_query we get

action=search&page=10

which is correct.

Steps to reproduce

Proposed resolution

Modify buildQuery to ignore empty arrays and NULL valiues.

--- original proposal ---
To fix this we have to check if $params is empty or not before implode. Please check patch:

--- a/includes/common.inc
+++ b/includes/common.inc
@@ -489,7 +489,8 @@ function drupal_http_build_query(array $query, $parent = '') {
     }
   }
 
-  return implode('&', $params);
+  $params = array_filter($params);
+  return empty($params) ? NULL : implode('&', $params);
 }

Remaining tasks

Review
Commit

User interface changes

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Base  →

Last updated about 2 hours ago

Created by

🇷🇴Romania Sorin Sarca

Live updates comments and jobs are added and updated live.
  • Needs backport to D7

    After being applied to the 8.x branch, it should be considered for backport to the 7.x branch. Note: This tag should generally remain even after the backport has been written, approved, and committed.

  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • Comment 6 months ago →
    🇵🇭Philippines abhaypai

    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
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 6 months ago
    Custom Commands Failed
  • Status changed to Needs review 6 months ago
  • Comment 6 months ago →
    🇵🇭Philippines abhaypai

    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.

  • Status changed to Needs work 6 months ago
  • Comment 6 months ago →
    🇺🇸United States smustgrave

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

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024