Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#22868 closed defect (bug) (fixed)

broken compat attachment attributes save

Reported by: geminorum's profile geminorum Owned by: nacin's profile 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 12 years ago.
sample plugin
22868.diff (563 bytes) - added by nacin 12 years ago.

Download all attachments as: .zip

Change History (17)

#1 @dd32
12 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
12 years ago 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 12 years ago by geminorum (previous) (diff)

#3 @geminorum
12 years ago

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

12 years ago

sample plugin

#4 @geminorum
12 years ago

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

#5 @crocro
12 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[] = '';

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
12 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
12 years ago

  • Milestone Awaiting Review deleted

#8 @crocro
12 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
12 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
12 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
12 years ago

  • Cc eddie.moya+wptrac@… added

#12 @nacin
12 years ago

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

12 years ago

#13 @nacin
12 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
12 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
12 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.