#28112 closed enhancement (fixed)
bulk_edit_posts() function needs an action hook
Reported by: | helgatheviking | Owned by: | audrasjb |
---|---|---|---|
Milestone: | 6.3 | Priority: | normal |
Severity: | normal | Version: | 3.9 |
Component: | Quick/Bulk Edit | Keywords: | has-patch dev-feedback needs-testing has-testing-info commit has-unit-tests add-to-field-guide |
Focuses: | Cc: |
Description
Currently, the only method that I can find to save additional data in via bulk edit requires using an .ajax()
call.
This could be avoided if bulk_edit_posts()
had a simple action hook at the end before it returns it's results.
do_action( 'bulk_edit_posts', $post_data ); return array( 'updated' => $updated, 'skipped' => $skipped, 'locked' => $locked );
Attachments (5)
Change History (66)
#2
@
11 years ago
I'm enjoying your bulk/quick edit tickets very much - just want to note (as I probably have elsewhere) that this is one of those times where we'd be better off with a revamp and that any additions to what exists need to be considered ever-more carefully in that light. A Backbone-ified bulk/quick edit has been tossed around as an idea a couple times, if you want to look into that.
#3
@
11 years ago
I'm flattered! Even if I didn't ever think that tickets could be 'enjoyed'. I've been revamping one of my plugins that relies heavily on quick/bulk edit and have come to realize that those areas need some love... particularly more hooks/filters for developers. Unfortunately, I don't know the first thing about backbone so I don't know that I'd be much use there.
Pertaining to this issue... so are you saying that admin hooks/triggers will not be added in favor of a possible, eventual overhaul? It seems a shame to lose out on some hooks while that maybe happens.
All that said, my digging did eventually bring me to the revelation that bulk edit is processed by a loop that hits wp_insert_post()
so actually the ability to save bulk edit details doesn't require ajax at all and can run on the exact same save_post
routine that quick edit and meta boxes already use.
#5
@
8 years ago
- Keywords dev-feedback needs-docs added; needs-patch removed
The patch is very simple, contain the hook with a little bit of documentation (that require a feedback) in the bulk_edit_posts()
function.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
7 years ago
#8
@
7 years ago
- Keywords needs-patch needs-refresh added; has-patch removed
28112.diff the docblock needs some coding standards.
#12
@
6 years ago
- Keywords needs-docs needs-refresh removed
- Milestone changed from 5.0.1 to 5.0.2
With the plan for 5.0.1
to be a 2-week quick release in mind, I am going to punt this to 5.0.2
.
28112.2.diff is still applying cleanly for me to trunk
, but a new patch will be needed if this will be committed directly to the 5.0 branch.
This ticket was mentioned in Slack in #core by desrosj. View the logs.
6 years ago
#15
@
6 years ago
- Milestone changed from 5.0.3 to 5.1
Going to punt this to 5.1. It is outside of the scope for 5.0.3 (block
editor bugs, regressions, and major bugs).
#17
follow-up:
↓ 18
@
6 years ago
- Milestone changed from 5.1 to Future Release
- Owner pento deleted
I'm not entirely sure what the bulk_edit_posts
action is supposed to do. It's passed $post_data
, which changes for each post, but it happens outside of the post loop, so it's only called once with the data from the last post edited.
#18
in reply to:
↑ 17
;
follow-up:
↓ 19
@
6 years ago
Replying to pento:
I'm not entirely sure what the
bulk_edit_posts
action is supposed to do. It's passed$post_data
, which changes for each post, but it happens outside of the post loop, so it's only called once with the data from the last post edited.
As far as I understand @helgatheviking wants to save additional data added to the bulk edit meta box by avoiding an ajax call. Assuming this to be the case the patch provided won't work for that purpose as @pento already mentioned.
Imho such a hook is not necessary at all because there are three action hooks available to further manipulate a post after it got updated: edit_post_{$post->post_type}
, edit_post
and post_updated
#19
in reply to:
↑ 18
@
6 years ago
Replying to mensmaximus:
Imho such a hook is not necessary at all because there are three action hooks available to further manipulate a post after it got updated:
edit_post_{$post->post_type}
,edit_post
andpost_updated
I did some testing and found none of these action hooks can be utilized to save custom data from bulk edit because $_POST
is empty if wp_insert_post()
gets invoked by bulk_edit_posts()
.
To save custom data from bulk edit you can use the action hook wp_insert_post_data
. It passes $postarr
as second argument containing all values from $_POST
.
This ticket was mentioned in Slack in #core by mte90. View the logs.
4 years ago
#22
@
2 years ago
- Keywords reporter-feedback added
I believe @mensmaximus is correct regarding available hooks:
- wp_insert_attachment_data for attachments.
- wp_insert_post_data for everything else.
However, hooking these filters means the callback will run on each insert - something that might be undesirable. For this reason, the proposed action hook would be more appropriate.
The action hook in the patch may need to use $shared_data
, because $post_data
is extended with data for each post in the foreach
loop, and this additional data would only be for the last post in the loop. @helgatheviking, @Mte90, what do you think about this?
#23
@
2 years ago
Is there a way to detect if wp_insert_post_data()
is running only in the quick edit? If so, that seems more than sufficient.
If not, after all these years, I don't see the harm in adding a hook. Needing the other array variable makes sense, but I certainly couldn't tell you what my test results were like back in 2014. :)
#24
@
2 years ago
Just as reference today the function is on https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/includes/post.php#L471
Looking at the code $shared_post_data
is a copy of $post_data
used inside the loop but $post_data
is used to save the data inside posts so seems the best choice.
This ticket was mentioned in PR #2813 on WordPress/wordpress-develop by Mte90.
2 years ago
#25
Trac ticket: https://core.trac.wordpress.org/ticket/28112
#26
@
23 months ago
- Keywords reporter-feedback removed
Thanks for the PR. I refreshed it, fixed some WPCS issues, and improved the related docblock.
@costdev I think we can probably consider this for 6.2, isn't it? :)
#27
@
23 months ago
- Milestone changed from Future Release to 6.2
@audrasjb Sounds good to me! Milestoning.
I've also dropped a minor comment on the PR I'd appreciate your input on.
#28
@
23 months ago
- Keywords dev-feedback removed
- Owner set to audrasjb
- Status changed from assigned to accepted
I added a commit to the PR to rephrase the docblock, thanks for the review @costdev!
Self assigning for commit
consideration.
@audrasjb commented on PR #2813:
23 months ago
#31
Committed in https://core.trac.wordpress.org/changeset/55108
#32
follow-up:
↓ 33
@
23 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
Thanks for the commit!
I believe comment:17 raises a good point: the current implementation of the action accepts $post_data
, which contains the submitted form results, plus some data for the last post in the loop, but the latter is not mentioned in the hook documentation and seems confusing for future maintenance, as it conflates two different data sets.
I think passing $shared_post_data
instead, as suggested in comment:22, would be the correct thing to do here. If we do feel strongly about also including the post data processed in the loop, it should be consistently collected for each post and passed as the second parameter.
Also, if my reading of comment:3 is correct, it seems like this action might not actually be needed after all, as the use case from the ticket description can be achieved via existing hooks, e.g save_post
?
All that said, my digging did eventually bring me to the revelation that bulk edit is processed by a loop that hits
wp_insert_post()
so actually the ability to save bulk edit details doesn't require ajax at all and can run on the exact samesave_post
routine that quick edit and meta boxes already use.
With the comment above, and also Helen's note in comment:2:
just want to note (as I probably have elsewhere) that this is one of those times where we'd be better off with a revamp and that any additions to what exists need to be considered ever-more carefully in that light.
are we sure there is still enough demand for this hook? I'm all for making core more flexible, but also not sure if it's a good idea to introduce hooks just because we can, as they will need to be maintained indefinitely due to WP's backward compatibility promise.
#33
in reply to:
↑ 32
;
follow-up:
↓ 35
@
23 months ago
Replying to SergeyBiryukov:
Also, if my reading of comment:3 is correct, it seems like this action might not actually be needed after all, as the use case from the ticket description can be achieved via existing hooks, e.g
save_post
?
All that said, my digging did eventually bring me to the revelation that bulk edit is processed by a loop that hits
wp_insert_post()
so actually the ability to save bulk edit details doesn't require ajax at all and can run on the exact samesave_post
routine that quick edit and meta boxes already use.
I might be wrong, but I think the issue if that there is no way to determine the context of the process (in this case: running a bulk edit process).
#34
@
23 months ago
If we are talking about focus it is better a dedicated hook to avoid to use save_post that is executed in all of them.
You can have different settings in the panel in the bulk mode as example, this let the developer just to focus on what to do on bulk.
#35
in reply to:
↑ 33
@
23 months ago
Replying to audrasjb:
I might be wrong, but I think the issue if that there is no way to determine the context of the process (in this case: running a bulk edit process).
It appears that $_GET['bulk_edit']
is set during bulk edit, so that might be a way to check for the context. Then again, what would be a use case of only saving some data on bulk edit but not on regular post edit?
#36
@
23 months ago
It is passed so much that since 2016 that I don't remember why I did that patch, but I remember that it was for something I was looking into.
Apart from what kind of data, I think that it is more simple for a dev to use that new filter instead looking on wp_insert_post and the $_GET variable.
As usually you don't have any guide or tutorial on how to do something so specific on WordPress as the docs are just for function but contain also list of hooks.
If someone is looking on how to save data on bulk it is more easy to look for a hook that investigate the WordPress internals.
#37
@
22 months ago
- Keywords needs-docs added
Bug review January 30, 2023:
- The discussion found it is useful to have the whole information at once, but questioned whether the implementation was the ideal way.
- Concerns on implementation resulting in an excessive load on the memory which could be a pain point for some projects.
- If we need this implementation is needed for bulk editing.
- Comment #35 mentions that we don't need this hook and Sergey's solution for bulk edit.
As there is also the question of maintaining hooks in the future, Sergey's solution appears to provide a way forward in many situations. Suggestion that a documentation would be useful if how to use bulk edit in this situation. This ticket could be reopened if reporters / users had additional needs, rather than single case usage.
- Included a link in Slack for any other comments.
Scrub attendees: webcommsat, marybaum, ogleckler, robinwpdeveloper
This ticket was mentioned in Slack in #core by abhanonstopnews. View the logs.
22 months ago
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
22 months ago
#40
follow-up:
↓ 41
@
22 months ago
- Keywords dev-feedback added
@audrasjb @SergeyBiryukov With 6.2 Beta 1 being released later today, what's the next step for this ticket?
Does [55108] need to be reverted pending confirmation that this hook is still needed, that the right variable is passed, and that the docs are correct?
Additional props: @mukesh27
#42
@
22 months ago
@SergeyBiryukov As there is still uncertainty about this and 6.2 Beta 1 is fast approaching, I'd suggest a revert and re-milestoning for 6.3 so that we can aim to land this next cycle once we get a final idea of whether this enhancement is needed, and how it should be implemented.
This ticket was mentioned in Slack in #core by abhanonstopnews. View the logs.
22 months ago
#46
@
22 months ago
- Keywords needs-dev-note needs-docs removed
For 6.2 this needs neither a dev note nor docs, because it won't be part of the release. If it becomes part of 6.3, then it will need dev note and docs.
Atm, it's not clear that the hook would offer new functionality.
#48
@
22 months ago
Right now the code was removed after adding it because we don't agree if adding or not.
This ticket was mentioned in Slack in #core-test by oglekler. View the logs.
21 months ago
#50
follow-up:
↓ 51
@
18 months ago
- Keywords has-testing-info added
I changed my mind, I think we should add this action, it can be useful. But use cases will not appear until it will be in the core. We are in 9 days to the Beta, and this is probably simplest from all other enhancements we have. So, it can be in Beta 1 in no time.
I tested the patch, even if it is a simple one, it is still applicable.
Here the data which is going to this action:
Array ( [s] => [post_status] => draft [post_type] => page [_wpnonce] => 0276e53341 [_wp_http_referer] => /wp-admin/edit.php?post_type=page&paged=1 [action] => edit [m] => 0 [paged] => 1 [bulk_edit] => Update [post_view] => list [screen] => edit-page [post] => Array ( [0] => 24 [1] => 25 [2] => 26 ) [action2] => edit [post_ID] => 26 [post_mime_type] => [comment_status] => open [ping_status] => closed [post_author] => 1 [ID] => 26 [user_ID] => 1 )
One of the use cases can be - update of these posts in some other place via webhook.
#51
in reply to:
↑ 50
@
18 months ago
Replying to oglekler:
I changed my mind, I think we should add this action, it can be useful.
Fair enough :)
But the current patch still conflates two different data sets, as noted in comment:32:
- The submitted form data
- Some data for the last post in the loop
This can be confusing for developers, as they might assume all the data applies to all posts, whereas some of it in fact only applies to the last one. Moreover, the list of post IDs can be incorrect, as some of the posts might be skipped and not updated because they are locked or due to insufficient permissions.
If we're going to implement this hook, I believe it should receive:
- The list of post IDs which were actually updated
- The submitted form data that applies to all of the updated posts
Would 28112.4.diff address the potential use cases here?
#52
@
18 months ago
@SergeyBiryukov, yes, I think $shared_post_data
is more accurate, but you've missed $ of this variable in the patch. $shared_post_data['post']
and $updated should be equal, but better to keep $updated
. I would have expected that if nothing has changed, the post ID will not be added to $updated, but this isn't the case, even if nothing has changed in post parameters, it's still going to this array.
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
18 months ago
This ticket was mentioned in PR #4716 on WordPress/wordpress-develop by @costdev.
18 months ago
#55
- Keywords has-unit-tests added
#56
@
18 months ago
Thanks @costdev for the PR.
@audrasjb Could you please take a look before beta so we can ship this also?
#58
@
17 months ago
@audrasjb can you please take a look? This ticket was reopened 5 months ago, and I cannot imagine any side effects from this patch.
@SergeyBiryukov commented on PR #4716:
17 months ago
#60
Thanks for the PR! Merged in r56091.
Some JS triggers in
inline-edit-posts.js
wouldn't go astray either. For example: