WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 15 months ago

#28053 reviewing defect (bug)

Fields with same name added to new media modal using 'attachment_fields_to_edit' do not pass as an array when saving.

Reported by: husobj Owned by: joemcgill
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: has-patch
Focuses: javascript, administration Cc:
PR Number:

Description

If you use the 'attachment_fields_to_edit' to add a set of checkboxes to the media modal attachment details (to implement a better UI for taxonomies for example) the save method of media.view.AttachmentCompat doesn't cater for fields with the same name to be passed as an array, it just passes the last field of that name that is found.

When getting the fields to save, if multiple fields with the same name are found they should be passed as an array.

I have attached a visual of the custom UI with checkboxes and a patch for testing.

Attachments (7)

media-views.js.patch (617 bytes) - added by husobj 5 years ago.
attachment-details-checkboxes.png (53.4 KB) - added by husobj 5 years ago.
Visual of media modal attachment details with checkboxes UI
media-views.js.v2.patch (737 bytes) - added by husobj 5 years ago.
media-modal.28053.diff (4.1 KB) - added by LindsayBSC 4 years ago.
media-views.js and attachment-compat.js patch for multiple value input as javascript array object patch
media-modal.28053.2.diff (3.2 KB) - added by LindsayBSC 4 years ago.
[fixed tabs] media-views.js and attachment-compat.js patch for multiple value input as javascript array object patch
media.modal.28053.diff (3.0 KB) - added by LindsayBSC 4 years ago.
[formatting fix] media-views.js and attachment-compat.js patch for multiple value input as javascript array object patch
media-models.patch (2.3 KB) - added by drosendo 3 years ago.
Allow multiple values in Media Attachment fields

Download all attachments as: .zip

Change History (31)

@husobj
5 years ago

Visual of media modal attachment details with checkboxes UI

#1 @husobj
5 years ago

Possibly related to ticket #22940

#2 @husobj
5 years ago

Updated patch checks for fields intended to be arrays be checking their name ends in "[]" e.g. "myfield[]"

These are then concatenated into a comma separated value with field name "myfield" (without the square brackets) which is the same format in which taxonomy terms are handles when currently associating taxonomies with attachments.

Works for the development I'm doing building a better taxonomy UI for the media modal but obviously wouldn't handle field values that contain commas (which should not be an issue for terms).

#3 @ericlewis
5 years ago

  • Keywords reporter-feedback added
  • Version changed from 3.9 to 3.5

Can you drop in the boilerplate for creating the nested checkboxes you have there?

#4 @husobj
5 years ago

Download this plugin which sets up attachment tag and category taxonomies.
https://github.com/benhuson/attachment-taxonomy-support/tree/dev

In Admin > Media set up some tags and categories.
If you edit a media item (from the list view, not the modal popup in 4.0) you'll get the default taxonomy UI meta boxes which work fine.

If you get into the media modal either via the WP 4.0 Grid view or clicking on "Add Media" when editing a post, you'll see the modal now shows a field for tags and category checkboxes.

The tags field is named in the format "attachments[{attachment_id}][attachment_tag]" and this autosaves fine when the field is updated.

With the checkboxes, I was expecting to be able to name them all in the format "attachments[{attachment_id}][attachment_category][]" and the "[]" on the end would signify the values get submitted as an array and saved (like the checkboxes in the standard categories meta box).

That currently isn't working and is the use case I am trying to fix.

#5 @ocean90
4 years ago

#33331 was marked as a duplicate.

This ticket was mentioned in Slack in #core by lindsaymac. View the logs.


4 years ago

#7 @LindsayBSC
4 years ago

This is my boilerplate code used that will replicate the problem with multiple field values being saved "one at a time" versus all in one shot.

https://gist.github.com/LinzardMac/dee0488120d436e5b43b

Last edited 3 years ago by LindsayBSC (previous) (diff)

@LindsayBSC
4 years ago

media-views.js and attachment-compat.js patch for multiple value input as javascript array object patch

@LindsayBSC
4 years ago

[fixed tabs] media-views.js and attachment-compat.js patch for multiple value input as javascript array object patch

@LindsayBSC
4 years ago

[formatting fix] media-views.js and attachment-compat.js patch for multiple value input as javascript array object patch

#8 @Asgaros
4 years ago

#33918 was marked as a duplicate.

#9 follow-up: @ocean90
3 years ago

#36825 was marked as a duplicate.

@drosendo
3 years ago

Allow multiple values in Media Attachment fields

#10 @drosendo
3 years ago

  • Keywords has-patch added

Hi,

So here goes my core patch done on WordCamp Porto to help fix this problem.

Attachment https://core.trac.wordpress.org/attachment/ticket/28053/media-models.patch​ added

Please do test it, feel free to contribute if needed.

All the best,
David

Last edited 3 years ago by drosendo (previous) (diff)

#11 in reply to: ↑ 9 @drosendo
3 years ago

Replying to ocean90:

#36825 was marked as a duplicate.

I think I did the patch ok, will wait for confirmation. Hope it helps.

#12 follow-up: @drosendo
3 years ago

Hi,

Any feedback on this? is my patch viable?

Thanks,
David

#13 in reply to: ↑ 12 @LindsayBSC
3 years ago

Replying to drosendo:

Hi,

Any feedback on this? is my patch viable?

Thanks,
David

Can you explain how your patch is more efficiently coded than the one already provided in this ticket? The previously submitted one looks for the default scenario for checkbox naming where if you are provided a list of checkboxes as options for a single "question", you use brackets [] in your naming convention. Because this is default behavior, we just look for that to know if we are looking at a multiple checkbox select option or not.

Last edited 2 years ago by LindsayBSC (previous) (diff)

#14 follow-up: @drosendo
3 years ago

Hi @LindsayBSC ,

I don't fully understand your question, but how is my better? I'm really not discussing witch one is better, I believe both are viable solutions, but since none have been approved over the past 2 years since reported. I just rather use my code, I believe it to be more "readable/clean" and I do think that it covers all scenarios. Correct me if I'm wrong.

Thanks,
David

#15 in reply to: ↑ 14 ; follow-up: @LindsayBSC
3 years ago

Replying to drosendo:

Hi @LindsayBSC ,

I don't fully understand your question, but how is my better? I'm really not discussing witch one is better, I believe both are viable solutions, but since none have been approved over the past 2 years since reported. I just rather use my code, I believe it to be more "readable/clean" and I do think that it covers all scenarios. Correct me if I'm wrong.

Thanks,
David

I was asking why you felt the patch needed rewriting - which, even though you said you didn't want to discuss why, you actually did explain.

So you feel your patch is more readable than the provided previous patch? I am asking for my own reference to understand if/when/how my code could be improved.

#16 in reply to: ↑ 15 ; follow-up: @drosendo
3 years ago

Replying to LindsayBSC:

I was asking why you felt the patch needed rewriting - which, even though you said you didn't want to discuss why, you actually did explain.

So you feel your patch is more readable than the provided previous patch? I am asking for my own reference to understand if/when/how my code could be improved.

Hi,

Actually I now recall why I did my patch, I believe your patch doesn't include multiple select fields, and only handles checkboxes.
Am I incorrect?

Thanks,
David

#17 in reply to: ↑ 16 @LindsayBSC
3 years ago

It works for any data sent as an array so it would completely depends on how you collect your form values. If you are using select boxes then are you using a selectbox javascript plugin for this?

Replying to drosendo:

Replying to LindsayBSC:

I was asking why you felt the patch needed rewriting - which, even though you said you didn't want to discuss why, you actually did explain.

So you feel your patch is more readable than the provided previous patch? I am asking for my own reference to understand if/when/how my code could be improved.

Hi,

Actually I now recall why I did my patch, I believe your patch doesn't include multiple select fields, and only handles checkboxes.
Am I incorrect?

Thanks,
David

This ticket was mentioned in Slack in #core by user-267. View the logs.


3 years ago

#19 @joemcgill
3 years ago

  • Keywords reporter-feedback removed
  • Owner set to joemcgill
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #core-media by user-267. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-media by user-267. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-media by user-267. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-media by user-267. View the logs.


2 years ago

#24 @Uriahs Victor
15 months ago

Any progress on this?

Note: See TracTickets for help on using tickets.