Unexpected output: Affected paths: relative - resubmit patch.
⚙ D243 INP-1571 Form field validation should account for array-like field values
Page MenuHomeIn-Portal Phabricator
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

  1. 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');
  1. in the users_config php remove type key from FirstName field configuration (to bypass type validation)
  2. 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

  1. login to Admin Console
  2. go to User Management → Users section
  3. press Add toolbar button
  4. press Save toolbar button
  5. confirm that strlen() expects parameter 1 to be string, array given error or similar is reported in Debugger
  6. confirm that Valid group virtual fields doesn't report any errors in Debugger
  7. 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

  1. login to Admin Console
  2. go to User Management → Users section
  3. press Add toolbar button
  4. press Save toolbar button
  5. 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 ErrorsExcuse: Full file re-formatting is not part of this task.
SeverityLocationCodeMessage
Errorcore/kernel/utility/validator.php:237PHPCS.E.CodingStandard.Commenting.InlineComment.InvalidEndCharCodingStandard.Commenting.InlineComment.InvalidEndChar
Errorcore/kernel/utility/validator.php:237PHPCS.E.CodingStandard.Commenting.InlineComment.NotCapitalCodingStandard.Commenting.InlineComment.NotCapital
Unit
No Unit Test Coverage
Build Status
Buildable 458
Build 458: 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.
alex requested changes to this revision.Nov 20 2016, 1:24 PM
alex edited edge metadata.

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
237

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)
erik updated this revision to Diff 654.Nov 30 2016, 5:19 AM
erik edited edge metadata.
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
  1. if this isn't a scalar value or value is empty, then exit immediately
  2. remove any empty value length checking code in the method (found 2 places)

refactoring

259–269
  1. move this inside a previous if statement (when we've detected, that it's a number)
  2. instead of return false; inside if ( !$res ) { IF always return $res;

prevents string-validation rules to be apply to a valid number

289–290

remove

see above

293

replace with return $res;

see above

erik updated this revision to Diff 867.Sep 27 2018, 6:33 AM
erik edited edge metadata.

Done refactoring.

erik added a comment.Sep 27 2018, 6:37 AM

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.

alex added a comment.Sep 27 2018, 7:20 AM
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?

erik added a comment.Sep 27 2018, 7:35 AM

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

alex requested changes to this revision.Jul 19 2022, 7:51 AM
  1. 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.

  1. call settype function inside kValidator::ValidateType only, when is_scalar($val) returns true
  2. 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
erik updated this revision to Diff 1065.Jul 19 2022, 9:53 AM
erik edited the test plan for this revision. (Show Details)

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

Revision Contents
Changeset List

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 159416382Aug 10 2016, 6:24 AM
★
★
Diff 265416502Fixed CS errors.Nov 30 2016, 5:19 AM
★
★
Diff 386716632Done refactoring.Sep 27 2018, 6:33 AM
★
★
Diff 4106516694Added code to avoid "Array to string conversion" notice. Added related test…Jul 19 2022, 9:53 AM
★
★

Diff 594

View Options

core/kernel/utility/validator.php

Loading...
In-Portal CMS · In-Portal Community Pages · In-Portal Issue Tracker · In-Portal Repository Browser · Quality Assurance · In-Portal CI Server