W3C HTML5 Validation error with PHP array-based query-strings built with url() containing [ ] characters (eg views filters)

This is never a problem in practice, but we should fix it anyway.

You patch leads to $key being encoded twice, so needs work for that. We probably also need to fix the tests.

-    $key = ($parent ? $parent . '[' . rawurlencode($key) . ']' : rawurlencode($key));
+    $key = ($parent ? $parent . rawurlencode('[' . $key . ']') : rawurlencode($key));

You can move the rawurlencode() outside the ternary operation.

-    $this->assertEqual(drupal_http_build_query(array('a' => array('b' => '2', 'c' => '3'), 'd' => 'foo')), 'a[b]=2&a[c]=3&d=foo', 'Nested array was properly encoded.');
+    $this->assertEqual(urldecode(drupal_http_build_query(array('a' => array('b' => '2', 'c' => '3'), 'd' => 'foo'))), 'a[b]=2&a[c]=3&d=foo', 'Nested array was properly encoded.');

No, please check the result explicitly. Modified like this, this test would pass with our without your patch :)

Same patch as above, reworked with Damien's changes.

The test result says this passed, but the message here seems to be stuck at test request sent. What's up testbot?

Has this been reviewed/committed in D8 so that it can be backported to D7?

wording improvement

Hi Waverate, I applied the changes shown in the patch from comment #6 to D7 and they work without modification. So the code works on D7.

Any chance this can get committed into D7 and D8 core (and D6 if someone can test it?) ? We applied this fix and were able to pass w3c validation on our facet search page.

Anyone else using this patch?
Thanks,

renamed patch to follow naming convention (description_description-NODEID-COMMENTNUMBER.patch ) thus w3c_url_validation_html5_changes-1827854-12.patch

Not sure why #12 failed (it is a copy of #6),
So use #6 instead, it passed.

Closed the D7 backport issue, #2289867: [D7] W3C HTML5 Validation error with PHP array-based query-strings built with url() containing [ ] characters, as a duplicate.

I kind of doubt this is backportable, but I'll tag it and the committers can make the call.

#6 needs a reroll.

Rerolling... Looks like the majority of this patch has already been done.

The tests in
- core/modules/system/lib/Drupal/system/Tests/Common/UrlTest.php:testDrupalGetQueryParameters
may be sufficient now. Someone else can look at these if they would like some more.

The patch 5 works on D7. I've got over 300 contrib modules running for quite some time using this patch and no side-effects yet seen. The client web browser handles decoding and nothing really changes from the drupal side of things except for properly encoding the two square brackets before generating a link.

The simpleTest failure seen above is actually a problem with UrlHelperTest.php .

Line 40 of UrlHelperTest.php should be changed..

It's now:

      array(array('a' => array('b' => '2', 'c' => '3'), 'd' => 'foo'), 'a[b]=2&a[c]=3&d=foo', 'Nested array was properly encoded.'),

Should be changed to:

      array(array('a' => array('b' => '2', 'c' => '3'), 'd' => 'foo'), 'a%5Bb%5D=2&a%5Bc%5D=3&d=foo', 'Nested array was properly encoded (with square bracket encoding).'),

%5B is the [
%5D is the ]

If you look at line 37, 38 , 39 they have the encoding for special characters using % but line 40 does not have the encoding.

So, the code is right, the test case UrlHelperTest.php needs to be updated for the new patch to pass testing .

Click here to view the source code for the test case

Thanks

See comments #5, #6 and #18

We may be able to test this in more than one D7 environment, but as Joseph reported we've seen no problems.

This doesn't reroll but does need further work.

We've been using this w3c validation fix to core for months now and it works.

Obviously the simple test needs to be adjusted, but the actual patch from superspring in comment #5 is good.

Anyone know how to adjust/update the simplytest? Obviously it's checking for the square bracket [ character instead of the uriencoded characters

for instance "[" is uri encoded as "%5B"
and "]" is uri encoded as "%5D"

the patch does this correctly but the simply test keeps checking for the non-encoded characters [ and ].

Please adjust the simplytest accordingly and then the patch will pass testing. If we can get this included in core then Drupal core will become HTML5 w3c compliant.

Rerolled

So for D8 we have the following comment:

   * @todo Remove this function once PHP 5.4 is required as we can use just
   *   http_build_query() directly.

We're still using the D7 version of this patch.

This is all good, until you paste such a URL as a target into the Redirect module. At that point the % gets encoded again, resulting in %255B and %255D, respectively. Filter query broken. You have to put the [] back into the URL before using it anywhere else in Drupal. Things like this make me want to throw Drupal out and write my site from scratch.

In response to dawehner and using "http_build_query() directly" (instead of rawurlencode?) , the official system requirements page says that 5.4.5 or higher is required for D8. So that means that the D8 patch could now use http_build_query()

"Drupal 8: PHP 5.4.5 or higher"

cdonner in comment #27 you mentioned the redirect module. I installed and tested the "redirect" module (version 7.x-1.0-rc1) in combination with this core patch and the redirect worked correctly for a link that contained square brackets and there was no problem dealing with the patch that url encodes the square brackets. The redirect of url encoded square brackets still works correctly after patching. The client browser interprets the square brackets correctly. Our production D7 site has been using the D7 version of this patch since it was created and we have over 400 modules and features and not one of them has had a problem with this.

Just applied and reviewed the patch in #24. Changes seem sensible to me and I haven't found any breakage.
Re-uploaded the patch to make it run on the new test infrastructure.

My colleague mdeletter has reviewed this issue but is being hindered by the fact that is not a "confirmed user" yet. His comment got lost in translation which is why I am posting his comment for him.

Reviewed and tested the patch in #24. The changes seems sensible and I couldn't find any breakage.
Re-uploaded the patch to run it on the new test infrastructure.

Please credit mdeletter on commit for reviewing the patch.

Also D.O bugged out with his patch upload, which is why I am re-uploaded the patch again.

In case it wasn't already clear, patches #30 and #32 are identical to patch #24 and only re-uploaded to make sure they are running in the new testing infrastructure.

Can someone confirm that we have test coverage of making a request using square brackets and it still working as expected after this change?

@alexpott,

Why does the way we generate a URL have anything to do with how a request is handled?

@legolasbo so I might not have asked the question probably - but what we need is confirmation that PHP still handles the query string as expected.

@alexpott, So if I understand you correctly, you want to make sure that a request to example.com/blog?tags%5B3%5D=3 and example.com/blog?tags[3]=3 results in the same page without errors?

We have been unable to locate any automated tests that verify this in D8. However, since the request is (AFAIK) handled by Symfony, they should have test coverage for this (assumption). I have manually tested the case mentioned above and it is handled as expected.

Testing the same patch as #32 for D7

Re #36 this is tested by symfony in HttpFoundation/Tests/RequestTest.php line 648:
2.8/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php

Linked to 2.8 as that is what D8 core is using.

array('foo[]=1&foo[]=2', 'foo%5B%5D=1&foo%5B%5D=2', 'allows array notation'),

This is confirming that the un-encoded query string gets encoded properly. Encoded query strings are handled as-is. I tried hacking in the example in #36 and it passed the same test.

Would love to see this move towards RTBC for D8 here so we can get the D7 backport in.

Rerolled for 8.3.x

@jamesrward , looks like 4 new (failing) tests also need the conversion from [] to %5B%5D

@alexpott after this change nothing breaks , this is because the webbrowser automatically converts the square brackets , what is broken is w3c compliance (as silly as it is in this case IMHO ) when drupal without the patch renders links with [] , then when the w3c validator runs, it sees the square brackets and flags it as non html5 compliant. The patch does not prevent square brackets from being used (thats actually handled by web browsers when you click on a link). What the patch actually does is make Drupal link rendering be html5 compliant.

been using the D7 patch with hundreds of contrib modules and custom modules , no problem. Did not have to rewrite any code in my contrib or custom modules.

Here's a patch and an interdiff with the BigPipe tests fixed. Basically these tests expected to put square bracket arguments in and get square bracket arguments out. Our patch urlencodes those brackets so the input and output no-longer match. Per #45 I don't think we are actually breaking BigPipe with the patch in #42, just the tests. To me this is a good indicator that we need to get this fix in soon. The longer core stays like this, the more places we are going to have to hunt down and replace non-W3C compliant tests.

I think the only thing left to decide here is if this goes on 8.3.x as a bug-fix or waits for 8.4. Paging @alexpott

@jamesrward, yes you are correct, just the tests needed adjusting.

Given http_build_query does this:

>>> http_build_query(['a' => ['foo' => 'bar']]);
=> "a%5Bfoo%5D=bar"

Our own equivalent will now do the same.

Let's get this done. I considered whether or not we need a CR for this and I don't think so. The return of \Drupal\Component\Utility\UrlHelper::buildQuery() is still a valid URL rawencoded string - only more so.

I guess it is technically possible that someone somewhere is doing preg_match on the output but that'd be wrong... you'd do:

>>> parse_str(http_build_query(['a' => ['foo' => 'bar']]), $result)
=> null
>>> $result
=> [
     "a" => [
       "foo" => "bar",
     ],
   ]

I tested core's menu form for @cdonner's comment #27.
I installed standard and did an advanced search after logging in as user 1. This gave me the beautiful url of search/node?keys=search%20term&f%5B0%5D=type%3Aarticle&f%5B1%5D=language%3Aen&advanced-form=1 and I used that as a menu link. All works great.

Committed 02d56af and pushed to 8.4.x. Thanks!

I will discuss backporting this to 8.3.x with the other committers.

I credited @Damien Tournoud for his review that identified an issue with the original patch. Thanks to everyone else for their comments.

Perhaps someone can open a follow up to convert \Drupal\Component\Utility\UrlHelper::buildQuery() to just called http_build_query() - it will be a minor performance improvement.

Discussed with @catch we agreed to backport to 8.3.x

The followup to use http_build_query() already exists - there are problems with it see #2322059: Keep the UrlHelper::buildQuery method and remove @todo

D7 patch is ready to go, been using it on several sites for over a year without issues.

@joseph.olstad, can you create a separate issue for D7? Our current process is to create separate issues for D7 if needed and just reference the D8 one in the related issues. (Also, none of the file attachments on this issue are D7 patches.)

@xjm backport issue re-opened at #2289867: [D7] W3C HTML5 Validation error with PHP array-based query-strings built with url() containing [ ] characters

#40 appears to be latest D7 ready so I will drop it into that issue for testing.

Thanks @jamesrward!

this is the only issue tagged with "w3c validator", moving to the other tag for posterity.

Years later, regressions found in facets module carrying over , still affecting probably both 7x and 8x/9x facets.

#2988736: Encoded characters are in the url

and

#3226121: Missing facet caused by inconsistent encoded url that confuses facets module, facets uses spaces but search_api_fulltext submits + symbols