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

Update term edit link generation in WP_Terms_List_Table::handle_row_actions() #5198

Closed
wants to merge 5 commits into from
Closed

Update term edit link generation in WP_Terms_List_Table::handle_row_actions() #5198

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4f42cd6
Update terms edit link generation in `WP_Terms_List_Table::handle_row…
thelovekesh Sep 13, 2023
aa8f368
Add test cases for `WP_Terms_List_Table::handle_row_actions()`
thelovekesh Sep 14, 2023
60de64a
Fix PHP version comapre
thelovekesh Sep 14, 2023
ace127b
Fix docblock
thelovekesh Sep 15, 2023
2c11912
Remove unnecessary test case and improve code qulaity
thelovekesh Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/wp-admin/includes/class-wp-terms-list-table.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,18 +468,18 @@ protected function handle_row_actions( $item, $column_name, $primary ) {
$taxonomy = $this->screen->taxonomy;
$uri = wp_doing_ajax() ? wp_get_referer() : $_SERVER['REQUEST_URI'];

$edit_link = add_query_arg(
'wp_http_referer',
urlencode( wp_unslash( $uri ) ),
get_edit_term_link( $tag, $taxonomy, $this->screen->post_type )
);

$actions = array();

if ( current_user_can( 'edit_term', $tag->term_id ) ) {
$actions['edit'] = sprintf(
'<a href="%s" aria-label="%s">%s</a>',
esc_url( $edit_link ),
esc_url(
add_query_arg(
'wp_http_referer',
urlencode( wp_unslash( $uri ) ),
get_edit_term_link( $tag, $taxonomy, $this->screen->post_type )
)
),
/* translators: %s: Taxonomy term name. */
esc_attr( sprintf( __( 'Edit &#8220;%s&#8221;' ), $tag->name ) ),
__( 'Edit' )
Expand Down
125 changes: 125 additions & 0 deletions tests/phpunit/tests/admin/wpTermsListTable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
<?php

/**
* @group admin
*
* @covers WP_Terms_List_Table
*/
class Tests_Admin_WpTermsListTable extends WP_UnitTestCase {

/**
* List table.
*
* @var WP_Terms_List_Table $terms_list_table
*/
private $terms_list_table;

private static $admin_id;
private static $author_id;
private static $term_object;

const CATEGORY_TAXONOMY = 'category';

public static function set_up_before_class() {
parent::set_up_before_class();

self::$admin_id = self::factory()->user->create( array( 'role' => 'administrator' ) );
self::$author_id = self::factory()->user->create( array( 'role' => 'author' ) );

self::$term_object = self::factory()->term->create_and_get( array( 'taxonomy' => self::CATEGORY_TAXONOMY ) );

require_once ABSPATH . 'wp-admin/includes/class-wp-list-table.php';
require_once ABSPATH . 'wp-admin/includes/class-wp-terms-list-table.php';
}

public function set_up() {
parent::set_up();

$this->terms_list_table = new WP_Terms_List_Table();
}

/**
* Call a private method as if it was public.
*
* @param object|string $object Object instance or class string to call the method of.
* @param string $method_name Name of the method to call.
* @param array $args Optional. Array of arguments to pass to the method.
* @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() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

$method = ( new ReflectionClass( $object ) )->getMethod( $method_name );
$method->setAccessible( true );
return $method->invokeArgs( $object, $args );
}

/**
* This test proves the existence and reproducibility of the deprecation warnings
* caused by passing null as an argument to `add_query_arg()`
*
* @ticket 59336
*
* @covers WP_Terms_List_Table::handle_row_actions()
*/
public function test_handle_row_actions_should_generate_deprecation_notice() {
Copy link
Member

Choose a reason for hiding this comment

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

This complete test can be removed as it is unnecessary.

if ( PHP_VERSION_ID < 80100 ) {
$this->markTestSkipped( 'This test requires PHP 8.1 or higher.' );
}

wp_set_current_user( self::$admin_id );

$edit_link = add_query_arg(
'wp_http_referer',
admin_url( 'index.php' ),
get_edit_term_link( self::$term_object, self::CATEGORY_TAXONOMY, 'post' )
);

$this->assertStringContainsString( admin_url( 'term.php' ), $edit_link );

wp_set_current_user( self::$author_id );

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/' );
Copy link
Member

Choose a reason for hiding this comment

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

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().

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


$edit_link = add_query_arg(
'wp_http_referer',
admin_url( 'index.php' ),
get_edit_term_link( self::$term_object, self::CATEGORY_TAXONOMY, 'post' )
);

$this->assertStringNotContainsString( admin_url( 'term.php' ), $edit_link );

restore_error_handler();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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()?

Copy link
Member

Choose a reason for hiding this comment

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

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() {
Copy link
Member

Choose a reason for hiding this comment

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

Not a must, but a nice to have/best practice recommendations:

  1. 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.
  2. Add the $message parameter to each assertion.
    When there are multiple assertions in a test, it can sometimes be confusing to see which assertion failed. Using the $message parameter 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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 2c11912

wp_set_current_user( self::$author_id );

$actions = $this->call_private_method( $this->terms_list_table, 'handle_row_actions', array( self::$term_object, 'title', 'title' ) );

$this->assertStringContainsString( '<div class="row-actions">', $actions );
$this->assertStringContainsString( 'View', $actions );
$this->assertStringNotContainsString( 'Edit', $actions );
$this->assertStringNotContainsString( 'Delete', $actions );

wp_set_current_user( self::$admin_id );

$actions = $this->call_private_method( $this->terms_list_table, 'handle_row_actions', array( self::$term_object, 'title', 'title' ) );

$this->assertStringContainsString( '<div class="row-actions">', $actions );
$this->assertStringContainsString( 'View', $actions );
$this->assertStringContainsString( 'Edit', $actions );
$this->assertStringContainsString( 'Delete', $actions );
$this->assertStringContainsString( admin_url( 'term.php' ), $actions );
}
}