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 fixed broken validation of page_size parameter #77

Closed ppaulis closed 2 years ago

ppaulis commented 2 years ago

Signed-off-by: Pascal Paulis ppaulis@gmail.com

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 GUI 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.

The Unit Test in this case was incorrect because it only tested on integers.

ppaulis commented 2 years ago

@froschdesign Here's the new PR with the commit correctly signed off and the fixed Unit Test.

froschdesign commented 2 years ago

@ppaulis

The Unit Test in this case was incorrect because it only tested on integers.

But this means that integers are valid and we need a test for this. Therefore, a new dataset needs to be added to the provider instead of changing existing ones.

ppaulis commented 2 years ago

@froschdesign I completed the tests, so that now all kinds of values are tested. I reorganized the code a little bit to avoid duplicated code fragments.

Here's a resume: value return
'25' valid
'-1' valid
25 valid
-1 valid
'string' not valid
'25.5' not valid
'-2' not valid
25.5 not valid
-2 not valid
weierophinney commented 2 years ago

The CS and Psalm failures are due to an invalid lockfile; I'll fix that in a different PR and then we can rebase against that here.