WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 months ago

#18833 new enhancement

Pass raw data into save_post/wp_insert_post

Reported by: rmccue Owned by: rmccue
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: has-patch
Focuses: administration Cc:

Description

Almost all tutorials about custom metaboxes (in addition to post meta) use $_POST to get the data that was submitted. Unfortunately, this breaks if a plugin uses anything that calls wp_insert_post() in the same request.

The patch attached here includes the $postarr array, which is the raw data passed in, and which is $_POST from wp-admin/post.php. This allows one to use the third parameter to the callback to set meta data instead of having to use $_POST directly.

Attachments (1)

18833.diff (469 bytes) - added by rmccue 3 years ago.
Add $postarr to the save_post and wp_insert_post actions.

Download all attachments as: .zip

Change History (19)

rmccue3 years ago

Add $postarr to the save_post and wp_insert_post actions.

comment:1 rmccue3 years ago

To note, this also makes it much safer, and avoids metaboxes having to use their own nonces (though they probably should), and avoids the need for checking permissions.

comment:2 scribu3 years ago

Interesting suggestion. I think we should deprecate one of those filters by passing the $postarr arg to only one of them.

comment:3 follow-up: sirzooro3 years ago

  • Cc sirzooro added

+1 for this.

BTW, metaboxes needs to check its own nonces (or something similar) because wp_insert_post() is called from many contexts, and many of them does not include metaboxes at all (e.g. quickedit).

comment:4 in reply to: ↑ 3 rmccue3 years ago

Replying to scribu:

Interesting suggestion. I think we should deprecate one of those filters by passing the $postarr arg to only one of them.

I agree (although, I left the patch with both), and I'd say save_post is the most appropriate, given that it is consistent with the edit_post filter also used in the function.

Replying to sirzooro:

BTW, metaboxes needs to check its own nonces (or something similar) because wp_insert_post() is called from many contexts, and many of them does not include metaboxes at all (e.g. quickedit).

Instead of checking for a nonce, it can simply check for the values it actually uses (i.e. if (empty($postarr['myplugin_value'])) return; ) which is what functions should be doing anyway.

comment:5 rmccue3 years ago

Example usage:

function rm_savemydata($id, $post, $raw) {
	// Use `isset` here if it can be blank
	if (!empty($raw['rm_myfield'])) {
		return;
	}

	update_post_meta($id, 'rm_myfield', $raw['rm_myfield']);
}
add_action('save_post', 'rm_savemydata', 10, 3);
Last edited 3 years ago by rmccue (previous) (diff)

comment:6 follow-up: scribu3 years ago

The only case when that won't work is if you have a single checkbox.

comment:7 in reply to: ↑ 6 ; follow-up: sirzooro3 years ago

Replying to scribu:

The only case when that won't work is if you have a single checkbox.

The same is for radio buttons and selection lists, when nothing is selected.

comment:8 scribu3 years ago

Yeah. So you wouldn't need a nonce necessarily, just a hidden input.

comment:9 dd323 years ago

My only thoughts on this is that this ties us to passing the complete $_POST array in as $postarr. If passing $_POST in is a shortcut, I'd personally prefer not to go down this road, of course, if it already relies upon ($postarr === $_POST) that might be a different case.

comment:10 in reply to: ↑ 7 rmccue3 years ago

Replying to sirzooro:

The same is for radio buttons and selection lists, when nothing is selected.

Those should still just be equivalent to $_POST['something'] = ''; (i.e. empty value, but still set). I'm not sure for checkboxes, however.

Replying to dd32:

My only thoughts on this is that this ties us to passing the complete $_POST array in as $postarr. If passing $_POST in is a shortcut, I'd personally prefer not to go down this road, of course, if it already relies upon ($postarr === $_POST) that might be a different case.

It is already passed in, as this is where wp_insert_post gets the actual data from. The only change (as noted in the patch) is to add it to the two actions.

comment:11 follow-up: scribu3 years ago

Almost all tutorials about custom metaboxes (in addition to post meta) use $_POST to get the data that was submitted. Unfortunately, this breaks if a plugin uses anything that calls wp_insert_post() in the same request.

Why is that?

comment:12 in reply to: ↑ 11 ; follow-up: rmccue3 years ago

Replying to scribu:

Almost all tutorials about custom metaboxes (in addition to post meta) use $_POST to get the data that was submitted. Unfortunately, this breaks if a plugin uses anything that calls wp_insert_post() in the same request.

Why is that?

If a plugin tries to add a post manually (by passing in a data array), all data will be taken from this for wp_insert_post. However, these metabox callbacks will instead get the data from $_POST, and hence, the wrong data will be added.

This patch enables the metaboxes to use the data that is passed in. For a POST request, this is the $_POST data. For a manual insert by a plugin, it will be the data that is passed in.

comment:13 dd323 years ago

It is already passed in, as this is where wp_insert_post gets the actual data from. The only change (as noted in the patch) is to add it to the two actions.

Yes, However nothing but the core post fields are used from the array. Passing it to functions like this will introduce a reliance upon having to (forever) pass the complete $_POST array in. Right now for example, it is entirely possible to only pass select fields in (although we don't).

Where this comes up, is from #18322, As part of switching from the slashed to unslashed globals, certain functions which expect slashed data will need some special handling. One suggestion which I have seen floating around (which I personally prefer) would result in only passing select elements through functions like this. I'm thinking of the future use of the functions, not just what's easiest today.

comment:14 in reply to: ↑ 12 scribu3 years ago

Replying to rmccue:

If a plugin tries to add a post manually (by passing in a data array), all data will be taken from this for wp_insert_post. However, these metabox callbacks will instead get the data from $_POST, and hence, the wrong data will be added.

In other words, the problem is that you can't distinguish between the original call made by Core and other calls.

It's similar to the problem of differentiating the main WP_Query instance from others.

I think what we actually need here is a new action that's fired only after the "Publish/save" button is pushed.

comment:15 scribu3 years ago

Ideally, we would move the 'save_post' action out of wp_insert_post(), but that's probably not possible, due to back-compat.

comment:16 rmccue3 years ago

Replying to dd32:

Where this comes up, is from #18322, As part of switching from the slashed to unslashed globals, certain functions which expect slashed data will need some special handling. One suggestion which I have seen floating around (which I personally prefer) would result in only passing select elements through functions like this. I'm thinking of the future use of the functions, not just what's easiest today.

In which case, the fields that are passed in could be selected through a filter. In addition, in terms of backwards compatibility, this is a new addition, and hence carries no expectation of unslashed/slashed globals.

It's also not about what's easiest (there's no real change in terms of that), but more about making wp_insert_post safe to call twice.

Replying to scribu:

In other words, the problem is that you can't distinguish between the original call made by Core and other calls.

Also, calls to wp_insert_post can't allow custom meta data without setting $_POST manually.

comment:17 dd323 years ago

In which case, the fields that are passed in could be selected through a filter.
In addition, in terms of backwards compatibility, this is a new addition, and hence carries no expectation of unslashed/slashed globals.

I'm assuming that this would be implemented before #18322 here, #18322 has a longer term goal which would need to be planned, this, at first glance just looks like a easy nice thing to do - but one that has future backcompat potential issues

comment:18 jeremyfelt3 months ago

  • Component changed from Administration to Posts, Post Types
  • Focuses admin added
Note: See TracTickets for help on using tickets.