WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 8 months ago

Last modified 8 months ago

#44354 closed enhancement (fixed)

Improve `WP_Privacy_Requests_Table` to manage columns

Reported by: 7studio Owned by: garrett-eclipse
Milestone: 5.7 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-screenshots needs-dev-note commit
Focuses: administration Cc:

Description

It could be interesting to improve WP_Privacy_Requests_Table with the help of actions and filters, for consistency with other post list tables.

The best place to start is to add a filter like "manage_{$post_type}_posts_columns" and an action like "manage_{$post->post_type}_posts_custom_column" to enhance columns contents
or add new columns with content recovered by submission form or other.

This issue goes in the same way as #43960, #44015, #44025 or #44267

Hope you will consider this ticket and it contributes to WP.

Attachments (10)

44354.diff (2.0 KB) - added by birgire 3 years ago.
custom-column.jpg (32.5 KB) - added by birgire 3 years ago.
44354.2.diff (3.8 KB) - added by pbiron 2 years ago.
patch refresh and addition of 2 more hooks.
44354.3.diff (6.1 KB) - added by garrett-eclipse 12 months ago.
Refresh
44354.5.diff (7.4 KB) - added by garrett-eclipse 11 months ago.
Minor refresh to remove the return;
44354.6.diff (6.1 KB) - added by garrett-eclipse 11 months ago.
Refresh without the tests/qunit/fixtures/wp-api-generated.js as they were committed previously.
44354-export-after.png (179.1 KB) - added by hellofromTonya 8 months ago.
"Export Personal Data" custom columns after applying 44354.6.diff. Works as expected.
44354-erase-after.png (180.7 KB) - added by hellofromTonya 8 months ago.
"Erase Personal Data" custom columns after applying 44354.6.diff. Works as expected.
44354-erase-after-pr888.png (183.1 KB) - added by hellofromTonya 8 months ago.
"Erase Personal Data" custom columns after applying PR888. Works as expected.
44354-export-after-pr888.png (179.6 KB) - added by hellofromTonya 8 months ago.
"Export Personal Data" custom columns after applying PR888. Works as expected.

Download all attachments as: .zip

Change History (47)

#1 @desrosj
3 years ago

  • Focuses administration added
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

@birgire
3 years ago

@birgire
3 years ago

#2 follow-up: @birgire
3 years ago

Thanks for the ticket @7studio

44354.diff is a first pass to improve the column management for the Requests list table.

We add the dynamic action in the default column handler:

do_action( "manage_{$this->request_type}_custom_column", $column_name, $item );

that generates the following actions:

  • manage_export_personal_data_custom_column.
  • manage_remove_personal_data_custom_column

We also add the following columns filter:

return apply_filters( "manage_{$this->request_type}_columns", $columns );

that generates the following filters:

  • manage_export_personal_data_columns.
  • manage_remove_personal_data_columns

Another naming suggestion could be:

  • "manage_{$this->request_type}_requests_custom_column"
  • "manage_{$this->request_type}_requests_columns"

but that generates longer hook names.

It's interesting to see the mismatch regarding the output of column methods in various list tables.

It's both echo and return, but from the WP_List_Table::single_row_columns() it seems to expect return:

} elseif ( method_exists( $this, 'column_' . $column_name ) ) {
	echo "<td $attributes>";
	echo call_user_func( array( $this, 'column_' . $column_name ), $item );
	echo $this->handle_row_actions( $item, $column_name, $primary );
	echo '</td>';
} else {
	echo "<td $attributes>";
	echo $this->column_default( $item, $column_name );
	echo $this->handle_row_actions( $item, $column_name, $primary );
	echo '</td>';
}

Test Example

Here's a test example where we add a custom column called "Resend Email":

add_action( 'manage_export_personal_data_custom_column', function( $column_name, $item ) {
    if ( 'resend_email' === $column_name ) {
        printf( '<input type="button" class="button" value="%s" />', esc_attr( __( 'Resend Email' ) ) );
	}
}, 10, 2 );

add_filter('manage_export_personal_data_columns', function( $columns ) {
    if( is_array( $columns ) && ! isset( $columns['resend_email'] ) ) {
        $columns['resend_email'] = __( 'Resend Email' );
	}
    return $columns;
} );


See the screenshot in custom-column.jpg.

#3 @birgire
3 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#4 follow-up: @7studio
3 years ago

Sorry for this late answer @birgire

Thank you very much for this patch :D IMHO, long hook names are not a problem. Most of the time, it's more understandable ;)

It's interesting to see the mismatch regarding the output of column methods in various list tables.

It's the same thing in examples on the Web but I've always used echo ;)

If I must do something let me know.

#5 in reply to: ↑ 4 @birgire
3 years ago

Replying to 7studio:

Sorry for this late answer @birgire

No problem :-)

Thank you very much for this patch :D IMHO, long hook names are not a problem. Most of the time, it's more understandable ;)

So you would prefer, the longer hook name version here?

It's interesting to see the mismatch regarding the output of column methods in various list tables.

It's the same thing in examples on the Web but I've always used echo ;)

Yes, since both approaches work, it's understandable that both versions are being used out there.

If I must do something let me know.

It would be great if you could test the patch and the example above, when you've time. Thanks.

#6 @garrett-eclipse
3 years ago

  • Keywords needs-refresh added
  • Owner set to garrett-eclipse
  • Status changed from new to assigned

Hi @birgire thank you for the patch.

Applying it I found the table vanished and gone due to PHP errors exhausting memory;
PHP Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 20480 bytes) in .../wp-includes/class-wp-hook.php on line 271
Same error affected a few files. Looking deeper the cause is the update to column_default which introduced the "manage_{$this->request_type}_custom_column" action. The column_default function expects a return so returning with apply_filter should work.

Reviewing the original code I believe we want to preserve the @return, as well as the $cell_value = $item->$column_name; and then apply a filter to that with a return call.
*Although maybe just use $column_name not sure why the original code needed to access it from $item.

An existing example in core;
https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/includes/class-wp-terms-list-table.php#L579-L598

All the best

#8 in reply to: ↑ 2 @pbiron
2 years ago

Replying to birgire:

It's interesting to see the mismatch regarding the output of column methods in various list tables.

It's both echo and return, but from the WP_List_Table::single_row_columns() it seems to expect return:

Yeah, single_row_columns() appears to expect a value, which implies that we should use a filter instead of an action (as in the example from WP_Terms_List_Table mentioned by garrett-eclipse).

However, of the 11 core list tables that currently support custom columns, only 2 of them use a filter and return a value (WP_Users_List_Table is the other one). You can see that by searching the Code Reference for hooks that use "custom_column".

So, my vote is to go with action, as it is more prevalent in core already.

@pbiron
2 years ago

patch refresh and addition of 2 more hooks.

#9 @pbiron
2 years ago

  • Keywords needs-refresh removed

44354.2.diff refreshes the patch after the file reorganization in #43895. It also updates the @since tags to `5.3.0'.

Additionally, it adds 2 new hooks:

  1. manage_privacy_requests_columns (in get_columns(), this is a filter). This allows for adding columns that are "agnostic" as to the request type (similar to wp_insert_post()'s use of both save_post and save_post_{$post_type}). The request type is passed as the 2nd param, so it can also be used to output columns that are specific to the request type.
  2. manage_privacy_request_custom_column (in column_default(), this is an action, see comment 8). Again, this allows for outputting values for custom columns that are both "agnostic" as to request-type and request-type-specific columns.

The hooks in 44354.diff are still there. These new hooks just give plugin authors different ways to add/output custom columns.

p.s. I have not experienced the memory problems mentioned by garrett-eclipse. I didn't try the initial patch until after #43895...maybe that had something to do with it?

@garrett-eclipse
12 months ago

Refresh

#10 follow-up: @garrett-eclipse
12 months ago

  • Keywords has-screenshots added
  • Milestone changed from Future Release to 5.6
  • Status changed from assigned to accepted

Thanks for ticket @7studio and the patches @birgire @pbiron they're looking great, I'm not seeing the errors I had before so probably was my own issue in the filter test code I was using.

I've refreshed the patch in 44354.3.diff to do minor updates to the phpdocs, but also added a return; to the end of column_default() as the list tables do expect a return value as it's output in the default list table class here;
https://core.trac.wordpress.org/browser/tags/5.5.1/src/wp-admin/includes/class-wp-list-table.php#L1410

I also reordered the column_ functions to have column_default first followed by the custom columns.

Please review and retest and we can move this forward.

@garrett-eclipse
11 months ago

Minor refresh to remove the return;

#11 in reply to: ↑ 10 @garrett-eclipse
11 months ago

Replying to garrett-eclipse:

I've refreshed the patch in 44354.3.diff to do minor updates to the phpdocs, but also added a return; to the end of column_default() as the list tables do expect a return value as it's output in the default list table class here;
https://core.trac.wordpress.org/browser/tags/5.5.1/src/wp-admin/includes/class-wp-list-table.php#L1410

Not sure why I felt return; was needed, removed in 44354.5.diff as it's unnecessary.

#12 @garrett-eclipse
11 months ago

  • Keywords needs-dev-note added

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


11 months ago

#14 @hellofromTonya
11 months ago

📣 Call for testers 📣

This ticket is ready for commit once we get some help with testing. Gentle reminder that Beta 1 is less than a week away (ie 20th Oct).

This ticket was mentioned in Slack in #core-privacy by hellofromtonya. View the logs.


11 months ago

This ticket was mentioned in Slack in #core-privacy by carike. View the logs.


11 months ago

#18 @hellofromTonya
11 months ago

  • Keywords commit added; needs-testing removed

#19 @hellofromTonya
11 months ago

  • Keywords needs-testing added; commit removed

Whoops tester had the wrong ticket. Reverting this one until testing is done.

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


11 months ago

#21 @mukesh27
11 months ago

  • Keywords commit added; needs-testing removed

Hi there,

Tested the patch, it working fine.

Let's mark as commit

@garrett-eclipse
11 months ago

Refresh without the tests/qunit/fixtures/wp-api-generated.js as they were committed previously.

#22 @garrett-eclipse
11 months ago

Thanks for the testing, in 44354.6.diff I just removed the changes to tests/qunit/fixtures/wp-api-generated.js as they were previously committed in r49031/#50805

#23 @Mista-Flo
11 months ago

Thanks Garrett, I am now able to apply the patch as I was blocked with the qunit file.

The code looks good.

I can definitely use the two hooks, so good to go for me

This ticket was mentioned in Slack in #core by helen. View the logs.


11 months ago

#25 @TimothyBlynJacobs
11 months ago

I don't think the filter on the column names is necessary. This extension support is built in as WP_List_Table calls get_column_headers() which already has a filter.

<?php
add_filter( 'manage_export-personal-data_columns', function ( $columns ) {
        $columns['test'] = __( 'Test' );
        return $columns;
} );

The action in the column_default is still necessary as WordPress doesn't provide a default one there.

#26 @garrett-eclipse
11 months ago

  • Keywords commit removed
  • Milestone changed from 5.6 to 5.7

Let's revisit in 5.7 thanks for pointing that out @TimothyBlynJacobs

This ticket was mentioned in Slack in #core by lukecarbis. View the logs.


8 months ago

#28 @hellofromTonya
8 months ago

  • Keywords needs-refresh added

Marking as needs-refresh to address the filter Timothy notes here.

This ticket was mentioned in PR #888 on WordPress/wordpress-develop by mrxkon.


8 months ago

  • Keywords needs-refresh removed

#30 @xkon
8 months ago

As mentioned by @TimothyBlynJacobs in 25 filtering the column names is already provided. In the PR provided an extra action is added to the column_default() of WP_Privacy_Requests_Table to allow custom column data to be added as well.

For consistency instead of using the $this->request_type from previous patches & comments, I'm using the $this->screen->id instead to keep the action & filter naming convention aligned.

Testing:
You can test the PR provided by adding this code in a mu-plugin and you should see the new columns being created on the Personal Data Export & Erasure tables.

<?php
add_filter(
	'manage_export-personal-data_columns',
	function ( $columns ) {
		$columns['my_export_column'] = __( 'Custom Column EX' );
		return $columns;
	}
);

add_action(
	'manage_export-personal-data_custom_column' ,
	function( $columns, $item ) {
		switch( $columns ) {
			case 'my_export_column':
				echo 'Something';
			break;
		}
	},
	10,
	2
);

add_filter(
	'manage_erase-personal-data_columns',
	function ( $columns ) {
		$columns['my_erase_column'] = __( 'Custom Column ER' );
		return $columns;
	}
);

add_action(
	'manage_erase-personal-data_custom_column',
	function( $columns, $item ) {
		switch( $columns ) {
			case 'my_erase_column':
				echo 'Something Else';
			break;
		}
	},
	10,
	2
);

#31 @hellofromTonya
8 months ago

  • Keywords needs-testing added

The PR is ready for testing including having a testing strategy.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


8 months ago

@hellofromTonya
8 months ago

"Export Personal Data" custom columns after applying 44354.6.diff. Works as expected.

@hellofromTonya
8 months ago

"Erase Personal Data" custom columns after applying 44354.6.diff. Works as expected.

#33 @hellofromTonya
8 months ago

  • Keywords needs-testing removed

Using @xkon code plus had a little fun customizing the Next Steps column too, works. Tested in Firefox and Chrome on MacOS.

This ticket was mentioned in Slack in #core-privacy by hellofromtonya. View the logs.


8 months ago

#35 @hellofromTonya
8 months ago

  • Keywords commit added

Checked with @xkon. This ticket is ready for commit consideration.

@hellofromTonya
8 months ago

"Erase Personal Data" custom columns after applying PR888. Works as expected.

@hellofromTonya
8 months ago

"Export Personal Data" custom columns after applying PR888. Works as expected.

#36 @hellofromTonya
8 months ago

Whoopsie, used wrong patch in first step of screenshots (face palm).

Retested with PRR 888 and must-use plugin code. Works as expected. See 2 screenshots above this comment.

#37 @SergeyBiryukov
8 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 50145:

Privacy: Introduce manage_{$this->screen->id}_custom_column action in WP_Privacy_Requests_Table::column_default().

This brings some consistency with other list tables and allows for adding custom column data to columns registered with manage_export-personal-data_columns or manage_erase-personal-data_columns filters.

Props xkon, garrett-eclipse, birgire, pbiron, hellofromTonya, TimothyBlynJacobs, 7studio, mukesh27, Mista-Flo.
Fixes #44354.

Note: See TracTickets for help on using tickets.