Improve bytes conversion method #7456

Merged
merged 1 commit into from Mar 25, 2013

Conversation

Projects
None yet
4 participants
@jfsimon
Contributor

jfsimon commented Mar 23, 2013

This PR improves bytes conversion regex method introduced in #7413 (thanks to @vicb's comments).

  • Adds support of + prefix.
  • Adds support of blank chars between +, number and unit.
  • Adds support of octal/hexa bases.

Notice that this can not be unit tested for ServerParams and UploadedFile classes because ini_set() function does not work with post_max_size and upload_max_filesize settings.

For information, this convertion is located in 3 classes:

  • Symfony\Component\Form\Extension\Validator\Util\ServerParams
  • Symfony\Component\HttpFoundation\File\UploadedFile
  • Symfony\Component\HttpKernel\DataCollector\MemoryDataCollector
Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #7413
@vicb

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

@jfsimon, you probably mis-read my comment, the code needs to be reverted.

Contributor

vicb commented Mar 23, 2013

@jfsimon, you probably mis-read my comment, the code needs to be reverted.

@jfsimon

This comment has been minimized.

Show comment Hide comment
@jfsimon

jfsimon Mar 23, 2013

Contributor

@vicb What's wrong with this solution? I need to understand.

Contributor

jfsimon commented Mar 23, 2013

@vicb What's wrong with this solution? I need to understand.

@vicb

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor
  • What was wrong with the previous (working) solution ?
  • The current look of your code.

I don't think anybody will ever use a form to upload TB of data, but anyway "t" is not supported (I think the sames goes for "b")

Contributor

vicb commented Mar 23, 2013

  • What was wrong with the previous (working) solution ?
  • The current look of your code.

I don't think anybody will ever use a form to upload TB of data, but anyway "t" is not supported (I think the sames goes for "b")

@jfsimon

This comment has been minimized.

Show comment Hide comment
@jfsimon

jfsimon Mar 23, 2013

Contributor

Okay. What about octal/hexadecimal values? Don't they need to be supported?

Contributor

jfsimon commented Mar 23, 2013

Okay. What about octal/hexadecimal values? Don't they need to be supported?

@vicb

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

Good point. You should ask @fabpot.

Contributor

vicb commented Mar 23, 2013

Good point. You should ask @fabpot.

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 23, 2013

Member

I'm the one who asked @jfsimon to use this new way. Before the last comment of @jfsimon, I was inclined to agree with @vicb and revert the change. But @jfsimon had a good point, so I think the new way is indeed better as it covers more cases.

Member

fabpot commented Mar 23, 2013

I'm the one who asked @jfsimon to use this new way. Before the last comment of @jfsimon, I was inclined to agree with @vicb and revert the change. But @jfsimon had a good point, so I think the new way is indeed better as it covers more cases.

@vicb

View changes

src/Symfony/Component/Form/Extension/Validator/Util/ServerParams.php
+ if (preg_match('#^\s*\+?\s*([0x]*)([1-9][0-9]*)\s*([bkmgt]?)#i', $iniMax, $match)) {
+ $shift = array('' => 0, 'b' => 0, 'k' => 10, 'm' => 20, 'g' => 30, 't' => 40);
+ $bases = array('' => 10, '0' => 8, '0x' => 16);
+ $iniMax = (base_convert($match[2], $bases[$match[1]], 10) * (1 << $shift[strtolower($match[3])]));

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

strtoloweris not needed as the value is already normalized here (upercase)

@vicb

vicb Mar 23, 2013

Contributor

strtoloweris not needed as the value is already normalized here (upercase)

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

ìntval might be better than base_convert

@vicb

vicb Mar 23, 2013

Contributor

ìntval might be better than base_convert

This comment has been minimized.

Show comment Hide comment
@jfsimon

jfsimon Mar 23, 2013

Contributor

I didnt knew intval, seems better indeed.

@jfsimon

jfsimon Mar 23, 2013

Contributor

I didnt knew intval, seems better indeed.

@vicb

View changes

src/Symfony/Component/Form/Extension/Validator/Util/ServerParams.php
- if (preg_match('#^(\d+)([bkmgt])#i', $iniMax, $match)) {
- $shift = array('b' => 0, 'k' => 10, 'm' => 20, 'g' => 30, 't' => 40);
- $iniMax = ($match[1] * (1 << $shift[strtolower($match[2])]));
+ if (preg_match('#^\s*\+?\s*([0x]*)([1-9][0-9]*)\s*([bkmgt]?)#i', $iniMax, $match)) {

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

leading "\s*" is not needed as value are trimmed internaly by PHP

@vicb

vicb Mar 23, 2013

Contributor

leading "\s*" is not needed as value are trimmed internaly by PHP

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

If think "0x05" is a valid hexa number.

1.1 is not supported

@vicb

vicb Mar 23, 2013

Contributor

If think "0x05" is a valid hexa number.

1.1 is not supported

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

"a-f" is also valid hexa

@vicb

vicb Mar 23, 2013

Contributor

"a-f" is also valid hexa

This comment has been minimized.

Show comment Hide comment
@jfsimon

jfsimon Mar 23, 2013

Contributor

I dont think float values are allowed here.

@jfsimon

jfsimon Mar 23, 2013

Contributor

I dont think float values are allowed here.

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

read my previous comments.

@vicb

vicb Mar 23, 2013

Contributor

read my previous comments.

This comment has been minimized.

Show comment Hide comment
@lazyhammer

lazyhammer Mar 23, 2013

Contributor

Internally php uses strtol(), so it supports values in format ^\s*(valid_number)(any_other_char_sequence)?[kKmMgG]$, where valid_number is valid octal or (hexa)decimal number (i.e. 0x[0-9a-f]+, 0[0-7]*, [1-9][0-9]*), and any_other_char_sequence is any other char sequence, which is ignored. So it actually supports setting ini values like 1testG.

@lazyhammer

lazyhammer Mar 23, 2013

Contributor

Internally php uses strtol(), so it supports values in format ^\s*(valid_number)(any_other_char_sequence)?[kKmMgG]$, where valid_number is valid octal or (hexa)decimal number (i.e. 0x[0-9a-f]+, 0[0-7]*, [1-9][0-9]*), and any_other_char_sequence is any other char sequence, which is ignored. So it actually supports setting ini values like 1testG.

@lazyhammer

View changes

src/Symfony/Component/Form/Extension/Validator/Util/ServerParams.php
- if (preg_match('#^(\d+)([bkmgt])#i', $iniMax, $match)) {
- $shift = array('b' => 0, 'k' => 10, 'm' => 20, 'g' => 30, 't' => 40);
- $iniMax = ($match[1] * (1 << $shift[strtolower($match[2])]));
+ if (preg_match('#^\+?\s*([0x]*)([0-9a-f]*)\s*([bkmgt]?)#i', $iniMax, $match)) {

This comment has been minimized.

Show comment Hide comment
@lazyhammer

lazyhammer Mar 23, 2013

Contributor

I think you should remove b, since it's a valid hex digit. Actually, my point is we should not support something that's not supported by php (see my comment in outdated diff), so I suggest something like to replicate php's behavior:

if (preg_match('#^\+?(0x?)?(.*)([KMG])?$#', $iniMax, $match)) {
    // ...

    return $iniMax;
}

return 0;
@lazyhammer

lazyhammer Mar 23, 2013

Contributor

I think you should remove b, since it's a valid hex digit. Actually, my point is we should not support something that's not supported by php (see my comment in outdated diff), so I suggest something like to replicate php's behavior:

if (preg_match('#^\+?(0x?)?(.*)([KMG])?$#', $iniMax, $match)) {
    // ...

    return $iniMax;
}

return 0;

This comment has been minimized.

Show comment Hide comment
@jfsimon

jfsimon Mar 23, 2013

Contributor

Great, thank you for explanations!

@jfsimon

jfsimon Mar 23, 2013

Contributor

Great, thank you for explanations!

This comment has been minimized.

Show comment Hide comment
@jfsimon

jfsimon Mar 23, 2013

Contributor

ps: #^\+?(0x?)?(.*)([kmg]?)# was not working, I replaced it whith #^\+?(0x?)?([^kmg]*)([kmg]?)#.

@jfsimon

jfsimon Mar 23, 2013

Contributor

ps: #^\+?(0x?)?(.*)([kmg]?)# was not working, I replaced it whith #^\+?(0x?)?([^kmg]*)([kmg]?)#.

This comment has been minimized.

Show comment Hide comment
@lazyhammer

lazyhammer Mar 23, 2013

Contributor

Sorry, I've not tested it before posting :)
BTW, you can make .* in the middle 'ungreedy' and add missing $ to the end, and it will work as expected.

@lazyhammer

lazyhammer Mar 23, 2013

Contributor

Sorry, I've not tested it before posting :)
BTW, you can make .* in the middle 'ungreedy' and add missing $ to the end, and it will work as expected.

@vicb

View changes

src/Symfony/Component/Form/Extension/Validator/Util/ServerParams.php
- $shift = array('b' => 0, 'k' => 10, 'm' => 20, 'g' => 30, 't' => 40);
- $iniMax = ($match[1] * (1 << $shift[strtolower($match[2])]));
+ if (preg_match('#^\+?(0x?)?([^kmg]*)([kmg]?)#', $iniMax, $match)) {
+ $shift = array('' => 0, 'k' => 10, 'm' => 20, 'g' => 30);

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

Should be KMG on the above 2 lines

@vicb

vicb Mar 23, 2013

Contributor

Should be KMG on the above 2 lines

This comment has been minimized.

Show comment Hide comment
@jfsimon

jfsimon Mar 23, 2013

Contributor

Oh, I red strtolower :/
Don't you think I should turn strtoupper into strtolower to have the same code everywhere?

@jfsimon

jfsimon Mar 23, 2013

Contributor

Oh, I red strtolower :/
Don't you think I should turn strtoupper into strtolower to have the same code everywhere?

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

should probably be $shifts to be consistent.

@vicb

vicb Mar 23, 2013

Contributor

should probably be $shifts to be consistent.

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

Don't you think I should turn strtoupper into strtolower to have the same code everywhere?

That would be a BC break without any upside, so no.

@vicb

vicb Mar 23, 2013

Contributor

Don't you think I should turn strtoupper into strtolower to have the same code everywhere?

That would be a BC break without any upside, so no.

@vicb

View changes

src/Symfony/Component/HttpFoundation/File/UploadedFile.php
@@ -229,17 +229,19 @@ public function move($directory, $name = null)
*/
public static function getMaxFilesize()
{
- $max = trim(ini_get('upload_max_filesize'));
+ $max = strtolower(trim(ini_get('upload_max_filesize')));

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

you can remove the trim as this is done internally by the PHP parser

@vicb

vicb Mar 23, 2013

Contributor

you can remove the trim as this is done internally by the PHP parser

@vicb

View changes

src/Symfony/Component/HttpKernel/DataCollector/MemoryDataCollector.php
@@ -25,7 +25,7 @@ public function __construct()
{
$this->data = array(
'memory' => 0,
- 'memory_limit' => $this->convertToBytes(ini_get('memory_limit')),
+ 'memory_limit' => $this->convertToBytes(strtolower(trim(ini_get('memory_limit')))),

This comment has been minimized.

Show comment Hide comment
@vicb

vicb Mar 23, 2013

Contributor

same here

@vicb

vicb Mar 23, 2013

Contributor

same here

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Mar 25, 2013

Member

Is it ready to be merged?

Member

fabpot commented Mar 25, 2013

Is it ready to be merged?

@jfsimon

This comment has been minimized.

Show comment Hide comment
@jfsimon

jfsimon Mar 25, 2013

Contributor

@fabpot Yes.

Contributor

jfsimon commented Mar 25, 2013

@fabpot Yes.

fabpot added a commit that referenced this pull request Mar 25, 2013

merged branch jfsimon/issue-7413 (PR #7456)
This PR was merged into the master branch.

Discussion
----------

Improve bytes conversion method

This PR improves bytes conversion `regex` method introduced in #7413 (thanks to @vicb's comments).

* Adds support of `+` prefix.
* Adds support of blank chars between `+`, number and unit.
* Adds support of octal/hexa bases.

Notice that this can not be unit tested for `ServerParams` and `UploadedFile` classes because `ini_set()` function does not work with `post_max_size` and `upload_max_filesize` settings.

For information, this convertion is located in 3 classes:
* `Symfony\Component\Form\Extension\Validator\Util\ServerParams`
* `Symfony\Component\HttpFoundation\File\UploadedFile`
* `Symfony\Component\HttpKernel\DataCollector\MemoryDataCollector`

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

Commits
-------

21291ca improved bytes conversion method

@fabpot fabpot merged commit 21291ca into symfony:master Mar 25, 2013

1 check passed

default Scrutinizer: No Comments — Travis: Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment