Update term edit link generation in WP_Terms_List_Table::handle_row_actions()
#5198
Conversation
…_actions()` - Restrict edit terms link generation if user lacks `edit_term` cap.
There was a problem hiding this comment.
This seems like a good change.
✔️ Checked: The $edit_link variable is not used in any other place in the function.
❌ Missing: a unit test to proof the bug and safeguard the fix. Please add one.
Coding standards wise, I'd prefer to still keep the creation of the URL separate from the creation of the HTML as it makes the code more readable without the nested function calls.
|
@jrfnl Since the above patch only eliminates a variable assignment and uses the expression directly, I'm not sure if it actually necessitates a test case. The function Should I add a test case to |
Sorry, but no. The So, it is this function which needs a test proving the PHP 8.1 issue and safeguarding the fix. |
There was a problem hiding this comment.
Hi @thelovekesh, thank you for adding those tests!
The test_handle_row_actions_should_generate_deprecation_notice() test can be removed. The test_handle_row_actions() test is sufficient.
I've checked this in the following manner:
- Using PHP 8.1 or higher (in my case PHP 8.3-RC1).
- Revert the first commit containing the fix.
- In my local
phpunit.xmloverload file, setconvertDeprecationsToExceptions="false". - Ran the test(s).
This will show the following as part of the output:This proves the bug you were reporting and also confirms that the2) Tests_Admin_WpTermsListTable::test_handle_row_actions This test printed output: Deprecated: strstr(): Passing null to parameter #1 ($haystack) of type string is deprecated in path\to\src\wp-includes\functions.php on line 1147 Deprecated: stripos(): Passing null to parameter #1 ($haystack) of type string is deprecated in path\to\src\wp-includes\functions.php on line 1154 Deprecated: stripos(): Passing null to parameter #1 ($haystack) of type string is deprecated in path\to\src\wp-includes\functions.php on line 1157 Deprecated: str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated in path\to\src\wp-includes\functions.php on line 1164 Deprecated: str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated in path\to\src\wp-includes\functions.php on line 1167test_handle_row_actions()alone is sufficient to proof the bug and safeguard the fix. - Brought back the first commit containing the fix.
- Ran the tests again and the
test_handle_row_actions()test now passes without notices.
Pro-tip for future patches for these type of issues: have the tests in a separate commit (like you already did 👍🏻 ) and let that be the first commit.
Then in the second commit, add the patch fixing the issue.
That way a reviewer can just check out the first commit, run the tests, see the problem, check out the head of the patch branch and confirm that the issue is fixed.
| * @return mixed Return value of the method call. | ||
| * @throws ReflectionException If the object could not be reflected upon. | ||
| */ | ||
| private function call_private_method( $object, $method_name, $args = array() ) { |
There was a problem hiding this comment.
| private function call_private_method( $object, $method_name, $args = array() ) { | |
| private function call_inaccessible_method( $object, $method_name, $args = array() ) { |
Reason: the function you are calling is protected, not private, so the suggested new name is more appropriate.
| * | ||
| * @covers WP_Terms_List_Table::handle_row_actions() | ||
| */ | ||
| public function test_handle_row_actions_should_generate_deprecation_notice() { |
There was a problem hiding this comment.
This complete test can be removed as it is unnecessary.
| set_error_handler( | ||
| static function ( $errno, $errstr ) { | ||
| throw new ErrorException( $errstr, $errno ); | ||
| }, | ||
| E_ALL | ||
| ); | ||
|
|
||
| $this->expectException( ErrorException::class ); | ||
| $this->expectExceptionMessageMatches( '/^strstr\(\): Passing null to parameter #1 \(\$haystack\) of type string is deprecated/' ); |
There was a problem hiding this comment.
Just a side-note (as the test should be removed): why are you setting a custom error handler here ?
WP is using PHPUnit 6 - 9 with the PHPUnit Polyfills, so this could/should be $this->expectDeprecation() + $this->expectDeprecationMessage().
There was a problem hiding this comment.
I added the custom error handler while keeping in mind that PHPUnit 10 has removed these fixtures. But I think my concern is invalid as PHPUnit polyfills are in play here.
There was a problem hiding this comment.
Well, it's not about the PHPUnit Polyfills (as they make tests forward-compatible, not backward-compatible), it's about the WP Test suite not (yet) supporting PHPUnit 10.
While that's the case, tests should be written for the supported versions of the dependency (PHPUnit 6-9), not for a future version which is not yet supported.
This will make it easier for when the WP Test suite will be made compatible with PHPUnit 10, as in that case, all code which needs to be updated can easily be found via code searches and the same solution (probably extracted away in one of the base TestCase classes), can be used everywhere.
Having one-off "solutions" like this in the codebase would make that more difficult.
| $this->assertStringNotContainsString( admin_url( 'term.php' ), $edit_link ); | ||
|
|
||
| restore_error_handler(); |
There was a problem hiding this comment.
In PHPUnit 6-9, tests stop as soon as the expected deprecation/exception is caught, so this code will never be executed, which also means that the error handler is never restored back (!!).
In other words, this test is buggy and may negatively influence other tests.
There was a problem hiding this comment.
It's a good point, and I honestly had no idea about that. In such a situation, should a custom error handler be restored within tearDown()?
There was a problem hiding this comment.
Either that, or from within the custom error handler itself. For WP that logic will be sorted out when the test suite will be made compatible with PHPUnit 10 though, so is not something for this PR.
| /** | ||
| * @covers WP_Terms_List_Table::handle_row_actions() | ||
| */ | ||
| public function test_handle_row_actions() { |
There was a problem hiding this comment.
Not a must, but a nice to have/best practice recommendations:
- Split this test in two, one to test with
$author_id, another to test with$admin_id.
I would suggest a data provider, but in this case that wouldn't be a good solution as the assertions are different for the two situations being tested, so in this case, splitting the test into two separate tests each testing one specific situation is the better solution. - Add the
$messageparameter to each assertion.
When there are multiple assertions in a test, it can sometimes be confusing to see which assertion failed. Using the$messageparameter you can pass a "failure message" to each assertion to make it more clear cut to see in the command line output, what the expectation was which wasn't met.
Ref: https://docs.phpunit.de/en/9.6/assertions.html#assertstringcontainsstring
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
There was a problem hiding this comment.
Thanks for making those updates @thelovekesh !
Aside from this earlier remark of mine, all looks good and my earlier remark is not a blocker, so I'm approving this patch.
Coding standards wise, I'd prefer to still keep the creation of the URL separate from the creation of the HTML as it makes the code more readable without the nested function calls.
|
Committed in https://core.trac.wordpress.org/changeset/56631 |
Restrict edit term link generation if the user lacks the
edit_termcap.Trac ticket: https://core.trac.wordpress.org/ticket/59336
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.