Fixes problem with url rewrites not being matched when the request path has a trailing forward slash

[hostep]

See #8264.

[okorshenko]

@hostep Thank you for your contribution to Magento 2 project!

This pull request provides fix for the issue #8264, but at the same time introduces new issue:

fixed: [+] http://magento.dev/test/ --> http://magento.dev/contact/
new issue: [-] http://magento.dev/test --> 404

I'd recommend trim values provided in admin UI before saving to DB (new behavior) and trim request path in URL Rewrite processor (as it is now). In this case we will align write and read flows for URL rewrites.

It would be great if you can provide new Pull Request for the issue #8264. Unfortunately, we need to reject this Pull Request.

Thank you for your time and contribution!

[hostep]

Thanks for the review @okorshenko.

If I understand you correctly, a url rewrite where the request_path doesn't end with a forward slash should always match an incoming url both with and without a trailing forward slash?

Then it indeed makes sense to only save them without a forward slash in the database.
This also implies we'll need to create a database migration script which strips all trailing slashes in the database in the request_path column to remain backwards compatible, correct?

Unfortunately I don't see myself having time to create a new PR for this in the near future. If anybody else wants to create a fix like what is suggested above, that would be great!

[redelschaap]

@hostep From SEO perspective a URL with a trailing slash is completely different than a URL without a trailing slash. When URL's with or without a trailing are presenting the same page, search engines like Google can see them as duplicates. That is not what we want.

Because of that behaviour of search engines, URL's should still be stored in the DB with a trailing slash if the store configuration is set to a trailing slash.

[hostep]

@redelschaap: I think I agree, but it looks like the Magento folks don't agree.
We've been using the solution presented in this PR on a production environment for a couple of months now, without any downsides. But not sure if it won't cause any issues with all the different kind of configuration options available in Magento.

if the store configuration is set to a trailing slash

I never talked about this in #8264, so you are probably talking about another issue that might be involved with this url trailing slash behavior?

[redelschaap]

Well, then the Magento folks should reconsider their thoughts, because SEO is one of the most important parts of a great webshop these days.

Yes I am, but it is indeed related. PR #7041 was originally intended as a fix for my issue where category (or product) pages would return a 404 error page when the URL suffix for categories and/or pages was set to a forward slash. See also my question on StackExchange: http://magento.stackexchange.com/questions/107477/slash-as-category-url-suffix-gives-404-error-on-all-category-pages.

The solution offered in PR #7041 fixes my issue, but conflicts with this issue. Because from SEO perspective URL's with or without a trailing slash are completely different, I would not recommend to change that behaviour and let example.com/test return a 404 page when the URL key is set to test/. Additionally you/someone could write something that redirects to the URL with trailing slash if that URL key exists and one without a trailing slash doesn't.

[hostep]

@redelschaap: I again agree 100%, and this PR was going to fix your issue (it is the same solution as presented in PR #7041).
But it looks like @okorshenko rather has it that url's ending with or without a forward slash gets rewritten to the same url (unless I misinterpret his comment?) and that's why this PR has been rejected.

I'm still not sure if applying the fix presented in #7041 or in this PR won't cause any new issue, so maybe @okorshenko has a point?
Would be great if we could get a clearer comment from Magento as to why they want this behavior. :)