Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for redirect url, duplicated url in redirect. causes more 404's #17997

Merged

Conversation

@tonypartridge
Copy link
Contributor

@tonypartridge tonypartridge commented Sep 19, 2017

Pull Request for Issue #17996 .

Summary of Changes

Have an absolute 404 generated i.e.

http://localhost/artriclessssss.html
find in redirect manager and redirect to:
http://localhost/articles.html

404 redirected to:

http://localhost/http://localhost/articles.html

Expected result

Redirects to
http://localhost/articles.html

To fix, install patch. Test both absolutely and relative redirects work.

Tony Partridge - xtech86 and others added 14 commits Aug 22, 2017
@tonypartridge tonypartridge changed the title Staging com redirects improvements Fix for redirect url, duplicated url in redirect. causes more 404's Sep 19, 2017
Copy link
Contributor

@wilsonge wilsonge left a comment

Looks good on review. Btw bugs introduced adds hours back onto the amount owed tony ;)

@ghost
Copy link

@ghost ghost commented Sep 20, 2017

I have tested this item successfully on 3569fd6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17997.

Copy link
Contributor

@HermanPeeren HermanPeeren left a comment

This fixes the symptom, not the cause why external URLS were wrongly redirected. The error is in line 282, where in strpos('http', $redirect->new_url) === false the needle and haystack is changed; it must be strpos($redirect->new_url, 'http') === false

@tonypartridge
Copy link
Contributor Author

@tonypartridge tonypartridge commented Sep 20, 2017

Great spot @HermanPeeren Updated now.

@ghost
Copy link

@ghost ghost commented Sep 20, 2017

I have tested this item successfully on 915e25f


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17997.

@HermanPeeren

This comment has been minimized.

Copy link
Contributor

@HermanPeeren HermanPeeren commented on 915e25f Sep 20, 2017

Still looks a bit strange to me, those lines 282-283: the string $redirect->new_url is passed to JRoute::_(). In https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Router/Route.php#L60 any URL-string not starting with '&' or 'index.php' is returned without any change. So, I'd say we could change those two lines in just:
$dest = JRoute::_($redirect->new_url);
and leave the ternary conditional out. Or did I overlook some reason to put those tests on external urls there?

This comment has been minimized.

Copy link
Contributor Author

@tonypartridge tonypartridge replied Sep 20, 2017

@HermanPeeren
Copy link
Contributor

@HermanPeeren HermanPeeren commented Sep 21, 2017

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?

@tonypartridge
Copy link
Contributor Author

@tonypartridge tonypartridge commented Sep 21, 2017

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.

@HermanPeeren
Copy link
Contributor

@HermanPeeren HermanPeeren commented Sep 21, 2017

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?

@tonypartridge
Copy link
Contributor Author

@tonypartridge tonypartridge commented Sep 21, 2017

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.

@HermanPeeren
Copy link
Contributor

@HermanPeeren HermanPeeren commented Sep 21, 2017

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.

@tonypartridge
Copy link
Contributor Author

@tonypartridge tonypartridge commented Sep 21, 2017

$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 ?

This comment has been minimized.

@Fedik

Fedik Sep 21, 2017
Contributor

need to test http only at 0 position, because it also can be at middle, as part of URL path (eg /my-http-stuff),
strpos($redirect->new_url, 'http') !== 0
I think

This comment has been minimized.

@HermanPeeren

HermanPeeren Sep 21, 2017
Contributor

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

This comment has been minimized.

@gojets

gojets Oct 2, 2017

Works for me. Thanks to all the contributors for working diligently on this issue!!!!

@tonypartridge
Copy link
Contributor Author

@tonypartridge tonypartridge commented Sep 21, 2017

@HermanPeeren
Copy link
Contributor

@HermanPeeren HermanPeeren commented Sep 21, 2017

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?

@tonypartridge
Copy link
Contributor Author

@tonypartridge tonypartridge commented Sep 21, 2017

@HermanPeeren
Copy link
Contributor

@HermanPeeren HermanPeeren commented Sep 21, 2017

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.

@tonypartridge
Copy link
Contributor Author

@tonypartridge tonypartridge commented Sep 21, 2017

@Ruud68
Copy link
Contributor

@Ruud68 Ruud68 commented Sep 21, 2017

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!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17997.

@ghost
Copy link

@ghost ghost commented Sep 21, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Sep 21, 2017
@mbabker mbabker added this to the Joomla 3.8.1 milestone Sep 25, 2017
@mbabker mbabker merged commit 656b1a4 into joomla:staging Sep 25, 2017
5 checks passed
5 checks passed
JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
@joomla-cms-bot joomla-cms-bot removed the RTC label Sep 25, 2017
@ghost ghost mentioned this pull request Sep 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.