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

Support exclude_if and exclude_unless rule #1165

Closed
wants to merge 7 commits into from

Conversation

atomita
Copy link
Contributor

@atomita atomita commented Jan 21, 2020

  • Added or updated tests
    - [ ] Added Docs for all relevant versions
  • Updated CHANGELOG.md

Changes

Fixed an issue where exclude_if or exclude_unless rules would not be excluded from args when applied.

Breaking changes

@atomita atomita marked this pull request as ready for review January 21, 2020 10:20
@atomita atomita changed the title Support exclude if rule Support exclude_if and exclude_unless rule Jan 21, 2020
@spawnia
Copy link
Collaborator

spawnia commented Jan 21, 2020

It looks like the exclude_if and exclude_unless rules do not only validate the input but also change it.

That seems like a really bad design decision from Laravel, are there any other validation rules that work like this?

@atomita
Copy link
Contributor Author

atomita commented Jan 22, 2020

@spawnia
There seems to be no other than exclude_if and exclude_unless at laravel v6.12

@spawnia
Copy link
Collaborator

spawnia commented Jan 22, 2020

I think we might not support this feature of Laravel.

First of all, it just seems wrong to use validation to sanitize incoming data. It should be a discrete mechanism in a different step.

Also, i don't think that masking client errors is a great idea in general. This behaviour is not transparent to clients. Without a description, there is no way for them to know that the argument they passed is going to be ignored. If the backend ever changes, there is no way for the client to know - the schema does not reflect that.

In terms of implementation, changing the incoming data in a validation step breaks some assumptions that Lighthouse currently makes. We would have to go out of our way to support this, for instance, we would also need to mutate the ArgumentSet - a data structure that holds the passed argument values and their types.

@atomita
Copy link
Contributor Author

atomita commented Jan 22, 2020

@spawnia
I'm sad, but I understand.

@spawnia
Copy link
Collaborator

spawnia commented Jan 22, 2020

Thank you for your contribution and your understanding. Please don't let this discourage you, your PR certainly was well done and i very much welcome future contributions.

I mentioned the fact that those rules are not supported in the directive definition, i am glad you brought it up: #1168. I hope the added section about sanitization helps you.

@atomita atomita deleted the fix/support-exclude-if-rule branch January 23, 2020 01:27
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.

None yet

2 participants