Fix value annotation in isValid method for Regex validator. #179

h3nnry · 2023-02-23T13:11:33Z

Changed annotation for parameter value in isValid method for Regex validator, from string to string|numerical as long as this method accepts also integer and float parameters.

Signed-off-by: Andrei <[email protected]>

src/Regex.php

Ocramius · 2023-02-23T14:46:38Z

src/Regex.php

@@ -107,7 +107,7 @@ public function setPattern($pattern)
    /**
     * Returns true if and only if $value matches against the pattern option
     *
-     * @param  string $value
+     * @param  string|numeric $value


If we accept non-string values, a string cast is probably needed for acceptable conversions

IMO, isValid() should always accept mixed as per the interface.

In my opinion better to avoid mixed type.

…method. Signed-off-by: Andrei <[email protected]>

Ocramius · 2023-02-24T16:51:11Z

src/Regex.php

+        if (is_int($value) || is_float($value)) {
+            $value = (string) $value;
+        }


Since new logic was added, we should have a test covering this scenario?

I have reservations against allowing floats for the same reasons it was removed from moneyphp (I believe by @Ocramius), and as was discussed here: moneyphp/money#650.

It would then be the responsibility of the user to handle that casting.

$regExValidator->isValid((string)$value);

Maybe there needs to be a ToString filter for use with laminas/laminas-inputfilter?

Although not as immediately important as with a package named money, since this is for validation I could see some users unknowingly making a mistake. Thoughts? Is this an overreaction?

Ooooh, that is a good point! A floating point number may indeed cast to something unexpected 🤔

@internalsystemerror

Maybe there needs to be a ToString filter…

See: laminas/laminas-filter#98

In my opinion it's user who need to avoid scientific notation, and if he's ok with it it need to created the correct pattern.

A (string) cast of a float value may lead to the scientific notation regardless.

Yeah - I get that concern, but there's already an implicit cast to string during preg_match anyhow. I think the only way to "fix" this is to reject int|float up-front and I don't think we can get away with that due to existing behaviour.

Perhaps a note should be added to remove the int|float checks for the next major and fail non-string input, making it the callers responsibility to cast if necessary, leaving the current implementation broken in the same predictable way.

My understanding is that this patch does nothing to change the existing behaviour, i.e:

preg_match('/^\d{1}\.\d{2}$/', 0.01); // 1 preg_match('/^\d{1}\.\d{2}$/', '0.01'); // 1 preg_match('/^\d{1}\.\d{2}$/', 35 + -34.99); // 0 preg_match('/^\d{1}\.\d{2}$/', '35' + '-34.99'); // 0

If it's currently broken because there is no parameter type hint then I don't think we can "fix" it. But we CAN prevent people from making the mistake if they're intending to use static analysis.

I was thinking we use @param string|int instead of @param string|int|float more than any specific handling using is_float. Though now that consider I @gsteel's position, I guess validators will always be required to accept a value of mixed type, so should it be throwing an error (by setting a parameter type hint in a future major), or should it just be returning false?

If the latter then it should stay as mixed and then if(!is_string($value) && !is_int($value)) { return false; }?

Beware that the interface does indeed say mixed:

laminas-validator/src/ValidatorInterface.php

Lines 22 to 26 in b904e5d

* @param mixed $value

* @return bool

* @throws Exception\RuntimeException If validation of $value is impossible.

*/

public function isValid($value);

Signed-off-by: Andrei <[email protected]>

Ocramius

LGTM, although asking for another review by others.

internalsystemerror

#179 (comment) is my only reservation. But if people think that's overcautious then I'm happy to approve.

gsteel · 2023-03-02T21:29:40Z

src/Regex.php

@@ -107,7 +107,7 @@ public function setPattern($pattern)
    /**
     * Returns true if and only if $value matches against the pattern option
     *
-     * @param  string $value
+     * @param  string|numeric $value


IMO, isValid() should always accept mixed as per the interface.

gsteel · 2023-03-02T21:41:42Z

src/Regex.php

+        if (is_int($value) || is_float($value)) {
+            $value = (string) $value;
+        }


I don't think we can restrict the input type of the validator beyond mixed to begin with, so I think that the string cast is necessary to preserve existing behaviour. In hind-sight, the first conditional should have failed on any non-string value, but as it doesn't, at least the cast prevents int|float reaching preg_match

test/RegexTest.php

Signed-off-by: Andrei <[email protected]>

gsteel · 2024-06-25T21:53:14Z

Thanks for your work here @h3nnry - #280 has been merged into 3.0.x, so I think this PR is no longer relevant.

Fix value annotation in isValid method for Regex validator.

2da3eae

Signed-off-by: Andrei <[email protected]>

h3nnry force-pushed the fix-regex-validator-annotation branch from 5be4d5f to 2da3eae Compare February 23, 2023 13:13

Ocramius requested changes Feb 23, 2023

View reviewed changes

Regex validator: cast int|float to string parameter value of isValid …

cee7dc0

…method. Signed-off-by: Andrei <[email protected]>

h3nnry force-pushed the fix-regex-validator-annotation branch from af716b8 to cee7dc0 Compare February 23, 2023 19:08

Ocramius requested changes Feb 24, 2023

View reviewed changes

Added testcases for numbers.

d79db66

Signed-off-by: Andrei <[email protected]>

h3nnry force-pushed the fix-regex-validator-annotation branch from 9868322 to d79db66 Compare March 2, 2023 16:59

Ocramius approved these changes Mar 2, 2023

View reviewed changes

Ocramius added Awaiting Maintainer Response Enhancement labels Mar 2, 2023

Ocramius requested review from gsteel and internalsystemerror March 2, 2023 19:26

Ocramius added this to the 2.31.0 milestone Mar 2, 2023

internalsystemerror reviewed Mar 2, 2023

View reviewed changes

gsteel requested changes Mar 2, 2023

View reviewed changes

Added more testcases for failing.

f75cb50

Signed-off-by: Andrei <[email protected]>

Ocramius assigned gsteel Mar 3, 2023

gsteel assigned Ocramius and unassigned gsteel Mar 3, 2023

gsteel removed this from the 2.31.0 milestone May 19, 2023

gsteel mentioned this pull request Jun 21, 2024

Refactor Regex validator #280

Merged

gsteel closed this Jun 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix value annotation in isValid method for Regex validator. #179

Fix value annotation in isValid method for Regex validator. #179

h3nnry commented Feb 23, 2023

Ocramius Feb 23, 2023

h3nnry Feb 23, 2023

gsteel Mar 2, 2023

h3nnry Mar 3, 2023 •

edited

Loading

Ocramius Feb 24, 2023

h3nnry Mar 2, 2023

internalsystemerror Mar 2, 2023 •

edited

Loading

Ocramius Mar 2, 2023

froschdesign Mar 2, 2023

h3nnry Mar 3, 2023

Ocramius Mar 3, 2023

gsteel Mar 3, 2023

internalsystemerror Mar 3, 2023 •

edited

Loading

Ocramius Mar 3, 2023

Ocramius left a comment

internalsystemerror left a comment •

edited

Loading

gsteel Mar 2, 2023

gsteel Mar 2, 2023

gsteel commented Jun 25, 2024

	* @param mixed $value
	* @return bool
	* @throws Exception\RuntimeException If validation of $value is impossible.
	*/
	public function isValid($value);

Fix value annotation in isValid method for Regex validator. #179

Fix value annotation in isValid method for Regex validator. #179

Conversation

h3nnry commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h3nnry Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

internalsystemerror Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

internalsystemerror Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius left a comment

Choose a reason for hiding this comment

internalsystemerror left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsteel commented Jun 25, 2024

h3nnry Mar 3, 2023 •

edited

Loading

internalsystemerror Mar 2, 2023 •

edited

Loading

internalsystemerror Mar 3, 2023 •

edited

Loading

internalsystemerror left a comment •

edited

Loading