[Validator] Fix #10648 : Incorrect validation for maxSize option in the FileValidator #10661

snoob · 2014-04-09T13:00:48Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#10648
License	MIT
Doc PR	N/A

stloyd · 2014-04-09T13:48:59Z

This should go into 2.3 as this is bugfix.

stloyd · 2014-04-09T13:50:41Z

src/Symfony/Component/Validator/Constraints/FileValidator.php

                $limit = (int) $constraint->maxSize;
                $suffix = 'kB';
            } elseif (preg_match('/^\d++M$/', $constraint->maxSize)) {
-                $size = round(filesize($path) / 1000000, 2);
+                $size = round(filesize($path) / (1024 * 1024), 2);


Just 1048576, same like it's used when error: UPLOAD_ERR_INI_SIZE occurs.

actually, I prefer 1024 * 1024 as everyone understands where it comes from.

Then other parts should be changed too I guess =)

@stloyd The first code (before the commit) was using the computed value. I already asked the change when reviewing the PR live.

And anyway, OPCache will probably be able to optimize this computation according to what @jpauli presented yesterday (unless it works only for additions, which would be weird)

and yeah, changing it on line 50 would be logical

Tobion · 2014-04-09T14:32:18Z

I also once noticed this problem. But

In the International System of Quantities, the kilobyte (symbol kB) is 1000 bytes. The binary representation of 1024 bytes typically uses the symbol KB, using an upper-case K. Informally sometimes the B is dropped, making K generally understood as 1024 bytes; however, this usage is not standardized and may be found used arbitrarily.
http://en.wikipedia.org/wiki/Kilobyte

So since we use kB in the validation message one can argue it's correct at the moment.

stof · 2014-04-10T07:42:38Z

@Tobion all systems are using base 2 for their file sizes (although HDD are marketed using base 10 to have bigger numbers), so most people are expecting base2 when seeing file sizes as they are accustomated to it (even without knowing it is base2)

Tobion · 2014-04-10T11:31:35Z

Sure but we need to change the unit symbol then.

fabpot · 2014-04-28T09:59:36Z

@snoob Can you finish this one by applying the suggestion made by @Tobion? Thanks.

snoob · 2014-04-29T11:28:20Z

@fabpot Done

There are 2 "kB" suffixes remaining in the classes Symfony\Component\HttpFoundation\File\UploadedFile and Symfony\Component\Console\Helper\Helper. The size seems to be on base 2 so maybe we should change these suffixes there too.

fabpot · 2014-04-29T11:48:18Z

@snoob Thanks, can you change the 2 other ones so that we are consistent everywhere in the framework (you can do so in this PR)?

Tobion · 2014-04-29T17:13:01Z

I think we need to change even more and use the binary prefixes of IEC.
But since currently $constraint->maxSize reads SI prefixes (M and k), it should also validate against SI prefixes as it has been. If we want to support base 2, then the constraint should also allow binary prefixes (Ki, Mi) and validate them accordingly. This way developers can choose which style they prefer and it avoids BC breaks.
If we change the validation as done at the moment, it just adds to the confusion.

Accordingly, the SI prefixes should only be used in the decimal sense, even when referring to data storage capacities: kilobyte and megabyte denote one thousand bytes and one million bytes respectively (consistent with SI), while new terms such as kibibyte, mebibyte and gibibyte, having the symbols KiB, MiB, and GiB, denote 1024bytes, 1048576bytes, and 1073741824bytes, respectively.
http://en.wikipedia.org/wiki/Binary_prefixes

Tobion · 2014-04-29T17:19:29Z

src/Symfony/Component/Validator/Tests/Constraints/FileValidatorTest.php

                '{{ file }}'    => $this->path,
-            ));
+        ));


wrong indention for array closing

snoob · 2014-04-30T13:59:28Z

@fabpot : Done

Tobion · 2014-05-01T22:16:25Z

Definitely -1 on the current state. Prefer to go my above suggestion

snoob · 2014-05-02T13:36:15Z

@Tobion : I agree with the IEC convention but there is a problem in my opinion, it is not widespread at all.

Do we really want to keep the base 10 system ?

If we chose to do, we can't use KB, because in my opinion, the difference between kb and KB is too thin and it will create confusions and mistakes.

If we decide not to use the base 10 system, then I think it is better to use KB as it is clearer than Ki.

There are no BC breaks here. It was an expected behavior.

webmozart · 2014-05-20T15:55:22Z

Continued in #10951.

This PR was merged into the 2.5-dev branch. Discussion ---------- Made use of "kB" vs. "KiB" consistent | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Continuation of #10661. This PR makes the usage of "kB" and "KiB" consistent across the project. The notations equal the internationally recognized ones: Short form | Long form | Value in Bytes --- | --- | --- B | bytes | 1 kB | kilobytes | 1000 KiB | kibibytes | 1024 MB | megabytes | 1000000 MiB | mebibytes | 1048576 GB | gigabytes | 1000000000 GiB | gibibytes | 1073741824 The reason for differentiating between the two is that several users got confused with the current mix (see #10648, #10917, #10661). FileValidator, UploadedFile and the ProgressBar helper were changed accordingly. Follow-up feature request: #10962 Commits ------- e4c6da5 [Validator] Improved to-string conversion of the file size/size limit bbe1045 [Validator][Console][HttpFoundation] Use "KiB" everywhere (instead of "kB")

[Validator] Fix #10648

840d252

stloyd reviewed Apr 9, 2014
View reviewed changes

stof added the Validator label Apr 10, 2014

snoob added 2 commits April 29, 2014 12:44

Refactored 1048576 by 1024*1024

5351c4f

The file validator now uses the suffix KB

b39d1c4

Tobion reviewed Apr 29, 2014
View reviewed changes

snoob added 3 commits April 30, 2014 08:54

Fixed extra line and wrong array indentation

e9a089d

Refactored kB by KB

69b98e7

Refactored kB by KB

0989796

webmozart mentioned this pull request May 20, 2014

Made use of "kB" vs. "KiB" consistent #10951

Merged

webmozart closed this May 20, 2014

snoob deleted the issue#10648 branch May 23, 2014 09:27

Uh oh!

[Validator] Fix #10648 : Incorrect validation for maxSize option in the FileValidator #10661

[Validator] Fix #10648 : Incorrect validation for maxSize option in the FileValidator #10661

Uh oh!

Conversation

snoob commented Apr 9, 2014

Uh oh!

stloyd commented Apr 9, 2014

Uh oh!

stloyd Apr 9, 2014

Choose a reason for hiding this comment

Uh oh!

fabpot Apr 9, 2014

Choose a reason for hiding this comment

Uh oh!

stloyd Apr 9, 2014

Choose a reason for hiding this comment

Uh oh!

stof Apr 9, 2014

Choose a reason for hiding this comment

Uh oh!

stof Apr 9, 2014

Choose a reason for hiding this comment

Uh oh!

Tobion commented Apr 9, 2014

Uh oh!

stof commented Apr 10, 2014

Uh oh!

Tobion commented Apr 10, 2014

Uh oh!

fabpot commented Apr 28, 2014

Uh oh!

snoob commented Apr 29, 2014

Uh oh!

fabpot commented Apr 29, 2014

Uh oh!

Tobion commented Apr 29, 2014

Uh oh!

Tobion Apr 29, 2014

Choose a reason for hiding this comment

Uh oh!

snoob commented Apr 30, 2014

Uh oh!

Tobion commented May 1, 2014

Uh oh!

snoob commented May 2, 2014

Uh oh!

webmozart commented May 20, 2014

Uh oh!

Uh oh!