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

fix: required_without rule logic in Validation class. #6589

Merged
Merged
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
modify: put the codition that don't have dot in the needed area.
ping-yee committed Sep 29, 2022

Verified

This commit was signed with the committer’s verified signature. The key has expired.
mmarchini mary marchini
commit 3ea0c6a62a89047e68d036592ac9199a40a81663
18 changes: 10 additions & 8 deletions system/Validation/Rules.php
Original file line number Diff line number Diff line change
@@ -282,8 +282,8 @@ public function required_with($str = null, ?string $fields = null, array $data =
*/
public function required_without($str = null, ?string $fields = null, array $data = [], ?string $error = null, ?string $keyField = null): bool
kenjis marked this conversation as resolved.
Show resolved Hide resolved
{
if ($fields === null || empty($data) || $keyField === null) {
throw new InvalidArgumentException('You must supply the parameters: fields, data, keyField.');
if ($fields === null || empty($data)) {
throw new InvalidArgumentException('You must supply the parameters: fields, data');
kenjis marked this conversation as resolved.
Show resolved Hide resolved
}

// If the field is present we can safely assume that
@@ -298,19 +298,21 @@ public function required_without($str = null, ?string $fields = null, array $dat

$nowKeyField = 0;
Copy link
Member

Choose a reason for hiding this comment

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

What is the implication of the $now? We have a $keyField then $nowKeyField. Then we have a $nowField and $nowFieldValue

Copy link
Contributor Author

@ping-yee ping-yee Sep 26, 2022

Choose a reason for hiding this comment

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

 $this->validation->setRules([
    'a.*.c' => 'required_without[a.*.b]',
])->run($data);

Like this scenario, the implication of these name I set show as below .

a.*.c => key

$keyField => the key field is being passed in now (like: a.0.c, a.1.c).
$nowKeyField => the index of key field is being passed in now (like: 1 of a.1.c, 2 of a.2.c).

required_without[a.*.b] => the param field of required_without[]

$nowField => the field of required_without is changed from asterisk field to current field (like: a.*.b => a.0.b, a.1.b...etc).
$nowFieldValue => the value of current field in data set.

I think the name of these variables aren't the best, or are there more suitable names?

Copy link
Member

Choose a reason for hiding this comment

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

I honestly don't understand what the variable names mean. Variable names need to be changed.

Copy link
Contributor Author

@ping-yee ping-yee Sep 28, 2022

Choose a reason for hiding this comment

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

Variable names need to be changed.

I agree with you, do you guys have any suggestion for these variable names?

Copy link
Member

@kenjis kenjis Sep 28, 2022

Choose a reason for hiding this comment

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

How about this?

    /**
     * The field is required when any of other fields do not pass `required` checks.
     *
     * Example (field is required when the id or email field is missing):
     *     required_without[id,email]
     *
     * @param string|null $value       The value to check
     * @param string|null $otherFields The rule parameter fields. List of other fields to check if present
     * @param array       $data        All data to validate
     * @param string|null $field       The field name to check. If the rule parameter fields aren't present, this field is required.
     */
    public function required_without(
        $value = null,
        ?string $otherFields = null,
        array $data = [],
        ?string $error = null,
        ?string $field = null
    ): bool {
        if ($otherFields === null || empty($data)) {
            throw new InvalidArgumentException('You must supply the parameters: otherFields, data.');
        }

        // If the field is present we can safely assume that
        // the field is here, no matter whether the corresponding
        // search field is present or not.
        $otherFields = explode(',', $otherFields);
        $present     = $this->required($value ?? '');

        if ($present) {
            return true;
        }

        $otherFieldKey = 0;

        // Still here? Then we fail this test if
        // any of the fields are not present in $data
        foreach ($otherFields as $otherField) {
            if ((strpos($otherField, '.') === false) && (! array_key_exists($otherField, $data) || empty($data[$otherField]))) {
                return false;
            }
            if (strpos($otherField, '.') !== false) {
                if ($field === null) {
                    throw new InvalidArgumentException('You must supply the parameters: field.');
                }

                $otherFieldValue = dot_array_search($otherField, $data);
                $fieldArray      = explode('.', $field);
                $otherFieldKey   = $fieldArray[1];

                if (is_array($otherFieldValue)) {
                    return ! empty(dot_array_search($otherField, $data)[$otherFieldKey]);
                }

                $currentField      = str_replace('*', $otherFieldKey, $otherField);
                $currentFieldValue = dot_array_search($currentField, $data);

                return null !== $currentFieldValue;
            }
        }

        return true;
    }

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 think this naming is ok, more readability.
I change it immediately.

Copy link
Contributor Author

@ping-yee ping-yee Sep 28, 2022

Choose a reason for hiding this comment

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

I think the name of $fieldIndex is more appropriate to show the index of the field.
More readability?

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 have been modified, review please.

Copy link
Member

Choose a reason for hiding this comment

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

index implies 0, 1, 2, ...
But it is an array key and is a string.
$fieldKey is better?

Copy link
Contributor Author

@ping-yee ping-yee Sep 29, 2022

Choose a reason for hiding this comment

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

$fieldKey is better?

I agree with you.
I will change that.


if (strpos($keyField, '.') !== false) {
$keyFieldArray = explode('.', $keyField);
$nowKeyField = $keyFieldArray[1];
}

// Still here? Then we fail this test if
// any of the fields are not present in $data
foreach ($fields as $field) {
if ((strpos($field, '.') === false) && (! array_key_exists($field, $data))) {
return ! empty($data[$field]);
}
if (strpos($field, '.') !== false) {
$fieldData = dot_array_search($field, $data);
if ($keyField == null) {
throw new InvalidArgumentException('You must supply the parameters: keyField');
}

$fieldData = dot_array_search($field, $data);
$keyFieldArray = explode('.', $keyField);
$nowKeyField = $keyFieldArray[1];

if (is_array($fieldData)) {
return ! empty(dot_array_search($field, $data)[$nowKeyField]);
}