Please, followe the steps provided in issue: Bug in AbstractUri::buildQuery - invalid HTML emitted ('[' and ']' not encoded in tagged elements list) #17365 (joomla/joomla-cms#17365 ).

So you can test the invalid html emitted.
Removing urldecode will emit valid html.

B/C break?

Good question. In a sad bit of irony, that method has zero test coverage so we don't even have a valid assertion on what it should be doing right now and what this changes.

So even if the issue and fix are both valid (I'm not saying they aren't, it's just difficult to follow at first glance), we really need to find the code that's feeding data into the Uri class chain, extract that data, and turn it into a test case.

The problem is visible when URL SEF is not active as we get queries liek that:
option=com_tags&view=tag&id[0]=3&types[0]=2&Itemid=110
where [ and ] are not encoded

I'm shooting blind here but hopefully a2b635c gives us a code test case to validate the behavior.

1) Joomla\Uri\Tests\UriTest::testBuildQueryEncoding
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'option=com_tags&view=tag&id[0]=3&types[0]=2&Itemid=110'
+'option=com_tags&view=tag&id%5B0%5D=3&types%5B0%5D=2&Itemid=110'

So is the intended end state that buildQuery returns an encoded URL? Or am I misunderstanding the issue?

Looking at this result, I'm inclined to lean toward the current behavior being the preferred behavior as encoding is something you need to handle at the presentation layer (anything spitting out HTML), I'm not so sure the library should default to returning encoded query strings. Either way I need to think about this when I'm not quickly glancing in the middle of my lunch break at things.

Yes, my intended end state was that buildQuery returned an encoded URL.
May be you are right saying that the encoding should be done at the presentation layer. I'm not sure.
I suspect that fixing this at the presentation layer will request many modifications. For sure it will request modifying the login and logout modules (please see #17368 and #17387

To me, I see two issues with this fix:

1. It's a B/C break, the consumer would have to handle encoding/decoding to get the query string usable in their desired manner and that way won't always be an HTML document (this class gets used in our HTTP adapters to build and process requests, so the adapters would probably have to decode the URL before setting it to the underlying API).
2. It changes the logic of the class to be very heavily geared toward HTML output. Unfortunately this is a class that has arguably more use cases outside of it being output into an HTML context than it does within HTML.

If we had an HtmlUri class that were used then this would probably be fine within that scope. But at this level I think it has more potential to be disruptive than not.

In the whole Joomla codebase AbstractUri::buildQuery is called 3 times in 3 files (Joomla! 3.7.4):

1. in AbstractUri::setType (administrator\components\com_menus\controllers\item.php @ 544)
   Here we are in a menu item controller. I think this can be considered a presentation layer so the url-query should be html-valid.

2. in JUri::buildQuery (libraries\joomla\uri\uri.php @ 302)
   Its use is analogous to AbstractUri::buildQuery

3. in AbstractUri::getQuery (libraries\vendor\joomla\uri\src\AbstractUri.php @ 190)
   The use of this function should be inspected to se if the callers of this function expect to have a decoded or encoded url-query.

I do have to say, scanning for direct uses of getQuery is a painful task thanks to the database API. With that said, I did find two direct calls to getQuery that returned a query string (not passing the flag to return the vars as an array) where an encoded URL would be less than preferential, both places are being used to calculate URLs to be redirected to (so they would never hit the presentation layer).

With that said though, more frequently the getQuery method is being called as a result of converting an AbstractUri object to a string (either through the toString method or casting the object). And it's that code path that will be the most problematic with this change without inspecting every existing case in our core code. And I will say there are several places which get a query string which should not be encoded.

Ok, so we have to find a different solution.
The problem seems to arise just with tagged elements menu-items.
I think I should close this PR. Right?

Where do you suggest I can start for a new solution?
Is this related to tag-component routing? Or to menu component? Or what else?
Thanks.

So if I'm not mistaken, the main culprit here is if you configure a tag menu item (and only that with "normal" CMS use, I'm not finding any other code path that could generate such a URL with com_tags specifically). The only place we really "control" menu item rendering is mod_menu, so the only part of core that we could make sure that's handled correctly in is there. You might get away with changing the module's helper class to ensure the links are properly encoded/decoded so layouts don't have to be touched, otherwise you would have to change the layout files and that would only fix core and any template not overriding those.

Closing this PR, feel free to keep posting here if needed though.

I've looked where you suggested.
It seems to me that there are a few options.

1. saving in the menu-item table a link containing urlencoded '[' and ']'.
   I think it's not a good idea, that really seems not 'presentation layer'; it should be investigated if this is a B/C as I fear.

2. modify jRoute constructor to build a url-encoded query part.
   This is based on Uri/AbstractUri::toString which uses getQuery / buildQuery.
   So we return to my original idea to fix AbstractUri::buildQuery

I've been using solution 2) for 2 years now in some production sites that use various extensions without any problem, so in practice to me it has not been a B/C.

Any ideas?
Thanks

Opened a related New Issue and proposed a solution with narrower impact: Issue #17966