Dots in query parameter names converted to underscores

I ran into the same problem and created a new method to work around this default PHP behaviour. Created a patch to implement that method in the parse method of UrlHelper.php and in the fromUri method of Url.php.

There are more places in core where parse_str is used and where it might be replaced with this new method.

Created new patch. Basically the same as #4 but compliant to coding standards. I know, should have been paying more attention the first time.

Triggering the testbot. Besides that, there are lots of other occurrences of parse_str in core. Not sure if we want to replace those as well?

Looks good to me. This probably needs more reviews from the menu/link maintainers.

The testcoverage of this looks good, but I would love to also see unit/kernel testcoverage for this. Right now it looks like this is only tested by a webtest.

In any case, I like the approach here.

This solves my issue too.
Thanks

The testcoverage of this looks good, but I would love to also see unit/kernel testcoverage for this. Right now it looks like this is only tested by a webtest.

Drupal\Tests\Core\UrlTest is a unit test?

/headdesk - yes it is. I missed that, sorry. Setting to RTBC to move this up the chain to a subsystem/core maintainer.

Are we sure we want to do this. Symfony has chosen to not support it and neither has Drupal. Reimpleming parse_str() in Drupal introduces its own problems and surface area to attack since we are definitely handling user input here.

Personally I'm -1 on doing this for the reasons outlined in the https://github.com/symfony/symfony/issues/25541 - I'd prefer to try to change the behaviour of parse_str() in PHP.

Agreed with #14. Is there a PHP bug report for this already?

The problem is here is that we're changing very low level functionality. UrlHelper::parse() is used in all sorts of places. One of which is in external URL parsing. So its very very hard to predict the outcome of making that change. PHP has been altering query parameters like this forever. For example, if you make a request to PHP with the query param ?foo.bar=bar and dump $_GET the output will be

Array
(
    [foo_bar] => bar
)

One thing I think I support though is not mangling user inputted external URLs in this way since that is just frustrating. The user knows the correct URL but the system is breaking it for them. That's not good. In fact this is the second report we've had of this type of bug in 8.6.x and I don't think it is happening in 8.5.x. The other report is #2987114: Regression in external URLs in menu links with valueless query parameters and all of this appears to have been caused by #2955685: Unrouted URLs cannot have have overridden query or fragments

Any backporting on 8.6.x versions?

Thanks

#16 +1

Closed #3044507: Fix dots and spaces changing to underscores in destination query parameter names as a duplicate issue.

This issue also breaks front end libraries that require specific query parameter names. It is the result of a deprecated PHP behavior that can generate variables directly from a query string. For some reason, this name change is also applied to the top-level keys when the query string is being parsed to an array. I have updated the patch to apply to internal URLs and made clear that our wrapper method only restores original parameter names to the parsed query.

Patch in #24 does not resolve nested query parameters e.g ?param[nested][0]=value&param[nested][1]=2ndvalue.

currently checking the code for 8.9.x-dev ..... . i am working on it

I added the Needs issue summary update tag to the issue. The issue is still tagged as Needs subsystem maintainer review and needs security review, and I think it would help if the issue summary could be updated with what is requested to be reviewed. If after the issue summary is updated, it isn't clear, please also remove the Novice tag when removing the Needs issue summary update tag.

I removed the Assigned attribute. Generally we shouldn't assign issues to ourselves per Assigning Ownership.

I have corrected the tests and accounted for the possibility of name collisions caused by parse_str changing parameter names.

Hello,

I also encountered the issue in a Symfony application. In order to fix this, I created a bundle that might be helpful for the Drupal community as well.

Find it here: https://github.com/loophp/unaltered-psr-http-message-bridge-bundle

I see that Drupal is already using symfony/psr-http-message. This package is a drop-in replacement, so adding my package should be just enough to fix the issue.

Adds urldecode for name parameter and merge parsed parameters recursivly.

Preserve integer keys when recursively merging nested parameters.

I can confirm that the patch for 8.9 in #36 resolves the issue in my case.

I was also able to cleanly apply the patch for Drupal 8.7.14 running in a local dev environment with PHP v7.0.32

Don't forget to update the issue status.

Confirming that #36 resolves the issue on a Drupal 8.8.6 and PHP 7.4.7. Thanks!

So one thing that's good is that no tests are failing with this change. BUT that only means it's likely that core is not relying on PHP's behaviour. Unfortunately that doesn't mean anything for existing contrib and custom code that might very well be relying on it. It's almost 100% for sure that someone somewhere is relying on the way that PHP mangles the query parameter names. See discussion on https://externals.io/message/106213, https://github.com/symfony/symfony/issues/29664, https://github.com/symfony/symfony/issues/25541 for examples of the BC concerns.

One of the probable reasons nothing is broken is that we're only changing UrlHelper - which is mainly used for processing external Urls - but Drupal's underlying request handling infrastructure is still using parse_str() and so will continue to mangle query parameter keys. Also we have no idea what UrlHelper might be used for in the wild.

This is a super tricky issue.

A possible way forward would be to a flag to UrlHelper::parse() to allows the current behaviour to be maintained but the new behaviour to be opted into. Also we could target the problem outlined in the issue summary - when linking to an external url don't mangle it at all - which would a smaller (in terms of underlying impact) change than this.

As mentioned by javiereguiluz in https://github.com/symfony/symfony/issues/29664#issuecomment-512147641 :

Note that the automatic conversion of dots into underscores is a PHP behavior ... which could finally be changed/removed in PHP 8: https://externals.io/message/106213

Code from #36 issues PHP warning when $parsed[$name] is not an array at the following line :

$parsed[$name] = NestedArray::mergeDeepArray([$parsed[$name], reset($param)], TRUE);

I have encountered an issue when using this patch with a URL where the query string begins with a number. If the query string begins with a number instead of an alphabetic character, the query string that appears in the processed link is 0. So if my query string begins "?1335453" the end result I get from Drupal after the URL is processed is "?0=". I have tried running through the php commands in the public static function parseQueryString() from the patch, but have not been able to figure out where the issue is. It seems like it should be fine, but maybe the problem is introduced in a different step.

I used the #36 patch and I also encountered the error mentioned in #41. In my case it was because the same parameter was used twice in the supplied URL:
http://example.com/?a=1&b=2&a=3

The parseQueryString function chokes on the second occurrence of "a" and rewrites the URL like this:
http://example.com/?&b=2

You can make the warning go away if you change the "isset($parsed[$name])" to "isset($parsed[$name]) && is_array($parsed[$name])". This will make it preserve the "a" variable, allowing the second occurrence to overwrite the first:
http://example.com/?a=3&b=2

I think this is a more graceful way to deal with a malformed URL. This does however assume that the URL is malformed and that it's OK for this function to rewrite it.

I also ran into the issue in #41. It seems like the behavior being attempted here is to allow multiple values for a single parameter name, thus the invocation of the mergeDeepArray function. Therefore, if the name already exists but only has one value, the value should be converted into an array so the next value could be added.

if (!is_array($parsed[$name])) {
  $parsed[$name] = [$parsed[$name]];
}

I made this change on my Drupal 8.9.5 site (in development) and it appears to have cleared up the errors I was receiving without also creating new problems.

I am attempting to upload a revised version of the patch in #36, but I have to admit I'm not very good at this.

Adding patch again with correct naming convention

Patch in #45 was broken, trying again

Okay, sorry for making a mess of this issue. The patch I provided in #46 doesn't seem to fix the error; for some reason It worked initially in dev but is now not working as a patch. I am passing what should be an array of arrays to mergeDeepArray, but am still getting the invalid argument error.

In addition to preserving the dots in the parameter names, I needed to allow duplicate parameters, like "?a=1&b2&a=2". There is no way to do this using the parsed query array because the array can't have duplicate keys. However, I believe that for external links, if you're not altering the query by adding parameters to it, you should preserve the original string without parsing it at all. These patches do that.

Fixed the failed CS issues.
Not able to generate interdiff. getting error.

The #49 patch had an error that was causing tests to fail. Here's a redo.

I missed a test.

I missed another test.

I've just run into this same issue and tested the latest patch and works for me. Though I'm not sure about any security implications it might have if any.

Here's an after-the-fact `git format-patch` file with the same changeset as the issue fork and merge request, for composer-based patching.

Quick solution:

function my_module_preprocess_node__articles(array &$variables) {
}

function HOOK_preprocess_node_HOOK(array &$variables) {
if (isset($variables['content']['field_link'])) {
if (!empty($variables['content']['field_link'][0]['#url'])) {
$variables['content']['field_link'][0]['#url'] = [
'#markup' => $variables['content']['field_link'][0]['#url']->getUri(),
'#type' => 'markup',
];
}
}
}

The patch in #59 fixes the issue for me (using Drupal 9.4.8).

So I have a field on a node that uses the 'link' field type. I have some content where the URL contains a query string that has semi-colons in it. When that URL renders, the semi-colons are getting encoded to %3B, and the link breaks. After applying this patch, the semi-colon in the query string is no longer encoded and the link works as expected.

The merge request is against 9.3, it needs to be against 9.5 or 10.1 with a clean test run before it can be RTBC. I haven't reviewed the issue otherwise.

Reroll the patch #59 with Drupal 9.5.x

Re-rolled patch #66 with Drupal 10.1.x

seems to have a test failure

Did not test or review.

Thanks to those who have worked on this. The test fail is in a test introduced in this MR and appears to be worth researching and fixing.

I updated to issue summary based on my review of this issue and the related symfony issue.