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.Problem/Motivation
Whilst working on #1927648: Allow creation of file entities from binary data via REST requests, I realised that the code in \Drupal\file\Entity\File::preSave does not allow a file with zero bytes/length to be set. This does against how numeric fields check their emptiness. The offending code:
if ($size = @filesize($uri)) {
$this->setSize($size);
}
So if filesize is legitimately 0, it will set no value at all.
Proposed resolution
Check numeric emptiness properly and set for all cases, unless the filesize call fails.
Remaining tasks
Reviews
User interface changes
N/A
API changes
N/A
Data model changes
Some additional field values of 0 being saved if an empty file is saved.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 2901998-5.patch | 1.45 KB | Anonymous (not verified) |
| #5 | interdiff.txt | 468 bytes | kim.pepper |
| #5 | 2901998-5.patch | 1.45 KB | kim.pepper |
| #2 | 2901998-PASS.patch | 1.41 KB | damiankloip |
| #2 | 2901998-tests-only-FAIL.patch | 905 bytes | damiankloip |
Comments
Comment #2
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #3
damiankloip CreditAttribution: damiankloip at Acquia commentedCould do with a comment above the
!== FALSEcheck I think.Comment #5
kim.pepperAdded a comment.
Comment #6
andypostLooks good to go
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commented+1. File with zero size is legal and can be useful. Examples:
Maybe add CR for warn developers, who used this bug as a feature for filtering files? It is also possible to add the checkbox "Prevent upload empty files" in the file system settings (/admin/config/media/file-system)?
Comment #8
Wim Leers+1, and +1 for the great comment by @vaplas.
Comment #10
Wim LeersRandom fail in the JS test
SettingsTrayBlockFormTest. Retesting.Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commented#8: thank you, @Wim Leers!
This patch contains second part of #7 about checkbox "Prevent upload empty files".
Also bit change test. Now it is separated, and it checks three configuration cases (default/allow/prevent save empty file).
Feel free to ignore, if it is not what we should do in this issue. Back to review due to this patch.
Comment #12
Wim LeersAssigning to Berdir to get his thoughts on whether it's necessary to have the abc layer that #11 is adding.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks!
A small correction to not keep the red issue.
Comment #15
damiankloip CreditAttribution: damiankloip at Acquia commentedI agree that is a good idea worth dicussing but I'm sorry, this seems to be opening up the scope of this fix a little too much IMO. We're now checking some global config in the preSave of the file entity. I'm not convinced there are really any real world examples of people relying on there being no field value for empty files still... Maybe berdir has some feedback on that.
This doesn't seem accurate, all that's happening here is we're saving the file size or not. This implies the file will not be saved at all. So for me, that means the 'prevent_save_empty_file' config value is also not named accurately.
Great examples BTW :)
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedThank you, @damiankloip! I'm really wrong. Back to #10 state with #5 patch. Sorry for noise.
Comment #20
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!