MaxServ / t3ext-fal_s3

S3 driver for FAL
GNU General Public License v2.0
13 stars 10 forks source link

[BUGFIX] Do not try to get image data for files that are not images #36

Closed mkarulin closed 6 years ago

mkarulin commented 6 years ago

Fix a logic error in a check that tests if a file is an Image before trying to get additional image metadatas.

arnoschoon commented 6 years ago

Is this something that occurs if another FAL driver is used (in conjunction with S3)?

$file === null
            || (
                $file !== null
                && $file->getType() !== AbstractFile::FILETYPE_IMAGE
                && $file->getStorage()->getDriverType() !== AmazonS3Driver::DRIVER_KEY
            )

In the current scenario the recordUpdatedOrCreated method will break if the file is not available (=== null) OR the file is available but not an image stored on S3.

$file === null
            || (
                $file !== null
                && $file->getType() !== AbstractFile::FILETYPE_IMAGE
                || $file->getStorage()->getDriverType() !== AmazonS3Driver::DRIVER_KEY
            )

This change still checks if the file is available and break if it's not an image, but the driver check might break if $file === null (maybe a bit far fetched but still a possibility). I would propose that you check if $file !== null before invoking $file->getStorage().

arnoschoon commented 6 years ago

Hi @mkarulin,

just thinking about it again, would it be possible to check for the driver type separately from the file type?

The identification of image dimensions is already taken care of by the FAL core for the LocalDriver, in my opinion this should be handled by each driver on its own. Therefore this slot only makes sense in the context of the S3 driver, using a separate check could improve readability of this piece of code :)

arnoschoon commented 6 years ago

Fixed by @DerFrenk in https://github.com/MaxServ/t3ext-fal_s3/pull/38