Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Changed bytes conversion method #7413

Merged
merged 1 commit into from Mar 20, 2013

Conversation

Projects
None yet
8 participants
Contributor

jfsimon commented Mar 18, 2013

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes

The old way:

switch (strtolower(substr($memory, -1))) {
    case 'g':
        $memory *= 1024;
    case 'm':
        $memory *= 1024;
    case 'k':
        $memory *= 1024;
}

The new way:

if (preg_match('#^(\d+)([bkmgt])#i', $memory, $match)) {
    $shift = array('b' => 0, 'k' => 10, 'm' => 20, 'g' => 30, 't' => 40);
    $memory = ($match[1] * (1 << $shift[strtolower($match[2])]));
}
Contributor

bendavies commented Mar 18, 2013

pretty unreadable, no?

Contributor

benja-M-1 commented Mar 18, 2013

I agree, I would not like to have to debug it.

Contributor

pborreli commented Mar 18, 2013

just for my culture, what does : 1 << $var ?

Contributor

jfsimon commented Mar 18, 2013

@bendavies @benja-M-1 it's concise and easily recognised (if you understood it the first time).
FYI I didn't find it myself... pretty clever isn't it?

Contributor

benja-M-1 commented Mar 18, 2013

Clearly too much clever for me :)

And what about moving this code in its own class to avoid the copy/paste?

Contributor

jfsimon commented Mar 18, 2013

@benja-M-1 It would add a dependency to the components using it :(

Contributor

bendavies commented Mar 18, 2013

@jfsimon clever indeed, but not necessarily better!

Contributor

jfsimon commented Mar 18, 2013

@bendavies that's true.

Member

Tobion commented Mar 18, 2013

There are other places where it could be used too (e.g. FileValidator).

Contributor

bendavies commented Mar 18, 2013

on the other side of the argument, i hate the sneaky fall through on the switch statement.
very confusing the first time you see it!

Contributor

bendavies commented Mar 18, 2013

this method has already made it into symfony here: #7395

Contributor

jfsimon commented Mar 19, 2013

@Tobion I have some questions about the FileValidator:

  • Why is th k in lower case and the M in upper case?
  • Why is the size divided by 1000 and not 1024?
Member

Tobion commented Mar 19, 2013

I was wondering the same. I guess this config (which is also displayed to users) uses the official metric prefixes (k = kilo, M = mega). So it's not about the computer terms where 1 KB = 1024 byte.

Contributor

vicb commented Mar 19, 2013

kB =1000, kiB=1024.
Imo regexps should be case insensitive and account for the "i".
I am not in favor of the changes in this pr (the current way is also documented on php.net fwiw)

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

@fabpot fabpot merged branch jfsimon/preg-bytes-conversion (PR #7413)
This PR was merged into the master branch.

Commits
-------

3674c22 changed bytes conversion method

Discussion
----------

Changed bytes conversion method

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

**The old way:**

```php
switch (strtolower(substr($memory, -1))) {
    case 'g':
        $memory *= 1024;
    case 'm':
        $memory *= 1024;
    case 'k':
        $memory *= 1024;
}
```

**The new way:**

```php
if (preg_match('#^(\d+)([bkmgt])#i', $memory, $match)) {
    $shift = array('b' => 0, 'k' => 10, 'm' => 20, 'g' => 30, 't' => 40);
    $memory = ($match[1] * (1 << $shift[strtolower($match[2])]));
}
```

---------------------------------------------------------------------------

by bendavies at 2013-03-18T16:27:52Z

pretty unreadable, no?

---------------------------------------------------------------------------

by benja-M-1 at 2013-03-18T16:29:25Z

I agree, I would not like to have to debug it.

---------------------------------------------------------------------------

by pborreli at 2013-03-18T16:31:43Z

just for my culture, what does : `1 << $var` ?

---------------------------------------------------------------------------

by bendavies at 2013-03-18T16:33:23Z

@pborreli it's a left shift http://php.net/manual/en/language.operators.bitwise.php

---------------------------------------------------------------------------

by jfsimon at 2013-03-18T16:47:15Z

@bendavies @benja-M-1 it's concise and easily recognised (if you understood it the first time).
FYI I didn't find it myself... pretty clever isn't it?

---------------------------------------------------------------------------

by benja-M-1 at 2013-03-18T16:50:53Z

Clearly too much clever for me :)

And what about moving this code in its own class to avoid the copy/paste?

---------------------------------------------------------------------------

by jfsimon at 2013-03-18T16:52:51Z

@benja-M-1 It would add a dependency to the components using it :(

---------------------------------------------------------------------------

by bendavies at 2013-03-18T16:55:26Z

@jfsimon clever indeed, but not necessarily better!

---------------------------------------------------------------------------

by jfsimon at 2013-03-18T16:57:18Z

@bendavies that's true.

---------------------------------------------------------------------------

by Tobion at 2013-03-18T17:00:56Z

There are other places where it could be used too (e.g. FileValidator).

---------------------------------------------------------------------------

by bendavies at 2013-03-18T17:06:01Z

on the other side of the argument, i *hate* the sneaky fall through on the switch statement.
very confusing the first time you see it!

---------------------------------------------------------------------------

by bendavies at 2013-03-18T17:19:42Z

this method has already made it into symfony here: #7395

---------------------------------------------------------------------------

by jfsimon at 2013-03-19T08:16:19Z

@Tobion I have some questions about the `FileValidator`:
* Why is th `k` in lower case and the `M` in upper case?
* Why is the size divided by 1000 and not 1024?

---------------------------------------------------------------------------

by Tobion at 2013-03-19T08:30:23Z

I was wondering the same. I guess this config (which is also displayed to users) uses the official metric prefixes (k = kilo, M = mega). So it's not about the computer terms where 1 KB = 1024 byte.

---------------------------------------------------------------------------

by vicb at 2013-03-19T16:03:21Z

kB =1000, kiB=1024.
Imo regexps should be case insensitive and account for the "i".
I am not in favor of the changes in this pr (the current way is also documented on php.net fwiw)
4047bf2

@fabpot fabpot merged commit 3674c22 into symfony:master Mar 20, 2013

1 check failed

default Scrutinizer: No Comments — Travis: Failed
Details

Of course, now you have two problems... (http://regex.info/blog/2006-09-15/247)

:-)

Contributor

vicb commented Mar 22, 2013

"+1k" and "1 k" are 2 examples of why this PR is wrong. It should be reverted.

Contributor

vicb commented Mar 22, 2013

php.ini also supports:

  • decimal (the cast should happen before the switch as the fractional part is ignored, as in ServerParams),
  • octal,
  • hexa.

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

@fabpot fabpot 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
e8b7f0f

@fabpot fabpot added a commit that referenced this pull request Sep 16, 2013

@fabpot fabpot merged branch fabpot/bytes-conversion-epic-fail (PR #9048)
This PR was merged into the 2.3 branch.

Discussion
----------

fixed bytes conversion when used on 32-bits systems

| Q             | A
| ------------- | ---
| Bug fix?      | yes (on 32-bits systems)
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #8977
| License       | MIT
| Doc PR        | n/a

This PR reverts #7413 and #742, which does not work well when a number is big (3Go for instance) and the machine is 32bits.

Commits
-------

b3ae29d fixed bytes conversion when used on 32-bits systems
bb97f64
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment