The fix for #2492241 inverted the logic that determines whether or not the "display" value should be set from the relevant field setting - making the "Display" checkbox useless.

CommentFileSizeAuthor
#2 incorrect-logic-display-value-2545738-1.patch603 bytesagileware

Comments

Agileware created an issue. See original summary.

agileware’s picture

agileware commented
StatusFileSize
new incorrect-logic-display-value-2545738-1.patch603 bytes
agileware’s picture

agileware commented
Status: Active » Needs review
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Postponed (maintainer needs more info)

Wasn't the original logic actually incorrect? I fail to see how this actually makes things work as expected.

BarisW’s picture

BarisW commented
Status: Postponed (maintainer needs more info) » Reviewed & tested by the community

I can confirm the issue and the fix.

Steps to reproduce:

  1. Add a multiple file field of type Media
  2. Enable the Display field
  3. Set the Display by default unchecked
  4. Create/edit a node, upload some files.
  5. Uncheck the 'Display' checkbox for one or a few files
  6. Save the node. All files are displayed, even when the checkbox was unchecked
  7. Edit the node
  8. See that all checkboxes are checked again

The patch fixes the issue.

circuscowboy’s picture

circuscowboy commented

I too can confirm that this patch fixes the issue for me too.

I have not reviewed why it works but can confirm that it does work.

agileware’s picture

agileware commented

Apologies for the long response time, Dave - I hope this clears things up for you:

#2492241 made things such that if this display option was unchecked (NULL), it would check if the setting to display the field checkbox was on (ie. allow the node author to decide if the field item should be rendered), it would force the value to 1, meaning that only the field setting checkbox would have any effect.
(Before: setting to allow optional item display is on? Always display the field: otherwise behaviour dependent on [possibly undefined] default value)

This patch fixes it so that if the field setting is off it forces the display option to on, ie. only force the value if the setting to show the display checkbox is off and the items should always be rendered.
(After: setting to allow optional item display is on? Display the field only if the display option is checked: otherwise always display every item)

jurriaanroelofs’s picture

jurriaanroelofs commented

thanks! +1
this patch fixes the display field in multi file fields with display option.
Conveniently, my site also uses a media field in a field collection, so I could check the referenced issue https://www.drupal.org/node/2492241
The patch in this issue did not bring back any notices.

ultrabob’s picture

ultrabob commented

Here's another vote for #2. Before the patch, I had two files on a node, one that had display set on, and the other off. I turned both on to check something, and then could not turn the second one back off. This patch fixed the issue.

brockfanning’s picture

brockfanning commented

#2 works for me as well, thank you!

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

  • Dave Reid committed 3584855 on 7.x-2.x authored by Agileware 
    Issue #2545738 by Agileware, BarisW: Fixed media_field_widget_value()...
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Reviewed & tested by the community » Fixed

Ok I had a chance to verify this logic and the reasoning in #5 is correct. Thanks all.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

ultrabob’s picture

ultrabob commented

I know it is a newbie question, but I can't seem to find a good answer anywhere. Is there a good way to know when this makes it into the non-dev branch?

ultrabob’s picture

ultrabob commented

Just in case google leads anyone else here, it seems like the best way is to watch the commits that go into each release for the issue number on the patch. This patch was committed in 7.x-2.0-beta2