WordPress.org

Make WordPress Core

Opened 17 months ago

Closed 16 months ago

Last modified 16 months 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:

Description

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 16 months ago.
sample plugin
22868.diff (563 bytes) - added by nacin 16 months ago.

Download all attachments as: .zip

Change History (17)

comment:1 dd3217 months 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?

comment:2 geminorum17 months ago

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

wp_ajax_save_attachment_compat() expects attachments, update-post_(post_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.

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

comment:3 geminorum17 months ago

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

geminorum16 months ago

sample plugin

comment:4 geminorum16 months ago

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

comment:5 crocro16 months 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.

Regards

comment:6 geminorum16 months 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.

comment:7 helen16 months ago

  • Milestone Awaiting Review deleted

comment:8 crocro16 months 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 ?

comment:9 nacin16 months 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.

comment:10 geminorum16 months 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.

comment:11 eddiemoya16 months ago

  • Cc eddie.moya+wptrac@… added

comment:12 nacin16 months ago

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

nacin16 months ago

comment:13 nacin16 months 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.

comment:14 nacin16 months 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.

comment:15 nacin16 months 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.