Improve `WP_Privacy_Requests_Table` to manage columns

[7studio]

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.

[birgire]

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​.

[7studio]

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.

[birgire]

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.

[garrett-eclipse]

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

[garrett-eclipse]

Related - #47488

[pbiron]

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]

patch refresh and addition of 2 more hooks.

[pbiron]

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]

Refresh

[garrett-eclipse]

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]

Minor refresh to remove the return;

[garrett-eclipse]

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.

[hellofromTonya]

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

[hellofromTonya]

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

[mukesh27]

Hi there,

Tested the patch, it working fine.

Let's mark as commit

[garrett-eclipse]

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

[garrett-eclipse]

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

[Mista-Flo]

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

[TimothyBlynJacobs]

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.

[garrett-eclipse]

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

[hellofromTonya]

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

Trac ticket: https://core.trac.wordpress.org/ticket/44354

[xkon]

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
);

[hellofromTonya]

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

[hellofromTonya]

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

[hellofromTonya]

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

[hellofromTonya]

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

[hellofromTonya]

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

[hellofromTonya]

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

[hellofromTonya]

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

[hellofromTonya]

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.

[SergeyBiryukov]

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.

Closed with changeset https://core.trac.wordpress.org/changeset/50145