Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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'
Comment | File | Size | Author |
---|---|---|---|
#13 | interdiff-2916595-11-13.txt | 563 bytes | phenaproxima |
#13 | 2916595-13.patch | 2.68 KB | phenaproxima |
#11 | interdiff-2916595-7-11.txt | 1.13 KB | phenaproxima |
#11 | 2916595-11.patch | 2.68 KB | phenaproxima |
#7 | 2916595-7.patch | 2.56 KB | phenaproxima |
Comments
Comment #2
AdamPS CreditAttribution: AdamPS commentedComment #6
phenaproximaThis blocks some refactoring work on the Media Library module, so tagging this issue appropriately.
Comment #7
phenaproximaHere 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.
Comment #9
Wim LeersWill review.
Comment #10
Wim LeersThis 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:
Nit: needs
@inheritdoc
Nit: needs one-line comment
Nit: I suggest s/test/cagatio/
Comment #11
phenaproximaAll fixed. Thanks!
Comment #12
Wim Leers👍
Comment #13
phenaproxima#11 will fail. I should probably run tests locally before I post patches.
Comment #15
alexpottCommitted 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.
Comment #19
idebr CreditAttribution: idebr at ezCompany commentedClosed #2948714: Multiple image upload widget has no accept attribute as a duplicate issue.
Comment #20
Wim Leers