-
Notifications
You must be signed in to change notification settings - Fork 82
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
Extracts redirect matching logic to its own match_redirect method. #198
Extracts redirect matching logic to its own match_redirect method. #198
Conversation
Introduces a new match_redirect method and updates maybe_redirect to use this new method. Also introduces a srm_match_redirect function to export match_redirect to themes and plugins.
@nicholasio thanks for the PR, it's greatly appreciated! I've pinged @tlovett1 for review on this in planning for getting it into our next milestoned release, so stay tuned for any feedback from that review. |
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.
LGTM 👍
Edit: This is just my preference, but if we return early in maybe_redirect
method, we can save one level of indentation:
if ( empty( $matched_redirect ) ) {
return;
}
// Do the rest
...
@dinhtungdu done, that is generally my preference as well. |
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.
🎉
Description of the Change
Introduces a new
match_redirect
method and updatesmaybe_redirect
to use this new method.It also introduces a
srm_match_redirect
function to expose matching redirect logic to themes and plugins.Alternate Designs
See #197
Benefits
Code is more testable now and maybe_redirect now only does one thing.
Possible Drawbacks
None
Verification Process
Manual testing
Checklist:
Applicable Issues
Changelog Entry