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.

Patch #64 updated to apply on Drupal 10.2.4.

I tested the last two patches (#71 #64) with core 10.3.5 and both don't work.

Patch #64 applies on Drupal 10.3.x and fixes the issue.
I updated the IS and I believe that comments from #60 and #61 don't apply anymore with the approach followed in the patch.

For novices: please convert #64 patch to a MR to ease maintainers job.

Fixes should be in MRs

Have not reviewed yet.

Working on it.

Created MR for patch #64. Please review it.

Patch #64 is working for me in a Drupal version 11.0.4.
Thank you!.

Have re-ran tests twice and they still fail.

Tests are now passing, moving to Needs Review.

Hi Sayan, thanks for the effort.
Despite the fact that tests have "Pass" status, there's one warning left, and checking the detail says:

Drupal\Tests\Core\Utility\UnroutedUrlAssemblerTest::testAssembleWithExternalUrl with data set #7
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'https://example.com/test?foo'
+'https://example.com/test?foo='

which sounds like pretty much what we are testing here, isnt?

@igorgoncalves yes, indeed the two strings of urls are different, that is what we need here. Do I need to make changes to test files, to remove the warnings. I don't have much knowledge of phpunit tests, so if someone can look into it, it would be better.

All tests are now passing, warnings are resolved. Please review.

Checked in my local setup before and after changes. Attaching screenshots. LGTM+

The fix has issues - added a comment to MR.

Updated the MR with @alexpott's recommended changes and also replaced strpos() with preg_match() using a regex that should prevent the false positives due to substring overlap that @alexpott noted by ensuring that the matches are either at the beginning of the query string or after a &.

Rebased as this MR was 800 commits back

Feedback appears to be addressed on this one.

NR for a small commment improvement suggestion I made on the MR. It's okay to re-RTBC the issue if the suggestion is accepted (no need for an extra review cycle over something small).

Feedback from xym appears to be addressed, saving credit for xjm review in the MR.

Line 227 of UrlHelper.php, removed comment that could confuse the meaning of the comment addition in this MR. Cannot see any usefulness in removed comment at all except to confuse people.

Restored comment removed from else block in UrlHelper::parse(). Not sure why that was removed (was removed in one of the earlier patches that preceded the MR).

Reverted my most recent commit to restore removed comment (see #95). Failed to realize that @oily was describing a change that had already been made and not requesting a change. Apologies for the confusion.

I agree that the comment that has been removed was confusing. @oily's change seems like a good one to me.

Thanks for the update!

Despite my earlier comment: This is pretty low-level, and the approach has changed several times over the issue's 13-year (!) history. So, I'd like subystem and/or framework signoff for the fix. Leaving RTBC for that.

I discussed this a bit with @longwave in slack and I have minor concerns about whether there's a way to do this without a regexp and/or whether the behaviour is definitely a bug or not, but these are minor and not anything to hold it up - I was not able to come up with a better suggestion and @longwave convinced me we should actually fix it. So removing the FM review tag.

I think this needs pointing out...

$ vendor/bin/drush ev "dump(\Drupal\Component\Utility\UrlHelper::parse('http://example.com?flag'));"
^ array:3 [
  "path" => "http://example.com"
  "query" => array:1 [
    "flag" => null
  ]
  "fragment" => ""
]

$ vendor/bin/drush ev "dump(\Drupal\Component\Utility\UrlHelper::parse('http://example.com?flag='));"
^ array:3 [
  "path" => "http://example.com"
  "query" => array:1 [
    "flag" => ""
  ]
  "fragment" => ""
]

So we are back at the solution that was rejected all the way back in #15.

FWIW I think #15 was wrong because URLs are not only processed by PHP based webservers and also if you use something like https://uri.thephpleague.com/ it was behave similarly.

I still think we should do something like #3038774-31: Url only outputs the last value of a query parameter and not futz at all with external URLs unless we have to and that would solve the majority of issues with using PHP to process URLs supported by systems not running PHP.

We definitely need a CR to cover this change of behaviour as outlined in #100.

I've opened #3554522: Only parse and mess with the URI in \Drupal\Core\Utility\UnroutedUrlAssembler::buildExternalUrl() if we have to to do #3038774-31: Url only outputs the last value of a query parameter which I believe would actually solve most people's issue here and on the other issue.

I would encourage people to review #3554522: Only parse and mess with the URI in \Drupal\Core\Utility\UnroutedUrlAssembler::buildExternalUrl() if we have to as I expected that that minimal change solves quite a lot of the reasons why people end up on this issue.

Moving this to postponed, then we can refocus this issue on anything that's left.