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

WIP Fix required_without rule bug. v1.0.1 #1747

Closed
wants to merge 2 commits into from

Conversation

bangbangda
Copy link
Contributor

Description
Verify that the field exists. no validation value is valid.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@bangbangda bangbangda mentioned this pull request Feb 20, 2019
4 tasks
@niteraven7
Copy link

niteraven7 commented Feb 20, 2019

@bangbangda Looks good, I'm glad you acknowledged my logic. I would change code to:

if (! isset($data[$field]) || $data[$field] == '') { return false; }

Even though this should rarely happen - string with spaces ' ' should be valid input.

@lonnieezell
Copy link
Member

The required rules have always required that it is not empty or not 0. Empty strings have always failed the require rule. For cases like that, we do have the permit_empty rule.

@bangbangda
Copy link
Contributor Author

Is this better?
Support for empty arrays.
if (! array_key_exists($field, $data) || ! $this->required($data[$field] ?? ''))

@lonnieezell @niteraven7

@niteraven7
Copy link

Hi @lonnieezell I think you may have confused him :)

I've been thinking about the permit_empty rule and I'd like to propose a change. I don't understand why the rule seems to superseded all other rules if it's in there? Rules are supposed to cascade from one rule to the next and I think it would be helpful if permit_empty functioned the same way. For example, I have a form that requires an email address OR phone number OR both. The validation rules are as follows:

public $recipient = [
'first_name' => 'required|min_length[1]|max_length[50]',
'last_name' => 'required|min_length[1]|max_length[50]',
'email' => 'required_without[number]|permit_empty|valid_email',
'number' => 'required_without[email]|permit_empty|numeric'
];

I'd like the rule set to check if email OR number exists and then if one does permit the other to be empty. permit_empty seems to hijack the whole thing. Why can't it just cascade like all the other rules?

Please put some thought into this.

Thank you,
Kyle

@lonnieezell
Copy link
Member

I'd like the rule set to check if email OR number exists and then if one does permit the other to be empty. permit_empty seems to hijack the whole thing. Why can't it just cascade like all the other rules?

Please put some thought into this.

Because of the way required works currently, but it wouldn't hurt for that all to have a revisit. I did that library pretty early on and it's showing some cracks. Anyone want to take a crack at it? :)

@jim-parry jim-parry added this to the 4.0.0-beta.2 milestone Mar 5, 2019
@jim-parry jim-parry changed the title Fix required_without rule bug. v1.0.1 WIP Fix required_without rule bug. v1.0.1 Mar 11, 2019
@atishhamte
Copy link
Contributor

There is confusion in the method definition of required_without.
It says, "The field is required when all of the other fields are not present in the data."

I guess the definition should be like,
"The field is required when all of the other fields are present in the data but not required(means, the value can be blank or empty)."

@lonnieezell
Copy link
Member

Ah, that makes sense then. Yes, it should be modified to only use non-empty fields to check against so that it's functionality matches required_with.

foreach ($fields as $field)
{
if (! array_key_exists($field, $data))
if (! array_key_exists($field, $data) || ! $this->required($data[$field] ?? ''))
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is not required as we just intend to check here whether the field exists in the data or not. It doesn't matter whether the field is required or not.

@atishhamte
Copy link
Contributor

I guess we can close this PR as the definitions are changed in the #1862.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants