laminas-api-tools / api-tools-admin

Laminas API Tools Admin module
https://api-tools.getlaminas.org/documentation
BSD 3-Clause "New" or "Revised" License
13 stars 21 forks source link

#75 bugfix in Rest Service PATCH parameter validation #76

Closed ppaulis closed 2 years ago

ppaulis commented 2 years ago
Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

This fixes the bug described in #75 .

The concerned part is:

if (is_numeric($value) && intval($value) !== $value) {
    return false;
}

This validation makes no sense the way it is, because the Webinterface sends strings like "50", and in this case intval($value) !== $value always evaluates to true, due to the strict comparison. Instead, the validation should be :

if (!is_numeric($value) || intval($value) != $value) {
    return false;
}

as proposed in this PR.

ppaulis commented 2 years ago

@weierophinney The 1.11.x seems to have the same problem. Do you want me to do the same PR for 1.11.x?

froschdesign commented 2 years ago

@ppaulis This change needs a test case which illustrates the problem.

The 1.11.x seems to have the same problem. Do you want me to do the same PR for 1.11.x?

Not necessary, if this bugfix is accepted and merged then it will also be included in the 1.11.x branch.

ppaulis commented 2 years ago

hi @froschdesign and thanks for the quick reply! There is indeed already a unit test but it's wrong: https://github.com/laminas-api-tools/api-tools-admin/blob/1406d72b84938937d2cf6aa09a24437f40856381/test/InputFilter/RestService/PatchInputFilterTest.php#L58

The line should be :

'page_size'                  => "25",

I'll fix the test.

ppaulis commented 2 years ago

@froschdesign there are a few problems with my signoff in the commits. I'll close this PR and reopen a new proper one.

weierophinney commented 2 years ago

@ppaulis You don't need to close! Our contributor documentation details how to add sign-off after the fact - it's basically a rebase with a flag to add sign-off, plus a force push back to your branch. Go ahead and re-open, and follow the documentation, and we can then review.

ppaulis commented 2 years ago

@weierophinney thanks! Should have read that before -.- There's already a new one that has more progress : #77