When adding an external link to a menu, dots in query parameter names are converted to underscores, even though dots are allowed in query parameter names in general. This is a confusing experience for site owners trying to create links to external sites, the link is accepted, but its rendered differently.
This seems to be because Symfony uses parse_str() to convert parameters to PHP safe variable names. See https://github.com/symfony/symfony/issues/25541 It would be ideal for this to be fixed by PHP or Symfony instead of Drupal. However, it seems there's not much movement in that direction. Therefore, it could make sense for Symfony, or Drupal, to have a function used to parse_str for the scenario of rendering links to other sites.
To reproduce:
- From any installation of Drupal
- Add an external link to the main menu (https://www.drupal.org?first.name=Aaron&last.name=Wolfe)
- Expected behavior: The url in the menu link should be https://www.drupal.org?first.name=Aaron&last.name=Wolfe
- Actual behavior: Instead, the menu item links to https://www.drupal.org?first_name=Aaron&last_name=Wolfe
This also happens with redirects, and I imagine any other place that prepares URLs with query strings.
Comment | File | Size | Author |
---|---|---|---|
#67 | 2984272-67.patch | 11.29 KB | Shubham Chandra |
#66 | fix-mangled-query-parameter-names-2984272-66.patch | 11.29 KB | Ankit.Gupta |
#59 | fix-mangled-query-parameter-names-2984272-59.patch | 11.91 KB | ao5357 |
Issue fork drupal-2984272
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
awolfey CreditAttribution: awolfey at Capellic LLC commentedComment #3
awolfey CreditAttribution: awolfey at Capellic LLC commentedUrlHelper.php
seems to be the problem at line 164I'm not sure why the the query string of an external url should have parameter names parsed into php variables.
Comment #4
Wilfred Waltman CreditAttribution: Wilfred Waltman as a volunteer commentedI 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.
Comment #5
Wilfred Waltman CreditAttribution: Wilfred Waltman as a volunteer commentedCreated new patch. Basically the same as #4 but compliant to coding standards. I know, should have been paying more attention the first time.
Comment #6
Wilfred Waltman CreditAttribution: Wilfred Waltman as a volunteer commentedComment #7
seanBTriggering 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?
Comment #8
seanBLooks good to me. This probably needs more reviews from the menu/link maintainers.
Comment #9
awolfey CreditAttribution: awolfey at Capellic LLC commentedThis solves my issue with the menu and redirects (redirect module). Thank you!
Comment #10
borisson_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.
Comment #11
Anas_maw CreditAttribution: Anas_maw as a volunteer commentedThis solves my issue too.
Thanks
Comment #12
seanBDrupal\Tests\Core\UrlTest
is a unit test?Comment #13
borisson_/headdesk - yes it is. I missed that, sorry. Setting to RTBC to move this up the chain to a subsystem/core maintainer.
Comment #14
alexpottAre 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.
Comment #15
catchAgreed with #14. Is there a PHP bug report for this already?
Comment #16
awolfey CreditAttribution: awolfey at Capellic LLC commentedI think that if dots are allowed in query parameter names, which they are from what I can tell, then we should allow them.
Our clients are linking to third party services that have dots in parameter names.
Comment #17
alexpottThe 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 beOne 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
Comment #18
alexpottComment #19
charly71 CreditAttribution: charly71 commentedAny backporting on 8.6.x versions?
Thanks
Comment #20
charly71 CreditAttribution: charly71 commented#16 +1
Comment #22
idebr CreditAttribution: idebr at iO commentedClosed #3044507: Fix dots and spaces changing to underscores in destination query parameter names as a duplicate issue.
Comment #23
Darren OhComment #24
Darren OhThis 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.
Comment #25
Darren OhComment #28
om_kha CreditAttribution: om_kha as a volunteer commentedPatch in #24 does not resolve nested query parameters e.g ?param[nested][0]=value¶m[nested][1]=2ndvalue.
Comment #29
Rithesh BK CreditAttribution: Rithesh BK as a volunteer and at Valuebound for Valuebound commentedcurrently checking the code for 8.9.x-dev ..... . i am working on it
Comment #30
mradcliffeI 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.
Comment #31
Darren OhI have corrected the tests and accounted for the possibility of name collisions caused by parse_str changing parameter names.
Comment #34
PolHello,
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.
Comment #35
om_kha CreditAttribution: om_kha as a volunteer commentedAdds urldecode for name parameter and merge parsed parameters recursivly.
Comment #36
rob_pr CreditAttribution: rob_pr commentedPreserve integer keys when recursively merging nested parameters.
Comment #37
aaronpinero CreditAttribution: aaronpinero as a volunteer commentedI 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
Comment #38
Darren OhDon't forget to update the issue status.
Comment #39
RenrhafConfirming that #36 resolves the issue on a Drupal 8.8.6 and PHP 7.4.7. Thanks!
Comment #40
alexpottSo 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.
Comment #41
RenrhafAs mentioned by javiereguiluz in https://github.com/symfony/symfony/issues/29664#issuecomment-512147641 :
Code from #36 issues PHP warning when $parsed[$name] is not an array at the following line :
Comment #42
aaronpinero CreditAttribution: aaronpinero as a volunteer commentedI 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.
Comment #43
jberube CreditAttribution: jberube commentedI 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.
Comment #44
aaronpinero CreditAttribution: aaronpinero as a volunteer commentedI 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.
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.
Comment #45
aaronpinero CreditAttribution: aaronpinero as a volunteer commentedAdding patch again with correct naming convention
Comment #46
aaronpinero CreditAttribution: aaronpinero as a volunteer commentedPatch in #45 was broken, trying again
Comment #47
aaronpinero CreditAttribution: aaronpinero as a volunteer commentedOkay, 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.
Comment #49
jberube CreditAttribution: jberube commentedIn 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.
Comment #50
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedFixed the failed CS issues.
Not able to generate interdiff. getting error.
Comment #52
jberube CreditAttribution: jberube commentedThe #49 patch had an error that was causing tests to fail. Here's a redo.
Comment #53
jberube CreditAttribution: jberube commentedI missed a test.
Comment #54
jberube CreditAttribution: jberube commentedI missed another test.
Comment #56
cbanman CreditAttribution: cbanman at Acro Commerce commentedI'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.
Comment #59
ao5357 CreditAttribution: ao5357 commentedHere's an after-the-fact `git format-patch` file with the same changeset as the issue fork and merge request, for composer-based patching.
Comment #60
jeetmail72Quick 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',
];
}
}
}
Comment #64
tlo405 CreditAttribution: tlo405 commentedThe 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.
Comment #65
catchThe 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.
Comment #66
Ankit.Gupta CreditAttribution: Ankit.Gupta at Dotsquares Ltd. commentedReroll the patch #59 with Drupal 9.5.x
Comment #67
Shubham Chandra CreditAttribution: Shubham Chandra as a volunteer and at Dotsquares Ltd. commentedRe-rolled patch #66 with Drupal 10.1.x
Comment #72
Darren OhComment #73
smustgrave CreditAttribution: smustgrave at Mobomo commentedseems to have a test failure
Did not test or review.
Comment #74
gregglesThanks 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.