-
Notifications
You must be signed in to change notification settings - Fork 9.4k
[FIXES] #20309: URL Rewrites redirect loop #25848
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
Conversation
This adds a meaningful exception if request path equals target path on url rewrite edit page in admin
Hi @mautz-et-tong. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mautz-et-tong,
Your changes looks good to me.
Could you cover your changes with some tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I remembered that such PR we already had in #20313 (review).
With this solution we have few issues:
- If such URL rewrite was created before these changes introduced - issue will be persisted. In order to fix that we have two options (or both?)
- create data patch that will find & delete all such incorrect URL rewrites
- adjust logic to do NOT perform redirect in case of target path equal to request path
- URL rewrites could be created not only Magento admin, but also through custom modules, so would be better to move this validation into URL rewrite model itself.
Could you adjust your PR?
Actually I would do everything:
|
Hi @mautz-et-tong, |
Hi @mautz-et-tong, |
Hi @mautz-et-tong, thank you for your contribution! |
Description (*)
This adds a meaningful exception if request path equals target path on url rewrite edit page in admin
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)