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

Bug: ImageHandler has no reality checks #2421

Closed
MGatner opened this issue Nov 20, 2019 · 7 comments · Fixed by #2573
Closed

Bug: ImageHandler has no reality checks #2421

MGatner opened this issue Nov 20, 2019 · 7 comments · Fixed by #2573
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@MGatner
Copy link
Member

MGatner commented Nov 20, 2019

Describe the bug
ImageHandler does not do any verification that it has a valid image loaded, which can lead to some errors with unhelpful descriptions. E.g. fit() will try to calculate the aspect ration with a null value, so $xRatio = $width / $origWidth; results in "Division by zero".

I don't know where validation should happen - withFile() is maybe the best bet but doesn't catch instances where developers forget to use it before calling other methods.

CodeIgniter 4 version
develop

Affected module(s)
Image Manipulation Class

@MGatner MGatner added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 20, 2019
@lonnieezell
Copy link
Member

withFile makes sense to me.

Anytime the developer forgot to do a step it should crash loudly so they can catch it...

@MGatner
Copy link
Member Author

MGatner commented Nov 20, 2019

So errors like "division by zero" are okay if they skip withFile() and call fit() directly?

@MGatner
Copy link
Member Author

MGatner commented Nov 20, 2019

Actually to take it one step deeper... Image is actually just an extension of File with the additional methods copy() and getProperties() (and some properties) but no constructor... Should Image have its own constructor to verify the file is an image?

@MGatner
Copy link
Member Author

MGatner commented Nov 20, 2019

Deeper still... getProperties() relies on PHP's getimagesize that has the following note in the manual:

Caution This function expects filename to be a valid image file. If a non-image file is supplied, it may be incorrectly detected as an image and the function will return successfully, but the array may contain nonsensical values.
Do not use getimagesize() to check that a given file is a valid image. Use a purpose-built solution such as the Fileinfo extension instead.

I'm thinking we either need a lot more involvement or just some way of failing more explicitly.

@lonnieezell
Copy link
Member

In general yes. Every example in the docs show that you have to use withFile(). That library is not intended for general image creation, only manipulating existing images. At that point, it's a developer error and an exception should be thrown so they can locate the error during their testing.

I suppose we could give a more helpful exception message (that an image is required), but that would require a check on pretty much every function.

The File constructor already has the option to check if a file exists, but that's an optional check. In the BaseHandller, though we do check the file exists. That requires the user has actually used withFile, though...

@MGatner
Copy link
Member Author

MGatner commented Nov 20, 2019

Hmm this is tricky.

On the one hand, it seems like a valid $path and valid image at that path are requirements for the handlers, and presupposed for every method - that makes me think it should be part of the constructor (but keep withFile() to make handling multiple files easy).

On the other hand having the handlers be generic classes makes them easy to use with the service, but creates this situation where it is easy to mess up.

@MGatner
Copy link
Member Author

MGatner commented Nov 21, 2019

Floating an idea... Since all the methods use $this->image and require it to be a valid file & image, we replace that with $this->image() (or getImage() or whatever) that makes a one-time check (reset by withFile()) to do validate path and image and then return the Image instance, and it can handle throwing appropriate messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants