Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#22868 closed defect (bug) (fixed)

broken compat attachment attributes save

Reported by: geminorum Owned by: nacin
Milestone: 3.5.1 Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch
Focuses: Cc:


cannot save data via attachment_fields_to_save filter.
probably because wp_ajax_save_attachment_compat() expects $_REQUESTattachments? which not provided by save-attachment-compat js sender.
there are only post_id, id (attachment_id), and the changes.

Attachments (2)

media-bug.php (1.9 KB) - added by geminorum 8 years ago.
sample plugin
22868.diff (563 bytes) - added by nacin 8 years ago.

Download all attachments as: .zip

Change History (17)

#1 @dd32
8 years ago

To make it easier for others to test and debug, can you please post some code which can be used to trigger it, or a plugin name (and version) which can be used to duplicate it?

#2 @geminorum
8 years ago

wp.media.model.Attachment.saveCompat sends id, post_id, nonce and the new changes.

wp_ajax_save_attachment_compat() expects attachments or attachmets id.

so cannot use attachment_fields_to_save to save extra data.

it triggers by my theme framework, not published yet.

unfamiliar with new js workflow, and sorry, don't know how to build a patch.

Last edited 8 years ago by geminorum (previous) (diff)

#3 @geminorum
8 years ago

  • Summary changed from broken backward attachment attributes save to broken compat attachment attributes save

8 years ago

sample plugin

#4 @geminorum
8 years ago

the attached works on WP 3.4.2, but not on WP 3.5

#5 @crocro
8 years ago

I'm experiencing this problem as well : this occurs because of the checkboxes and jQuery's serializeArray function.

Further explanation : when the checkbox goes unchecked, the serializeArray function (at line 4139 of media-views.js) won't take it into account (the function is designed to work like a browser : when the checkbox is unchecked, no info is sent via HTTP POST requests).

As a consequence, when wordpress receives the AJAX request, the post parameters will look like that :

action	save-attachment-compat
id	34872
nonce	a1b5113007
post_id	34865

Whereas it's expecting something like this to work (just like when a classic text input is empty) :

action	save-attachment-compat
id	34872
custom_checkbox_field	(empty)
nonce	a1b5113007
post_id	34865

Therefore, it won't even bother calling "attachment_fields_to_save" filter and returns an error right away because it can't guess which field has been edited.

The best (and cleanest) patch I could imagine of would be changing serializeArray to a function that takes into account empty checkboxes as well.
Sending the "custom_checkbox_field (empty)" additional post parameter will be enough to make it work correctly.

I can work on a patch but I'm not very familiar with Wordpress core code.
However, editing the media-views.js file by adding :

jQuery('input[type=checkbox]:not(:checked)', this.$el).map(function() {
    data[this.name] = '';

at line 4201 would be enough to make it work correctly.

Notice for whom may use this patch : when you add a custom field thanks to "attachment_fields_to_edit" filter, and if you use 'input' => 'html' option for your custom field, you've to name it "attachments[$attachment->ID][name_of_your_custom_field]" (where $attachment->ID is the attachment ID and name_of_your_custom_field is a field name) in order for Wordpress to recognize it through the AJAX post request.


#6 @geminorum
8 years ago

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

I think we have no problem with empty checkboxes. if it's not on the POST then it's unchecked.

But the "attachments[$attachment->ID][name_of_your_custom_field]" part works. the problem was me not following the core.

thanks crocro, and sorry to bother.

#7 @helen
8 years ago

  • Milestone Awaiting Review deleted

#8 @crocro
8 years ago

Yep, but if it's not on the POST, Wordpress will never apply attachment_fields_to_save filter.

It means, when you try to uncheck the checkbox via the new AJAX media interface, there isn't any chance you can save the result to the database or such things. "wp-admin/admin-ajax.php" will answer "{"success":false}" and that's it - attachment_fields_to_save won't be called.

I don't know if you can test this ?

#9 @nacin
8 years ago

  • Milestone set to 3.5.1
  • Resolution invalid deleted
  • Status changed from closed to reopened

I think in order to account for unchecked checkboxes, the filter should always be applied.

#10 @geminorum
8 years ago

I've got it working by including a hidden attachments[$attachment->ID] field.

Also, I think it's better to delay the ajax call until the last one responses. It gets weird when you click multiple checkboxes on the same time.

#11 @eddiemoya
8 years ago

  • Cc eddie.moya+wptrac@… added

#12 @nacin
8 years ago

  • Owner set to nacin
  • Status changed from reopened to accepted

8 years ago

#13 @nacin
8 years ago

  • Keywords has-patch added

The problem with attachments[$attachment_id] is it must come at the front, and then we must also turn a zero-length string into an empty array in the ajax handler (or in the JS).

22868.diff just adds a hidden menu_order field, which was always sent in 3.4 anyway.

#14 @nacin
8 years ago

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

In 23290:

Whenever we have compat fields to render, send a dummy menu_order field (which was always sent in 3.4) to ensure an unchecked checkbox can still be processed by attachment_fields_to_save. fixes #22868.

#15 @nacin
8 years ago

In 23291:

Whenever we have compat fields to render, send a dummy menu_order field (which was always sent in 3.4) to ensure an unchecked checkbox can still be processed by attachment_fields_to_save.

Merges [23290] to the 3.5 branch.
fixes #22868.

Note: See TracTickets for help on using tickets.