@julienV can you please test as you reported Issue #19130?

Yes, i will test asap, maybe just not today...

Is there a chance to test this week?

tested, works as expected

tested successfully

I have tested this item ✅ successfully on 1e64a41

With/without PR, I followed the test instructions #19130 and it redirected to the home page instead of article category list page.

The New button URL has the +:
/~user/joomla/component/content/?task=article.add&return=aHR0cDovL2xvY2FsaG9zdC9+dXNlci9qb29tbGEvYXJ0aWNsZS1jYXRlZ29yeS1saXN0&a_id=0&catid=19

well, worked fine with me the method with module editing: without patch, went back to index, with patch, went back to initial page.
are you sure you refreshed properly after applying the patch (i'm asking before it happened to me... patch, go back to index, then to another menu item, edit module, save and close)

Yes. To reproduce:

• Install staging.
• Log in on front end.
• Click Article Category List.
• Click New button.
• Click Cancel button.
• Redirects to home page.

On a site without ~, it redirects to the article category list page.

you are right, the base64 return link on the 'new' button stll has a +

it's because of this:
https://github.com/joomla/joomla-cms/blob/staging/components/com_content/helpers/icon.php#L49

the return uri will be decoded when passing through JRoute::_() -> you get the +

I have tested this item 🔴 unsuccessfully on 1e64a41

see PR comments, having issue with new button from icon content helper.
this might happen elsewhere as well...

Yes.
There is a problem with JRoute::() which decode %2B back to +.
I can use urlencode($return) twice in JRoute::() or try to fix JRoute::() which is not easy and it will break unit tests.

This create our problem:

May be I should move the value of return to outside: JRoute::_('...&return=') . urlencode($return)

Indeed, that is what i wrote in the issue: the problem is that JRoute::_ calls UriHelper::parse_url() , which decodes the params.
I don't think double url_encode is a good solution, it's a hack.
Adding the return outside of JRoute::_ is better, but you need to make sure if there is already a ?var=xx in the returned url, as sef components could return an url without get variables.

I fixed previous issues. Please test.

The best way would be to remove urldecode from:

and replace it by something better but this would break B/C.

I have tested this item ✅ successfully on 43c2028

This works, but it feels wrong... I agree with you it's JRoute::_() that should be fixed. I am not sure why we urldecode is used there, maybe this needs to be forwarded to people that coded this initially, or have a better understanding of the router maybe (not saying you don't, but speaking for me)

I tried to fix JUri and remove urldecode and decode only a few chars like ('[', ']') but it changes behaviour and unit tests fail.

The issue was introduced at:
https://github.com/joomla-framework/uri/blob/b91252fe255a7da9f8ab365750db693891d3324d/src/AbstractUri.php#L310

The last changes was done a long time ago.

I looked at drupal and wordpress:

• http://www.drupalcontrib.org/api/drupal/drupal%21core%21lib%21Drupal%21Component%21Utility%21Url.php/function/Url%3A%3AbuildQuery/8
• https://developer.wordpress.org/reference/functions/_http_build_query/

and they do not use urldecode.

I'll put this on Glip's channel.

7 days ago I asked for advice on Glip channel but there is no interest.

In this PR I use two solutions:

1. I used str_replace('return=return', 'return=' . $return, $link) because it is safe after JRoute/Juri will be patched.
2. I used urlencode(urlencode($return)) where I do not have a full link but only partial query available.
   In that situation I added a comment because it is not safe after JRoute/Juri will be patched.
   In this situation I can not use point 1 because it breaks B/C.

Using method like JRoute::_('...&return=') . urlencode($return) is wrong too because JRoute may add a new parameter &Itemid=... and then we end with &return=&Itemid=....

IMHO I could try to change JUri::buildQuery but it breaks B/C and has to wait till J4.

@julienV If you mark it as test success then maintainers will decide.

Due to lack of interest, I close it.