WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 19 hours ago

#16031 assigned enhancement

New bulk actions hook missing catcher behavior

Reported by: Veraxus Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

The new bulk actions filter allows you to modify the bulk-actions arrays, but neglects to add the ability to handle the new custom actions.

The fix is to simply add one new do_action() hook to the default case of each affected page (similar to the way 'wp_ajax_' handles custom ajax requests).

For example, the following might be added to users.php at line 285, immediately after <tt>default:</tt>

do_action( 'bulk_action-'. $wp_list_table->current_action() );

Attachments (6)

16031.patch (2.4 KB) - added by Veraxus 5 years ago.
Fixes submission handling for pages affected by the new bulk_action filter
16031.2.patch (5.4 KB) - added by scribu 5 years ago.
16031.minimal.patch (5.0 KB) - added by scribu 5 years ago.
remove-bulk-actions.diff (537 bytes) - added by westi 5 years ago.
Quick patch
16031-no-new.diff (751 bytes) - added by nacin 5 years ago.
16031-comprehensive.patch (46.3 KB) - added by Veraxus 36 hours ago.
Reverts cs 17297 and adds comprehensive "handle_other_bulk_action-{$screen_id}" filter to all WP admin screens with list tables

Download all attachments as: .zip

Change History (68)

@Veraxus5 years ago

Fixes submission handling for pages affected by the new bulk_action filter

comment:1 @Veraxus5 years ago

The hook in this patch follows the format 'do_bulk_action-screenid-currentaction'.

Without this patch, there is still no non-hacky way to handle custom batch actions that developers may create, which defeats the purpose of the bulk_action hook.

comment:2 @SergeyBiryukov5 years ago

There are also files currently missing a default action handler:

plugins.php
upload.php
network/site-themes.php
network/site-users.php
network/themes.php

Perhaps the hook should be added to them too?

comment:3 @Veraxus5 years ago

The bulk_actions filter wasn't implemented for those pages; I only added handlers for the filters that are already officially committed.

I'm not sure those screens would really benefit from custom bulk action support, though (excepting network/site-users.php).

comment:4 @Veraxus5 years ago

  • Keywords has_patch added

comment:5 @SergeyBiryukov5 years ago

  • Keywords has-patch added; bulk_actions bulk has_patch removed

comment:6 @nacin5 years ago

  • Milestone changed from Awaiting Review to 3.1
  • Owner set to westi
  • Status changed from new to reviewing

comment:7 @scribu5 years ago

The hook in this patch follows the format 'do_bulk_action-screenid-currentaction'.

'tags' isn't the screen id, for example. So, it's more like do_bulk_action-{list-table-type}-currentaction'.

Still, it's a good idea.

comment:8 @scribu5 years ago

If we do want to go with screenid, it should be reflected in the code:

do_action( 'do_bulk_action-' . $current_screen->id . '-' . $doaction );

comment:9 @westi5 years ago

This patch looks like a good starting place but we should try and cover all of these if we are going to do this.

Sounds like working off the screen id might be a good idea so we can have context when the tables are used in more than one place.

comment:10 @scribu5 years ago

  • Owner changed from westi to scribu
  • Status changed from reviewing to accepted

comment:11 @scribu5 years ago

Actually, using the screen id would be the only consistent way, due to this:

$this->_actions = apply_filters( 'bulk_actions-' . $screen->id, $this->_actions );

Anyway, going to clean up the patch.

@scribu5 years ago

comment:12 @scribu5 years ago

16031.2.patch introduces WP_List_Table::do_bulk_action() and calls it wherever WP_List_Table::current_action() is used.

There are several inconsistencies, though:

On network/sites.php, current_action() is not used, because bulk actions are handled in network/edit.php

On edit-comments.php, the action is run for each selected comment.

On users.php and edit-tags.php, the entire screen is rendered in the default: branch.

Last edited 5 years ago by scribu (previous) (diff)

comment:13 @scribu5 years ago

16031.minimal.patch skips edit-comments.php, which would remain to be handled in a future release.

Last edited 5 years ago by scribu (previous) (diff)

@scribu5 years ago

comment:14 @scribu5 years ago

The next step would be to pass the list of elements to be processed to the action.

comment:15 @scribu5 years ago

As useful as this may be, I think it's too big a change for this very late state in the dev cycle.

A workaround is to use the "load-$pagenow" action instead.

comment:16 @scribu5 years ago

  • Type changed from defect (bug) to enhancement

comment:17 @nacin5 years ago

#16166 and #16187 make this higher priority. In particular, WP_List_Table::add_query_args() would need to be used by bulk action handlers... Figuring out how to handle this and do the redirect for them would be best.

Maybe these need to be filters, rather than actions. And the filter is the sendback URL, and they can add_query_arg() before returning it.

comment:18 @westi5 years ago

For now I think the simplest solution is to remove the filter that lets you add bulk actions and revisit the ability to add extra ones in a future version.

That is the best thing for this stage of the cycle

comment:19 @nacin5 years ago

Indeed. Pull it.

@westi5 years ago

Quick patch

comment:20 @westi5 years ago

Looking for a list of plugins that have already adopted this

comment:21 @Veraxus5 years ago

We were using the bulk_actions filter in 'Page Security by Contexture'. I don't know who else might have implemented it, though.

comment:22 @scribu5 years ago

The 'bulk_actions_' filter can also be used to remove certain actions, for example. I don't see the point in removing it.

comment:23 @westi5 years ago

http://wordpress.org/extend/plugins/backwpup/
and
http://wordpress.org/extend/plugins/wpematico/

Both have the list tables code copied into them for back-compat

Grunion 2

comment:24 @scribu5 years ago

Viper007Bond pointed out that we already have an 'admin_action_*' hook:

http://core.trac.wordpress.org/browser/trunk/wp-admin/admin.php?rev=17272#L232

The problem with that, as with "load-$pagenow", is that it runs before any capability checking is done.

Still, I think it's good enough for now.

Last edited 5 years ago by scribu (previous) (diff)

@nacin5 years ago

comment:25 @nacin5 years ago

Attached patch prevents no new actions from being added via the filter.

They can still do it by extending get_bulk_actions(), but that isn't supported. Otherwise, this filter ties our hands for future attempts to dry up all of the bulk action handler code (#16166) and also AJAX bulk actions.

comment:26 @nacin5 years ago

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

(In [17297]) Prevent new bulk actions from being added through the bulk_actions-screen filter. fixes #16031 at least for 3.1.

comment:27 @scribu4 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 3.1 to Future Release
  • Resolution fixed deleted
  • Status changed from closed to reopened

We can't really call this 'fixed'.

comment:28 @sirzooro4 years ago

  • Cc sirzooro added

comment:29 @sillybean4 years ago

  • Cc steph@… added

comment:30 @pampfelimetten4 years ago

+1, I need exactly this for a custom plugin I'm writing, where I want admins to be able to deactivate and activate users (see http://develop.servus.at/trac/cbadevel/changeset/955).
I hate being forced to use a core hack.

Last edited 4 years ago by pampfelimetten (previous) (diff)

comment:31 @voyagerfan57614 years ago

  • Cc WordPress@… added

comment:32 @dontmindtk4 years ago

  • Cc dontmindtk added

comment:33 @stephenh19884 years ago

  • Cc stephenh1988 added

comment:34 @mikeschinkel4 years ago

  • Cc mikeschinkel@… added

comment:35 follow-up: @edward mindreantre4 years ago

  • Cc edward@… added

I can only wonder why the minimal patch was pulled.

My broadcast plugin would like to be able to mass-broadcast posts but that ain't happening unless I can add my own bulk action.

And would you believe that the table list code is a barren filter-desert? Not a filter or action as far as the eye I can see so I can't even add a custom button above the table, either. *sigh*

comment:36 in reply to: ↑ 35 @Veraxus4 years ago

You have a great point edward,

While the WP_List_Table class can be extended by developers creating NEW list tables, a few well-positioned filters and actions might be exactly the thing that's needed to make this a reality. It would certainly be a clean, professional solution. I'll look into it this week. That may need a new ticket, though, because it's such a different approach - but it could also be much more future-proof.

I'll report back once I've taken a look.

Last edited 4 years ago by Veraxus (previous) (diff)

comment:37 @edward mindreantre4 years ago

As far as I can see, adding bulk actions isn't really a problem. A quick add_bulk_actions filter in the bulk_actions() function will solve the adding of filters.

It's the handling of the $_POST that's somewhat more of a problem, seeing that handling should be done before page output.

And since none of the post, link, comment, etc pages have anything in common that will mean adding bulk _POST handling code individually into each page.

Maybe $wp_list_table->handle_bulk_actions(); above $wp_list_table->prepare_items(); ?

comment:38 @F J Kaiser4 years ago

  • Cc 24-7@… added

comment:39 @lopo3 years ago

  • Cc lopo added

+1 about that: a number of users of my plugin (Duplicate Post) ask about selecting and cloning several posts at once.

comment:40 follow-up: @scribu3 years ago

  • Owner scribu deleted
  • Status changed from reopened to assigned

This will be fixed eventually, probably right after the media library revamp. :P

comment:41 in reply to: ↑ 40 @pampfelimetten3 years ago

Replying to scribu:

This will be fixed eventually, probably right after the media library revamp. :P

Is there a chance to fix this for 3.6? After all, that would be exactly right after the media library revamp ;-)

comment:42 follow-up: @scribu3 years ago

After all, that would be exactly right after the media library revamp ;-)

Hm... I guess we have to stop using "after the media library revamp" as interchangeable with "never"...

Is there a chance to fix this for 3.6?

If no one steps up with a patch, there is no chance, IMO.

comment:43 in reply to: ↑ 42 @pampfelimetten3 years ago

Replying to scribu:

If no one steps up with a patch, there is no chance, IMO.

I'm willing to give my best if you could help me a bit by providing details about what's wrong with the 16031.minimal.patch​, and in which direction it should go. The patch looks good enough for me, but the comments from nacin seem to indicate that he wants to prevent some future incompatibilities - although I'm not sure what exactly he seems to have in mind.

comment:44 @scribu3 years ago

I'm willing to give my best if you could help me a bit by providing details about what's wrong with the 16031.minimal.patch​, and in which direction it should go.

Read edward mindreantre's comment again.

comment:45 @dcowgill3 years ago

  • Cc dcowgill@… added

comment:46 @danielbachhuber3 years ago

  • Cc danielbachhuber added

Maybe we can revert r17297? I'm trying to add bulk spam to Grunion right now and just ran into this. I'm not sure what we gained by making it harder to hack stuff onto a core list table.

comment:47 @mark-k2 years ago

Was looking for adding a bulk action and ran into this ticket, then found scribu's answer on stack exchange http://wordpress.stackexchange.com/questions/29822/custom-bulk-action, and I fail to see the point of forcing people to use JS to add a bulk action. Sure it is the easier part of the solution but better then nothing.

comment:48 @KoenRijpstra21 months ago

  • Cc KoenRijpstra added

comment:49 @ocean9019 months ago

  • Component changed from Administration to Quick/Bulk Edit

comment:50 @ocean9019 months ago

  • Component changed from Quick/Bulk Edit to Administration

comment:51 @theode11 months ago

  • Component changed from Administration to Plugins
  • Severity changed from normal to trivial

Any News on this? I tried serveral things like altering the class WP_List_Table from a Plugin.

Last edited 11 months ago by theode (previous) (diff)

comment:52 @ragulka10 months ago

  • Severity changed from trivial to normal

I have run twice into this now. IMHO, preventing new bulk actions from being added just because there is no universal catcher/handler available yet, is not a good idea at all. In fact, I think it is counterproductive.

If developers could easily at least add new bulk actions to the menus, sooner or later they would realise that handling those actions should be easier/more universal. That itself could lead to someone coming up with a better/good/acceptable patch that would take care of "catching".

At the moment, it is more than possible to manually catch those actions in most (if not all?) screens. For example, the do_action( 'admin_action_' . $_REQUEST['action'] ); or even do_action( 'load-' . $pagenow ); hook can be used on most cases.

Why should we artificially limit innovation and exploration? I mean, people used to build their own custom post type stuff way before WordPress even supported custom post types. Why don't we let them figure out how to handle bulk actions themselves and see where it takes us - perhaps a good pattern will appear.

I'm not sure if anyone has realised this, but the 16031-no-new.diff ALSO prevents from existing bulk action labels to be changed. This is an issue when dealing with custom post types where the action labels need to be customised based on the context (custom post type).

So, effectively it's impossible to add new bulk actions and even to edit existing bulk actions. And all that because there is no universal ability to handle those actions. The more I think about it, the more pointless it seems - there's no real benefit or "protection" gained here - at least none that I see.

I suggest reverting that patch, so it's possible to add and edit bulk actions.

Last edited 10 months ago by ragulka (previous) (diff)

comment:53 @alexfurr6 months ago

I agree with all the recent comments. Not having an easy way to add bulk actions is v annoying and counter-productive.

It SHOULD work like this, but it doesn't :(

add_filter('bulk_actions-edit-post', array( $this, 'my_custom_bulk_action' ));		

function my_custom_bulk_action($actions)
{
	$actions['newAction'] = 'New Action';
	return $actions;		
}

+1 sort it out please.
Alex

comment:54 @SergeyBiryukov6 months ago

  • Component changed from Plugins to Administration

comment:55 @marsjaninzmarsa5 months ago

What's the status? Has anyone working on it? May I help?

comment:56 @Max Ziebell4 months ago

Bump! Any updates?

comment:57 @flixos904 weeks ago

What is the current state here? Being able to add custom bulk actions is a feature I have been working around a lot over and over again. I would like to get involved here.

comment:58 @markoheijnen2 days ago

Currently at a contributor day and someone mentioned this issue.

I read all the comments and I don't understand why [17297] was committed. I would suggest to revert this in 4.4 and see what we can do to make it easier for people to build the logic for the action. I'm more then happy to work on the solution when we agree on one.

comment:59 @Veraxus39 hours ago

FYI, I'm giving this another go and should have a patch ready by tonight or tomorrow (PST).

There are several hurdles, which I think I've solved…

  1. The hook cannot be located inside WP_List_Table; This is because…
  2. Redirects are involved on almost every screen when bulk actions are handled. Bulk actions are handled, a redirect string is created, and the browser is redirected to a "cleaned" URL to prevent duplicated execution.
  3. The various admin screens have dramatically different bulk action & redirect handling… and different conventions.
  4. Admin notices must also be catchable post-bulk-handling (which requires that the redirect URL be filterable).

To get a proper handler hook, I'm going through every single admin screen that uses a list table and (where appropriate) adding a new dynamic filter following any other hard-coded bulk actions. The filter allows modification of the (1) redirect string and passes (2) the selected objects and (3) the current action as additional arguments.

The filter looks like this (although it is, by necessity, tailored to each screen's variables)…

/**
 * A filter that allows for handling of custom bulk actions.
 *
 * The dynamic portion of the hook name, `$current_screen->id`, refers
 * to the ID of the current screen, usually a string.
 * 
 * @since 4.4.0
 * 
 * @param string $redirect_to A string containing the post-action redirect URI
 * @param array $object_ids An array of the selected objects to execute action on
 * @param string $doaction The current bulk action
 */
$redirect_to = apply_filters("handle_other_bulk_action-{$current_screen->id}", $redirect_to, $object_ids, $doaction );

Usage works like so…

$screen = 'edit-post'; // for example

// Add custom bulk action
add_filter( 'bulk_actions-'.$screen, function ( $actions ) {
	//unset( $actions['trash'] );
	$actions['custom'] = __( 'Custom' );
	return $actions;
} );

// Handle custom bulk action & modify redirect for admin notice
add_action( 'handle_other_bulk_action-'.$screen, function ( $redirect_to, $items, $current_action ) {
	$redirect_to = remove_query_arg( 'custom_completed', $redirect_to );
	if ( 'custom' === $current_action ) {
		$redirect_to = add_query_arg( 'custom_completed', count( $items ), $redirect_to );
	}
	return $redirect_to;
}, 10, 3 );

// Output admin notice when bulk handling complete
add_action( 'admin_notices', function () {
	global $current_screen;
	
	if ( !empty($_REQUEST['custom_completed']) )
		echo "<div id=\"message\" class=\"updated fade\"><p>Custom bulk action caught for {$_REQUEST['custom_completed']} items on {$current_screen->id}!</p></div>";
} );

Stay tuned.

@Veraxus36 hours ago

Reverts cs 17297 and adds comprehensive "handle_other_bulk_action-{$screen_id}" filter to all WP admin screens with list tables

comment:60 @Veraxus36 hours ago

  • Keywords has-patch added; needs-patch removed

Comprehensive patch has been uploaded.

  • Reverts changeset 17297
  • Adds filter "handle_other_bulk_action-{$screen_id}" to all appropriate admin screens that use list tables.

Filter takes the following arguments:

  • $redirect_url - String. A redirect URL to return once handling is finished.
  • $object_ids - Array. Contains the ids of the checked items.
  • $current_action - String. The current action.
  • $site_id - Int. MS only. The id of the current site.

See previous comment for example usage. To test drive the hook, use the following plugin…

<?php
/*
Plugin Name: WP List Table Filter Tests
Plugin URI: 
Description: Tests the new list table filter.
Author: Matt van Andel
Version: 1.0
Author URI: 
*/

// Enable links for testing
add_filter( 'pre_option_link_manager_enabled', '__return_true' );

// Screens to test
$lt_screens = array(
	'edit-comments',
	'edit-post_tag',
	'edit-category',
	'edit-post',
	'edit-page',
	'link-manager',
	'edit-link_category',
	//'plugin-install', // ⇐ not applicable to this screen
	'plugins',
	'upload',
	'users',
	//'themes', // ⇐ not applicable to this screen
	'themes-network',
	'sites-network',
	'site-users-network',
	'users-network',
	'site-themes-network',
	'plugins-network',
);

// Apply to all specified screens
foreach ( $lt_screens as $screen ) {
	
	add_filter( 'bulk_actions-'.$screen, function ( $actions ) {
		//unset( $actions['trash'] );
		$actions['custom'] = __( 'Custom' );
		return $actions;
	} );

	add_action( 'handle_other_bulk_action-'.$screen, function ( $redirect_to, $items, $current_action, $site_id = null ) {
		$redirect_to = remove_query_arg( 'custom_completed', $redirect_to );
		if ( 'custom' === $current_action ) {
			$redirect_to = add_query_arg( 'custom_completed', count( $items ), $redirect_to );
		}
		return $redirect_to;
	}, 10, 4 );
	
}

// Admin notice handlers
add_action( 'admin_notices', 'lt_filter_tests_admin_notice' );
add_action( 'network_admin_notices', 'lt_filter_tests_admin_notice' );

function lt_filter_tests_admin_notice () {
	global $current_screen;
	
	if ( !empty($_REQUEST['custom_completed']) )
		echo "<div id=\"message\" class=\"updated fade\"><p>Custom bulk action caught for {$_REQUEST['custom_completed']} items on {$current_screen->id}!</p></div>";
}

comment:61 follow-up: @flixos9028 hours ago

+1 for the patch @Veraxus, looks like a great starting point to relive this issue.

One thing we need to consider is whether the hook name should be tied to the screen ID vs the action name. Sure, the action name is sent as parameter, but still... For example, if you wanted to allow the same bulk action for all post type tables, you would need to use multiple hooks. Might be an edge-case, but I would assume that a bulk action hook would contain the name of the bulk action.

An alternative would be to have hooks for both cases, though it's probably better to make a real decision here.

comment:62 in reply to: ↑ 61 @Veraxus19 hours ago

Replying to flixos90:

One thing we need to consider is whether the hook name should be tied to the screen ID vs the action name. Sure, the action name is sent as parameter, but still... For example, if you wanted to allow the same bulk action for all post type tables, you would need to use multiple hooks. Might be an edge-case, but I would assume that a bulk action hook would contain the name of the bulk action.

Screens are really the only reliable way to target this. The issue is that different screens can handle data very differently. You have screens that deal with post types, screens that deal with users, and others that handle taxonomies - for instance. If your hook target was the action, as opposed to the screen, there's a higher likelihood of accidentally including code that has unintended consequences. Targeting a specific screen is definitely the best way to approach that.

And if, for instance, you wanted to easily filter all post types, it's as easy as using get_post_types() and looping through your hook with the edit- prefix.

add_action( 'init', function (){
	$post_types = get_post_types();
	
	// Apply to all registered post types
	foreach ( $post_types as $screen ) {
		
		add_filter( 'bulk_actions-edit-'.$screen, function ( $actions ) {
			//unset( $actions['trash'] );
			$actions['custom'] = __( 'Custom' );
			return $actions;
		} );
	
		add_action( 'handle_other_bulk_action-edit-'.$screen, function ( $redirect_to, $items, $current_action, $site_id = null ) {
			$redirect_to = remove_query_arg( 'custom_completed', $redirect_to );
			if ( 'custom' === $current_action ) {
				$redirect_to = add_query_arg( 'custom_completed', count( $items ), $redirect_to );
			}
			return $redirect_to;
		}, 10, 4 );
	}
});
Note: See TracTickets for help on using tickets.