Adjust stream chunk size to load big exif metadata #40600
Conversation
jvillafanez
force-pushed
the
load_big_exif_metadata
branch
from
441f1f5 to
c7bea82
Compare
| $stream = $handle; | ||
| $metadata = \stream_get_meta_data($stream); | ||
| while ($metadata['stream_type'] === 'user-space') { | ||
| \stream_set_chunk_size($stream, 64 * 1024); |
There was a problem hiding this comment.
ok - not really - it only fails if the chunk size is negative or bigger then php max int ....https://www.php.net/manual/en/function.stream-set-chunk-size.php
|
I don't think unit tests are possible in the strict sense. We need a custom stream wrapper to trigger the behavior, so we either create a custom wrapper just for the tests or use ownCloud's node or view object to get the stream. Even if we include integration tests as unit tests, it will take some time (estimate ~2 days) and it's unlikely to cover all the cases (need to find TIFF images that trigger this behavior). |
I think it would be good to cover it with some integration/unit/acceptance test, or hybrid of it. Same story as with https://github.com/owncloud/files_classifier/tree/master/tests/acceptance/data and https://github.com/owncloud/files_classifier/blob/master/tests/unit/DocumentPropertiesTest.php#L37-L67 |
|
@jvillafanez @mrow4a do we want to have tests for this one? |
|
If it's required, I'd vote for open a tech debt ticket to include the tests because it will take time. Note that, as said, we'd need to create a custom wrapper to trigger the behavior. |
jvillafanez
force-pushed
the
load_big_exif_metadata
branch
from
e6b4179 to
5fc0e62
Compare
|
I've added a test that should cover the main use case, but there are things still not covered:
|
jvillafanez
force-pushed
the
load_big_exif_metadata
branch
from
5fc0e62 to
a945880
Compare
|
@DeepDiver1975 is this good enough? |
tests/lib/ImageTest.php
Outdated
|
|
||
| $img->loadFromFileHandle($stream); | ||
| \fclose($stream); | ||
| $this->assertSame(6, $img->getOrientation()); // orientation = right, top |
There was a problem hiding this comment.
all assert functions of phpunit are static if I am not totally miss taken ....
Should be self::assertSame ...
There was a problem hiding this comment.
$this->assertSomething() is done throughout the existing code.
But yes, the asserts are like:
public static function assertSame($expected, $actual, string $message = ''): void
I wonder why we have done $this-> rather than self:: for all these years?
There was a problem hiding this comment.
Maybe because earlier versions of phpunit have been using this-> ... no idea ..
I tend to fix such things when touching any code base ..... my idea is even throwing this as warnings into my face 🤷
|
💥 Acceptance tests pipeline apiWebdavDelete-mariadb10.2-php7.4 failed. The build has been cancelled. |
|
💥 Acceptance tests pipeline apiProvisioningGroups-v1-mariadb10.2-php7.4 failed. The build has been cancelled. |
|
I restarted CI - some pipelines failed when initially fetching docker images. |
|
Kudos, SonarCloud Quality Gate passed! |
Description
There were problems loading the exif metadata of some images if the metadata was too large. This was causing problems loading the images because the right orientation was in the metadata that wasn't loaded.
Related Issue
https://github.com/owncloud/enterprise/issues/5500
Motivation and Context
Users expect the image to be loaded with the right orientation, like most of the apps.
How Has This Been Tested?
Just upload the image below:
Tested with and without encryption. I'm not aware of any additional stream wrapper that could affect the behavior (quota isn't applied to read streams)
Screenshots (if appropriate):
Types of changes
Checklist:
Supersedes #40586