Make WordPress Core

Opened 10 years ago

Closed 10 months ago

Last modified 9 months ago

#28112 closed enhancement (fixed)

bulk_edit_posts() function needs an action hook

Reported by: helgatheviking's profile helgatheviking Owned by: audrasjb's profile 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)

28112.diff (663 bytes) - added by Mte90 8 years ago.
28112.2.diff (533 bytes) - added by mrasharirfan 6 years ago.
I have applied WordPress coding standards to the patch.
28112.3.diff (576 bytes) - added by itowhid06 5 years ago.
Updated @since to reflect actual version the hook is introduce
28112.4.diff (620 bytes) - added by SergeyBiryukov 10 months ago.
28112.5.diff (1.5 KB) - added by costdev 10 months ago.
Added missing $ and a PHPUnit test to ensure the action is always fired in future.

Download all attachments as: .zip

Change History (66)

#1 @helgatheviking
10 years ago

Some JS triggers in inline-edit-posts.js wouldn't go astray either. For example:

t.trigger( "setBulk-inlineedit" );

#2 @helen
10 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 @helgatheviking
10 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.

#4 @chriscct7
8 years ago

  • Keywords needs-patch added

@Mte90
8 years ago

#5 @Mte90
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.

#6 @Mte90
8 years ago

  • Keywords has-patch added

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


6 years ago

#8 @afercia
6 years ago

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

28112.diff the docblock needs some coding standards.

#9 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 5.0

#10 @afercia
6 years ago

  • Keywords has-patch added; needs-patch removed

@mrasharirfan
6 years ago

I have applied WordPress coding standards to the patch.

#11 @SergeyBiryukov
6 years ago

  • Milestone changed from 5.0 to 5.0.1

#12 @desrosj
5 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.

#13 @pento
5 years ago

  • Milestone changed from 5.0.2 to 5.0.3

@itowhid06
5 years ago

Updated @since to reflect actual version the hook is introduce

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


5 years ago

#15 @desrosj
5 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).

#16 @pento
5 years ago

  • Owner set to pento
  • Status changed from new to assigned

#17 follow-up: @pento
5 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: @mensmaximus
5 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 @mensmaximus
5 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 and post_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 filter hook wp_insert_post_data. It passes $postarr as second argument containing all values from $_POST.

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

#20 @Mte90
3 years ago

This ticket is still valid after all those years?

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


3 years ago

#22 @costdev
22 months ago

  • Keywords reporter-feedback added

I believe @mensmaximus is correct regarding available hooks:

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 @helgatheviking
22 months 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 @Mte90
22 months 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.

#26 @audrasjb
15 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 @costdev
15 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 @audrasjb
15 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.

#29 @audrasjb
15 months ago

  • Keywords needs-dev-note added

#30 @audrasjb
15 months ago

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

In 55108:

Quick/Bulk Edit: Add an action hook on bulk_edit_posts().

This changeset introduces the bulk_edit_posts action hook, triggered after processing the post data for bulk edit and before it returns its results. For example, it allows developers to save additional data without having to perform any .ajax() call.

Props helgatheviking, helen, Mte90, afercia, mrasharirfan, desrosj, itowhid06, pento, mensmaximus, audrasjb, costdev.
Fixes #28112.

#32 follow-up: @SergeyBiryukov
15 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 same save_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.

Last edited 15 months ago by SergeyBiryukov (previous) (diff)

#33 in reply to: ↑ 32 ; follow-up: @audrasjb
15 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 same save_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 @Mte90
15 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 @SergeyBiryukov
15 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?

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

#36 @Mte90
15 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 @webcommsat
15 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.


15 months ago

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


14 months ago

#40 follow-up: @costdev
14 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

Last edited 14 months ago by costdev (previous) (diff)

#41 in reply to: ↑ 40 @SergeyBiryukov
14 months ago

Replying to costdev:

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?

I think that would be preferred for now, though curious to see what others think.

#42 @costdev
14 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.

#43 @SergeyBiryukov
14 months ago

In 55265:

Quick/Bulk Edit: Remove the bulk_edit_posts action for now.

This reverts [55108] pending confirmation that this hook is still needed, that the right variable is passed, and that the docs are correct.

The necessity and implementation of this hook will be reconsidered in a future release.

See #28112.

#44 @SergeyBiryukov
14 months ago

  • Milestone changed from 6.2 to 6.3

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


14 months ago

#46 @marybaum
14 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.

#47 @oglekler
14 months ago

  • Keywords needs-testing added

#48 @Mte90
14 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.


14 months ago

#50 follow-up: @oglekler
10 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 @SergeyBiryukov
10 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 @oglekler
10 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.

@costdev
10 months ago

Added missing $ and a PHPUnit test to ensure the action is always fired in future.

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


10 months ago

#54 @oglekler
10 months ago

  • Keywords commit added

Patch is ready for review

This ticket was mentioned in PR #4716 on WordPress/wordpress-develop by @costdev.


10 months ago
#55

  • Keywords has-unit-tests added

#56 @mukesh27
10 months ago

Thanks @costdev for the PR.

@audrasjb Could you please take a look before beta so we can ship this also?

#57 @oglekler
10 months ago

PR looks fine for me, ready for commit. Please proceed.

#58 @oglekler
10 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.

#59 @SergeyBiryukov
10 months ago

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

In 56091:

Quick/Bulk Edit: Add an action hook to bulk_edit_posts() function.

This changeset introduces the bulk_edit_posts action hook, triggered after processing the post data for bulk edit and before the function returns its results. For example, it allows developers to save additional data without having to perform any .ajax() calls.

Follow-up to [8973].

Props helgatheviking, helen, Mte90, afercia, mrasharirfan, desrosj, itowhid06, pento, mensmaximus, audrasjb, costdev, webcommsat, marybaum, oglekler, mukesh27, SergeyBiryukov.
Fixes #28112.

@SergeyBiryukov commented on PR #4716:


10 months ago
#60

Thanks for the PR! Merged in r56091.

#61 @milana_cap
9 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.