Correctly assign post_parent to uploaded attachments in the new media modal

[koopersmith]

...because it's broken right now.

[mdgl]

Sorry I need to do a bit of "thinking out loud" here and I may have more to say on this later, but my first reaction to this is to ask "why would we want to assign post_parent to uploaded attachments"?

Granted there are backwards-compatibility issues but I thought that in 3.5 we were trying to do a thorough overall of media handling? Scribu for example hints (comment:ticket:21776:22) that we are trying to "de-emphasize" the post_parent field for attachments.

We also have long-standing tickets against the inflexibility of the current data model surrounding attachments (#10657 and related).

So how is 3.5 moving us forwards other than just cleaning up the implementation of parts of the admin front-end? What exactly is the content data model we are trying to create for attachments?

I think that a clearer statement on the current and future intent here would be useful, to guide authors of themes/plugins and to help ensure that we create APIs with the right kind of abstractions.

From my cursory inspection and knowledge, it seems that attachments only need to be given a post_parent to make the "gallery" work. This is so we can obtain a list of all of the images attached to a post for display and create the appropriate next/previous image links. Really what this is saying is that the "post" post type is (semi-)hierarchichal.

Surely there is a better way to do this, even whilst retaining backwards compatibility?

To me, the "gallery" looks just like a collection of posts, somewhat like a "nav menu" and should perhaps be a custom post type of its own. Maybe we could reuse the current "nav_menu" but this might be a messy implementation. In future, both could be abstracted into some kind of generic "post_collection" object.

The ability to deal with collections of images might also be useful for handling retina displays and the [potentially - depending on standardisation activities] forthcoming HTML/CSS "image set" concepts.

The current "attachment" post type is badly named as it appears to be really just a container for additional image/file metadata. It could perhaps be renamed or at least reconsidered as "media_item" as it has a 1:1 relationship with the uploaded image file.

A new post type could be created to record the attachment of an image to a post (i.e. a real "attachment" post type). This would be the actual child of a post and would contain metadata about the referenced image (i.e. a link to the "media_item" post) and how how the image should be treated as part of the post to which it is attached, for example different size, placement, caption and so on. This would fix #10657 and is I think more elegant than the copying method described by aesqe for the "File Gallery" plugin (comment:ticket:10657:15).

But why stop at just image attachments? Why shouldn't I be able to attach any number of posts/pages or other objects to another post? We already have the ability to have hierarchical posts (for "pages!) so this shouldn't be *that* difficult, should it? IMHO, the combination of the current post hierarchy tree and generic "collection" post type would create a very simple and extremely powerful CMS...

[helenyhou]

Replying to mdgl:

Sorry I need to do a bit of "thinking out loud" here and I may have more to say on this later, but my first reaction to this is to ask "why would we want to assign post_parent to uploaded attachments"?

Granted there are backwards-compatibility issues but I thought that in 3.5 we were trying to do a thorough overall of media handling? Scribu for example hints (comment:ticket:21776:22) that we are trying to "de-emphasize" the post_parent field for attachments.

De-emphasizing doesn't mean outright removing the relationship. It is not uncommon for a site to be developed using the attachment-post_parent relationship to power certain functionality. The assignment already happens on upload or insertion of a previously-unattached attachment, so this is about maintaining the current behavior. To suddenly stop doing this would be more than an issue - more along the lines of at least a serious headache, if not a nightmare or disaster, for developers and users alike.

3.5 isn't about a thorough overhaul of media handling. It's about re-thinking and re-doing the workflows and thus interfaces and related code for the selection and insertion of media and galleries. The gallery shortcode takes a comma separated list of IDs for attachments that is independent of the attachment/parent relationship. What we are getting, among other things, for this release is what you can think of as a real UI for creating that shortcode and attribute, since you can't possibly expect an end-user (even a savvy one) to go through and find IDs for all the attachments they want. This ability to decouple the gallery shortcode in a functional way from that attachment model is the de-emphasis. See #21390, #21815, #21809, and probably others.

But why stop at just image attachments? Why shouldn't I be able to attach any number of posts/pages or other objects to another post?

Why can't you now? You can assign as many objects to a given parent as you want, without any need for them to be of the same post type (there's just not necessarily the right UI built in for it because it's not common), and there's the lovely ​Posts 2 Posts for many-to-many relationships that, if not built into core, does what you need.

[koopersmith]

Helen's comment above is exactly on point. In 3.5, we're working with the following concept:

If an attachment is uploaded while editing another post, we record that post's ID as the attachment's post_parent.

This will allow code that utilizes the post_parent field to continue working as intended, but expresses the concept in a less restrictive way: an attachment can only be uploaded to a single post. This does not prevent us from inserting the attachment into any number of posts or galleries, and allows us to retain backwards compatibility while continuing to treat the data as meaningful (rather than deprecated).

[deltafactory]

Pass post ID so that attachment is properly linked

[deltafactory]

One way or another, it looks like the post_id parameter needs to be passed during the upload-attachment action. One possibility is to set it during the upload handler, but that requires finding the ID stored in a global variable. The old uploader used a quick script snippet that put it in the global post_id.

One possible solution: add a filter that adds it to the plupload settings. This potentially conflicts with other plugins that want to use plupload in different ways elsewhere on the page, so this may be a bad solution. The actual add_filter could be placed on the individual pages where it's useful, instead of in media.php.

[koopersmith]

In [22267]:

Correctly assign post_parent to attachments uploaded in the new media modal. fixes #22085, see #21390.

[mdgl]

Reopening because current behaviour at 3.5-RC1 appears to be rather different to previous releases. As pointed out by helenyhou (comment:2):

...assignment already happens on upload or insertion of a previously-unattached attachment...

Presently, we do not appear to set post_parent when inserting an existing unattached media item into a post/page, only when uploading a new media item whilst editing a post/page. This has numerous consequences (see comment:ticket:22556:3).

[nacin]

It's handled by this in get_media_item() —

if ( $post->post_parent < 1 && isset( $_REQUEST['post_id'] ) ) {
	$parent = (int) $_REQUEST['post_id'];
	$parent_name = "attachments[$attachment_id][post_parent]";
	$item .= "\t<input type='hidden' name='$parent_name' id='$parent_name' value='$parent' />\n";
}

And then this in media_upload_form_handler() —

if ( isset($send_id) && $attachment_id == $send_id ) {
	if ( isset($attachment['post_parent']) )
		$post['post_parent'] = $attachment['post_parent'];
}

I think we can handle this in wp_ajax_send_attachment_to_editor() in a much simpler fashion. If the attachment's post_parent is 0 at the time of insertion, then we can update the attachment.

Previously, we saved and inserted at the same time. Now, they are three separate ajax requests, handled by wp_ajax_save_attachment(), wp_ajax_save_attachment_compat(), and wp_ajax_send_attachment_to_editor(). We need to either chain compat() and send() in a single request or make sure that compat() returns before triggering send(), as we need to make sure the new values are in place before firing filters.

[nacin]

22085.diff​ handles it in the send-attachment-to-editor ajax request. Tested and working for me.

[nacin]

To test:

• Have an unassigned attachment (post_parent = 0, uploaded from media-new.php)
• Insert it into post A
• Verify it is now assigned to that post
• Insert it into post B
• Verify it is still assigned to post A

[mdgl]

Tested patch this morning and it seems to be getting us there. A bit difficult to tell completely because of numerous other issues, so perhaps best wait until media stabilises a bit before taking another look:

1) Upload new image from media library "add new", click "edit" to set caption opens a new window (why?), enter caption and hit "Publish" generates AYS message for leaving the page (this is in IE9 by the way).

2) Create new post and enter some text to introduce image, leave cursor at end of post, hit "Add Media" and select/insert previously uploaded image using attachment URL. Image is inserted at beginning of post, not at position of cursor. Save/update post.

3) Verify image is inserted correctly, hyperlink is correct, image is now "attached" to post.

Further discussion on what we were trying to achieve by "de-emphasising" attachment would be useful. This looks impractical if we are to preserve back-compat so perhaps also need to consider reverting #22439.

Looking at debug log after testing contains traces of other issues (probably due to unrelated activity): undefined "post_id" at line 1935 in ajax-actions.php, various "fopen" failures with redirection limit reached and 404 not found for attachments and failing to enable crypto for cron (via https? - hmnnn). Will reexamine those later on.

[nacin]

Replying to mdgl:

Tested patch this morning and it seems to be getting us there. A bit difficult to tell completely because of numerous other issues, so perhaps best wait until media stabilises a bit before taking another look:

Media is "stabilized". Most things reported here is already in progress:

1) Upload new image from media library "add new", click "edit" to set caption opens a new window (why?), enter caption and hit "Publish" generates AYS message for leaving the page (this is in IE9 by the way).

It's a new window because you could upload more than one image and may want to come back. Of course, if you upload only one image, it doesn't really make sense to open it in a new window. I'm indifferent but wouldn't mind a slight change in behavior. (This does not have a ticket, but you are welcome to open one.)

The AYS issue is covered in #22491.

2) Create new post and enter some text to introduce image, leave cursor at end of post, hit "Add Media" and select/insert previously uploaded image using attachment URL. Image is inserted at beginning of post, not at position of cursor. Save/update post.

Cannot reproduce in Chrome. Please leave a note on #22446.

3) Verify image is inserted correctly, hyperlink is correct, image is now "attached" to post.

Good.

Further discussion on what we were trying to achieve by "de-emphasising" attachment would be useful. This looks impractical if we are to preserve back-compat so perhaps also need to consider reverting #22439.

I don't think a label change is going to affect back compat. There's nothing inherently wrong with #22439.

Looking at debug log after testing contains traces of other issues (probably due to unrelated activity): undefined "post_id" at line 1935 in ajax-actions.php

I don't see this in trunk.

various "fopen" failures with redirection limit reached and 404 not found for attachments and failing to enable crypto for cron (via https? - hmnnn). Will reexamine those later on.

When WP_DEBUG is on, error suppression operators are removed from fopen() and other calls in the HTTP API, which can result in things in the logs either because of a one-off issue, or an actual problem with a transport on your server.

[ryan]

22085.diff looks good here using the comment:10 test plan and some free-form testing.

[nacin]

In 22843:

Media: When inserting an attachment, if the item is unattached (no post_parent), attach it to the post. fixes #22085.

[nacin]

[22843] does not work in QuickPress, because no post ID is passed in media_buttons() (via get_post()) to wp_enqueue_media(), which means no postId in JS. I imagine that media.view.settings.postId should be 0 by default, if no setting got passed from PHP.

Separately, QuickPress tests the $post_ID global. I'm fine with checking that global if get_post() finds nothing in the $post global.

[ryan]

wp_enqueue_media() sometimes gets a post object, sometimes and ID in .2.diff, but it looks like wp_enqueue_media() uses get_post() so it accepts either. Looks good so far.

[nacin]

Replying to ryan:

wp_enqueue_media() sometimes gets a post object, sometimes and ID in .2.diff, but it looks like wp_enqueue_media() uses get_post() so it accepts either. Looks good so far.

Yep. wp_enqueue_media() can take either by design.

[ryan]

In 22846:

In media_buttons(), consult global post_ID if get_post() comes up with nothing.
Default postID to 0 in wp_enqueue_media().

Props nacin
fixes #22085