Adding image size validation #9465

burzum · 2016-09-16T12:37:46Z

This works without any external extension or lib, see

Please see http://php.net/manual/en/function.getimagesize.php

Note: This function does not require the GD image library.

stickler-ci · 2016-09-16T12:38:12Z

src/Validation/Validation.php

@@ -1082,6 +1082,76 @@ public static function uploadedFile($file, array $options = [])
    }

    /**
+     * Validates the size of an uploaded image.
+     *
+     * @param array $value


Missing parameter comment

stickler-ci · 2016-09-16T12:38:12Z

src/Validation/Validation.php

+     * Validates the size of an uploaded image.
+     *
+     * @param array $value
+     * @param array $options


Missing parameter comment

stickler-ci · 2016-09-16T12:38:12Z

src/Validation/Validation.php

+     *
+     * @param array $value
+     * @param array $options
+     * @return boolean


Expected "bool" but found "boolean" for function return type

stickler-ci · 2016-09-16T12:38:13Z

src/Validation/Validation.php

+    /**
+     * Validates the image width.
+     *
+     * @param array $value


Missing parameter comment

stickler-ci · 2016-09-16T12:38:13Z

src/Validation/Validation.php

+     * Validates the image width.
+     *
+     * @param array $value
+     * @param string $operator


Missing parameter comment

stickler-ci · 2016-09-16T12:38:13Z

src/Validation/Validation.php

+     *
+     * @param array $value
+     * @param string $operator
+     * @param integer $width


Missing parameter comment
Expected "int" but found "integer" for parameter type

stickler-ci · 2016-09-16T12:38:13Z

src/Validation/Validation.php

+     * @param array $value
+     * @param string $operator
+     * @param integer $width
+     * @return boolean


Expected "bool" but found "boolean" for function return type

stickler-ci · 2016-09-16T12:38:13Z

src/Validation/Validation.php

+    /**
+     * Validates the image width.
+     *
+     * @param array $value


Missing parameter comment

stickler-ci · 2016-09-16T12:38:13Z

src/Validation/Validation.php

+     * Validates the image width.
+     *
+     * @param array $value
+     * @param string $operator


Missing parameter comment

stickler-ci · 2016-09-16T12:38:14Z

src/Validation/Validation.php

+     *
+     * @param array $value
+     * @param string $operator
+     * @param integer $height


Missing parameter comment
Expected "int" but found "integer" for parameter type

stickler-ci · 2016-09-16T12:38:14Z

src/Validation/Validation.php

+     * @param array $value
+     * @param string $operator
+     * @param integer $height
+     * @return boolean


Expected "bool" but found "boolean" for function return type

stickler-ci · 2016-09-16T12:38:14Z

tests/TestCase/Validation/ValidationTest.php

+     *
+     * @return void
+     */
+    public function testImageSize() {


Opening brace should be on a new line

inoas · 2016-09-16T12:40:02Z

OT: I wish I could auto ignore stickler-ci-mail-spam unless it is my own PR.

dakota · 2016-09-16T12:46:50Z

getimagesize requires the GD extension to be installed?

thinkingmedia · 2016-09-16T12:50:37Z

This is a cool feature. thanks. I think anyone working with images will have GD installed.

ndm2 · 2016-09-16T12:52:00Z

It doesn't require GD, this is all built-in. It however doesn't guarantee that what you're testing is really an image.

burzum · 2016-09-16T12:52:59Z

It doesn't require any external lib, it's built in. That's why I've linked http://php.net/manual/en/function.getimagesize.php

Note: This function does not require the GD image library.

ndm2 · 2016-09-16T12:58:21Z

src/Validation/Validation.php

+            throw new InvalidArgumentException('Invalid image size validation parameters! Missing `width` and / or `height`!');
+        }
+
+        list($width, $height) = getimagesize($file['tmp_name']);


I think it would be good if this could handle non-uploads too, like for example mimeType().

So check if the passed value is a string and that the file exists? If yes, I can do that next week. I'm about to leave for a weekend trip.

I'd say the other way around, check if it's an array, like in the linked method. And an is_file() check would be good too, yes.

Should this also handle PSR7 UploadedFile objects? That input has a getStream() method which lets you read the underlying file data.

I'd think that in a regular request CakePHP should never see such objects, but I guess that handling them generally wouldn't hurt in case CakePHP is being advertised with PSR7 support.

I'm hoping that in the future people will use the uploaded files objects more than the low level array data.

inoas · 2016-09-16T13:07:26Z

It doesn't require GD, this is all built-in. It however doesn't guarantee that what you're testing is really an image.

Maybe an additional check for the mimeType then doesn't hurt? For all the types supported:
For example: GIF, JPEG, PNG, PSD, BMP, TIFF_II, TIFF_MM, JPC, JP2, JPX, JB2, IFF, WBMP, JPEG2000, XBM, ICO
aka: http://stackoverflow.com/a/1282373
Doesn't really help, see #9465 (comment)

codecov-io · 2016-09-16T13:15:45Z

Current coverage is 94.92% (diff: 92.59%)

Merging #9465 into master will increase coverage by <.01%

@@             master      #9465   diff @@
==========================================
  Files           412        412          
  Lines         28204      28273    +69   
  Methods        3382       3387     +5   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          26771      26838    +67   
- Misses         1433       1435     +2   
  Partials          0          0

Powered by Codecov. Last update 996440c...4ed813f

ADmad · 2016-09-16T14:28:14Z

src/Validation/Validation.php

+     */
+    public static function imageSize($file, $options)
+    {
+        if (!isset($options['height']) && !isset($options['width'])) {


The method should allow checking for only height or width too.

Sorry I misread the code, it does.

ndm2 · 2016-09-16T15:42:24Z

Maybe an additional check for the mimeType then doesn't hurt?

@ionas Not sure how that would help? The mime type check usually also only relies on file header information. Without the use of an image library that checks whether the given file is actually a valid image, and doesn't only rely on the header info for the size check, there's not much to do here I guess.

inoas · 2016-09-16T15:45:20Z

Then its no point doing more checks.

markstory

Looking good @burzum

markstory · 2016-09-17T01:49:40Z

src/Validation/Validation.php

+    public static function imageSize($file, $options)
+    {
+        if (!isset($options['height']) && !isset($options['width'])) {
+            throw new InvalidArgumentException('Invalid image size validation parameters! Missing `width` and / or `height`!');


Not sure we need ! in the error messages.

markstory · 2016-09-17T01:53:33Z

src/Validation/Validation.php

+            throw new InvalidArgumentException('Invalid image size validation parameters! Missing `width` and / or `height`!');
+        }
+
+        list($width, $height) = getimagesize($file['tmp_name']);


Should this also handle PSR7 UploadedFile objects? That input has a getStream() method which lets you read the underlying file data.

dereuromark · 2016-09-18T20:25:07Z

tests/TestCase/Validation/ValidationTest.php

+            'tmp_name' => $image
+        ];
+
+        $this->assertTrue(Validation::imageWidth($upload, '>', 100));


Is there also >= and <= as tests?

jimgwhit · 2016-09-19T03:40:52Z

Why when already built into php

if ($_FILES["fileToUpload"]["size"] > 500000) {
    echo "Sorry, your file is too large.";
    $uploadOk = 0;
}

reference http://www.w3schools.com/php/php_file_upload.asp

dakota · 2016-09-19T08:11:02Z

@jimgwhit This isn't a file size validation, but an image size (i.e width and height) validation.

ADmad · 2016-09-19T08:11:04Z

@jimgwhit What does file size have anything to do with validating image width and height? Please avoid posting comments which doesn't add anything constructive to the discussion.

lorenzo · 2016-09-20T08:26:37Z

This looks good to me

Adding image size validation

33cc7a9

stickler-ci reviewed Sep 16, 2016

View reviewed changes

src/Validation/Validation.php

/**

* Validates the image width.

*

* @param array $value

Copy link

Contributor

stickler-ci Sep 16, 2016

Choose a reason for hiding this comment

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

Missing parameter comment

stickler-ci reviewed Sep 16, 2016

View reviewed changes

src/Validation/Validation.php

/**

* Validates the image width.

*

* @param array $value

Copy link

Contributor

stickler-ci Sep 16, 2016

Choose a reason for hiding this comment

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

Missing parameter comment

stickler-ci reviewed Sep 16, 2016

View reviewed changes

Updating doc blocks for the validation

2496685

phpcs fix

bba091d

ndm2 reviewed Sep 16, 2016

View reviewed changes

ADmad reviewed Sep 16, 2016

View reviewed changes

markstory added enhancement validation labels Sep 17, 2016

markstory added this to the 3.3.4 milestone Sep 17, 2016

markstory reviewed Sep 17, 2016

View reviewed changes

dereuromark reviewed Sep 18, 2016

View reviewed changes

Adding support for UploadedFileInterface to imageSize()

86e6a80

Florian Krämer added 2 commits September 19, 2016 10:24

Adding more tests for imageSize() validation.

760ff6d

Testing the \InvalidArgumentException for imageSize()

4ed813f

burzum self-assigned this Sep 21, 2016

markstory merged commit b2f37db into cakephp:master Sep 21, 2016

markstory deleted the feature/imagesize-validation branch September 21, 2016 16:28

Adding image size validation #9465

Adding image size validation #9465

Conversation

burzum commented Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inoas commented Sep 16, 2016

dakota commented Sep 16, 2016

thinkingmedia commented Sep 16, 2016

ndm2 commented Sep 16, 2016

burzum commented Sep 16, 2016

Choose a reason for hiding this comment

burzum Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inoas commented Sep 16, 2016 • edited Loading

codecov-io commented Sep 16, 2016 • edited Loading

Current coverage is 94.92% (diff: 92.59%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndm2 commented Sep 16, 2016

inoas commented Sep 16, 2016

markstory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimgwhit commented Sep 19, 2016

dakota commented Sep 19, 2016

ADmad commented Sep 19, 2016

lorenzo commented Sep 20, 2016

burzum commented Sep 16, 2016 •

edited

Loading

burzum Sep 16, 2016 •

edited

Loading

inoas commented Sep 16, 2016 •

edited

Loading

codecov-io commented Sep 16, 2016 •

edited

Loading