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

[Form] fixes ServerParams::getPostMaxSize() regex pattern #7481

Merged
merged 1 commit into from Mar 26, 2013

Conversation

Projects
None yet
6 participants
Contributor

jfsimon commented Mar 26, 2013

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #7480
Contributor

pborreli commented Mar 26, 2013

please add some tests, at least the failing case of #7480

@lazyhammer lazyhammer and 1 other commented on an outdated diff Mar 26, 2013

...ponent/Form/Extension/Validator/Util/ServerParams.php
@@ -29,7 +29,7 @@ public function getPostMaxSize()
return null;
}
- if (preg_match('#^\+?(0x?)?([^kmg]*)([KMG]?)#', $iniMax, $match)) {
+ if (preg_match('#^\+?(0x?)?([^KMG]*)([KMG]?)#', $iniMax, $match)) {
@lazyhammer

lazyhammer Mar 26, 2013

Contributor

You should add the $ to the end of the pattern. For now it translates 1Mfoo to 1024 * 1024, but the right result is 1

@jfsimon

jfsimon Mar 26, 2013

Contributor

So this method should return 1 as fallback, isn't it?
What about upload_max_filesize and memory_limit, do they follow the same rule?

@lazyhammer

lazyhammer Mar 26, 2013

Contributor

No, fallback must be 0 to be consistent with php's internal behavior. But I suggest changing the pattern to '#^\+?(0x?)?(.*?)([KMG]?)$#' so we can ignore all characters between last digit and (optional) unit at the end. What do you think?

@lazyhammer

lazyhammer Mar 26, 2013

Contributor

Yes, another two parameters use the same conversion method.

Contributor

jfsimon commented Mar 26, 2013

@pborreli ini_set() does not work for post_max_size value, so I cant't test this feature.

Contributor

stloyd commented Mar 26, 2013

@jfsimon but you can extend/mock class ServerParams and overwrite the getNormalizedIniPostMaxSize() method, so it will be able to return anything.

Contributor

pborreli commented Mar 26, 2013

@jfsimon then maybe overwrite getNormalizedIniPostMaxSize ?

Contributor

jfsimon commented Mar 26, 2013

@stloyd right, let's go.

Contributor

jfsimon commented Mar 26, 2013

@stloyd @pborreli really better with some tests, thanks.

Contributor

lazyhammer commented Mar 26, 2013

@jfsimon How about adding i modifier, so we can have the same regex in all places and don't rely on strto(upper|lower)?

Contributor

jfsimon commented Mar 26, 2013

@lazyhammer I'd like to knwo what does @vicb think about that.

Contributor

vicb commented Mar 26, 2013

I don't care.

Owner

fabpot commented Mar 26, 2013

I think it's time to admit that this new way is just a fail and we need to revert to the previous way.

Contributor

jfsimon commented Mar 26, 2013

@fabpot we wont surrender now! everything is okay now.

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

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

Discussion
----------

[Form] fixes ServerParams::getPostMaxSize() regex pattern

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

Commits
-------

84541e7 [Form] fixed ServerParams::getPostMaxSize() regex pattern

@fabpot fabpot merged commit 84541e7 into symfony:master Mar 26, 2013

1 check failed

default Scrutinizer: Errored — Travis: Passed
Details
Contributor

lazyhammer commented Mar 26, 2013

@jfsimon, it's not yet okay :) See my comments on the outdated diff. With that change, we'll be in full compliance with php internals.

Contributor

vicb commented Mar 26, 2013

@jfsimon, may be you could submit an issue to php.net asking the creation of int ini_get_shortand ( string $varname ) - a better name might be better.

edit: ini_translate_shortand($shortand) might be better

Contributor

jfsimon commented Mar 26, 2013

@lazyhammer #^\+?(0x?)?(.*?)([KMG]?)$# and #^\+?(0x?)?([^KMG]*)([KMG]?)# are strictly equivalent, but the first one must be ungreedy and end with a $ to work.

@vicb why not!

Contributor

lazyhammer commented Mar 26, 2013

@jfsimon they are not :) As I've stated in the comment, current regex will stop on the first occurence of [kmg], so 1k. will be translated to 1024, instead of 1 (that's how php handles it).

Contributor

vicb commented Mar 26, 2013

@lazyhammer I am sure that if we amend the tickets telling that it took 1 week for a smart guy to get 5 LOC right, they'll consider adding a ini_translate_shorthand and fixing the doc !

Contributor

jfsimon commented Mar 26, 2013

@lazyhammer ah, okay, I finaly unsdertand the thing. The regex should match [KMG] as the final char of the string, that's iT? I didn't notice the final$, sorry for that. I'll have to send another PR :-\

@vicb I'll open a ticket tomorrow morning, but we still need to translate the value correctly.

Contributor

pborreli commented Mar 26, 2013

@jfsimon and add more test 😮 plz

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

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

Discussion
----------

Fixes bytes convertion method, last episode

This definitely fixes the bytes convertion method.

@lazyhammer thanks for your support & indulgence
@fabpot sorry for the running gag
@vicb I opened a ticket: https://bugs.php.net/bug.php?id=64530

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

Commits
-------

233b945 fixed bytes convertion method, again
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment