Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix the exif_read_data issue #36420

Merged
merged 1 commit into from
Jan 30, 2023
Merged

fix the exif_read_data issue #36420

merged 1 commit into from
Jan 30, 2023

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented Jan 28, 2023

Fix #34958

@szaimen szaimen added the 2. developing Work in progress label Jan 28, 2023
@szaimen szaimen added this to the Nextcloud 26 milestone Jan 28, 2023
@szaimen szaimen force-pushed the enh/34958/fix-exif-log branch 2 times, most recently from d9c5383 to 5b4f201 Compare January 28, 2023 10:13
@szaimen szaimen added bug 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 28, 2023
@szaimen szaimen marked this pull request as ready for review January 28, 2023 10:26
@@ -56,10 +56,10 @@ public function execute(File $file): array {
// But I don't understand why 1 as a special meaning.
// Revert right after reading the exif data.
$oldBufferSize = stream_set_chunk_size($fileDescriptor, 1);
$data = exif_read_data($fileDescriptor, 'ANY_TAG', true);
$data = @exif_read_data($fileDescriptor, 'ANY_TAG', true);
Copy link
Contributor Author

@szaimen szaimen Jan 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was actually the only way how I could suppress the log message when testing with https://user-images.githubusercontent.com/12234510/205661916-3e7b14b4-6d4b-4dc2-9903-5265f95a2aee.png

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@szaimen szaimen changed the title try to fix the exif_read_data issue to fix the exif_read_data issue Jan 28, 2023
@szaimen
Copy link
Contributor Author

szaimen commented Jan 28, 2023

/backport to stable25

@szaimen szaimen changed the title to fix the exif_read_data issue fix the exif_read_data issue Jan 28, 2023
@simonspa
Copy link
Contributor

What I find odd is that this seems to be only triggered by Android client uploads, but not when adding the exact same file via web interface. In one case, the error is thrown, in the other case EXIF data is retrieved just fine - so I guess there is still something else going on.

Nevertheless, suppressing this error seems to make sense to me.

Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you understand why the try/catch is not enough ?
Not a fan of silencing error like that.
Maybe changing } catch (\Exception $ex) { for } catch (\Throwable $ex) { and changing log level to info would be enough ?

@come-nc
Copy link
Contributor

come-nc commented Jan 30, 2023

Do you understand why the try/catch is not enough ? Not a fan of silencing error like that. Maybe changing } catch (\Exception $ex) { for } catch (\Throwable $ex) { and changing log level to info would be enough ?

https://www.php.net/manual/function.exif-read-data.php states:

Errors of level E_WARNING and/or E_NOTICE may be raised for unsupported tags or other potential error conditions, but the function still tries to read all comprehensible information.

You cannot try/catch those errors, that goes straight to log.
The only way to catch them is to override the logger before the call, which is not practical.
From what I understand of the documentation, it does not throw because in some cases it still returns partial information.

@szaimen
Copy link
Contributor Author

szaimen commented Jan 30, 2023

Thanks @come-nc! So good to merge?

@artonge artonge merged commit 3bfc5a4 into master Jan 30, 2023
@artonge artonge deleted the enh/34958/fix-exif-log branch January 30, 2023 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
5 participants