Fix #20309 - URL Rewrites redirect loop

[Bartlomiejsz]

Description (*)

This is fix for #20309 - as @ihor-sviziev mentioned in #25848 (review) - adding validation for such redirect wouldn't fix the issue for i.e. existing stores. So I added check in url rewrite router, which doesn't allow to redirect if request path is equal to target path.

Related Pull Requests

Fixed Issues (if relevant)

1. URL Rewrites redirect loop #20309: URL Rewrites redirect loop

Manual testing scenarios (*)

Please check fixed issue for steps to test

Questions or comments

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 are green)

[m2-assistant]

Hi @Bartlomiejsz. 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 give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[ihor-sviziev]

@Bartlomiejsz,
Your changes looks good, but there is some nitration test failure. Could you fix it?

[Bartlomiejsz]

@ihor-sviziev done

[ihor-sviziev]

@Bartlomiejsz,
URL rewrites it quite complex thing, and there is a lot of potential issues. Thank you so much for covering your changes with unit tests, but I think it's better to cover this change with integration tests. Will you be able to do that?

Recently there were fix for URL rewrites, it was covered by integration tests, I think it's good example. Use cases were defined here:
#18717 (comment)

I've did integration test coverage for such cases:
b1a2a29

Note: seems like there not just one case, there should be also checked cases with get params

Pull Request state was updated. Re-review required.

[ihor-sviziev]

See above

[Bartlomiejsz]

Hi @ihor-sviziev,
I don't really have experience with writing integration tests, so if that would be possible for you that would be great.
Otherwise it will take some time for me to prepare those but can try - just please let me know

[ihor-sviziev]

@Bartlomiejsz not sure if I'll have free time to help you. But actually integration tests are quire similar to and even maybe simpler than unit tests, let's try.

Please try following instruction:
https://devdocs.magento.com/guides/v2.3/test/integration/integration_test_execution.html

[ihor-sviziev]

@Bartlomiejsz Good job! Approved!

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-6995 has been created to process this Pull Request

[engcom-Delta]

@Bartlomiejsz , @ihor-sviziev this PR implements only one point from #25848 (review):

• adjust logic to do NOT perform redirect in case of target path equal to request path

But there is no implemented patch that will find & delete all such incorrect URL and no validation for same request-path and target_path in admin.
Now behavior with PR is next:

• Url Rewrite with request-path: foo and target_path foo with 301 or 302 redirect is successfully created
• Getting 404 Page not found error when such url was requested
  

Is this expected behavior of PR?

Pull Request state was updated. Re-review required.

[Bartlomiejsz]

Hi @engcom-Delta, as @ihor-sviziev mentioned there, those two options, and I'm pretty sure the one implemented in current PR makes second not needed. Because even with those rewrites being still in database, no issue should be spotted on FE.
And for your example - I'm pretty sure this is correct. Please verify clean 2.4-develop for same case, you should receive ERR_TOO_MANY_REDIRECTS error. And with the fix, it redirects only if target_path is different then url on which you are currently.
So there is 404 if there is no such page, and if you will add rewrite with existing url (like contact -> contact) this should just show you the contact page

[ihor-sviziev]

@engcom-Delta i totally agree with @Bartlomiejsz . What you’ve shown - expected result. What you said - it could be improved, but main issue is fixed by this PR

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-6995 has been created to process this Pull Request

[engcom-Delta]

✔️ QA passed

[m2-assistant]

Hi @Bartlomiejsz, 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.