I have tested this item ✅ successfully on 3569fd6

Great spot @HermanPeeren Updated now.

I have tested this item ✅ successfully on 915e25f

If $redirect->new_url starts with 'index.php' then JUri::isInternal($redirect->new_url) is true. Line 282 then sends the $redirect->new_url through JRoute::(). No difference with what I'm suggesting, always going through JRoute::().

The redirect-plugin supports relative urls only relative to the JUri::root().

I have no example of a redirect that would go wrong if the $redirect->new_url goes trough JRoute::_(). Am I overlooking something?

Exactly, so if it is True we want to parse it through JRoute as it is an internal URL, rather than redirect and then parsing if it's not already parsed.

Sorry, I don't understand your latest remark, @tonypartridge.

• I suggested the conditional in line 282 might be superfluous and we might just always go through JRoute.
• You said it was necessary for "support relative and non-relative urls so someone could start with index.php"
• I said that if a url starts with 'index.php' then JUri::isInternal() is true and so we go through JRoute; which is the same as I proposed (always going through JRoute).

Do you have an example of a redirect-url that would go wrong if we always go through JRoute?

I haven't tested or checked it enough to know, but by doing a simple check there is nothing wrong with this code. It works and doesn't pass external urls through JRoute even if JRoute supports external URLs.

That is a very practical way to look at it. I agree with you that the current code does not give a wrong result.

So please ignore my esthetical code considerations. I'll try to find time to make unit tests and refactor the code. For now the current code is useful and should be in J!3.8.1 as it repairs the 3.8.0 redirect mistake.

Great thanks Herman! I agree :-)

--
Tony Partridge

On 21 September 2017 at 10:47:18, HermanPeeren (notifications@github.com)
wrote:

That is a very practical way to look at it. I agree with you that the
current code does not give a wrong result.

So please ignore my esthetical code considerations. I'll try to find time
to make unit tests and refactor the code. For now the current code is
useful and should be in J!3.8.1 as it repairs the 3.8.0 redirect mistake.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#17997 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABVglkftR6PU-iAZvpsd47GwMi8lRdgDks5skjCmgaJpZM4PdIed
.

So your argument is to skip everything and just chuck it all through
JRoute?

--
Tony Partridge

On 21 September 2017 at 11:37:45, HermanPeeren (notifications@github.com)
wrote:

@HermanPeeren commented on this pull request.

In plugins/system/redirect/redirect.php
#17997 (comment):

@@ -279,9 +279,8 @@ private static function doErrorHandling($error)
$redirect->new_url .= '?' . $urlQuery;
}

•                           $dest = JUri::isInternal($redirect->new_url) || strpos('http', $redirect->new_url) === false ?
  

•                                   JRoute::_(JUri::root() . $redirect->new_url) :
  

•                                   $redirect->new_url;
  

•                           $dest = JUri::isInternal($redirect->new_url) || strpos($redirect->new_url, 'http') === false ?
  

@Fedik https://github.com/fedik +1
(which gives an extra argument for leaving out code that would be
superfluous, as it can introduce new errors, as demonstated here)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#17997 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABVgltRyeWPUsofDmvvtbOcpedW1TRIFks5skjx5gaJpZM4PdIed
.

Yes.
But be careful: my argument is solely based on reading the code and results should be tested thoroughly. That is why it is good to make tests before refactoring. Edge-cases like 'http' being part of the url but not on position 0 must be added to the testcases. As we have no unittests yet we should test the redirect-plugin for a variety of urls (internal, external, starting with 'index.php' or not etc.). Is it an idea to share such test-urls here?

@mbabker when is 3.8.1 due out? Since this is a high priority issue, we
could merge as is then run tests and refactor for 3.8.2?

Or we do it all now

--
Tony Partridge

On 21 September 2017 at 12:23:53, HermanPeeren (notifications@github.com)
wrote:

Yes.
But be careful: my argument is solely based on reading the code and results
should be tested thoroughly. That is why it is good to make tests before
refactoring. Edge-cases like 'http' being part of the url but not on
position 0 must be added to the testcases. As we have no unittests yet we
should test the redirect-plugin for a variety of urls (internal, external,
starting with 'index.php' or not etc.). Is it an idea to share such
test-urls here?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#17997 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABVglhvA_LDFVUyCRs5dIaIjgm6C45G0ks5skkdIgaJpZM4PdIed
.

I would go for merging as is a.s.a.p. in 3.8.1, because a lot of sites have issues (like failing redirects) with 3.8.0.

The error that @Fedik came with is also not very severe, because most likely those internal-urls with 'http' on a position >0 are likely to give JUri::isInternal()== true, so won't give much practical problems.

Then we have some more time to test and refactor with 3.8.2.

I agree completely

--
Tony Partridge

On 21 September 2017 at 12:45:50, HermanPeeren (notifications@github.com)
wrote:

I would go for merging as is a.s.a.p. in 3.8.1, because a lot of sites
have issues (like failing redirects) with 3.8.0.

The error that @Fedik https://github.com/fedik came with is also not
very severe, because most likely those internal-urls with 'http' on a
position >0 are likely to give JUri::isInternal()== true, so won't give
much practical problems.

Then we have some more time to test and refactor with 3.8.2.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#17997 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABVgljuS_S7pDV0rPVccLxROtrolBT6eks5skkxtgaJpZM4PdIed
.

I have tested this item ✅ successfully on 915e25f

Hi, just found out this issue... It is killing my #SEO as redirects don't work anymore, also all my old URLs are not accessible anymore from facebook / twitter / and the likes :(
Tested this patch successfully! Thnx Tony!

RTC after two successful tests.