feat: [Validation] add `required_if` rule #9028

warcooft · 2024-07-07T05:25:06Z

Description
Supersedes #9018

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

…ray.

paulbalandan · 2024-07-09T05:24:18Z

system/Validation/Rules.php

+     */
+    public function required_if($str = null, ?string $fieldWithValue = null, array $data = []): bool
+    {
+        if ($fieldWithValue === null || $data === []) {


When will data be filled up? Using your original example: ['identity_number' => 'required_if[is_internal,1]', $fieldWithValue is is_internal,1.

The $data is filled out automatically when the rule is called.

paulbalandan · 2024-07-09T05:25:06Z

system/Validation/Rules.php

+     * @param string|null          $fieldWithValue that we should check if present
+     * @param array<string, mixed> $data           Complete list of field from the form
+     */
+    public function required_if($str = null, ?string $fieldWithValue = null, array $data = []): bool


Do we need the nullability here for both $str and $fieldWithValue?

It looks like yes. The $str can be null when the value does not come from the form. And $fieldWithValue can be null when the user skips the parameter and enters only: required_if.

paulbalandan · 2024-07-09T05:25:41Z

system/Validation/Rules.php

+    public function required_if($str = null, ?string $fieldWithValue = null, array $data = []): bool
+    {
+        if ($fieldWithValue === null || $data === []) {
+            throw new InvalidArgumentException('You must supply the parameters: field,expected_values, data.');


The message is not reflecting the correct arguments.

How about this?

throw new InvalidArgumentException('You must supply the parameters: field,value1,value2,...');

IMO better.

paulbalandan · 2024-07-09T05:29:50Z

system/Validation/Rules.php

+            throw new InvalidArgumentException('You must supply the parameters: field,expected_values, data.');
+        }
+
+        // Separate fields and expected values


Please refrain from adding comments that just describes what the code is doing. They're superfluous.

OK, noted. i will update later.

paulbalandan · 2024-07-09T05:30:58Z

system/Validation/Rules.php

+        $expectedValues = $parts; // The remainder is the expected value
+
+        if (trim($field) === '' || $expectedValues === []) {
+            throw new InvalidArgumentException('You must supply the expected values of field: E.g. field,value1,value2,...');


It may be better to have a more descriptive exception message by saying which field needs to be supplied with the expected values.

how about this one?

throw new InvalidArgumentException("You must supply the field value: {$field},value? or you can add multiple value like this {$field},value1,value2 and so on.");

paulbalandan · 2024-07-09T05:33:41Z

system/Validation/Rules.php

+     * This field is required when any of the other required fields have their expected values present
+     * in the data.
+     *
+     * Example (The special_option field is required when the normal_option,1,2 field has the given value.):


This error message is confusing:

The special_option field is required when the normal_option,1,2 field has the given value.

The other field is normal_option, not normal_option,1,2.

I thought about that too, but the fields and values are combined in the {param} placeholder. How to extract fields and values from {param}? At least we can separate them and differentiate between fields and values then provide proper error messages. do you have any ideas?

tests/system/Validation/RulesTest.php

user_guide_src/source/libraries/validation.rst

Co-authored-by: kenjis <[email protected]>

github-actions · 2024-12-02T06:18:57Z

👋 Hi, @warcooft!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

michalsn

Hey @warcooft - if you are still interested in this PR, it needs to be rebased.

I added some comments.

michalsn · 2024-12-21T17:42:48Z

system/Validation/Rules.php

+     * This field is required when any of the other required fields have their expected values present
+     * in the data.
+     *
+     * Example (The special_option field is required when the normal_option,1,2 field has the given value.):


Suggested change

* Example (The special_option field is required when the normal_option,1,2 field has the given value.):

* Example (The special_option field is required when the normal_option field has value 1 or 2.):

michalsn · 2024-12-21T17:51:37Z

system/Validation/Rules.php

+     * @param string|null          $fieldWithValue that we should check if present
+     * @param array<string, mixed> $data           Complete list of field from the form
+     */
+    public function required_if($str = null, ?string $fieldWithValue = null, array $data = []): bool


It looks like yes. The $str can be null when the value does not come from the form. And $fieldWithValue can be null when the user skips the parameter and enters only: required_if.

michalsn · 2024-12-21T18:11:54Z

system/Validation/Rules.php

+     */
+    public function required_if($str = null, ?string $fieldWithValue = null, array $data = []): bool
+    {
+        if ($fieldWithValue === null || $data === []) {


The $data is filled out automatically when the rule is called.

michalsn · 2024-12-21T18:14:08Z

system/Validation/Rules.php

+    public function required_if($str = null, ?string $fieldWithValue = null, array $data = []): bool
+    {
+        if ($fieldWithValue === null || $data === []) {
+            throw new InvalidArgumentException('You must supply the parameters: field,expected_values, data.');


IMO better.

michalsn · 2024-12-21T18:27:48Z

system/Validation/Rules.php

+        }
+
+        // If the field does not exist in the data, immediately return true
+        if (! array_key_exists($field, $data)) {


We should also add support for fields that are nested via dot_array_search().

warcooft added 8 commits July 7, 2024 11:35

add required_if rule

8f86617

add required_if strict rule

195bacf

adjust processPermitEmpty to work with the required_if rule

82954bf

add translation for required_if rule

20f3b4b

docs: add changelog

62737f1

docs: add required_if to rules for general use table

db6d923

remove whitespace

1407006

remove whitespace

de666cc

kenjis added enhancement PRs that improve existing functionalities 4.6 tests needed Pull requests that need tests labels Jul 7, 2024

warcooft added 6 commits July 7, 2024 14:01

test: add test cases

95bf9b5

test: add test cases with other rules

8cde4a0

update comment

5865971

remove whitespace by PHP CS Fixer

6e7f7cf

fix: TypeError in strict mode, string dataType required but null given

e127ddd

fix: parameter $data with no value type specified in iterable type ar…

c5c9578

…ray.

paulbalandan requested changes Jul 9, 2024

View reviewed changes

warcooft added 4 commits July 9, 2024 14:02

fix: return type has no value type specified in iterable type iterable.

6980b32

make Lint with PHP CS Fixer happy

1215f36

update: add missing Generator class

97eb7fe

fix: Generator class order

4ef7b2a

kenjis reviewed Jul 10, 2024

View reviewed changes

tests/system/Validation/RulesTest.php Outdated Show resolved Hide resolved

kenjis reviewed Jul 10, 2024

View reviewed changes

tests/system/Validation/RulesTest.php Show resolved Hide resolved

kenjis reviewed Jul 10, 2024

View reviewed changes

user_guide_src/source/libraries/validation.rst Outdated Show resolved Hide resolved

kenjis reviewed Jul 10, 2024

View reviewed changes

user_guide_src/source/libraries/validation.rst Outdated Show resolved Hide resolved

warcooft and others added 2 commits July 10, 2024 10:05

add more test cases

08dd975

Co-authored-by: kenjis <[email protected]>

docs: update description

afed8fb

kenjis removed the tests needed Pull requests that need tests label Jul 11, 2024

Merge branch '4.6' into feat-validation-required_if

c3df1b0

Update v4.6.0.rst

a3056c5

github-actions bot added the stale Pull requests with conflicts label Dec 2, 2024

michalsn reviewed Dec 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [Validation] add `required_if` rule #9028

feat: [Validation] add `required_if` rule #9028

warcooft commented Jul 7, 2024 •

edited

Loading

paulbalandan Jul 9, 2024

michalsn Dec 21, 2024

paulbalandan Jul 9, 2024

michalsn Dec 21, 2024

paulbalandan Jul 9, 2024

warcooft Jul 9, 2024

michalsn Dec 21, 2024

paulbalandan Jul 9, 2024

warcooft Jul 9, 2024

paulbalandan Jul 9, 2024

warcooft Jul 9, 2024

paulbalandan Jul 9, 2024

warcooft Jul 9, 2024

github-actions bot commented Dec 2, 2024

michalsn left a comment

michalsn Dec 21, 2024

michalsn Dec 21, 2024

michalsn Dec 21, 2024

michalsn Dec 21, 2024

michalsn Dec 21, 2024

	* Example (The special_option field is required when the normal_option,1,2 field has the given value.):
	* Example (The special_option field is required when the normal_option field has value 1 or 2.):

feat: [Validation] add required_if rule #9028

Are you sure you want to change the base?

feat: [Validation] add required_if rule #9028

Conversation

warcooft commented Jul 7, 2024 • 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

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

Choose a reason for hiding this comment

github-actions bot commented Dec 2, 2024

michalsn left a 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

Choose a reason for hiding this comment

feat: [Validation] add `required_if` rule #9028

feat: [Validation] add `required_if` rule #9028

warcooft commented Jul 7, 2024 •

edited

Loading