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

[Validator] Added support for validation of giga values #32485

Merged
merged 1 commit into from Jul 19, 2019
Merged

Conversation

kernig
Copy link
Contributor

@kernig kernig commented Jul 10, 2019

As described in issue #32479

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? just tested on my presta and it works. if further tests are necessary, it would be great if someone could help!
Fixed tickets #32479
License MIT
Doc PR

The validation(function normalizeBinaryFormat) in symfony/src/Symfony/Component/Validator/Constraints/File.php doesn't work with gigabyte values in php.ini.
In the PHP documentation it says "PHP allows shortcuts for byte values, including K (kilo), M (mega) and G (giga). " so in my opinion these values should work.
Thanks to @kijamve for the fix.

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Can you please add a test?

@@ -104,6 +104,8 @@ private function normalizeBinaryFormat($maxSize)
'ki' => 1 << 10,
'm' => 1000000,
'mi' => 1 << 20,
'g' => 1000000000,
Copy link
Contributor

Choose a reason for hiding this comment

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

i'd rather use constant expression as it is easier to see that the value is correct

Suggested change
'g' => 1000000000,
'g' => 1000 * 1000 * 1000,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea. Could you add the change for 'm' as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

as this is a bugfix it's advisable to leave it for the next commit

if you want to change it (i certainly like code consistency) this is the line:

    'm' => 1000 * 1000,

@kernig
Copy link
Contributor Author

kernig commented Jul 11, 2019

@derrabus ok, i'll try to setup a testing environment for it but it will take a few days, probably.

@nicolas-grekas nicolas-grekas changed the title Added support for validation of giga values [Validator] Added support for validation of giga values Jul 18, 2019
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

as is is consistent

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Tests fail, please have a look at them :)

@kernig
Copy link
Contributor Author

kernig commented Jul 19, 2019

@nicolas-grekas
fixed the problem, can you help with travis-ci check on PHP 7.2?

@fabpot
Copy link
Member

fabpot commented Jul 19, 2019

Thank you @kernig.

@fabpot fabpot merged commit 969f2c4 into symfony:3.4 Jul 19, 2019
fabpot added a commit that referenced this pull request Jul 19, 2019
…ernig)

This PR was squashed before being merged into the 3.4 branch (closes #32485).

Discussion
----------

[Validator] Added support for validation of giga values

As described in issue #32479

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | just tested on my presta and it works. if further tests are necessary, it would be great if someone could help!
| Fixed tickets | #32479
| License       | MIT
| Doc PR        |

The validation(function normalizeBinaryFormat) in symfony/src/Symfony/Component/Validator/Constraints/File.php doesn't work with gigabyte values in php.ini.
In the PHP documentation it says "PHP allows shortcuts for byte values, including K (kilo), M (mega) and G (giga). " so in my opinion these values should work.
Thanks to @kijamve for the fix.

Commits
-------

969f2c4 [Validator] Added support for validation of giga values
This was referenced Jul 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants