Only run filter once. #5260
Only run filter once. #5260
Conversation
spacedmonkey
requested review from
costdev,
joemcgill,
felixarntz,
mukeshpanchal27 and
peterwilsoncc
There was a problem hiding this comment.
Thanks @spacedmonkey for the PR. Left some unit test related feedbacks.
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Actioned feedback. |
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
There was a problem hiding this comment.
I like this approach much better. The tests that you've added already pass without this change in trunk, so it would be nice to add a test that demonstrates the exact problem you're trying to fix. I've been trying to work something up, but haven't had success yet. Will keep messing with it, but you may beat me to it.
Co-authored-by: Joe McGill <801097+joemcgill@users.noreply.github.com>
@joemcgill The fact that the test works in trunk was the point. To see no breakage. I have added one more unit test, that proved that sanativation is only run once. Can you take another look at this PR. |
There was a problem hiding this comment.
Thanks @spacedmonkey! I understood why you had written the original tests. I just wanted to make sure that we also had a test that confirmed the filter is only being run once (so a future change doesn't cause a regression). The new test you've added seems to do the trick!
There was a problem hiding this comment.
Thanks for the updates @spacedmonkey! Just a few final notes 🙂
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
Thanks @costdev Feedback actioned. Once I get an approval from you, I will commit. |
|
Committed c1247f6 |
Trac ticket: https://core.trac.wordpress.org/ticket/58329
This PR replaces #4874. Basically, only run the
filtermethod once inget_term. Run if the object has been filtered or the filter parameter does match the one passed in the function.This fix would also fix the issues found in https://core.trac.wordpress.org/ticket/58327
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.