Rewrite as URL adding equals sign to end of url.

The problem is that drupal_get_query_array('foo') doesn't return array('foo' => NULL) but array('foo' => '');

Regarding the issue in views: this first has to be fixed 8.x and then get's backported to 7.x and then should work in views.

Here is a test, lets see whether it fails.

So here is a path with an actual fix.

#4 looks good:

• patch applies
• testbot is happy
• tested with mysite.com?foo=1
• tested with mysite.com?foo=''
• tested with mysite.com?foo
• Adds tests ++++
• Fixes issue from OP ++++

RTBC

@robit8deb - for core patches always patch from your webroot. For contrib module patches always patch from the top-level of the module folder.

This needs a quick re-roll because the tests moved from ./core/modules/simpletest/tests/common.test to ./core/modules/system/tests/common.test. Otherwise this patch looks good to me.

Because it was just a file switch and no other conflicts, setting back to RTBC.

CommonURLUnitTestCase extends DrupalWebTestCase

Hmm the test added is a proper unit test, but CommonUrlUnitTestCase extends DrupalWebTestCase (which is right in the sense the existing tests aren't proper unit tests, but the name is very wrong). Could we put this into a new class that extends DrupalUnitTestCase? A bit scope creepy to rename CommonUrlUnitTestCase here but just removing the 'Unit' would probably do fine, follow works for me too though.

Tagging novice for the test cleanup catch describes in #10.

s/DrupalUnitTestCase/UnitTestBase, but yeah.

Thanks! Committed/pushed to 8.x.

Did anyone care to verify whether this change is actually correct?

The equal sign is optional for query parameters.

For a query string ?foo, PHP returns an empty string in $_GET['foo'].

Normally, all query parameter values are expected to be strings.

@sun I'm confused. What the use-case is for ?foo= with nothing after the equal sign. This is equivalent to ?foo is it not? In both cases $_GET['foo'] will have a value of empty string.

That's correct. But this patch changed our own user space parser to not comply with that:

+++ b/core/modules/system/tests/common.test
@@ -281,6 +281,32 @@ class CommonURLUnitTestCase extends WebTestBase {
+    $this->assertEqual(drupal_get_query_array('foo='), array('foo' => ''), 'An empty value is set as an empty string.');
+    $this->assertEqual(drupal_get_query_array('foo'), array('foo' => NULL), 'No value is set as a null value.');

PHP's value of $_GET['foo'] is an empty string in both cases.

OK reverted in 8.x for now so we can discuss some more.

I merged this into #1598564: Convert common.test to PSR-0 (before I noticed it had been reverted). Here's the class converted to PSR-0. In case this is to be committed again, use the PSR-0 variant straight away (for the added class)?

Add issue summary

12: drupal-1464244-12.patch queued for re-testing.

I think this is good to be closed now (outdated), function in reference (drupal_get_query_array) is removed - check https://www.drupal.org/node/1597784

Hm, but why I have this problem with current version 8.5.4 again? Or 8.5.x? I don't know how long this error exists because it was only reported today. I render a link field with external urls with

{{ content.field_external_url.0.['#url'] }}

This worked in the past, but now I get a trailing equal sign for a link of type http://domain.org/?flag. And this leads to an 404 on the external page.

I had to change to

{{ content.field_external_url.0.['#url'].uri }}

This works for me because I'm using external links only. But I don't know how if it works for internal links too.

Hi @all,

please have a look at this. There are more issues reporting by users now with the same wrong behavior. And I cannot fix it because of using default link formatter.

You can reproduce it with

1. Install latest stable Drupal Core 8.5.5 with standard profile
2. Update content type Article and add a new field of type Link and allow the use of external links
3. Create an Article and use the following content: http://www.domain.org/?flag
   
4. Save the Article. On the frontend you will see: http://www.domain.org/?flag=
   

Argh – how can I fix this?

Ciao
Ralf

I can fix the problem mentioned above by applying attached patch. But I think it could be a problem in future versions too. The @todo at UrlHelper::buildQuery() prefers the use of php build in http_build_query() but if I use this the same issue exists again.

The patch are usable for the 8.6.x and 8.7x branches too. What do you think?

I'm running into the same issue with 8.5.5. It looks like the code from #36 is in Core now, but I'm still getting the added "=".

We should have a test to demonstrate the problem and to prevent regressions in the future.

I strongly believe that the patch in #36 is not correct!

First of all, it must be possible to distinguish between 1) valueless query parameters (?foo), and 2) query parameters whose value is an empty string (?foo=). The original code, which used isset(), made that distinction possible, but the patched code, which uses empty(), makes the distinction impossible. The patch therefore is no solution at all, it simply shifts the problem. Before the patch, all occurrences of 1) were wrong, but 2) were correct. After the patch, all occurrences of 1) are correct, but 2) are now incorrect. Example: After the patch, http://www.example.com/cgi-bin/foo.cgi?bar= will be processed into http://www.example.com/cgi-bin/foo.cgi?bar, i.e. the "=" character is omitted.

Second, and this is even worse, empty() treats all kinds of values as "empty". According to the PHP docs (does anybody read the docs?) a variable is considered empty "[...] if its value equals FALSE." This means that the patch causes some query parameter values, which happen to be equal to PHP's definition of FALSE, to be omitted. Example: http://www.example.com/cgi-bin/foo.cgi?bar=0 would be processed into http://www.example.com/cgi-bin/foo.cgi?bar= because the query parameter value 0 is, of course, equal to FALSE.

The root of the problem, in my opinion, is not UrlHelper::buildQuery(), it is the use of the PHP function parse_str() in UrlHelper::parse(). I believe parse_str() is the wrong function to use, because it does not distinguish between valueless query parameters (?foo) and query parameters whose value is an empty string (?foo=) - it produces the same result for both. As long as parse_str() is used to break down a query string into its parts, it is therefore simply impossible to correctly reassemble the query string later on.

Also check out the discussion in issue 2987114, maybe the issue helps to understand where parse_str() was introduced.

After looking for the root cause of this bug (before I stumbled onto this issue), I came to roughly the same conclusion as herzbube;
parse_str() loses too much information about the query string. There is no way to distinguish a valueless query string and an empty-valued query string after this function has been executed.

Ideally NULL should be returned as value, when dealing with valueless query string parameters.

As a workaround, I've introduced https://github.com/thephpleague/uri-query-parser in my project and I've replaced the parse_str code in UrlHelper::parse() with something like this:

$query_string = \League\Uri\Parser\QueryString::parse($parts[1]);
foreach ($query_string as $key_value_pair) {
  $options['query'][$key_value_pair[0]] = $key_value_pair[1];
}

For my purposes (where the extra `=` causes a 404!), replacing parse_str with GuzzleHttp\Psr7\parse_query corrects this problem.

Adding a related issue since parse_str is messed up in other ways.

Attached patch adds test coverage for this issue.

But there seems to be other areas where the test coverage is checking for the opposite behaviour, as indicated in the failed tests on #45.

e.g. in UrlRewritingTest.php there is a test for this:

    $url = file_create_url($filepath . '?foo');
    $this->assertEqual($GLOBALS['base_url'] . '/' . $filepath . '?foo=', $url, 'Correctly generated URL. The query string is present.');

Which originates here: https://www.drupal.org/project/drupal/issues/2276203#comment-9289445
Why would that code be expecting the = to be added?

Updated patch which is a lighter approach to a fix, based on #45.

Only using GuzzleHttp\Psr7\parse_query for external urls as that is where the added equals sign can cause 404s. PHP should treat ?foo and ?foo= the same, so not changing the processing of the internal urls, which avoids the problems with the failing tests in #45.

#48 worked for me.

#48 worked for me as well -thank you!

@porchlight, @calamityjen perhaps you could mark as RTBC to help this progress towards being committed?

https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

Let's see if these pass with a rerun of the tests.

Drupal\BuildTests\Framework\Tests\BuildTestTest
exception: [Other] Line 0 of sites/default/files/simpletest/phpunit-2.xml:
PHPUnit Test failed to complete; Error: PHPUnit 7.5.20 by Sebastian Bergmann and contributors.

Testing Drupal\BuildTests\Framework\Tests\BuildTestTest
...E.                                                               5 / 5 (100%)

Time: 18.34 seconds, Memory: 6.00 MB

There was 1 error:

1) Drupal\BuildTests\Framework\Tests\BuildTestTest::testPortMany
RuntimeException: Unable to start the web server.
ERROR OUTPUT:


/var/www/html/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php:441
/var/www/html/core/tests/Drupal/BuildTests/Framework/Tests/BuildTestTest.php:145

ERRORS!
Tests: 5, Assertions: 74, Errors: 1.
✗	
Unknown
fail: [run-tests.sh check] Line 0 of :
FATAL Drupal\BuildTests\Framework\Tests\BuildTestTest: test runner returned a non-zero error code (2).

Seems to have tests, so removed needs test tag

Tests passed on a re-run so setting back to RTBC.

I notice that the proposed resolution in the IS does not match what is in the patch. Can that be updated?

Two thoughts/questions:

1. If we really want to add the guzzle helper here, why is this not added to the composer.json in core/lib/Drupal/Component/Utility/composer.json:17
2. It feels like just making this change could break people's assumptions. I wonder whether some level of BC layer including a deprecation notice would be good

We definitely need to add the dependency as per #60

Also the docs to the Guzzle function say:

     * If multiple values are found for the same key, the value of that key
     * value pair will become an array. This function does not parse nested
     * PHP style arrays into an associative array (e.g., `foo[a]=1&foo[b]=2`
     * will be parsed into `['foo[a]' => '1', 'foo[b]' => '2'])`.

I wonder if this will have any unintended impacts. Making a change like this is really hard because of all of the unknowns.

More on #61...

>>> parse_str('foo[a]=1&foo[b]=2', $query);
=> null
>>> $query
=> [
     "foo" => [
       "a" => "1",
       "b" => "2",
     ],
   ]
>>> \GuzzleHttp\Psr7\Query::parse('foo[a]=1&foo[b]=2');
=> [
     "foo[a]" => "1",
     "foo[b]" => "2",
   ]

That's quite a big difference in output for the same input. Does this matter... not sure.

For me we should do the solution in #3038774-31: Url only outputs the last value of a query parameter and not parse and alter the external uri at all. That would mean we could also fix #2984272: Dots in query parameter names converted to underscores too.

The patch in #64 still applies to Drupal 9.3.x, Changing version.

Needs work for #60 and #61 and still needs an Issue Summary Update.