Fix trailing slash used in url rewrites

[ihor-sviziev]

Description

Admin user configured two URL rewrites in Admin UI. Frontend user may request the page with or without trailing slash. In some case Magento will return 404 page.
We would like to avoid content duplication for the requests with and without trailing slash. If request path has configured url_rewrite (w/ or w/o trailing slash) redirect to this page automatically. If requested path matches configuration, just follow redirect rules.

#	request_path	target_path	User Request	Actual Response	Expected Response
1	page-one/	page-A/	http://magento.com/page-one	404	1. Redirect to http://magento.com/page-one/ (301); 2. Redirect to http://magento.com/page-A/
			http://magento.com/page-one/	Redirect to http://magento.com/page-A/	Redirect to http://magento.com/page-A/
2	page-two	page-B	http://magento.com/page-two	Redirect to http://magento.com/page-B	Redirect to http://magento.com/page-B
			http://magento.com/page-two/	Redirect to http://magento.com/page-B	1. Redirect to http://magento.com/page-two (301); 2. Redirect to http://magento.com/page-B

Fixed Issues (if relevant)

1. Product URL Suffix "/" results in 404 error #4876 Product URL Suffix "/" results in 404 error
2. Slash as category URL suffix gives 404 error on all category pages #3872 Slash as category URL suffix gives 404 error on all category pages
3. Multiple URLs causes duplicated content #4660 Multiple URLs causes duplicated content
4. Custom URL Rewrite where the request path ends with a forward slash is not matched #8264 Custom URL Rewrite where the request path ends with a forward slash is not matched

Related pull requests

1. Fix trailing slash used a product URL suffix #7041 Fix trailing slash used a product URL suffix
2. Fixes problem with url rewrites not being matched when the request path has a trailing forward slash #8319 Fixes problem with url rewrites not being matched when the request path has a trailing forward slash

Implementation Notes

1. Use \Magento\UrlRewrite\Model\UrlFinderInterface interface implementation to resolve URL rewrites
2. Remove trim function call from Magento\UrlRewrite\Controller\Router::getRewrite. We must know what was the original user request.
3. Keep original request path. Just use ltrim
4. Generate alternative path (on the PHP side only, not in DB). For example:
   a. if request path is page-one, generate page-one/
   b. if request path is page-one/, generate page-one
5. Fetch URL rewrites from database using single query. For example: select * from url_rewties where request_path in (page-one, page-one/) and store_id = .....
6. Database query may return 0, 1 or 2 rows
   a. 0 rows - redirect 404 not found
   b. 1 row:
   - if request path matches the DB value, just follow redirect rules
   - if request path does not match the DB value, redirect (301) to request_path from DB results and then the system will work according to b.i scenario
   c. 2 rows: use the row that matches user request and follow redirect rules

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[okorshenko]

Hi @ihor-sviziev
Thank you for Pull Request.

Please let's implement the logic that you suggested:

if request path doesn't match the DB values, if it's redirect (301 or 302) - follow redirect rules from DB results (to have only one redirect); if it's not redirect (category or product page) - redirect to request_path from DB. It will reduce negative effect for SEO and a little bit server load (1 request instead of 2).

[okorshenko]

@ihor-sviziev please let me know when you will be ready for review. thank you

[okorshenko]

CC: @redelschaap

[ihor-sviziev]

@okorshenko done. Could you review it?

I covered it with few tests, you can add or adjust them if needed.

[okorshenko]

@ihor-sviziev sure

[okorshenko]

From the brief look - it looks good. I will dig deeper into implementation and use cases right now. I would suggest to add integration test for this functionality. Unit test is ok, but integration test will be better in this case. Just create a fixture for URL rewrites and try different requests.

[okorshenko]

@ihor-sviziev Looks good. It would be great if you will create integration test to avoid regression on this. Thank you! great job! 👍

Also, please check this test: Magento\Cms\Controller\PageTest . There is some issue with this test after these fixes. For some reason I don't see Travis build for this PR.

[ihor-sviziev]

@okorshenko is it possible to cover it with integration tests by someone from magento team? I don't have any experience in writing integration tests.

[okorshenko]

@ihor-sviziev Sure. We will take care about this

[ihor-sviziev]

@okorshenko thank you for fast processing my PR. These changes not fully backward compatible because they are changing behavior in some cases to redirect, can we add them in 2.1?

[redelschaap]

@ihor-sviziev great work man!

[okorshenko]

Hi @ihor-sviziev I think yes. If you would like to have this fix in 2.1, just create a PR to 2.1-develop branch and we will try to process it.
Thank you