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 trailing slash used a product URL suffix #7041

Closed

Conversation

sitepodmatt
Copy link

@sitepodmatt sitepodmatt commented Oct 16, 2016

In reference to issue #7040
Seems a couple of other people are complaining too e.g. http://magento.stackexchange.com/questions/107477/slash-as-category-url-suffix-gives-404-error-on-all-category-pages

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Oct 16, 2016

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@redelschaap redelschaap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes an issue I had with URLs with trailing slashes.

@Ctucker9233
Copy link

Please merge.

@sitepodmatt
Copy link
Author

Is there any ETA on merging this?

@@ -129,7 +129,7 @@ protected function redirect($request, $url, $code)
protected function getRewrite($requestPath, $storeId)
{
return $this->urlFinder->findOneByData([
UrlRewrite::REQUEST_PATH => trim($requestPath, '/'),
UrlRewrite::REQUEST_PATH => ltrim($requestPath, '/'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to cover such an important case with integration test also.

@hostep
Copy link
Contributor

hostep commented Feb 15, 2017

I recently added the same PR, but it was rejected: #8319, I wasn't aware there was already a similar PR, sorry about that.

@orlangur: maybe talk with @okorshenko & @miakusha before proceeding?

@orlangur
Copy link
Contributor

Thanks for reference, @hostep, I was not aware of your PR as well.

Then this PR should be rejected for similar reasons and integration test should be written for new trimming slashes behavior when saving.

I'd recommend trim values provided in admin UI before saving to DB (new behavior)

Import should not be forgotten also.

@onepack
Copy link

onepack commented Mar 7, 2017

This saved me! But it only works when I put the change in the vendor/magento directory and not in app/code/magento.
Can anyone explain why?

@orlangur
Copy link
Contributor

orlangur commented Mar 7, 2017

@onepack it's because of composer autoload precedence. Just override core classes properly instead of in place editing. Here is some quickly googled link: http://stackoverflow.com/a/39424354

For this kind of issue better use Stack Overflow directly.

@onepack
Copy link

onepack commented Mar 7, 2017

@orlangur Thank you! Indeed, this belongs to Stack overflow.

@vrann vrann self-assigned this Mar 23, 2017
@vrann vrann added this to the March 2017 milestone Mar 23, 2017
@vrann
Copy link
Contributor

vrann commented Mar 23, 2017

see #8319

@vrann
Copy link
Contributor

vrann commented May 18, 2017

@sitepodmatt please see the discussion here: #8319

This fix dos not take into account the current use-case:
redirect: "test" => "contact"
http://magento.loc/test/ => http://magento.loc/contact

In order to make the complete fix next cases should be supported:

# redirect behaviour
1 "test" => "contact" http://magento.loc/test/ => http://magento.loc/contact
2 "test" => "contact" http://magento.loc/test => http://magento.loc/contact
3 "test/" => "contact/" http://magento.loc/test/ => http://magento.loc/contact/
4 "test/" => "contact/" http://magento.loc/test => http://magento.loc/contact/

@redelschaap
Copy link
Contributor

@vrann I disagree with you that http://magento.loc/test/ should redirect to http://magento.loc/contact when a redirect "test" => "contact" has been set. And many SEO experts would share that opinion with me, as /test/ is a completely different URL than /test from SEO perspective. /test should be able to redirect to (or load) a different URL than /test/.

If Magento wants still this behaviour, I think it should be a config setting to support whatever the store owner prefers - SEO optimization or convenience.

@vrann
Copy link
Contributor

vrann commented May 19, 2017

@redelschaap configuration makes sense, otherwise, the change will be backward incompatible

@vrann
Copy link
Contributor

vrann commented May 19, 2017

@redelschaap Thanks to @pboisvert I read this article https://webmasters.googleblog.com/2010/04/to-slash-or-not-to-slash.html which suggest a) both "test/" and "test" always serve same content b) have one redirected to another with 301 c) be consistent on which one getting redirected

If we apply these suggestions to URL rewrites, the current behavior redirecting "test/" and "test" to the content seems aligned with the requirements. Requests "test" and "test/" should not be redirected to the different urls. Do you agree?

@tdgroot
Copy link
Member

tdgroot commented May 19, 2017

FWIW, I think every suffix should be supported and configurable. There shouldn't be a forced behavior how Magento redirects the requests.

To make your life easier; just don't use a suffix. It's readable and SEO-friendly. It also keeps you from this troubling conversation.

@redelschaap
Copy link
Contributor

redelschaap commented May 20, 2017

@vrann I agree with the article, but theoretically /test and /test/ are two totally different URLs, so in theory they should be able to serve different content and/or to redirect or to two different URLs. I also agree with @tdgroot in that every suffix should be supported and configurable.

One other reason two support this, is because webdevelopment companies (like ourselves) have clients with existing websites which need to be migrated to Magento 2. URLs ending with a / traditionally indicated a folder which could have one or more child pages or folders. URLs ending without a / (or with an file extension) indicated a page which was last in the chain of structure, which could not conain more pages or folders. We have clients with existing websites (and SEO history) where folders (categories and CMS pages) end with a /. They want to maintain that behaviour, to keep their SEO status on those pages. I know we can (manually) redirect those URLs, but that costs a percentage of the SEO status.

It would indeed improve usablility and maybe even conversions if /test would redirect to /test/ when the suffix is set to / (or vice versa). One should not be forced to use one or the other, though.

@vrann
Copy link
Contributor

vrann commented May 22, 2017

@redelschaap if we agree that the article makes sense, the default scheme of url rewrite resolution should be that both "test" and "test/" are redirected to the same page.

The fallback scheme might be to have the possibility to configure "test/" pointing at a different location if that is done on the purpose. So that if the rule is defined for either one, "test" or "test/", both requests will be redirected to the same resource; if the rule is defined for both of them, it will match exactly the needed resource.

It sounds like larger PR with more logic. I would argue that the use-case is limited to legacy systems with the rewrites configured not in an optimal way, though if you observe big demand on it, it makes sense to have such PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.