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] Support "maxSize" given in KiB #11027

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@jderusse
Copy link
Contributor

commented May 31, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #10962
License MIT
Doc PR symfony/symfony-docs#3895

To display the constraint validation message with an expected suffix, this PR add a new option in File constraint.

{
if (is_array($options) && array_key_exists('maxSizeFormat', $options)) {
throw new ConstraintDefinitionException(sprintf(
'The option "maxSizeFormat" is not supported by the constraint %s',

This comment has been minimized.

Copy link
@Tobion

Tobion Jun 1, 2014

Member

I don't think this makes sense to disallow. Since it's a public property it could be manually set after the constructor anyway. So I would make it so that one could specifiy the size in binary format but still set the formatting to decimal for the validation message. Only set it to binary when it's null.

This comment has been minimized.

Copy link
@Tobion

Tobion Jun 1, 2014

Member

Then I would also rename the property to $binaryFormat = null|false|true when null then based on maxSize.

This comment has been minimized.

Copy link
@jderusse

jderusse Jun 1, 2014

Author Contributor

In a previous PR, @fabpot says that he prefered to avoid adding another parameter (#10917 (comment)).
And I agree with him, the purpose of this new parameter will just be to let the user choise the format of the error message 😕

But you're right this trick is trying to avoid to expose a new paramater, but in the reality, there is a new parameter.

Should I leave maxSize has the user defined it. And expose 2 getter in Constraint\File: getMaxSizeBytes and getMaxSizeFormat ? But this is not what @webmozart expected in his ticket #10962

This comment has been minimized.

Copy link
@Tobion

Tobion Jun 1, 2014

Member

I don't see what is bad at having an option for binary/decimal validation format. For example I can image to have that configurable in a CMS which can be useful when you expect your users won't understand the binary suffixes.

} elseif (preg_match('/^\d++M$/', $constraint->maxSize)) {
$limitInBytes = $constraint->maxSize * self::MB_BYTES;
} else {
if (!ctype_digit((string) $constraint->maxSize)) {
throw new ConstraintDefinitionException(sprintf('"%s" is not a valid maximum size', $constraint->maxSize));

This comment has been minimized.

Copy link
@Tobion

Tobion Jun 1, 2014

Member

IMO this can be removed now. The maxSize property could indeed be overridden manually but then, we could not rely on anything and would for example also need to check whether mimeTypes is an array at all. So I think we can just rely on the construct making it an integer.

@jderusse jderusse added the Validator label Jun 16, 2014

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 16, 2014

👍

1 similar comment
@romainneutron

This comment has been minimized.

Copy link
Member

commented Jun 17, 2014

👍

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 17, 2014

Thank you @jeremy-derusse.

@fabpot fabpot closed this in d217c7a Jun 17, 2014

fabpot added a commit that referenced this pull request Jun 17, 2014

minor #11140 [Validator] smaller CS fixes (Tobion)
This PR was merged into the 2.3-dev branch.

Discussion
----------

[Validator] smaller CS fixes

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT

First commit fixes some CS of #11027
Second one removes `null` initialization from Validator as we don't do it.

Commits
-------

2025778 [Validator] no need to initialize properties with null
967576a [Validator] smaller fixes for binary format

weaverryan added a commit to symfony/symfony-docs that referenced this pull request Aug 16, 2014

feature #3895 [Validator] Support "maxSize" given in KiB (jeremy-deru…
…sse)

This PR was merged into the master branch.

Discussion
----------

[Validator] Support "maxSize" given in KiB

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | symfony/symfony#11027
| Applies to    | 2.6
| Fixed tickets | symfony/symfony#10962

To display the constraint validation message with an expected suffix, this PR add a new option in  File constraint.

Commits
-------

1e8fa48 Replace bullet list by a table
77a0687 Add versionAdded on binary suffix
b414f5c Use comma as thousands separator
7dcd7c3 Move secion "binaryFormat" to the right place
71fdd60 Fix line wrap
3f3f4e0 Provide information about SI and Binary prefixes
deac0c3 Add option binaryFormat in constraint file
e3acdc5 Support MaxSize in KiB and MiB

@jderusse jderusse deleted the jderusse:maxsize-in-kib branch May 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.