edit_post() should call {un}stick_post() before calling wp_update_post()

[pbiron]

In a plugin I'm writing I have a need to hook into the save_post action and decide whether to allow a given post to be sticky based on postmeta associated with that post (and possibly other posts).

However, if the function I attach to the save_post action calls {un}stick_post() it doesn't have the desired effect because of the way edit_post() is written.

In particular, lines 320-332 of https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/post.php#L320 are:

wp_update_post( $post_data );

// Now that we have an ID we can fix any attachment anchor hrefs
_fix_attachment_links( $post_ID );

wp_set_post_lock( $post_ID );

if ( current_user_can( $ptype->cap->edit_others_posts ) ) {
	if ( ! empty( $post_data['sticky'] ) )
		stick_post( $post_ID );
	else
		unstick_post( $post_ID );
}

The save_post action is fired within wp_update_post() (technically, it is fired by wp_insert_post() which is called by wp_update_post()). Since edit_post() calls {un}stick_post() after it calls wp_update_post(), anything I do in my save_post function with regard to whether the post should be sticky is undone!

Hence, I suggest changing the relevant portion of edit_post() to

if ( current_user_can( $ptype->cap->edit_others_posts ) ) {
	if ( ! empty( $post_data['sticky'] ) )
		stick_post( $post_ID );
	else
		unstick_post( $post_ID );
}

wp_update_post( $post_data );

// Now that we have an ID we can fix any attachment anchor hrefs
_fix_attachment_links( $post_ID );

wp_set_post_lock( $post_ID );

Is there a specific reason why edit_post() currently calls wp_update_post() before {un}stick_post()? (it isn't apparent to me)

Another option I investigated was whether it was possible to have my plugin change $post_data via a filter but I couldn't find one.

If others agree that this change would be good and wouldn't break anything else, I'll submit a patch.

[Note: I went back and forth on whether to call this a bug or an enhancement. It's a bug in the sense that there is certainly an unexpected result ({un}sticking a post in a `save_post` action is undone...I lost about an hour trying to figure out why my `save_post` function wasn't doing the right thing); But it is an enhancement in the sense that core, itself, is not broken].

[pbiron]

a little more investigation suggests that it will also be necessary to make a similar change in https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-xmlrpc-server.php#4900.

In particular, wp_xmlrpc_server::mw_editPost() calls wp_insert_post() before calling {un}stick_post().

wp_xmlrcp_server::_insert_post() and wp_xmlrpc_server::mw_newPost(), however, call them in the "correct" order and won't require a change.

I don't even know what the wp_xmlrpc_server class does so I can't test that my proposed fix works here as well. I'd appreciate it someone who actively uses wp_xmlrcp_server could test that.

[Denis-de-Bernardy]

At a conceptual level, side effects like this belong after (or on, or immediately before) the save_post hook, not before the update occurs. Consider filtering the option's value instead. There's a hook for that in get_option(). (Don't forget to cache the result using e.g. A transient that gets deleted on save_post. That way, your plugin won't leave side-effects when disabled.)

[pbiron]

I'm not sure, in this context, what you mean by "side effect"...since conceptually sticky is a property of a post. The fact that sticky is implemented as an option (rather than as a column in the wp_posts table) has always seemed odd to me and have figured that it was done that way for some obscure technical reason.

Also, the ​documentation on the save_post action says "Runs after the data is saved to the database." and in this case, I think a good argument can be made that the sticky_posts option should be considered part of the "data" that "is saved to the database" before save_post is fired.

Given that the wp_xmlrpc_server class has 2 functions that call {un}stick_post() before calling wp_{insert,update}_post() and 1 that calls them in the same order as edit_post() at least points out that there is inconsistency on this matter in core.

But, yes, as a stopgap measure I guess I could implement my plugin using the option_sticky_posts filter but that seems like a very obscure workaround.

[Denis-de-Bernardy]

Replying to pbiron:

I'm not sure, in this context, what you mean by "side effect"...since conceptually sticky is a property of a post.

I meant it at a conceptual level from a database design viewpoint: worrying about whether we set some kind of sticky property somewhere is the kind of thing that ought to be done during or immediately after updating the row were things to run within a transaction. This, to avoid the use of deferrable constraints or triggers to enforce integrity.

The fact that sticky is implemented as an option (rather than as a column in the wp_posts table) has always seemed odd to me and have figured that it was done that way for some obscure technical reason.

Nope. I'll spell it out loud for you: it was an utterly absurd design decision. WP is paying dearly for it, too — e.g. #23336.

Also, the ​documentation on the save_post action says "Runs after the data is saved to the database." and in this case, I think a good argument can be made that the sticky_posts option should be considered part of the "data" that "is saved to the database" before save_post is fired.

On paper, doing things correctly would mean that the save_post hook acts like a "done updating, whoever wants to do more can do so now" event. Nothing should be occurring after wp_update_post() — but nor should anything be done before it, for that matter. Hence the remark I made in my previous comment: if anything, saving the sticky status should be done immediately before or *on* the save_post hook: after the row is inserted or updated, and before or while save_post gets called.

In practice, there's stuff over in wp_insert_post(), in wp_update_post(), in edit_post(), etc, etc. and, as you point out, not in a very consistent manner.

But, yes, as a stopgap measure I guess I could implement my plugin using the option_sticky_posts filter but that seems like a very obscure workaround.

Hooking directly into options or transients during reads or writes is fairly common in my own experience, due to the occasional lack of sensible hooks. There's also e.g. pre_update_option_sticky_posts in update_option() and a few more in add_option() and delete_option() which may come in handy.

[pbiron]

Replying to Denis-de-Bernardy:

Replying to pbiron:

The fact that sticky is implemented as an option (rather than as a column in the wp_posts table) has always seemed odd to me and have figured that it was done that way for some obscure technical reason.

Nope. I'll spell it out loud for you: it was an utterly absurd design decision. WP is paying dearly for it, too — e.g. #23336.

I'll accept that :-) But since I wasn't around when the decision was made I didn't want to put my foot in my mouth not knowing what went into the decision.

BTW, one of the checks my save_post action is doing is making sure that there is only 1 sticky post in any given category (to avoid some of the problems mentioned in #23336, where users "forget" to unstick posts).

It also accomplishes something similar to the Featured_Content class in the bundled twentyfourteen theme: allow certain posts to be marked as being available to be displayed in a special sidebar, in which case they should not be sticky (this is the part that's giving me trouble with stick_post() being called after my save_post function.

Also, the ​documentation on the save_post action says "Runs after the data is saved to the database." and in this case, I think a good argument can be made that the sticky_posts option should be considered part of the "data" that "is saved to the database" before save_post is fired.

On paper, doing things correctly would mean that the save_post hook acts like a "done updating, whoever wants to do more can do so now" event. Nothing should be occurring after wp_update_post() — but nor should anything be done before it, for that matter. Hence the remark I made in my previous comment: if anything, saving the sticky status should be done immediately before or *on* the save_post hook: after the row is inserted or updated, and before or while save_post gets called.

Then maybe I misunderstood your first comment...because the above sounds like you're agreeing with me that {un}stick_post() should be called before wp_{insert,update}_post() (and I took your first comment as arguing against that).

And what you said above gave me an idea for a possibly better "fix" than that in my original description: we could remove all the existing calls to {un}stick_post() and replace them by a function hooked (with the lowest possible priority number) to save_post in core. This would guaranteed that they are called before any plugin's save_post and clear up the "conceptual" problem as well.

[Denis-de-Bernardy]

Replying to pbiron:

BTW, one of the checks my save_post action is doing is making sure that there is only 1 sticky post in any given category (to avoid some of the problems mentioned in #23336, where users "forget" to unstick posts).

And what you said above gave me an idea for a possibly better "fix" (…)

Or, you could simply hook into pre_update_option_sticky_posts and run a sanity check on the array that is about to get stored, and return a trimmed version of the array.

[pbiron]

Replying to Denis-de-Bernardy:

Or, you could simply hook into pre_update_option_sticky_posts and run a sanity check on the array that is about to get stored, and return a trimmed version of the array.

there are many possible workarounds, but isn't it better to fix the problem than to work around it?

and hooking into pre_update_option_sticky_posts would not be a simple matter because at that point I would not have the info on the post that was just saved...so I'd have to do something like: check the post_date of the latest revision for each of the sticky posts to figure out which one had just been edited, etc. And that would totally violate the "conceptual" goal.

[nacin]

As a general rule, we should be doing things before the wp_update_post() call, not after. This includes setting a post format or whatever else.

[pbiron]

Replying to nacin:

As a general rule, we should be doing things before the wp_update_post() call, not after. This includes setting a post format or whatever else.

I just did a quick check and it may not always be always be possible to do things before wp_{insert,update}_post(). The first case I found is get_default_post_to_edit(): which calls wp_insert_post() and then set_post_format() when the $create_in_db parameter == true; and the way set_post_format() is written it requires the post to exist in the db before it is called :-(

What do you think of my idea of handling those things in functions hooked in core to save_post?

I'll volunteer to do a first pass on a list of all the places where wp_{insert,update}_post() is called where any info is written to the db after that call. However, I probably won't be able to do that for a few weeks: the plugin I'm writing that made me open this ticket is for a client site that has a hard go-live date in just a few weeks and I have to get that site launched before spending much time on this (of course, I'll have to find some workaround for my problem for that go-live, because I know that this issue won't be resolved and released before then).

[kevinlangleyjr]

Moving {un}stick_post() function calls to be before wp_update_post()

[desrosj]

I am thinking that a better solution here may be to add support for a new argument within $postarr for wp_{insert|update}_post() (maybe is_sticky?) that indicates if a post should be updated to be sticky or not.

This would simplify this a lot and make it easier to filter the intended sticky state going forward.

[pbiron]

Replying to desrosj:

I am thinking that a better solution here may be to add support for a new argument within $postarr for wp_{insert|update}_post() (maybe is_sticky?) that indicates if a post should be updated to be sticky or not.

This would simplify this a lot and make it easier to filter the intended sticky state going forward.

That's an interesting idea @desrosj.

This ticket is so old I forget many of the particulars of what I was encountering when I first opened it. But one thing I do remember was that it was hard to come up with a fix that maintained BC behavior. So, as @SergeyBiryukov ​indicated in today's bug scrub some unit tests will be very important.