WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 17 months ago

Last modified 17 months ago

#22186 closed defect (bug) (fixed)

Backwards compatibility for admin/includes/media.php hooks

Reported by: nacin Owned by:
Milestone: 3.5 Priority: low
Severity: normal Version:
Component: Media Keywords:
Focuses: Cc:

Description

Lots of hooks in this file. Probably the most common one is attachment_fields_to_edit, but there are others.

These need to be implemented in either the new post.php screen (#21391) or the media modal (#21390) depending.

I've been making mental notes, but what this really needs is a full audit of what we need to make sure is still backwards compatible, then keep it functional (in deprecated form, most likely).

koopersmith is already working on making media tabs back compat; that's a component of this too.

Attachments (3)

22186.diff (5.3 KB) - added by nacin 18 months ago.
22186.2.diff (796 bytes) - added by nacin 18 months ago.
Test code for core tabs.
22186.3.diff (5.5 KB) - added by nacin 17 months ago.
Back compat handlers for sending to the editor.

Download all attachments as: .zip

Change History (59)

comment:1 DrewAPicture19 months ago

  • Cc xoodrew@… added

It would be nice to maintain the post-upload-ui action in the modal somewhere.

Though I'm sure there are plenty of other plugins that hook into post-upload-ui our use case is related to my previous query about taking advantage of plupload's built-in scaling functionality. In 3.4.2, we hook to post-upload-ui which in effect allows us to set the height/width js vars that plupload needs to allow max-size scaling on upload.

comment:2 ocean9019 months ago

  • Cc ocean90 added

comment:3 Chouby18 months ago

  • Cc frederic.demarle@… added

Glad to see this ticket open. For me the most important part is the backward compatibility in the media modal as I still do not foresee how it will work.

I have an additionnal issue and I don't know if this ticket is the right place to ask for it as it is not a hook, though it concerns backward compatibility for the media modal:

In 3.4 and older, when we filter the query done to display media in the library tab of the 'add media' form, it is possible to know the current edited post through $_REQUEST['post_id']. In 3.5 beta 2, $_REQUEST does contain only 'action' and 'query' indexes. Could it be possible to set $_REQUEST['post_id']?

comment:4 koopersmith18 months ago

In 22523:

Media: Backwards compatibility for media_upload_tabs.

  • Adds createIframeStates() to the MediaFrame view. It creates states and bindings for the media_upload_tabs output, and is included on MediaFrame.Post by default.
  • Hijacks tb_remove() when the media modal is open to ensure the modal closes correctly.
  • Adds a chromeless parameter to thickbox media tab URLs to render the UI without the old row of tabs.

see #22186, #21390.

comment:5 nacin18 months ago

In 22524:

Don't unnecessarily translate media upload tab strings. see #22186.

nacin18 months ago

comment:6 nacin18 months ago

22186.diff is an attempt at providing a framework for making attachment_fields_to_edit and _save backwards compatible in some way. Here's some random testing code. Fire this up in 3.4 to see it in action when editing a media item from the Library.

add_filter( 'attachment_fields_to_edit', function( $fields, $post ) {
	$fields['nacin_menu_order']['label'] = 'Nacin Order (cannot be 42)';
	$fields['nacin_menu_order']['value'] = $post->menu_order;
	return $fields;
}, 10, 2 );

add_filter( 'attachment_fields_to_save', function( $post, $attachment ) {
	if ( isset( $attachment['nacin_menu_order'] ) ) {
		if ( $attachment['nacin_menu_order'] == '42' )
			$post['errors']['nacin_menu_order']['errors'][] = 'Nacin order cannot be 42.';
		else
			$post['menu_order'] = $attachment['nacin_menu_order'];
	}
	return $post;
}, 10, 2 );

nacin18 months ago

Test code for core tabs.

comment:7 koopersmith18 months ago

In 22541:

Media: Add backwards compatibility for attachment_fields_to_edit and attachment_fields_to_save. see #22186.

comment:8 nacin18 months ago

Here's the results of an audit of all hooks in wp-admin/includes/media.php. The big stuff is already handled. Some of these can simply fall by the wayside, while others might deserve a shim.

A lot of admin/includes/media.php was rendered in some way obsolete in 3.3 when the media buttons were merged into one (#17578) and Plupload was introduced.

Filters tied to the old dialog:

  • media_upload_tabs — handled
  • media_upload_default_tab — could be supported if we wanted, but isn't really relevant anymore, and appears unused by plugins
  • media_buttons_context — no longer relevant, this was the "Upload/Insert" label
  • {$type}_upload_iframe_src — no longer relevant
  • media_upload_form_url — bunch of filters on "media-upload.php"
  • get_media_item_args — could be used to decide whether certain things were displayed for an item, including "Insert into Post" and a "Delete" link. We can support these via get_compat_media_markup()
  • type_url_form_media — chunk of html for "Embed from URL", no need to support
  • media_upload_mime_type_links — mime-based filter links, which we could support

Filters for attachment data:

  • image_size_names_choose
  • attachment_fields_to_save — handled
  • attachment_fields_to_edit — handled
  • media_meta — handled

Filters for sending to the editor:

  • {$type}_send_to_editor_url (type = file, audio, or video) — used when inserting a non-image from an external URL
  • image_send_to_editor_url — this is nearly always bypassed already, thanks to addExtImage in JS
  • disable_captions — we should support this. Note, we only check it for post insertion itself: for whether it reaches the editor, and whether the field is shown for "from URL" insertion for images. The caption field is not otherwise hidden in the media library

Filters for "Insert into Post":

  • media_send_to_editor — fires for anything inserted into the post
  • image_send_to_editor — fires for any images inserted into the post (get_image_send_to_editor() is hooked into media_send_to_editor, via image_media_send_to_editor())
  • image_add_caption_shortcode — adds the caption shortcode for any images inserted into the post (inside image_add_caption(), which is hooked into image_send_to_editor)

Upload UI filters:

  • upload_post_params (new in 3.3) — pretty much inescapably tied to the old uploader, not much we can do here unless we wanted to support routing to old-style tabs
  • plupload_init (new in 3.3) — the plupload_default_settings hook seems sufficient. array_merging these seems like it would cause more harm than good

Upload UI actions:

  • upload_ui_over_quota (new in 3.5)
  • pre-upload-ui
  • pre-plupload-upload-ui (new in 3.3)
  • post-plupload-upload-ui (new in 3.3, used to display "use the browser uploader?")
  • pre-html-upload-ui
  • post-html-upload-ui (used to display "use the multi-file uploader?")
  • post-upload-ui (used to display "you can add titles and descriptions", which is useless)

comment:9 koopersmith18 months ago

In 22556:

Media: Improve backwards compatibility for image_size_names_choose filter. see #22186, #21390.

comment:10 koopersmith18 months ago

In 22557:

Media: Add support for applicable *-upload-ui hooks.

Supports pre-upload-ui, pre-plupload-upload-ui, post-plupload-upload-ui, post-upload-ui.

see #22186, #21390.

comment:11 k3davis18 months ago

  • Cc k3davis added

comment:12 trepmal17 months ago

It used to be possible to put an image inside the custom media tabs, but with the new modal, the <img> is converted to entities
http://cdn.dropmark.com/17023/17ed8b8d3b01d355536af80c648c63684d39e5ba/media-tab-image.png

Secondly, after inserting an image from a custom tab, clicking Add Media tries to return to the same tab - but it's empty.
That was awkward, here's a screencast: http://screencast.com/t/6sVsCfB6R1

And my test plugin: http://kdl.dropmark.com/25665/1224781

comment:13 goto1017 months ago

  • Cc dromsey@… added

comment:14 follow-up: Chouby17 months ago

Here are some comparison results between WordPress 3.4.2 and WordPress 3.5-beta3-22613.

For the "Edit Media" panel:

  • In WP 3.4, the filter attachment_fields_to_edit filter is called
    • $pagenow is set to 'media.php'
    • $screen->base is set to 'media'
  • In WP 3.5, the filter is not called

For the "library" tab of the "Add media" modal (only the new one for WP 3.5):

  • in WP 3.4, the filter parse_query is called
    • $pagenow is set to 'media-upload.php'
    • $screen->base is set to 'media-upload'
    • $_REQUEST['post_id'] refers to post currently edited
  • In WP 3.5, the filter parse_query is called but
    • $pagenow is set to 'admin-ajax.php'
    • $screen->base is not set
    • $_REQUEST['post_id'] is not set
  • In both cases, the attachment_fields_to_edit filter is called for each picture in the library

I will wait for a future nightly build to go on my tests (currently the new modal is broken when WP_DEBUG is set to true)

comment:15 in reply to: ↑ 14 ; follow-up: koopersmith17 months ago

Thanks for the comparison, Chouby.

Replying to Chouby:

I will wait for a future nightly build to go on my tests (currently the new modal is broken when WP_DEBUG is set to true)

Adding define( 'SCRIPT_DEBUG', true ); to wp-config.php will help — it makes sure you're using the development scripts instead of the minified scripts, which often lag behind.

comment:16 nacin17 months ago

In 22670:

Avoid doing all of the extra work in get_attachment_fields_to_edit() for fields we will just discard. We just need the filter, and the taxonomies. see #22186.

comment:17 nacin17 months ago

In 22673:

Move code from image_attachment_fields_to_edit() to get_attachment_fields_to_edit(). The level of abstraction is unnecessary, and it makes it more difficult to call only the filter in the new media modal. Also, that function is sloooow. see #22186.

comment:18 nacin17 months ago

In 22674:

Call wp_cache_set() with a group and then an expiration in wp_mime_type_icon(). fixes #22503. see #22186.

comment:19 nacin17 months ago

In 22679:

Remove errant return. props SergeyBiryukov. see #22186.

nacin17 months ago

Back compat handlers for sending to the editor.

comment:20 koopersmith17 months ago

In 22761:

Media: Backwards compatibility for the disable_captions filter. see #22186, #21390.

comment:21 koopersmith17 months ago

In 22768:

Media: Backwards compatibility for the many send_to_editor filters. props nacin. see #22186, #21390.

comment:22 nacin17 months ago

In 22778:

Remove legacy media uploader bits from custom-header.php and custom-background.php. see #22186, #21390.

comment:23 nacin17 months ago

In 22783:

Support attachment_fields_to_save and attachment_fields_to_edit for attachments going through post.php. see #22186. see #21391.

comment:24 nacin17 months ago

  • Priority changed from high to normal

Based on the summary above, we still need to review:

  • get_media_item_args
  • Upload UI actions

I'm not convinced on the feasibility or wisdom of supporting get_media_item_args, and upload UI actions are there; they just might get touched up. So, restoring this ticket's priority to normal.

comment:25 in reply to: ↑ 15 ; follow-up: Chouby17 months ago

Replying to koopersmith:

Adding define( 'SCRIPT_DEBUG', true ); to wp-config.php will help — it makes sure you're using the development scripts instead of the minified scripts, which often lag behind.

Yes. SCRIPT_DEBUG is set to true. But it's all my fault. Setting WP_DISPLAY_DEBUG to false solved the issue caused by a notice:
PHP Notice: Undefined offset: 1 in /wp-includes/media.php on line 1242 (build beta3-22734)
This notice was due to an attachment created without media by my plugin which currently does not work correctly with the current beta version of WP 3.5

comment:26 in reply to: ↑ 25 nacin17 months ago

Replying to Chouby:

PHP Notice: Undefined offset: 1 in /wp-includes/media.php on line 1242 (build beta3-22734)
This notice was due to an attachment created without media by my plugin which currently does not work correctly with the current beta version of WP 3.5

That sounds like a valid bug, can you report it? In 3.4 we never made the explicit assumption that a mime type has a minimum of two parts when you split it by "/". In 3.5, we do.

comment:27 Chouby17 months ago

I will do

Version 0, edited 17 months ago by Chouby (next)

comment:28 nacin17 months ago

  • Priority changed from normal to low

comment:29 batmoo17 months ago

  • Cc batmoo@… added

comment:30 ethitter17 months ago

  • Cc erick@… added

comment:31 koopersmith17 months ago

In 22818:

Media: Display WordPress and plupload error messages whenever an upload fails. see #22243, #22186, #21390.

comment:32 koopersmith17 months ago

In 22819:

Media: Make the pre-upload-ui hooks share an element with the post-upload-ui hooks. see #22186, #21390.

comment:33 koopersmith17 months ago

In 22821:

Media: Make friends with media_upload_form. Adds notices for browser incompatibility, upload limits, maximum file size, and large file issues to the uploader. fixes #22243, see #22186, #21390.

comment:34 Chouby17 months ago

I go on with my tests.

In RC1, the filter 'attachment_fields_to_edit' is OK. But unlike in WP 3.4, the attachment taxonomies are not automatically saved anymore. A workaround is to use the filter 'attachment_fields_to_save' as we would do for other custom fields.

I still did not find a way to know (in the filter 'parse_query') which post is currently edited when using the new "Add Media" modal (to replace $_REQUEST['post_id'] which was available in 3.4)

comment:35 follow-up: batmoo17 months ago

Is media_buttons_context being deprecated or removed completely? (from [22828])

comment:36 in reply to: ↑ 35 nacin17 months ago

Replying to batmoo:

Is media_buttons_context being deprecated or removed completely? (from [22828])

It *should* be removed completely, as it was only supposed to be a way to change the label "Upload/Insert". Alas, a lot of code inaccurately used it instead of media_buttons to insert media.

We need to remain compatible here, though it won't be easy. We need some research into the common ways of modifying this string to insert more things. I imagine since you're asking, you've found something?

comment:37 nacin17 months ago

I've carried media_buttons_context into #22559.

Last edited 17 months ago by nacin (previous) (diff)

comment:38 batmoo17 months ago

Replying to nacin:

We need to remain compatible here, though it won't be easy. We need some research into the common ways of modifying this string to insert more things. I imagine since you're asking, you've found something?

Yep, though, being used incorrectly as you note (specially, using it to add media button instead of the media_buttons action). I don't think we need to support compatibility for the incorrect usages unless there's a compelling reason that people are using the context filter over the action (which I don't have).

comment:39 follow-up: Chouby17 months ago

in RC1, it seems that the filter 'attachment_fields_to_save' is not called from the new "Add media" modal. In WP 3.4, it was called when clicking on "save all changes" (the button does not exist anymore) and when clicking "insert into post" (this does not work in WP 3.5 RC1)

comment:40 follow-up: Chouby17 months ago

Another issue found in RC1 with the new "Add media" modal:
If I directly upload file(s), the attachment details is displayed without fields created by 'attachment_fields_to_edit'
If I first go to the library, the fields are correctly displayed.

comment:41 in reply to: ↑ 39 nacin17 months ago

Replying to Chouby:

in RC1, it seems that the filter 'attachment_fields_to_save' is not called from the new "Add media" modal. In WP 3.4, it was called when clicking on "save all changes" (the button does not exist anymore) and when clicking "insert into post" (this does not work in WP 3.5 RC1)

It should be fired via Ajax request, as you go. Can you open up an individual ticket for this issue?

comment:42 in reply to: ↑ 40 nacin17 months ago

Replying to Chouby:

Another issue found in RC1 with the new "Add media" modal:
If I directly upload file(s), the attachment details is displayed without fields created by 'attachment_fields_to_edit'
If I first go to the library, the fields are correctly displayed.

Can you open up an individual ticket for this issue?

comment:43 Chouby17 months ago

Done. See #22577 and #22578

comment:44 follow-up: nacin17 months ago

To address some of Chouby's comments, we should definitely have the XHR requests query-attachments, save-attachments, and save-attachments-compat pass $_REQUEST['post_id'] back, for consistency with the old way.

I'm also okay with a set_current_screen( 'media-upload' ) in each of those, or at least in query-attachments.

Chouby, if there is anything else you have pointed out (or have not yet pointed out), please open fresh tickets reported against trunk. In an ideal world, your plugin works without modification in 3.5 — I'm more than willing to entertain all bug reports that can get us to that.

comment:45 in reply to: ↑ 44 nacin17 months ago

Replying to nacin:

To address some of Chouby's comments, we should definitely have the XHR requests query-attachments, save-attachments, and save-attachments-compat pass $_REQUEST['post_id'] back, for consistency with the old way.

I'm also okay with a set_current_screen( 'media-upload' ) in each of those, or at least in query-attachments.

I opened #22588 for these things.

comment:46 nacin17 months ago

An update on the audit:

Filters tied to the old dialog:

  • media_upload_tabs — Supported.
  • media_upload_default_tab — Not supported. Not really relevant, and appears unused by plugins.
  • media_buttons_context — Will be supported, see #22559.
  • {$type}_upload_iframe_src — No longer relevant.
  • media_upload_form_url — No longer relevant.
  • get_media_item_args — Not currently supported.
  • type_url_form_media — Not supported.
  • media_upload_mime_type_links — Not supported. Chunk of HTML.

Filters for attachment data:

  • image_size_names_choose — Supported.
  • attachment_fields_to_save — Supported.
  • attachment_fields_to_edit — Supported.
  • media_meta — Supported.

Filters for sending to the editor:

  • {$type}_send_to_editor_url (type = file, audio, or video) — Supported.
  • image_send_to_editor_url — Not supported. This filter was already bypassed thanks to addExtImage.
  • disable_captions — Supported.

Filters for "Insert into Post":

  • media_send_to_editor — Supported.
  • image_send_to_editor — Supported.
  • image_add_caption_shortcode — Supported.

Upload UI filters:

  • upload_post_params (new in 3.3) — Not supported.
  • plupload_init (new in 3.3) — Not supported.

Upload UI actions:

  • upload_ui_over_quota — Supported.
  • pre-upload-ui — Supported.
  • pre-plupload-upload-ui — Supported.
  • post-plupload-upload-ui — Supported.
  • pre-html-upload-ui — No browser uploader in the dialog, but kept on media-new.php.
  • post-html-upload-ui — No browser uploader in the dialog, but kept on media-new.php.
  • post-upload-ui Supported.

End result: Only get_media_item_args is left outstanding, as "Not currently supported". We need to figure out how it works, how it was used by plugins, how it applies to the new workflow, and whether it should still be supported.

comment:47 follow-up: bbrooks17 months ago

In the new media UI, custom fields added by hooking into: 'attachment_fields_to_edit' do not get submitted with a nonce, fail ajax verification, and thus do not retain any information entered by the user.

Here's the ajax form data being submitted for the custom fields:

action:save-attachment-compat
id:5555
attachments[5555][name_of_custom_field]:sample custom field text

Compare this to the same submit for the 'alt' field of an image:

action:save-attachment
id:5555
nonce: g5354sd343
changes[alt]:sample alt text

And this is the notice I get when a custom media field attempts to update on blur:

PHP Notice: Undefined index: nonce in [...]/wp-includes/pluggable.php on line 832

comment:48 in reply to: ↑ 47 nacin17 months ago

Replying to bbrooks:

In the new media UI, custom fields added by hooking into: 'attachment_fields_to_edit' do not get submitted with a nonce, fail ajax verification, and thus do not retain any information entered by the user.

Yep, good catch. Just fixed in #22577.

comment:49 nacin17 months ago

get_media_item_args is a filter on the arguments passed to get_media_item(), which are:

  • 'errors' => null
  • 'send' => $current_post_id ? post_type_supports( get_post_type( $current_post_id ), 'editor' ) : true
  • 'delete' => true
  • 'toggle' => true
  • 'show_title' => true

errors is an array of potential errors that get passed from the handler to get_attachment_fields_to_edit(). We don't actually display (or pass along) these errors in the back compat code for 3.5. Hey, at least we handle the fields...

send is a boolean, for whether the item should be given the opportunity to be sent to the editor. This was always simply "true" until 3.0, when it then changed to whether the post type supported the editor, it seems for post-thumbnail purposes. It's also changed quite a bit over the years since being introduced in [6876] (and filtered since [16476]) — see [19350], [15920], [14146], [14118]). There's never been a core reason for "false". Plus, with the new view changes, supporting this really doesn't make much sense.

delete is a way to specify if the delete link should show. It was set to false on media.php, which is now no longer exposed (#21391). We don't have delete links yet (#22524). If we did, this *could* be a reason to support this filter. But I don't think it will be missed.

toggle is whether the panel should be able to be opened and closed (the accordion-style media items). This isn't relevant with the new UI.

show_title was again used only on media.php. Not relevant with the new UI.

comment:50 nacin17 months ago

A little under 30 instances of get_media_item_args in plugins. All but 1 force send to on. Most of those (all but five) using one of two functions from a plugin options/form framework (the methods are cmb_force_send and allow_img_insertion).

The only one that is using get_media_item_args() for something other than send = true, is for send = false, to implement some kind of post thumbnail functionality. This plugin was recipe-schema. I activated it, and after figuring out how to use it (you need to create a recipe, then add that recipe to a post), I was able to trigger the old thickbox dialog and successfully attach a thumbnail.

So, it seems get_media_item_args shall die with the old dialog.

comment:51 nacin17 months ago

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

I know of no other backwards compatibility adjustments required for media in 3.5. New tickets for any new issues, please!

Last edited 17 months ago by nacin (previous) (diff)

comment:52 ZaneMatthew17 months ago

  • Cc zanematthew@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry Nacin, found this one, not sure if its an issue along the lines of what your looking for.

The second parameter in the hooks seems not to have the same indexes in that $attachments array.

I have a hook similar to this:

function my_func( $post, $attachment ){
    print '<pre>';
    print_r( $attachment );
    print '</pre>';
    die();
}
add_action('attachment_fields_to_save', 'my_func');

which on WP older than 3.5-rc

I receive this:

Array
(
    [menu_order] => 0
    [post_title] => 
    [image_alt] => 
    [post_excerpt] => 
    [post_content] => 
    [url] => 

)

RC gives me nothing.

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

comment:53 nacin17 months ago

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

Hi Zane, could you open a new ticket for this?

comment:54 ocean9017 months ago

But before this, be sure that you are using add_filter('attachment_fields_to_save', 'my_func', 10, 2);.

comment:55 Chouby17 months ago

I just checked with RC2 and confirm. Only custom fields are in $attachment. The same in "edit media" and the new "Add media" modal. So it's worth that Zane opens a new ticket for this.

comment:56 ZaneMatthew17 months ago

Ticket created here: #22664

Last edited 17 months ago by SergeyBiryukov (previous) (diff)
Note: See TracTickets for help on using tickets.