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.

Sign up for GitHub

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

Jump to bottom

Fix/189 ignore query params #196

Merged
merged 8 commits into from
Nov 10, 2020
Merged

Fix/189 ignore query params #196

merged 8 commits into from
Nov 10, 2020

Conversation

braders
Copy link
Contributor

@braders braders commented Nov 22, 2019

Description of the Change

This fixes #189.

In short, if the redirect is simple (i.e. not regex) and the redirect_from field does not contain query parameters, then any query parameters will be ignored when doing the matching. Any ignored query parameters will be appended to the eventual $redirect_to, to maintain marketing params.

Possible Drawbacks

This is a possible breaking change, but I consider it unlikely that anyone would be relying on the previous behaviour. Users who want to keep the existing behaviour can use the new srm_match_query_params filter

Verification Process

  • Manually tested against the codebase where the issue was discovered,
  • Tested through phpunit tests.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

#189

@jeffpaul jeffpaul added this to the 2.0.0 milestone Nov 22, 2019
@jeffpaul jeffpaul added the type:enhancement New feature or request. label Nov 22, 2019
@jeffpaul
Copy link
Member

@braders thanks for the PR, I'm pinging @tlovett1 for a review. Note that we just shipped a release here this past week so it may be a bit before we're able to get another one ready to go, but will let you know of any issues of concern here or when it's good to merge... thanks!

dinhtungdu
dinhtungdu reviewed Feb 4, 2020
View reviewed changes
Copy link
Collaborator

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

This PR conflicts with #198, so I think we should come back to this after merging #198.

dinhtungdu
dinhtungdu reviewed Feb 4, 2020
View reviewed changes
Copy link
Collaborator

@dinhtungdu dinhtungdu left a comment •
edited

Choose a reason for hiding this comment

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

Also, the coding style check is failing

@braders
Copy link
Contributor Author

braders commented Feb 21, 2020

This PR conflicts with #198, so I think we should come back to this after merging #198.

I don't see any merge conflicts with this code against develop, and #198 is now merged.

Also, the coding style check is failing

I believe the code now passes all formatting checks.

dinhtungdu
dinhtungdu reviewed Feb 24, 2020
View reviewed changes
Copy link
Collaborator

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

@braders It works! I added some comments to improve code readability.

inc/classes/class-srm-redirect.php Outdated
Comment on lines 115 to 116
if ( ! empty( $parsed_requested_path['path'] ) ) {
$normalized_requested_path_no_query = untrailingslashit( stripslashes( $parsed_requested_path['path'] ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

$parsed_requested_path['path'] will be always exist because the #normalized_requested_path variable is not empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is unfortunately not correct, as our debug log is getting flooded with undefined index warnings: Undefined index: path in .../plugins/safe-redirect-manager/inc/classes/class-srm-redirect.php on line 121

We're not yet sure what's wrong exactly, but it's clear that there's an issue here.

inc/classes/class-srm-redirect.php Outdated
$parsed_requested_path = parse_url( $normalized_requested_path );
}

if ( is_array( $parsed_requested_path ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

$parsed_requested_path is always an array, so this is always true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

inc/classes/class-srm-redirect.php Outdated
Comment on lines 112 to 113
$normalized_requested_path_no_query = '';
$requested_query_params = '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Duplicated variables declaration.
IMO, we can safely use $parsed_requested_path without declaring
intermediate variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

inc/classes/class-srm-redirect.php Show resolved Hide resolved
dinhtungdu
dinhtungdu previously approved these changes Feb 24, 2020
View reviewed changes
Copy link
Collaborator

@dinhtungdu dinhtungdu left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for your great work @braders, LGTM 🎉

@braders
Copy link
Contributor Author

braders commented Sep 2, 2020

Now that its been a few months I was just wondering if there is any update on when this might be merged? Thanks!

@jeffpaul jeffpaul merged commit 9d1baee into 10up:develop Nov 10, 2020
@davidmondok davidmondok mentioned this pull request Jun 29, 2021
Closed
@dinhtungdu dinhtungdu mentioned this pull request Jul 6, 2021
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidmondok davidmondok davidmondok left review comments

@dinhtungdu dinhtungdu dinhtungdu approved these changes

@tlovett1 tlovett1 Awaiting requested review from tlovett1

Assignees

@braders braders

Labels
type:enhancement New feature or request.
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

Add option to ignore query params
4 participants
@braders @jeffpaul @davidmondok @dinhtungdu