Skip to content
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

Allow regular expression parsing if the redirect target is a valid URL #395

Merged
merged 4 commits into from
Sep 17, 2024

Conversation

benlk
Copy link
Contributor

@benlk benlk commented Sep 16, 2024

Description of the Change

#269 reported a bug in the handling of regex redirects with a URL target: the URLs were being prefixed with a /.

#279 fixed #269 by excluding the section of code that prepends URLs with / from being applied if the redirect target was a valid URL:

// Allow for regex replacement in $redirect_to
if ( $enable_regex && ! filter_var( $redirect_to, FILTER_VALIDATE_URL ) ) {
$redirect_to = preg_replace( '@' . $redirect_from . '@' . $regex_flag, $redirect_to, $requested_path );
$redirect_to = '/' . ltrim( $redirect_to, '/' );
}

Unfortunately, this also excluded the code that applies the regular expression to the redirect target URL. If the redirect target was a URL, the final preg_replace was no longer applied.

This fix separates the prefix-with-/ behavior from regex parsing by separating the conditionals.

Closes #380

How to test the Change

Test cases, all with regular expressions:

Changelog Entry

Fixed - Allows use of full URLs as redirect targets when using absolute URLs (#395 for #380)

Credits

Props @benlk @peterwilsoncc

Checklist:

@benlk benlk added type:bug Something isn’t working. needs:tests This requires tests. labels Sep 16, 2024
@benlk benlk self-assigned this Sep 16, 2024
@github-actions github-actions bot added this to the 2.2.0 milestone Sep 16, 2024
@benlk benlk removed the needs:tests This requires tests. label Sep 16, 2024
@benlk benlk marked this pull request as ready for review September 16, 2024 22:51
@benlk benlk requested review from dhanendran and jeffpaul and removed request for dkotter and jeffpaul September 16, 2024 22:52
@github-actions github-actions bot added the needs:feedback This requires feedback to determine next steps. label Sep 16, 2024
Copy link
Contributor

@benlk thanks for the PR! Could you please fill out the PR template with description, changelog, and credits information so that we can properly review and merge this?

@peterwilsoncc
Copy link
Contributor

@benlk I took some liberties while testing this out.

I re-ordered the commits to make sure the tests were failing against the unchanged code but they were passing, see the action run.

I subsequently pushed some failing tests in 30c4c84, which did fail against the unchanged code, see the action run

I finally pushed the fix (the new commit hash is 5e7bb7e) and the action is now passing, see the action run

So... Are you able to confirm my updates to the test are testing the correct things? I'll review the changes in the mean time.

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me, I think this is good to merge once Ben validates my updates to the tests.

@benlk
Copy link
Contributor Author

benlk commented Sep 17, 2024

The changes look good to me as well. The revised tests appear to be testing what was intended.

@peterwilsoncc
Copy link
Contributor

I have no idea why the validation bot is failing to pick up the descriptions so ignoring the issue and merging regardless.

@peterwilsoncc peterwilsoncc merged commit 6cf8d09 into develop Sep 17, 2024
13 of 17 checks passed
@peterwilsoncc peterwilsoncc deleted the fix/380-regex-with-url branch September 17, 2024 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:feedback This requires feedback to determine next steps. type:bug Something isn’t working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regex pattern replacement fails if destination is absolute url. Regex redirects without leading / are buggy
2 participants