Fixed #18717 UrlRewrite removes query string from url, if url has trailing slash

[shikhamis11]

Fixed #18717 UrlRewrite removes query string from url, if url has trailing slash

Preconditions (*)

When customer opens url with trailing slash and query string like http://magento-host.com/some-url-key/?foo=bar, Magento redirects to url without trailing slash and truncates query string, so customer is redirected to http://magento-host.com/some-url-key.

1. Magento version 2.2.5 or 2.2.6
2. No customizations for the store, Luma + Sample Data

Steps to reproduce (*)

1. Open any product or category page.
2. Manually add /?foo=bar&bar=baz to the url and open this link

Expected result (*)

1. After the page has been loaded, query string remain in url

Actual result (*)

1. Query string is removed from url

Additional information

1. The issue is not reproduced when we avoid using url rewrites, e.g. http://magento-host.com/catalog/product/view/id/{product-id}/?foo=bar&bar=baz, and actually there is no redirect in this case.
2. The issue seems to be happen in \Magento\UrlRewrite\Model\Storage\DbStorage::doFindOneByData

$data[UrlRewrite::REQUEST_PATH] = [
    rtrim($requestPath, '/'),
    rtrim($requestPath, '/') . '/',
];

We are looking for both urls (with trailing slash and without it), but when verifying the result from database we do not check it.

// If request path matches the DB value or it's redirect - we can return result from DB
$canReturnResultFromDb = ($resultFromDb[UrlRewrite::REQUEST_PATH] === $requestPath || in_array((int)$resultFromDb[UrlRewrite::REDIRECT_TYPE], $redirectTypes, true));

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)

[magento-engcom-team]

Hi @shikhamis11. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[shikhamis11]

#MM19IN

[ihor-sviziev]

Looks like it's related to my very old fix of #10043

[ihor-sviziev]

Hi @shikhamis11,
Could you describe how trimming of slash at the end of request path will allow to fix issues with get parameters?

---

Ah, I understood, you will not have 301 redirect in case your request was /some-url-key/?a=b, but in URL rewrites table - /some-url-key, it causes regression and that's why unit tests are failing.

I think we should discuss expected behavior with @akaplya, he's assigned to Url Rewrites module.

After #10043 we have following actual & expected results without get parameters:

#	request_path	target_path	User Request	Actual Response	Expected Response	Is Valid
1	page-one/	page-A/	http://magento.com/page-one	1. Redirect to http://magento.com/page-one/ (301); 2. Redirect to http://magento.com/page-A/	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/	1. Redirect to http://magento.com/page-two (301); 2. Redirect to http://magento.com/page-B	1. Redirect to http://magento.com/page-two (301); 2. Redirect to http://magento.com/page-B	👍

I think for URLs with get parameters we should the same behavior.
So table with actual & expected results should be following:

#	request_path	target_path	User Request	Actual Response	Expected Response	Is Valid
1	page-one/	page-A/	http://magento.com/page-one?param1=value1	1. Redirect to http://magento.com/page-one/ (301); 2. Redirect to http://magento.com/page-A/	1. Redirect to http://magento.com/page-one/?param1=value1 (301); 2. Redirect to http://magento.com/page-A/?param1=value1	👎
			http://magento.com/page-one/?param1=value1	Redirect to http://magento.com/page-A/	Redirect to http://magento.com/page-A/?param1=value1	👎
2	page-two	page-B	http://magento.com/page-two?param1=value1	Redirect to http://magento.com/page-B	Redirect to http://magento.com/page-B?param1=value1	👎
			http://magento.com/page-two/?param1=value1	1. Redirect to http://magento.com/page-two (301); 2. Redirect to http://magento.com/page-B	1. Redirect to http://magento.com/page-two?param1=value1 (301); 2. Redirect to http://magento.com/page-B?param1=value1	👎

[shikhamis11]

hello @ihor-sviziev . Thank you for your comments . As I can see in the code the request path is being matched with results in the database so for avoiding mismatch due to trailing slash we can trim the slash from results and compare it

[ihor-sviziev]

Hi @shikhamis11,
As I said before - we can't just trim slash at the end. It will lead to regression in 301 redirects. See #10043 for more details.

I'm trying to contact to Magento Architects to define what is expected result. Until that time - no action items.

[shikhamis11]

ok thanks

[VladimirZaets]

Hi @shikhamis11. Thank for collaboration. I discused this case with architect that is responsible for UrlRewrite module and with product owner of this area and we took decision that the proposal provided by @ihor-sviziev above is correct. So, please update your fix according to rules described above.

[sidolov]

@shikhamis11 , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

[ghost]

Hi @shikhamis11, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.