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

fix/86: Display notice with vertices that cause a redirect cycle/loop #341

Merged
merged 3 commits into from Aug 29, 2023

Conversation

Sidsector9
Copy link
Member

@Sidsector9 Sidsector9 commented Aug 27, 2023

Closes #86
Closes #87

Description of the Change

In PR #336, we use DFS to bring down the cycle detection time from O(n^2) to O(V+E). We also returned as soon as the first cycle was detected.

In this PR, we make small adjustments to record all detected cycles by not returning early and then display the "Redirect To" values that cause cycles in a warning notice.

This PR also sets the default value of the filter srm_check_for_possible_redirect_loops to true as there is no negative performance hit and also helps to detect a redirect loop during the time of creating redirects.

Screenshot 2023-08-28 at 1 27 15 PM

How to test the Change

  1. Import the file srm-redirects-10000.csv. This file has 10,000 redirects.
  2. Set the srm_max_redirects number to something more than 10,000.
  3. Delete transients to gauge performance without it using wp transient delete --all
  4. Visit /wp-admin/edit.php?post_type=redirect_rule

Changelog Entry

Changed - Include links to redirects that cause redirect loops/chains in the warning.
Changed - Set the default value for the srm_check_for_possible_redirect_loops filter to true.

Credits

Props @joshbetz @nicoladj77 @Sidsector9

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@Sidsector9 Sidsector9 requested a review from a team as a code owner August 27, 2023 09:35
@Sidsector9 Sidsector9 requested review from ravinderk and removed request for a team August 27, 2023 09:35
@Sidsector9 Sidsector9 self-assigned this Aug 27, 2023
Copy link
Contributor

@ravinderk ravinderk left a comment

Choose a reason for hiding this comment

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

@Sidsector9 Can we add conflicted redirects with their editable page link in admin notices? In this way, the admin can view the redirect URL edit page instead of searching for them.

@jeffpaul jeffpaul added this to the 2.1.0 milestone Aug 28, 2023
@Sidsector9 Sidsector9 merged commit 92b575e into develop Aug 29, 2023
11 checks passed
@Sidsector9 Sidsector9 deleted the fix/86v2 branch August 29, 2023 14:30
@Sidsector9 Sidsector9 changed the title fix/86: Display starting vertices whenever a redirect cycle/loop is present fix/86: Display notice with vertices that cause a redirect cycle/loop Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve check_for_possible_redirect_loops() efficiency Simple loop detection
3 participants