[Form] fixes ServerParams::getPostMaxSize() regex pattern #7481
Conversation
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #7480 |
|
please add some tests, at least the failing case of #7480 |
| @@ -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)) { | |||
There was a problem hiding this comment.
You should add the $ to the end of the pattern. For now it translates 1Mfoo to 1024 * 1024, but the right result is 1
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, another two parameters use the same conversion method.
|
@pborreli |
|
@jfsimon but you can extend/mock class |
|
@jfsimon then maybe overwrite getNormalizedIniPostMaxSize ? |
|
@stloyd right, let's go. |
|
@jfsimon How about adding |
|
@lazyhammer I'd like to knwo what does @vicb think about that. |
|
I don't care. |
|
I think it's time to admit that this new way is just a fail and we need to revert to the previous way. |
|
@fabpot we wont surrender now! everything is okay now. |
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
|
@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. |
|
@jfsimon, may be you could submit an issue to php.net asking the creation of edit: |
|
@lazyhammer @vicb why not! |
|
@jfsimon they are not :) As I've stated in the comment, current regex will stop on the first occurence of |
|
@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 |
|
@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 @vicb I'll open a ticket tomorrow morning, but we still need to translate the value correctly. |
|
@jfsimon and add more test 😮 plz |
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