Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Validator] Fix #10648 : Incorrect validation for maxSize option in the FileValidator#10661
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
snoob commentedApr 9, 2014
| 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 commentedApr 9, 2014
This should go into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just1048576, same like it's used when error:UPLOAD_ERR_INI_SIZE occurs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
actually, I prefer1024 * 1024 as everyone understands where it comes from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Then other parts should be changed too I guess =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
and yeah, changing it on line 50 would be logical
Tobion commentedApr 9, 2014
I also once noticed this problem. But
So since we use |
stof commentedApr 10, 2014
@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 commentedApr 10, 2014
Sure but we need to change the unit symbol then. |
fabpot commentedApr 28, 2014
snoob commentedApr 29, 2014
@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 commentedApr 29, 2014
@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 commentedApr 29, 2014
I think we need to change even more and use the binary prefixes of IEC.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
wrong indention for array closing
snoob commentedApr 30, 2014
@fabpot : Done |
Tobion commentedMay 1, 2014
Definitely -1 on the current state. Prefer to go my above suggestion |
snoob commentedMay 2, 2014
@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 commentedMay 20, 2014
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 | 1kB | kilobytes | 1000KiB | kibibytes | 1024MB | megabytes | 1000000MiB | mebibytes | 1048576GB | gigabytes | 1000000000GiB | gibibytes | 1073741824The 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:#10962Commits-------e4c6da5 [Validator] Improved to-string conversion of the file size/size limitbbe1045 [Validator][Console][HttpFoundation] Use "KiB" everywhere (instead of "kB")