Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [Validation] add required_if rule #9028

Open
wants to merge 22 commits into
base: 4.6
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions system/Language/en/Validation.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
'required' => 'The {field} field is required.',
'required_with' => 'The {field} field is required when {param} is present.',
'required_without' => 'The {field} field is required when {param} is not present.',
'required_if' => 'The {field} field is required when the {param} field has the given value.',
'string' => 'The {field} field must be a valid string.',
'timezone' => 'The {field} field must be a valid timezone.',
'valid_base64' => 'The {field} field must be a valid base64 string.',
Expand Down
41 changes: 41 additions & 0 deletions system/Validation/Rules.php
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,47 @@ public function required_without(
return true;
}

/**
* 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.):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

*
* required_if[normal_option,1,2]
*
* @param string|null $str
* @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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
if ($fieldWithValue === null || $data === []) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw new InvalidArgumentException('You must supply the parameters: field,expected_values, data.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message is not reflecting the correct arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this?

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

}

// Separate fields and expected values
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, noted. i will update later.

$parts = explode(',', $fieldWithValue);
$field = array_shift($parts); // Get field
$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,...');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.");

}

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

// If the value of a field matches one of the expected values, then this field is required
if (in_array($data[$field], $expectedValues, true)) {
// The field to be checked must exist and cannot be empty
return trim($str ?? '') !== '';
}

return true;
}

/**
* The field exists in $data.
*
Expand Down
17 changes: 17 additions & 0 deletions system/Validation/StrictRules/Rules.php
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,23 @@ public function required_without(
return $this->nonStrictRules->required_without($str, $otherFields, $data, $error, $field);
}

/**
* 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.):
*
* required_if[normal_option,1,2]
*
* @param string|null $str
* @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
{
return $this->nonStrictRules->required_if($str, $fieldWithValue, $data);
}

/**
* The field exists in $data.
*
Expand Down
2 changes: 1 addition & 1 deletion system/Validation/Validation.php
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ private function processPermitEmpty($value, array $rules, array $data)
$rule = $match[1];
$param = $match[2];

if (! in_array($rule, ['required_with', 'required_without'], true)) {
if (! in_array($rule, ['required_with', 'required_without', 'required_if'], true)) {
continue;
}

Expand Down
65 changes: 65 additions & 0 deletions tests/system/Validation/RulesTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use CodeIgniter\Test\CIUnitTestCase;
use Config\Services;
use ErrorException;
use Generator;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\Attributes\Group;
use stdClass;
Expand Down Expand Up @@ -923,4 +924,68 @@ public function testFieldExistsErrorMessage(): void
$this->validation->getErrors()
);
}

/**
* @param array<string, mixed> $data
*/
#[DataProvider('provideRequiredIf')]
public function testRequiredIf(bool $expected, array $data): void
{
$this->validation->setRules([
'is_internal' => 'in_list[0,1,2,3]|permit_empty',
'identity_number' => 'required_if[is_internal,1,2]',
]);

$result = $this->validation->run($data);

$this->assertSame($expected, $result);
}

public static function provideRequiredIf(): Generator
{
yield from [
// `is_internal` and `identity_number` do not exist
[true, []],
// `identity_number` is not required because field `is_internal`
// value does not match with any value in the rule params
[true, ['is_internal' => '', 'identity_number' => '']],
[true, ['is_internal' => '0', 'identity_number' => '']],
[true, ['is_internal' => '3', 'identity_number' => '']],
// `identity_number` is required and exist
[false, ['is_internal' => '1', 'identity_number' => '']],
[false, ['is_internal' => '2', 'identity_number' => '']],
// `identity_number` is required but do not exist
[false, ['is_internal' => '1']],
[false, ['is_internal' => '2']],
warcooft marked this conversation as resolved.
Show resolved Hide resolved
];
}

/**
* @param array<string, mixed> $data
*/
#[DataProvider('provideRequiredIfWorkWithOtherRule')]
public function testRequiredIfWorkWithOtherRule(bool $expected, array $data): void
{
$this->validation->setRules([
'is_internal' => 'in_list[0,1,2,3]|permit_empty',
'identity_number' => 'required_if[is_internal,1,2]|permit_empty|integer',
]);

$result = $this->validation->run($data);

$this->assertSame($expected, $result);
}

public static function provideRequiredIfWorkWithOtherRule(): Generator
{
yield from [
// `identity_number` with integer value
[true, ['is_internal' => '1', 'identity_number' => '3207783']],
[true, ['is_internal' => '2', 'identity_number' => '3207783']],
// `identity_number` is not empty, but it is not an integer
// value, which triggers the Validation.integer error message.
[false, ['is_internal' => '1', 'identity_number' => 'a']],
[false, ['is_internal' => '2', 'identity_number' => 'b']],
warcooft marked this conversation as resolved.
Show resolved Hide resolved
];
}
}
6 changes: 4 additions & 2 deletions user_guide_src/source/changelogs/v4.6.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,9 @@ Libraries

- **FileCollection:** Added ``retainMultiplePatterns()`` to ``FileCollection`` class.
See :ref:`FileCollection::retainMultiplePatterns() <file-collections-retain-multiple-patterns>`.
- **Validation:** Added ``min_dims`` validation rule to ``FileRules`` class. See
:ref:`Validation <rules-for-file-uploads>`.
- **Validation:**
- Added ``min_dims`` validation rule to ``FileRules`` class. See :ref:`Validation <rules-for-file-uploads>`.
- Added ``required_if`` validation rule to ``Rules`` class. See :ref:`Validation <rules-for-general-use>`.

Helpers and Functions
=====================
Expand All @@ -165,6 +166,7 @@ Message Changes
***************

- Added ``Validation.min_dims`` message
- Added ``Validation.required_if`` message

*******
Changes
Expand Down
5 changes: 4 additions & 1 deletion user_guide_src/source/libraries/validation.rst
Original file line number Diff line number Diff line change
Expand Up @@ -972,10 +972,13 @@ regex_match Yes Fails if field does not match the regular
expression.
required No Fails if the field is an empty array, empty
string, null or false.
required_if Yes The field is required when any of the other ``required_if[field,value1,value2,...]``
field has the given value. (This rule was
added in v4.6.0.)
kenjis marked this conversation as resolved.
Show resolved Hide resolved
required_with Yes The field is required when any of the other ``required_with[field1,field2]``
fields is not `empty()`_ in the data.
required_without Yes The field is required when any of the other ``required_without[field1,field2]``
fields is `empty()`_ in the data.
fields is `empty()`_ in the data.
warcooft marked this conversation as resolved.
Show resolved Hide resolved
string No A generic alternative to the **alpha*** rules
that confirms the element is a string
timezone No Fails if field does not match a timezone
Expand Down
Loading