Differential D243

INP-1571 Form field validation should account for array-like field values
Needs ReviewPublic
Actions

Authored by erik on Aug 10 2016, 6:24 AM.

Details

Reviewers

alex

Summary

Used array-safe string length detection.

Test Plan

Change system code

in the u:OnBeforeItemCreate event, after $object variable was initialized, add these lines:

$object->SetDBField('FirstName', array(1,2,3));
$object->SetDBField('MaxValueInc0Valid', 0);
$object->SetDBField('MinValueInc0Valid', 0);
$object->SetDBField('MaxValueExc0Valid', -5);
$object->SetDBField('MinValueExc0Valid', 5);
$object->SetDBField('MaxLen2Valid', '1');
$object->SetDBField('MinLen2Valid', '22');
$object->SetDBField('MaxValueInc0InValid', 5);
$object->SetDBField('MinValueInc0InValid', -5);
$object->SetDBField('MaxValueExc0InValid', 0);
$object->SetDBField('MinValueExc0InValid', 0);
$object->SetDBField('MaxLen2InValid', '333');
$object->SetDBField('MinLen2InValid', '1');

in the users_config php remove type key from FirstName field configuration (to bypass type validation)
in the users_config php add elements to VirtualFields array:

'MaxValueInc0Valid' => array('type' => 'int', 'max_value_inc' => 0),
'MinValueInc0Valid' => array('type' => 'int', 'min_value_inc' => 0),
'MaxValueExc0Valid' => array('type' => 'int', 'max_value_exc' => 0),
'MinValueExc0Valid' => array('type' => 'int', 'min_value_exc' => 0),
'MaxLen1Valid' => array('type' => 'string', 'max_len' => 1),
'MinLen1Valid' => array('type' => 'string', 'min_len' => 1),
'MaxValueInc0InValid' => array('type' => 'int', 'max_value_inc' => 0),
'MinValueInc0InValid' => array('type' => 'int', 'min_value_inc' => 0),
'MaxValueExc0InValid' => array('type' => 'int', 'max_value_exc' => 0),
'MinValueExc0InValid' => array('type' => 'int', 'min_value_exc' => 0),
'MaxLen2InValid' => array('type' => 'string', 'max_len' => 2),
'MinLen2InValid' => array('type' => 'string', 'min_len' => 2),

Testing

login to Admin Console
go to User Management → Users section
press Add toolbar button
press Save toolbar button
confirm that strlen() expects parameter 1 to be string, array given error or similar is reported in Debugger
confirm that Valid group virtual fields doesn't report any errors in Debugger
confirm that all six value_out_of_range errors from InValid group virtual fields report errors in Debugger

Change system code

In the users_config php return type key for FirstName field configuration.

Testing

login to Admin Console
go to User Management → Users section
press Add toolbar button
press Save toolbar button
confirm that Array to string conversion notice is not reported in Debugger

Diff Detail

Repository

rINP In-Portal

Branch

/in-portal/branches/5.2.x

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 887
Build 887: arc lint + arc unit

Event Timeline

erik updated this revision to Diff 594.Aug 10 2016, 6:24 AM

erik retitled this revision from to INP-1571 Form field validation should account for array-like field values.

erik updated this object.

erik edited the test plan for this revision. (Show Details)

erik added a reviewer: alex.

erik added 1 JIRA issue(s): INP-1571.

All length checking code was changed and therefore test plan must include testing of all existing behavior (regression testing), not just the part that was fixed. I guess you can create field declaration with all possible validation options and just confirm, that:

when array is passed in no validation error happens
when invalid scalar value (maybe several values needs to be used to match each of validation options) is passed the validation error happens
when valid scalar value (maybe several values needs to be used to match each of validation options) is passed no validation error happens

core/kernel/utility/validator.php
232–233	In fact change `// validate number` into `// Validate number.` and this should fix all CS errors.

This revision now requires changes to proceed.Nov 20 2016, 1:24 PM

erik edited the test plan for this revision. (Show Details)Nov 30 2016, 4:55 AM

erik edited edge metadata.

erik edited the test plan for this revision. (Show Details)

erik edited the test plan for this revision. (Show Details)Nov 30 2016, 5:01 AM

erik edited the test plan for this revision. (Show Details)

Fixed CS errors.

alex edited the test plan for this revision. (Show Details)Sep 27 2018, 3:51 AM

This revision now requires changes to proceed.Sep 27 2018, 3:51 AM

alex requested changes to this revision.Sep 27 2018, 5:10 AM

alex added inline comments.

core/kernel/utility/validator.php
231	if this isn't a scalar value or value is empty, then exit immediately remove any empty value length checking code in the method (found 2 places) refactoring
258–275	move this inside a previous if statement (when we've detected, that it's a number) instead of `return false;` inside `if ( !$res ) {` IF always `return $res;` prevents string-validation rules to be apply to a valid number
292–293	remove see above
295	replace with `return $res;` see above

Done refactoring.

No strlen error in debugger from changed ValidateRange method. 'FirstName' test value detected as invalid in other validation method (ValidateType) and not passed to ValidateRange method.

In D243#6845, @erik wrote:

No strlen error in debugger from changed ValidateRange method. 'FirstName' test value detected as invalid in other validation method (ValidateType) and not passed to ValidateRange method.

@erik, was this started to happen only after refactoring you've performed?

No, this error is not related to current task changes.

Please keep the current test plan and add another test plan part covering original scenario from associated Confluence discussion:

When performing checkout (add product to cart > enter shipping info > enter billing info > preview order > place order) in In-Commerce on Front-End, then following warning happens, when going from shipping to billing step:
Warning (#3): strlen() expects parameter 1 to be string, array given in ...\core\kernel\utility\validator.php on line 267
In some cases (and shipping step is that case) form can contain non-scalar type fields (e.g. arrays). When these fields are checked with "strlen" (or any other string-only function), then this results in above displayed warning.

call settype function inside kValidator::ValidateType only, when is_scalar($val) returns true
update test plan to reflect above made code changes

The current test plan doesn't cause the kValidator::ValidateRange method execution with a non-scalar (e.g. array) value.

This revision now requires changes to proceed.Jul 19 2022, 7:51 AM

erik edited the test plan for this revision. (Show Details)Jul 19 2022, 9:46 AM

Added code to avoid "Array to string conversion" notice. Added related test plan part.

Harbormaster completed remote builds in B1095: Diff 1065.Jul 19 2022, 9:53 AM

Revision Contents
Changeset List

			Path	Packages
M			core/kernel/utility/validator.php (54 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	594	16382		Aug 10 2016, 6:24 AM	★	★
Diff 2	654	16502	Fixed CS errors.	Nov 30 2016, 5:19 AM	★	★
Diff 3	867	16632	Done refactoring.	Sep 27 2018, 6:33 AM	★	★
Diff 4	1065	16694	Added code to avoid "Array to string conversion" notice. Added related test…	Jul 19 2022, 9:53 AM	★	★

Commit	Parents	Author	Summary	Date
#fake-hash			Ignore it :) (Show More…)

Diff 867

View Options

core/kernel/utility/validator.php

INP-1571 Form field validation should account for array-like field valuesNeeds ReviewPublicActions

Details

Change system code

Testing

Change system code

Testing

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 867

core/kernel/utility/validator.php

INP-1571 Form field validation should account for array-like field values
Needs ReviewPublic
Actions

Revision Contents
Changeset List