Example consequences

The easiest way to see the problem is using Image Field.

  • Create a content type
  • Add image field "Image Single" with 'Allowed number of values' = 1.
  • Add image field "Image Multiple" with 'Allowed number of values' = 'Unlimited'.
  • Create a new node of this type
  • Click browse on "Image Single": browser correctly shows only images
  • BUG: Click browse on "Image Multiple": browser shows all files

Expected behaviour: "Image Multiple": browser shows only images

The reason for the bug is that the 'accept' attribute is missing.

Other consequences

This bug is not necessarily specific to Image, hence I set component = "file system". The 'data-drupal-selector' attribute is missing too - not sure what effect that has.

Code problem

See \Drupal\Core\Render\Element\File::processFile, where existing value of #attributes is overwritten. However there could be valid and important attributes already present.

Proposed solution

Keep existing attributes and in addition set 'multiple'

CommentFileSizeAuthor
#13 interdiff-2916595-11-13.txt563 bytesphenaproxima
#13 2916595-13.patch2.68 KBphenaproxima
#11 interdiff-2916595-7-11.txt1.13 KBphenaproxima
#11 2916595-11.patch2.68 KBphenaproxima
#7 2916595-7.patch2.56 KBphenaproxima
#7 2916595-7-FAIL.patch1.96 KBphenaproxima
#2 file-attributes.2916595-2.patch605 bytesadamps
#2 Image multiple.PNG1.6 KBadamps
#2 Image single.PNG3.63 KBadamps

Comments

AdamPS created an issue. See original summary.

adamps’s picture

adamps commented
Status: Active » Needs review
StatusFileSize
new Image single.PNG3.63 KB
new Image multiple.PNG1.6 KB
new file-attributes.2916595-2.patch605 bytes

Status: Needs review » Needs work

The last submitted patch, 2: file-attributes.2916595-2.patch, failed testing. View results

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: +Media Initiative, +blocker

This blocks some refactoring work on the Media Library module, so tagging this issue appropriately.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2916595-7-FAIL.patch1.96 KB
new 2916595-7.patch2.56 KB

Here we go. There's no test coverage that exactly hit the problem spot, so I wrote some new stuff for that. If anyone can suggest an existing place that this might fit better, please let me know.

The last submitted patch, 7: 2916595-7-FAIL.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +BarcelonaMediaSprint

Will review.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

This looks great, and I'm surprised this was only discovered recently, since it must have been around for a long time!

It does fix the bug, but doesn't explicitly test the original bug report. I don't think it makes sense to have an explicit regression test for such a specific sequence of steps, so I did manual testing to verify it indeed fixes the originally reported problem! ✅

Just two nits:

  1. +++ b/core/modules/system/tests/src/Kernel/Form/FileElementTest.php
@@ -0,0 +1,26 @@
+  protected static $modules = ['form_test'];

    Nit: needs @inheritdoc 

  2. +++ b/core/modules/system/tests/src/Kernel/Form/FileElementTest.php
@@ -0,0 +1,26 @@
+  public function testFileElement() {

    Nit: needs one-line comment 

  3. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestFileForm.php
@@ -0,0 +1,42 @@
+        'class' => ['test'],

    Nit: I suggest s/test/cagatio/

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2916595-11.patch2.68 KB
new interdiff-2916595-7-11.txt1.13 KB

All fixed. Thanks!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

👍

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new 2916595-13.patch2.68 KB
new interdiff-2916595-11-13.txt563 bytes

#11 will fail. I should probably run tests locally before I post patches.

The last submitted patch, 11: 2916595-11.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed f7f07d81ac to 8.7.x and 5a42530358 to 8.6.x. Thanks!

Backported to 8.6.x since this is a bugfix that has very limited BC impact.

  • alexpott committed f7f07d8 on 8.7.x 
    Issue #2916595 by phenaproxima, AdamPS, Wim Leers: File element discards...

  • alexpott committed 5a42530 on 8.6.x 
    Issue #2916595 by phenaproxima, AdamPS, Wim Leers: File element discards...

Status: Fixed » Closed (fixed)

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

idebr’s picture

idebr commented
Version: 8.7.x-dev » 8.6.x-dev

Closed #2948714: Multiple image upload widget has no accept attribute as a duplicate issue.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2948714: Multiple image upload widget has no accept attribute