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

Conversation

warcooft
Copy link
Contributor

@warcooft warcooft commented Jul 7, 2024

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

@kenjis kenjis added enhancement PRs that improve existing functionalities 4.6 tests needed Pull requests that need tests labels Jul 7, 2024
*/
public function required_if($str = null, ?string $fieldWithValue = null, array $data = []): bool
{
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.

* @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?

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.');
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,...');

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

// 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.

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

* 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?

@kenjis kenjis removed the tests needed Pull requests that need tests label Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.6 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants