Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
In Drupal\Core\Image\Image::construct(), filesize() is being used to determine the size of file, but if for example files are on s3, filesize() returns FALSE and there is a warning that can't be suppressed.
Also, \Drupal\Core\Image\ImageInterface::getFileSize() expects this to be NULL not FALSE if the file is invalid, so we should handle that too.
Proposed resolution
We should suppress the warning, and check if @filesize() returns FALSE.
Remaining tasks
- Review MR
- Merge
User interface changes
N/A
API changes
N/A
Data model changes
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #31 | 2831320-filesize-over-s3-file.png | 117.56 KB | vidorado |
Issue fork drupal-2831320
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #15
kim.pepperWe should suppress the warning, and check if
@filesize()returnsFALSE. Also\Drupal\Core\Image\ImageInterface::getFileSize()expects this to beNULLnotFALSEif the file is invalid, so we should handle that too.Comment #16
kim.pepperComment #17
pameeela commentedIssue summary updated. I can't really provide steps to reproduce as it requires a custom architecture to host files on s3.
Comment #18
smustgrave commentedPossible to add test coverage for when it is NULL?
Comment #20
vidorado commentedI've added a couple of tests and attempted to simulate an S3 filesystem where
filesize()returnsFALSE, even though the toolkit returnsTRUEinparseFile()Comment #21
kim.pepperGood test coverage. I ran the test-only pipeline and it fails as expected. The current docblock already says it should return
int|nullso there is no API breaks afaik, and we don't need a change record for a bug fix. I think we are safe to RTBC.Comment #22
vidorado commentedThanks for the review @kim.pepper!
Mind the "null" in the @return of the docblock was added in this issue, it wasn´t already there: https://git.drupalcode.org/project/drupal/-/merge_requests/7217/diffs?co...
I believe that in the cases where a false was taken into account, it would have been with a simple truthy evaluation if($value). I think it's rare that anyone could have used if(FALSE === $value), so the NULL is probably not harming.
Comment #23
alexpottWe need to add a comment about why we're suppressing errors here.
Comment #24
vidorado commentedDone @alexpott! :)
Comment #25
kim.pepperFeedback resolved. Back to RTBC
Comment #26
mondrakeLooking at the motivation, I am not sure about this. The file must have a size, but we cannot get it so we hide the problem. BTW possibly the image cannot be loaded by GD either. IMHO the right solution here would be to download the file from S3 to local storage, get its stats and use it for manipulation. IIRC the ImageMagick toolkit does something similar since it does not support stream wrappers.
Comment #27
mfbfilesize() can return zero for empty files; in this case, zero should be returned, not NULL
Comment #28
vidorado commentedI agree with you @mfb. I took that into account, but that's an imposible case, since the toolkit will never validate an image as valid if the file is zero size.
Comment #29
vidorado commentedSorry, I didn't see @mondrake's comment. I agree that returning NULL is hiding the problem in the case of a VALID image. However, I stand by saying that zero should never be returned by
getFileSize(), since its docblock states this:A zero size image would never be valid, so a NULL must be returned. But in case of a valid image that we can't retrieve easily with
filesize()... returning NULL is wrong, that seems clear.I will take a look at the ImageMagick toolkit to get ideas about how to download and parse the file from S3.
Thanks all for the fedback! :)
Comment #30
mfbOk, sounds reasonable to not worry about the zero size case then (would only happen in edge cases like buggy toolkit, file getting truncated by another process etc.)
Comment #31
vidorado commentedI've installed an AWS S3 mock with Localstack and amazon-ec2-metadata-mock.
Then, I've been able to check that
filesize()works well with as3://stream.Perhaps this is not longer an issue? I don't have a real S3 bucket, maybe someone could do a real test.
Comment #32
vidorado commentedComment #33
smustgrave commentedBelieve feedback has been addressed for this one.
Comment #34
catchIt looks like #31 needs to be answered here and the issue summary updated based on the results.
If the file is actually invalid, then I don't think we need to suppress the error in that case - it's correctly informing people about the invalid file. We could convert the FALSE to NULL though but that's not the only thing the MR does.