Add new filter 'mu_plugins' #3728
Conversation
There was a problem hiding this comment.
Thanks @nate-allen, Left one nit-pick document changes. Can you please add unit tests for the new filter?
After some discussion on the trac ticket, it was suggested that a general purpose filter would be of more use. See: https://core.trac.wordpress.org/ticket/57278#comment:6
This commit updates the code to check for null before calling strlen() to prevent a TypeError in PHP 8.
There was a problem hiding this comment.
Thanks for the PR @nate-allen! I've left some feedback in this review.
| @@ -292,11 +292,20 @@ public function prepare_items() { | |||
| } | |||
| } | |||
|
|
|||
| if ( strlen( $s ) ) { | |||
| if ( isset( $s ) && strlen( $s ) ) { | |||
There was a problem hiding this comment.
This change doesn't appear to be related to the addition of the new filter. @nate-allen Was this part of a patch for another ticket?
There was a problem hiding this comment.
Hi @costdev. It's required for the unit test for this ticket.
There was a problem hiding this comment.
Thanks @nate-allen!
Pinging @TimothyBJacobs and @spacedmonkey as REST API maintainers. Are you happy for this source change to be made here to allow PHPUnit tests to pass, or should it be made in a separate ticket, or should the test(s) instead set a value for the $s global?
There was a problem hiding this comment.
@costdev This doesn't look REST API related to me? But since I'm here 🙂.
To me it looks like the relevant globals should be getting initialized in the test. Though I think @SergeyBiryukov or @hellofromtonya could give a more informed opinion on the direction for these kinds of unit tests.
There was a problem hiding this comment.
Thanks @TimothyBJacobs! It appears my brain skipped a beat when I posted that at 3AM 😅
Yeah that's what I was thinking - and that the isset( $s ) check, if it's going to be added at some point, should probably be separate rather than bundled in the addition of a new filter.
Let's see what Sergey or Tonya think too 🙂
There was a problem hiding this comment.
Thanks for ping @TimothyBJacobs and @costdev.
To me it looks like the relevant globals should be getting initialized in the test.
In this PR, yes, I agree.
How to do it?
- Store each global's original value in a new property added to the test class.
- Add the property.
- The initialization of the property in either the
before_set_up_class()orset_up()fixture method.
- The test is then able to modify the global to its needs.
- How to restore / reset the original value? In the
tear_down(), set the global to its the original value using the property.- Why the
tear_down()and not within the test method? Thetear_down()ensures the global gets reset, as it runs after each test regardless if the test fails and skips any of it test code.
- Why the
There was a problem hiding this comment.
Thank you @hellofromtonya (and everyone else that has helped move this along!) I will make these changes and reach out if I have any issues.
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
|
I committed fb4a416 to solve a conflict against trunk. |
|
closing in favor of #4717 |
Adds a new filter called
mu_pluginsthat allows the filtering of the must-use plugins array.Trac ticket: https://core.trac.wordpress.org/ticket/57278
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.