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 validation rule accepted #438

Merged
merged 4 commits into from
Mar 21, 2022

Conversation

hishpeck
Copy link
Contributor

When using accepted or accepted_if validation rules on a field, the generated body parameters documentation hints that the value is an optional string. This is not true, as you can see in the Laravel Validation documentation: https://laravel.com/docs/9.x/validation#rule-accepted

This PR aims to change the type, description and requirement of fields with those rules.

Documentation and example request generated before the changes:
image
image

Documentation and example request generated after the changes:
image
image

@@ -188,6 +188,12 @@ protected function parseRule($rule, array &$parameterData, bool $independentOnly
case 'required':
$parameterData['required'] = true;
break;
case 'accepted':
$parameterData['required'] = true;
$parameterData['type'] = 'boolean';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good, but doesn't the documentation say it can be "yes" or 1? I don't think we can conclusively assign a boolean type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right if we think in terms of strict typing. I wanted it to be consistent with the boolean validation rule, which can be true, false as well as 1, 0, "1" and "0" (but I see there's no "yes" and "no" option). It is still considered a boolean type in Scribe.

If we leave the type empty, it will default to string type, which I don't think makes much sense either, and from what I can see, displaying multiple available types would be pretty hacky.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. And it's an API after all. Not very likely we'll have a "yes" sent in.

@hishpeck
Copy link
Contributor Author

@shalvah I'm sorry, can you provide guidance on issues that popped up here?

I noticed that lint errors appeared before I started implementing thi PR. I can fix the issue about Model::first() being undefined (but I think it's outside of this PR's scope), but I don't really know how to fix the Filesystem errors, as I believe they are caused by code supporting versions 1 and 2, and I'm not really sure how to fix it other than adding // @phpstan-ignore-line.

@shalvah
Copy link
Contributor

shalvah commented Feb 19, 2022

Just ignore any lint errors that aren't from your code.

@shalvah
Copy link
Contributor

shalvah commented Mar 7, 2022

@Hiszpek can you fix the tests?

@shalvah shalvah closed this Mar 7, 2022
@shalvah shalvah reopened this Mar 7, 2022
@hishpeck
Copy link
Contributor Author

@shalvah The reason the tests on 7.4 fail is because it contains Laravel 6.20, but the accepted_if rule has been implemented somewhere around version 8. I couldn't find any information in contribution guide on how to handle situations like this, should I just scrap the accepted_if part and only leave the accepted rule?

Sorry for leaving this hanging for a while.

@shalvah
Copy link
Contributor

shalvah commented Mar 12, 2022

@shalvah
Copy link
Contributor

shalvah commented Mar 12, 2022

Seems like there's already an example for what to do in that file:

if (version_compare(Application::VERSION, '7.0.0', '>=')) {
yield 'different' => [
['different_param' => 'string|different:other_field'],
[],
['description' => "The value and <code>other_field</code> must be different."],
];
}

@hishpeck hishpeck force-pushed the fix-validation-rule-accepted branch from 6260a1f to 5e751b0 Compare March 13, 2022 17:39
@shalvah shalvah merged commit d224c4a into knuckleswtf:master Mar 21, 2022
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.

2 participants