Opened 14 years ago
Closed 8 years ago
#16031 closed enhancement (fixed)
Bulk actions: Reactivate bulk actions hook + add hander hook for all admin screens
Reported by: | Veraxus | Owned by: | johnbillion |
---|---|---|---|
Milestone: | 4.7 | Priority: | normal |
Severity: | normal | Version: | 3.1 |
Component: | Administration | Keywords: | has-patch |
Focuses: | docs | 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 (18)
Change History (126)
#1
@
14 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.
#2
@
14 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?
#3
@
14 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).
#6
@
14 years ago
- Milestone changed from Awaiting Review to 3.1
- Owner set to westi
- Status changed from new to reviewing
#7
@
14 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.
#8
@
14 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 );
#9
@
14 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.
#11
@
14 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.
#12
@
14 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.
#13
@
14 years ago
16031.minimal.patch skips edit-comments.php, which would remain to be handled in a future release.
#14
@
14 years ago
The next step would be to pass the list of elements to be processed to the action.
#15
@
14 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.
#17
@
14 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.
#18
@
14 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
#21
@
14 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.
#22
@
14 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.
#23
@
14 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
#24
@
14 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.
#25
@
14 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.
#27
@
13 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'.
#30
@
13 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.
#35
follow-up:
↓ 36
@
13 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*
#36
in reply to:
↑ 35
@
13 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.
#37
@
13 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(); ?
#39
@
13 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.
#40
follow-up:
↓ 41
@
12 years ago
- Owner scribu deleted
- Status changed from reopened to assigned
This will be fixed eventually, probably right after the media library revamp. :P
#41
in reply to:
↑ 40
@
12 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 ;-)
#42
follow-up:
↓ 43
@
12 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.
#43
in reply to:
↑ 42
@
12 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.
#44
@
12 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.
#46
@
12 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.
#47
@
12 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.
#51
@
10 years 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.
#52
@
10 years 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.
#53
@
10 years 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
#57
@
9 years 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.
#58
@
9 years 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.
#59
@
9 years 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…
- The hook cannot be located inside
WP_List_Table
; This is because… - 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.
- The various admin screens have dramatically different bulk action & redirect handling… and different conventions.
- 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.
@
9 years ago
Reverts cs 17297 and adds comprehensive "handle_other_bulk_action-{$screen_id}" filter to all WP admin screens with list tables
#60
@
9 years 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>"; }
#61
follow-up:
↓ 62
@
9 years 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.
#62
in reply to:
↑ 61
@
9 years 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 ); } });
#63
@
9 years ago
Is there any reason we shouldn't be targetting 4.4 for this ticket? Is there anything else we need to move forward on this? I'm willing to take the lead on whatever else needs to be done to get this ticket finally resolved.
#64
@
9 years ago
- Owner set to DrewAPicture
- Status changed from assigned to accepted
I'm not inclined to support opening up the bulk actions filter while still lacking a solid approach for a catcher behavior.
@Veraxus: While I appreciate the effort in 16031-comprehensive.patch, there's an awful lot of fairly unrelated stuff going on in there.
I think our best bet would be to introduce a handle_bulk_actions()
method in WP_List_Table
that's called at the top of prepare_items()
in the sub-classes and contains a dynamic action hook using the screen ID.
At its base, this approach would give a more straightforward path for "catching" the request data on the other end. I think wherever possible, we'd want to adapt core list tables leveraging bulk actions to extend that new handle_bulk_actions()
method.
This isn't "no we can't have this" or wontfix even, but it's not going to happen in time for 4.4. Realistically, we'd need to get a patch together at the beginning of the 4.5 cycle to give any outlying bugs time to surface.
All that said, I look forward to working with you on this in 4.5 and getting it over the finish line. Let's do this thing :-)
#65
@
9 years ago
Thanks for taking charge of this ticket, @DrewAPicture !
When I first decided to tackle this problem, I actually started with WP_List_Table
, as per previous tweaks and suggestions by @scribu and @edward mindreantre - but quickly discovered that it was impossible. Some notes about the admin screens...
- Every admin screen handles actions outside of the
WP_List_Table
class, on its respective screen. - After hard-coded default actions are handled, a custom redirect is built by feeding the current URL through
add_query_arg()
as necessary. This ensures that duplicate actions aren't accidentally executed on refresh, since the redirect effectively flushes the request data after it's handled. Any handler hook we create must allow for modifying the redirect URL, so that devs can create custom result messages. $wp_list_table->prepare_items()
is usually run after the aforementioned redirect (and after actions are caught and handled), meaning our request data will never reach it. It's also not executed consistently before or after hard-coded actions are handled (and therefore that redirect).- When and where the redirect is handled by each screen sometimes varies wildly. But in each case the hook must occur before the forced redirect occurs (which flushes our request data).
And this is why the comprehensive patch adds the hooks to each admin screen, contingent on the redirect, and not WP_List_Table
.
Unfortunately, we simply won't be able to neatly package this behavior into WP_List_Table
without dramatically reworking every single core admin screen as well. We'd have to merge the action-handler logic and redirect handling into each screens respective list table child class... but that seems like a different project/ticket.
BUT... maybe that's what we should do given the wild lack of any common style or convention in those files (i.e. use it as an opportunity to tidy up the code for each screen).
My point is: putting the handler inside WP_List_Table
is anything but straightfoward due to the way these screens are constructed (action handling + redirect building). It would be a pretty substantial refactoring project.
Thoughts?
P.S. I just realized that the comprehensive patch includes some extra indentation on a couple files. I remember addeding it to make it easier for me to follow the code, which is something of a mess tbh, and forgot to revert it for a cleaner patch. *sigh* :-/
#67
@
9 years ago
- Summary changed from New bulk actions hook missing catcher behavior to Bulk actions: Reactivate bulk actions hook + add hander hook for all admin screens
This ticket was mentioned in Slack in #core by veraxus. View the logs.
9 years ago
#69
@
9 years ago
@DrewAPicture I'm ready to take another stab at this for 4.5... but we have a decision to make: do we want to try to clean up the admin screens and move everything (including the hook) into the WP_List_Table class(es) OR do we want to include the hook in each of the admin screens as my previous comprehensive patch did?
Those are really the only two possible approaches. Due to the save-redirect process being handled on each of the admin screens (and not in any WP_List_Table class), putting this hook into WP_List_Table, by necessity, means a massive refactoring of each of the admin screens as well as the associated WP_List_Table child classes.
IMO, this should be a 2-release process. The first (targeting 4.5) being to update the comprehensive patch to refactor each of the admin screens, making them more consistent while adding the necessary hook. The second (targeting 4.6) being to further refactor them by moving the list table related functionality into the appropriate classes.
This ticket was mentioned in Slack in #core by veraxus. View the logs.
9 years ago
#71
@
9 years ago
@Veraxus. Thanks for this nice initiative to take this issue further.
I do like your suggestion to clean things up and make them more maintainable. Also your analysis to use multiple releases sounds workable.
One suggestion I would like to make is to consider adding a new hook to WP_List_Table in 4.5 while keeping the current hooks. The current hooks in each of the admin screens can than be removed in 4.6. This offers plugin developers (like myself) the time to migrate their functionality.
This ticket was mentioned in Slack in #core by veraxus. View the logs.
9 years ago
#73
@
9 years ago
- Keywords dev-feedback added
Worked through this with @Veraxus in chat today.
Let's discuss implementation on one WordPress asset type (posts), and whatever we decide to do there should trickle down.
I think we should add these actions inline, where the form handlers already exist. Refactoring WP_List_Table
is a non-trivial and perhaps separate effort. That can happen even after these hooks go in.
Please review a proposed approach in attachment:16031.diff, and what a plugin's implementation with this looks like in attachment:16031-plugin-example.php. Feedback welcome :)
We add a form submission handler action for plugins to tap into, a filter for the sendback URL, and allow plugins to add bulk actions.
#74
@
9 years ago
- Keywords has-patch removed
@nacin @SergeyBiryukov @danielbachhuber @westi any thoughts on the above?
#75
follow-up:
↓ 76
@
9 years ago
@ericlewis I think it would be helpful if the action hook would also send the items to perform the bulk action on - for example we could send the post IDs to the action callback. Otherwise plugin developers would need to always get the post IDs from $_REQUEST
on their own although this could be handled by Core I think.
Also, in your patch it's possible to do something even on the Core actions. I don't think developers should be able to adjust the way WordPress edits/trashes/deletes posts from here, so I'd propose to put the action into a default
in the switch clause above.
#76
in reply to:
↑ 75
@
9 years ago
Replying to flixos90:
@ericlewis I think it would be helpful if the action hook would also send the items to perform the bulk action on
[Also,] I don't think developers should be able to adjust [core bulk actions]
Both good points! attachment:16031.2.diff takes these into consideration.
#77
follow-up:
↓ 78
@
9 years ago
Some thoughts on having handle_bulk_actions-
and after_handle_bulk_actions_sendback_link-
as separate hooks...
Pros:
- Clear separation of purpose.
- Can potentially tweak the sendback url without a bulk action check.
Cons:
- Would be forced to use
$GLOBALS
to pass bulk action status fromhandle_bulk_actions-
toafter_handle_bulk_actions_sendback_link-
, which seems a little hinky.
The alternative is combining the two into a single hook...
$sendback_link = apply_filters( "handle_bulk_action-{$current_screen->id}", $sendback_link, $post_ids, $action );
Thoughts?
#78
in reply to:
↑ 77
@
9 years ago
Replying to Veraxus:
[Having the actions as separate hooks would force you to] to use globals to pass bulk action status [between them]
That's a good point. Let's combine the two into one as you suggest, in attachment:16031.3.diff
#79
@
9 years ago
And example plugin to go along with attachment:16031.3.diff
<?php // Add the button to the bulk action dropdown. add_action( 'bulk_actions-edit-post', function($actions) { $actions['glerf'] = 'Glerf'; return $actions; }); // Add a form submission handler. add_action( 'handle_bulk_actions-edit-post', function($sendback, $action) { if ( $action !== 'glerf' ) { return $sendback; } $sendback = add_query_arg('glerfed', 1, $sendback); return $sendback; }, 10, 2);
This ticket was mentioned in Slack in #core by veraxus. View the logs.
9 years ago
#81
@
9 years ago
- Keywords needs-patch added; dev-feedback removed
- Milestone changed from Future Release to 4.6
Moving this to 4.6 for consideration. I think the approach in attachment:16031.3.diff is good to extend to other list table handlers, will need a new patch.
This ticket was mentioned in Slack in #core by chriscct7. View the logs.
9 years ago
#84
@
8 years ago
- Keywords has-patch added; needs-patch removed
I've uploaded a new comprehensive patch (16031-4.7.patch) for the latest trunk per the format that @ericlewis and I discussed previously; I've also added a new sample plugin that allows for testing on all admin screens.
#85
@
8 years ago
@Veraxus this is looking great! Functionally your test plugin works as expected.
We should move this logic into the default
handler in existing switch statements as per flixos90's comment. Would you mind adjusting the patch accordingly?
#87
@
8 years ago
The patch has been updated.
You'll notice it's slightly bigger than before. Three of the screens ( edit-comments.php, network/sites.php, and network/users.php ) would loop over items first and check actions second. I needed to flip the logic so the action switch comes first and the loops are handled second. I've tested all screens thoroughly to ensure that both default and custom actions are working.
The original bulk_action_test plugin continues to work as-is for testing & example purposes.
#88
@
8 years ago
Thanks for the updated patch @Veraxus! In attachment 16031.4.diff
- Instead of rewriting some of the surrounding action handling
switch() {}
statements, I've written some specifically targeted if statements to trigger the custom action. This avoids larger refactors of this code. - Add
check_admin_referer()
calls for XSRF protection. - Add type coercion and checking borrowed from neighboring bulk handlers.
@DrewAPicture can you take a look at the docs here if you have a chance? Thanks!
I've updated the test plugin in attachment:bulk-action-test-plugin.php — two squirelly bits:
- You need to manually remove the query string from the action-completed handler, like in edit.php. Otherwise a success or failure message displayed will persist over refreshes or completing other bulk edit actions.
- Should also add the query string key to the
removable_query_args
filter.
Could use some testing, code review and any other feedback.
This ticket was mentioned in Slack in #core by eric. View the logs.
8 years ago
#90
@
8 years ago
In attachment:16031.5.diff, @azozz suggests strict in_array()
checks, and small edits in documentation.
This ticket was mentioned in Slack in #core by chriscct7. View the logs.
8 years ago
#93
@
8 years ago
- Resolution set to fixed
- Status changed from assigned to closed
With great happiness, closing as no issues have been reported in 10 days.
#94
follow-ups:
↓ 97
↓ 98
@
8 years ago
- Keywords needs-patch added; has-patch removed
- Resolution fixed deleted
- Status changed from closed to reopened
The dynamically-named 'handle_bulk_actions-*
hook receives different parameters depending on the value of the dynamic portion of the hook name. Yikes! Clear documentation for this will get ugly.
These should either be statically named hooks, or a better way of clearly passing different formats and number of data to the hooks is needed.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#96
@
8 years ago
- Owner set to ericlewis
- Status changed from reopened to assigned
@ericlewis Can you take a look at @johnbillion's comment.
#97
in reply to:
↑ 94
@
8 years ago
Replying to johnbillion:
These should either be statically named hooks, or a better way of clearly passing different formats and number of data to the hooks is needed.
We can't statically name the hooks in the current implementation, as some of the hooks are covering multiple screens. eg. the edit.php handle_bulk_actions-* hook invokes handle_bulk_actions-edit-post
, handle_bulk_actions-edit-page
and handle_bulk_actions-my-custom-post-type
.
If we want statically named hooks here, perhaps we could change all hooks to use a single hook, handle_bulk_actions
, and expect developers to perform the screen check in their callback. Since different screens will include different contextual data, perhaps we consolidate it in an array in a $context
argument. eg.
// For users.php
$context = array(
'user_ids' => $userids
);
// For network/site-themes.php
$context = array(
'themes' => $themes,
'site_id' => $site_id
);
// The hook invocation might look like this
$redirect_to = apply_filters( 'handle_bulk_actions', $redirect_to, $do_action, $context );
// A bound callback might look like this
add_action( 'handle_bulk_actions', function($redirect_to, $do_action, $context) {
if ( get_current_screen()->id !== 'users' ) {
return $redirect_to;
}
// Do something with $context['user_ids'], modify redirect link
return $redirect_to;
}, 10, 3 );
#98
in reply to:
↑ 94
;
follow-up:
↓ 99
@
8 years ago
Replying to johnbillion:
The dynamically-named
handle_bulk_actions-*
hook receives different parameters depending on the value of the dynamic portion of the hook name. Yikes! Clear documentation for this will get ugly.
I'm not entire sure I follow, but I think there's a misunderstanding about the patch.
The hook accepts the following arguments...
- First argument is always the redirect URL (string).
- Second argument is always the name of the action (string).
- Third argument is always the array of items (array).
- Fourth argument applies to network screens only, but is the site id (int)
If you're looking at the patches, you might have noticed that the names of the variables being fed to the hook vary from screen to screen, which may be where this misunderstanding comes from. This is because core is not following any sort of standard naming conventions (or even standard pattern) across screens.
However, the data that each argument receives is the same, no matter what.
Personally, I think it's good as-is, but some alternative options include:
- Refactor the admin screens to standardize variable naming conventions
- Note: Refactoring previously vetoed as out of scope for this ticket.
- If the fourth (site id) argument is the main concern, we can either:
- Separate the network-targetted hooks into
handle_network_bulk_actions-*
solely because those hooks get the site id as an extra argument (unnecessary, IMO). - Provide a default value (e.g.
false
) for the fourth argument on non-network implementations of the hook.
- Separate the network-targetted hooks into
If your concern is with the PhpDocs having disparate variable names, the first option is the only solid option IMO... since proxying data into another variable just for the sake of the docs seems like incurring more tech debt on screens that already have quite a bit of that (see: disparate naming conventions and patterns).
#99
in reply to:
↑ 98
@
8 years ago
- Focuses docs added
Replying to Veraxus:
- Third argument is always the array of items (array).
- Fourth argument applies to network screens only, but is the site id (int)
The problem is that the items contained within the third parameter -- and therefore the description needed for it -- differ between instances of this hook. The items can be posts, comments, users, etc, and this means the documentation for the same hook name varies across its different instances. Same applies for the fourth argument - this argument is not always present so its description differs.
By the way -- for clarity -- I'm thinking about this from the perspective of the generated documentation on developer.wordpress.org
. It's not possible to have multiple different @param
descriptions for one hook that varies depending on the value of the dynamic part of its name.
If you're looking at the patches, you might have noticed that the names of the variables being fed to the hook vary from screen to screen, which may be where this misunderstanding comes from.
The actual variable name doesn't matter to the hook docs -- its description and contents does.
- Separate the network-targetted hooks into
handle_network_bulk_actions-*
solely because those hooks get the site id as an extra argument (unnecessary, IMO).
Yes, this is a good idea to solve the problem of the fourth parameter.
This ticket was mentioned in Slack in #docs by johnbillion. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#102
@
8 years ago
Spoke about this on Slack with @johnbillion.
We're going to move ahead and
- change the network-targeted hooks to
handle_network_bulk_actions-*
- genericize the docblock parameter names and descriptions
- leave only one docblock for each action instance, and replace duplicates with links to the documented version.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#104
@
8 years ago
In attachment:16031.7.diff, the changes suggested in comment:102.
Fixes submission handling for pages affected by the new bulk_action filter