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.

Sign up for GitHub

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

Jump to bottom

do not fail if size metadata for an image fail #38011

Closed
wants to merge 1 commit into from
Closed

do not fail if size metadata for an image fail #38011

wants to merge 1 commit into from

Conversation

mgallien
Copy link
Contributor

@mgallien mgallien commented May 1, 2023 •
edited
Loading

some images may trigger an exception when trying to get their size

Signed-off-by: Matthieu Gallien [email protected]

fix for a blocking exception when running occ files:scan --generate-metadata for some images

Checklist

@mgallien mgallien requested a review from a team May 1, 2023 18:49
@mgallien
do not fail if size metadata for an image fail
9e5521f 
some images may trigger an exception when trying to get their size

Signed-off-by: Matthieu Gallien <[email protected]>
@mgallien mgallien force-pushed the doNotFailWhenGetImageSizeMetadata branch from 8dd1da6 to 9e5521f Compare May 1, 2023 18:50
@szaimen szaimen added the 3. to review Waiting for reviews label May 1, 2023
@szaimen szaimen added this to the Nextcloud 27 milestone May 1, 2023
@Pytal Pytal requested review from a team, ArtificialOwl, icewind1991 and come-nc and removed request for a team May 1, 2023 22:45
@szaimen
Copy link
Contributor

szaimen commented May 2, 2023

Isnt #37944 the better approach?

kesselb
kesselb reviewed May 2, 2023
View reviewed changes
lib/private/Metadata/Provider/ExifProvider.php
$sizeResult = getimagesizefromstring($file->getContent());
try {
$sizeResult = getimagesizefromstring($file->getContent());
} catch (\Exception $ex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} catch (\Exception $ex) {
} catch (\TypeError $ex) {

As per php/php-src@b9171d8

@kesselb
Copy link
Contributor

kesselb commented May 2, 2023

Isnt #37944 the better approach?

Both make sense. The execute method has a proper error handling already. PHP 8 behaves different and throws an exception, but returned false for earlier versions.

come-nc
come-nc requested changes May 2, 2023
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

We should not catch TypeError like this

@come-nc
Copy link
Contributor

come-nc commented May 2, 2023

@mgallien What is the trace of the error you are trying to fix?

@skjnldsv skjnldsv mentioned this pull request May 3, 2023
Merged
@mgallien
Copy link
Contributor Author

mgallien commented May 4, 2023

@come-nc see

Exception during scan: getimagesizefromstring(): Error reading from !
#0 [internal function]: OCA\Files\Command\Scan->exceptionErrorHandler()
#1 /var/www/nextcloud/lib/private/Metadata/Provider/ExifProvider.php(71): getimagesizefromstring()
#2 /var/www/nextcloud/lib/private/Metadata/MetadataManager.php(68): OC\Metadata\Provider\ExifProvider->execute()
#3 /var/www/nextcloud/apps/files/lib/Command/Scan.php(138): OC\Metadata\MetadataManager->generateMetadata()
#4 [internal function]: OCA\Files\Command\Scan->OCA\Files\Command\{closure}()
#5 /var/www/nextcloud/lib/private/Hooks/EmitterTrait.php(105): call_user_func_array()
#6 /var/www/nextcloud/lib/private/Hooks/PublicEmitter.php(40): OC\Hooks\BasicEmitter->emit()
#7 /var/www/nextcloud/lib/private/Files/Utils/Scanner.php(131): OC\Hooks\PublicEmitter->emit()
#8 [internal function]: OC\Files\Utils\Scanner->OC\Files\Utils\{closure}()
#9 /var/www/nextcloud/lib/private/Hooks/EmitterTrait.php(105): call_user_func_array()
#10 /var/www/nextcloud/lib/private/Files/Cache/Scanner.php(168): OC\Hooks\BasicEmitter->emit()
#11 /var/www/nextcloud/lib/private/Files/Cache/Scanner.php(436): OC\Files\Cache\Scanner->scanFile()
#12 /var/www/nextcloud/lib/private/Files/Cache/Scanner.php(382): OC\Files\Cache\Scanner->handleChildren()
#13 /var/www/nextcloud/lib/private/Files/Cache/Scanner.php(385): OC\Files\Cache\Scanner->scanChildren()
#14 /var/www/nextcloud/lib/private/Files/Cache/Scanner.php(385): OC\Files\Cache\Scanner->scanChildren()
#15 /var/www/nextcloud/lib/private/Files/Cache/Scanner.php(385): OC\Files\Cache\Scanner->scanChildren()
#16 /var/www/nextcloud/lib/private/Files/Cache/Scanner.php(333): OC\Files\Cache\Scanner->scanChildren()
#17 /var/www/nextcloud/lib/private/Files/Utils/Scanner.php(256): OC\Files\Cache\Scanner->scan()
#18 /var/www/nextcloud/apps/files/lib/Command/Scan.php(161): OC\Files\Utils\Scanner->scan()
#19 /var/www/nextcloud/apps/files/lib/Command/Scan.php(217): OCA\Files\Command\Scan->scanFiles()
#20 /var/www/nextcloud/apps/recognize/vendor/symfony/console/Command/Command.php(298): OCA\Files\Command\Scan->execute()
#21 /var/www/nextcloud/core/Command/Base.php(177): Symfony\Component\Console\Command\Command->run()
#22 /var/www/nextcloud/apps/recognize/vendor/symfony/console/Application.php(1040): OC\Core\Command\Base->run()
#23 /var/www/nextcloud/apps/recognize/vendor/symfony/console/Application.php(301): Symfony\Component\Console\Application->doRunCommand()
#24 /var/www/nextcloud/apps/recognize/vendor/symfony/console/Application.php(171): Symfony\Component\Console\Application->doRun()
#25 /var/www/nextcloud/lib/private/Console/Application.php(215): Symfony\Component\Console\Application->run()
#26 /var/www/nextcloud/console.php(100): OC\Console\Application->run()
#27 /var/www/nextcloud/occ(11): require_once('...')
#28 {main}

@come-nc
Copy link
Contributor

come-nc commented May 4, 2023

@mgallien This is fixed by #37944

@mgallien
Copy link
Contributor Author

mgallien commented May 4, 2023

@come-nc so let's close #38011

@mgallien mgallien closed this May 4, 2023
@mgallien mgallien deleted the doNotFailWhenGetImageSizeMetadata branch May 5, 2023 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc requested changes

@Pytal Pytal Pytal approved these changes

@solracsf solracsf solracsf approved these changes

@JuliaKirschenheuter JuliaKirschenheuter JuliaKirschenheuter approved these changes

@kesselb kesselb kesselb approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 was automatically assigned from nextcloud/server-backend

Assignees
No one assigned
Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 27
Development

Successfully merging this pull request may close these issues.
7 participants
@mgallien @szaimen @kesselb @come-nc @JuliaKirschenheuter @solracsf @Pytal