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

Required_without validation rule #1007

Closed
Iamscalla opened this issue Apr 30, 2018 · 7 comments
Closed

Required_without validation rule #1007

Iamscalla opened this issue Apr 30, 2018 · 7 comments
Labels
bug Verified issues on the current code behavior or pull requests that will fix them waiting for info Issues or pull requests that need further clarification from the author

Comments

@Iamscalla
Copy link
Contributor

Iamscalla commented Apr 30, 2018

The above subject does not act as its supposed to, the only part it handles is if the rule field is not available in the request.

before the loop, at this line that has this:
$present = $this->required($data[$str] ?? null); it's should be $present = $this->required($str ?? null); since $str is more of a field in the request.

Then in the field loop, within the array key check, it's supposed to run this way:
if(!array_key_exists($field, $data) || (isset($data[$field]) && empty($data[$field])){
return false:
}

pardon my no code formating, as am on mobile.

@georgeio
Copy link
Contributor

georgeio commented May 3, 2018

I can confirm this case, is anyone working on a fix already? I can submit a PR. I also noticed a typo in the error message it reads 'The {field} field is required when {param} in not present.' I guess the "in" should have been "is".

@lonnieezell
Copy link
Member

I haven’t had a chance to work on it yet so a PR is very welcome.

@georgeio
Copy link
Contributor

georgeio commented May 6, 2018

@lonnieezell Ok. Something else needed my attention at work could not jump on this immediately. I have submitted a PR for the typo that was really minor, working on this issue now.

@lonnieezell
Copy link
Member

@georgeio Did you submit the fix fo this one? I'm a bit scattered at the moment. :)

@georgeio
Copy link
Contributor

@lonnieezell I haven't pushed the changes yet. When I try to install dependencies on my local using composer I noticed ZendEscaper is updated, this file is also being tracked by git, if i push the changes to remote this updates will be captured as well. I just wanted to be sure this is Ok? Here is the file system/ThirdParty/ZendEscaper/Escaper.php

@lonnieezell
Copy link
Member

it would be better at this point to NOT include Escaper, but not too big of a deal, I don't believe.

@jim-parry jim-parry added bug Verified issues on the current code behavior or pull requests that will fix them waiting for info Issues or pull requests that need further clarification from the author labels Oct 19, 2018
@jim-parry
Copy link
Contributor

The original behavior, that was claimed to be bad, actually looks correct.
Further, the second part of the proposed fix seems backwards.
Nothing further on this in months, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them waiting for info Issues or pull requests that need further clarification from the author
Projects
None yet
Development

No branches or pull requests

4 participants