No error handling for filesize() in Drupal\Core\Image\Image

kim.pepper → made their first commit to this issue’s fork.

We should suppress the warning, and check if @filesize() returns FALSE. Also \Drupal\Core\Image\ImageInterface::getFileSize() expects this to be NULL not FALSE if the file is invalid, so we should handle that too.

Issue summary updated. I can't really provide steps to reproduce as it requires a custom architecture to host files on s3.

Possible to add test coverage for when it is NULL?

I've added a couple of tests and attempted to simulate an S3 filesystem where filesize() returns FALSE, even though the toolkit returns TRUE in parseFile()

Good test coverage. I ran the test-only pipeline and it fails as expected. The current docblock already says it should return int|null so 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.

Thanks 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.

We need to add a comment about why we're suppressing errors here.

Done @alexpott! :)

Feedback resolved. Back to RTBC

Looking 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.

filesize() can return zero for empty files; in this case, zero should be returned, not NULL

I agree with you @mfb, and I took that into account, but it's an imposible case, as the toolkit will never validate an image as valid if the file is zero size.

Sorry, 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:

/**
   * Returns the size of the image file.
   *
   * @return int|null
   *   The size of the file in bytes, or NULL if the image is invalid.
   */
  public function getFileSize();

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! :)

Ok, 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.)

I'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 a s3:// stream.

Perhaps this is not longer an issue? I don't have a real S3 bucket, maybe someone could do a real test.

Believe feedback has been addressed for this one.

It 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.