From 84ac2ea71f38d2428f1b52eca91e3cf4e09930a9 Mon Sep 17 00:00:00 2001 From: "John Paul E. Balandan, CPA" Date: Wed, 5 Jan 2022 22:19:53 +0800 Subject: [PATCH] Validation: support placeholders for anything --- system/Validation/Validation.php | 71 +++--- tests/system/Validation/ValidationTest.php | 204 +++++++++++++++++- user_guide_src/source/changelogs/v4.2.0.rst | 1 + .../source/libraries/validation.rst | 20 +- 4 files changed, 263 insertions(+), 33 deletions(-) diff --git a/system/Validation/Validation.php b/system/Validation/Validation.php index 031d8f51a398..1060527cb689 100644 --- a/system/Validation/Validation.php +++ b/system/Validation/Validation.php @@ -17,6 +17,7 @@ use CodeIgniter\View\RendererInterface; use Config\Validation as ValidationConfig; use InvalidArgumentException; +use TypeError; /** * Validator @@ -362,18 +363,30 @@ public function withRequest(RequestInterface $request): ValidationInterface * 'rule' => 'message' * ] * + * @param array|string $rules + * + * @throws TypeError + * * @return $this */ - public function setRule(string $field, ?string $label, string $rules, array $errors = []) + public function setRule(string $field, ?string $label, $rules, array $errors = []) { - $this->rules[$field] = [ - 'label' => $label, - 'rules' => $rules, + if (! is_array($rules) && ! is_string($rules)) { + throw new TypeError('$rules must be of type string|array'); + } + + $ruleSet = [ + $field => [ + 'label' => $label, + 'rules' => $rules, + ], ]; - $this->customErrors = array_merge($this->customErrors, [ - $field => $errors, - ]); + if ($errors) { + $ruleSet[$field]['errors'] = $errors; + } + + $this->setRules($ruleSet + $this->getRules()); return $this; } @@ -401,16 +414,18 @@ public function setRules(array $rules, array $errors = []): ValidationInterface $this->customErrors = $errors; foreach ($rules as $field => &$rule) { - if (! is_array($rule)) { - continue; - } + if (is_array($rule)) { + if (array_key_exists('errors', $rule)) { + $this->customErrors[$field] = $rule['errors']; + unset($rule['errors']); + } - if (! array_key_exists('errors', $rule)) { - continue; + // if $rule is already a rule collection, just move it to "rules" + // transforming [foo => [required, foobar]] to [foo => [rules => [required, foobar]]] + if (! array_key_exists('rules', $rule)) { + $rule = ['rules' => $rule]; + } } - - $this->customErrors[$field] = $rule['errors']; - unset($rule['errors']); } $this->rules = $rules; @@ -581,23 +596,27 @@ protected function fillPlaceholders(array $rules, array $data): array $replacements["{{$key}}"] = $value; } - if (! empty($replacements)) { + if ($replacements !== []) { foreach ($rules as &$rule) { - if (is_array($rule)) { - foreach ($rule as &$row) { - // Should only be an `errors` array - // which doesn't take placeholders. - if (is_array($row)) { - continue; - } + $ruleSet = $rule['rules'] ?? $rule; - $row = strtr($row ?? '', $replacements); + if (is_array($ruleSet)) { + foreach ($ruleSet as &$row) { + if (is_string($row)) { + $row = strtr($row, $replacements); + } } + } - continue; + if (is_string($ruleSet)) { + $ruleSet = strtr($ruleSet, $replacements); } - $rule = strtr($rule ?? '', $replacements); + if (isset($rule['rules'])) { + $rule['rules'] = $ruleSet; + } else { + $rule = $ruleSet; + } } } diff --git a/tests/system/Validation/ValidationTest.php b/tests/system/Validation/ValidationTest.php index dbc1c03e4b50..fa5e769411b0 100644 --- a/tests/system/Validation/ValidationTest.php +++ b/tests/system/Validation/ValidationTest.php @@ -19,7 +19,9 @@ use Config\App; use Config\Services; use Generator; +use PHPUnit\Framework\ExpectationFailedException; use Tests\Support\Validation\TestRules; +use TypeError; /** * @internal @@ -83,7 +85,109 @@ public function testSetRulesStoresRules(): void $this->assertSame($rules, $this->validation->getRules()); } - public function testRunReturnsFalseWithNothingToDo(): void + public function testSetRuleStoresRule() + { + $this->validation->setRules([]); + $this->validation->setRule('foo', null, 'bar|baz'); + + $this->assertSame([ + 'foo' => [ + 'label' => null, + 'rules' => 'bar|baz', + ], + ], $this->validation->getRules()); + } + + public function testSetRuleAddsRule() + { + $this->validation->setRules([ + 'bar' => [ + 'label' => null, + 'rules' => 'bar|baz', + ], + ]); + $this->validation->setRule('foo', null, 'foo|foz'); + + $this->assertSame([ + 'foo' => [ + 'label' => null, + 'rules' => 'foo|foz', + ], + 'bar' => [ + 'label' => null, + 'rules' => 'bar|baz', + ], + ], $this->validation->getRules()); + } + + public function testSetRuleOverwritesRule() + { + $this->validation->setRules([ + 'foo' => [ + 'label' => null, + 'rules' => 'bar|baz', + ], + ]); + $this->validation->setRule('foo', null, 'foo|foz'); + + $this->assertSame([ + 'foo' => [ + 'label' => null, + 'rules' => 'foo|foz', + ], + ], $this->validation->getRules()); + } + + /** + * @dataProvider setRuleRulesFormatCaseProvider + * + * @param mixed $rules + */ + public function testSetRuleRulesFormat(bool $expected, $rules): void + { + if (! $expected) { + $this->expectException(TypeError::class); + $this->expectExceptionMessage('$rules must be of type string|array'); + } + + $this->validation->setRule('foo', null, $rules); + $this->addToAssertionCount(1); + } + + public function setRuleRulesFormatCaseProvider(): iterable + { + yield 'fail-simple-object' => [ + false, + (object) ['required'], + ]; + + yield 'pass-single-string' => [ + true, + 'required', + ]; + + yield 'pass-single-array' => [ + true, + ['required'], + ]; + + yield 'fail-deep-object' => [ + false, + new Validation((object) $this->config, Services::renderer()), + ]; + + yield 'pass-multiple-string' => [ + true, + 'required|alpha', + ]; + + yield 'pass-multiple-array' => [ + true, + ['required', 'alpha'], + ]; + } + + public function testRunReturnsFalseWithNothingToDo() { $this->validation->setRules([]); $this->assertFalse($this->validation->run([])); @@ -1008,4 +1112,102 @@ public function provideStringRulesCases(): iterable ['required', 'regex_match[/^(01|2689|09)[0-9]{8}$/]', 'numeric'], ]; } + + /** + * internal method to simplify placeholder replacement test + * REQUIRES THE RULES TO BE SET FOR THE FIELD "foo" + * + * @param array|null $data optional POST data, needs to contain the key $placeholderField to pass + * + * @source https://github.com/codeigniter4/CodeIgniter4/pull/3910#issuecomment-784922913 + */ + private function placeholderReplacementResultDetermination(string $placeholder = 'id', ?array $data = null) + { + if ($data === null) { + $data = [$placeholder => 'placeholder-value']; + } + + $validationRules = $this->getPrivateMethodInvoker($this->validation, 'fillPlaceholders')($this->validation->getRules(), $data); + $fieldRules = $validationRules['foo']['rules'] ?? $validationRules['foo']; + if (is_string($fieldRules)) { + $fieldRules = $this->getPrivateMethodInvoker($this->validation, 'splitRules')($fieldRules); + } + + // loop all rules for this field + foreach ($fieldRules as $rule) { + // only string type rules are supported + if (is_string($rule)) { + $this->assertStringNotContainsString('{' . $placeholder . '}', $rule); + } + } + } + + /** + * @see ValidationTest::placeholderReplacementResultDetermination() + */ + public function testPlaceholderReplacementTestFails() + { + // to test if placeholderReplacementResultDetermination() works we provoke and expect an exception + $this->expectException(ExpectationFailedException::class); + $this->expectExceptionMessage('Failed asserting that \'filter[{id}]\' does not contain "{id}".'); + + $this->validation->setRule('foo', 'foo-label', 'required|filter[{id}]'); + + // calling with empty $data should produce an exception since {id} can't be replaced + $this->placeholderReplacementResultDetermination('id', []); + } + + public function testPlaceholderReplacementSetSingleRuleString() + { + $this->validation->setRule('foo', null, 'required|filter[{id}]'); + + $this->placeholderReplacementResultDetermination(); + } + + public function testPlaceholderReplacementSetSingleRuleArray() + { + $this->validation->setRule('foo', null, ['required', 'filter[{id}]']); + + $this->placeholderReplacementResultDetermination(); + } + + public function testPlaceholderReplacementSetMultipleRulesSimpleString() + { + $this->validation->setRules([ + 'foo' => 'required|filter[{id}]', + ]); + + $this->placeholderReplacementResultDetermination(); + } + + public function testPlaceholderReplacementSetMultipleRulesSimpleArray() + { + $this->validation->setRules([ + 'foo' => ['required', 'filter[{id}]'], + ]); + + $this->placeholderReplacementResultDetermination(); + } + + public function testPlaceholderReplacementSetMultipleRulesComplexString() + { + $this->validation->setRules([ + 'foo' => [ + 'rules' => 'required|filter[{id}]', + ], + ]); + + $this->placeholderReplacementResultDetermination(); + } + + public function testPlaceholderReplacementSetMultipleRulesComplexArray() + { + $this->validation->setRules([ + 'foo' => [ + 'rules' => ['required', 'filter[{id}]'], + ], + ]); + + $this->placeholderReplacementResultDetermination(); + } } diff --git a/user_guide_src/source/changelogs/v4.2.0.rst b/user_guide_src/source/changelogs/v4.2.0.rst index 7556b28f8608..67934fae25da 100644 --- a/user_guide_src/source/changelogs/v4.2.0.rst +++ b/user_guide_src/source/changelogs/v4.2.0.rst @@ -12,6 +12,7 @@ Release Date: Not Released BREAKING ******** +- The method signature of ``Validation::setRule()`` has been changed. The ``string`` typehint on the ``$rules`` parameter was removed. Extending classes should likewise remove the parameter so as not to break LSP. Enhancements diff --git a/user_guide_src/source/libraries/validation.rst b/user_guide_src/source/libraries/validation.rst index 3c9d2a16a022..00cf3177a493 100644 --- a/user_guide_src/source/libraries/validation.rst +++ b/user_guide_src/source/libraries/validation.rst @@ -261,20 +261,28 @@ methods. setRule() ========= -This method sets a single rule. It takes the name of the field as -the first parameter, an optional label and a string with a pipe-delimited list of rules -that should be applied:: +This method sets a single rule. It has the method signature - $validation->setRule('username', 'Username', 'required'); + setRule(string $field, ?string $label, array|string $rules[, array $errors = []]) -The **field name** must match the key of any data array that is sent in. If +The ``$rules`` either takes in a pipe-delimited list of rules or an array collection of rules. + + $validation->setRule('username', 'Username', 'required|min_length[3]'); + $validation->setRule('password', 'Password', ['required', 'min_length[8]', 'alpha_numeric_punct']); + +The value you pass to ``$field`` must match the key of any data array that is sent in. If the data is taken directly from ``$_POST``, then it must be an exact match for the form input name. +.. warning:: Prior to v4.2.0, this method's third parameter, ``$rules``, was typehinted to accept + ``string``. In v4.2.0 and after, the typehint was removed to allow arrays, too. To avoid LSP being + broken in extending classes overriding this method, the child class's method should also be modified + to remove the typehint. + setRules() ========== -Like, ``setRule()``, but accepts an array of field names and their rules:: +Like ``setRule()``, but accepts an array of field names and their rules:: $validation->setRules([ 'username' => 'required',