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

[8.x] Add array_keys validation rule #40720

Merged
merged 8 commits into from
Feb 1, 2022
Merged

[8.x] Add array_keys validation rule #40720

merged 8 commits into from
Feb 1, 2022

Conversation

Drewdan
Copy link
Contributor

@Drewdan Drewdan commented Jan 31, 2022

I have recently been finding it more common to have to validate that certain items are available in an array. However, the items that are required are usually dynamically driven and then stored as JSON in a DB column.

A really simple example of this might be shifts for a team. Each team has working days which are pre-defined and stored in a database. There is then a dynamically generated form which asks for the shift times for these days.

The required data for each shift would be the same, a start time and an end time and would be required. Typically, to validate this dynamic data, you might do something such as:

public function rules() {

$dayRules = BusinessHourDay::sameCompany()->get()->map(function ($businessHourDay) {  
  return [  
  'days.' . $businessHourDay->day . '.start_time' => 'required',  
  'days.' . $businessHourDay->day . '.end_time' => 'required',  
  // more rules  
  ];  
});  
  
return [  
  'name' => 'required',  
  'description' => 'required'
  'days' => 'array',
  ...$dayRules,  
];

This allows us to validate the days that the user has configured in the database are present in the dynamically generated form.

This proposed change would allow this instead:

public function rules() {

$dayRules = BusinessHourDay::sameCompany()->get()->pluck('day');  
return [  
  'name' => 'required',  
  'description' => 'required',  
  'days' => [  
  'bail',
  'array',  
  'contains_all,' . $dayRules->implode(','),  
  ],  
  'days.*.start_time' => 'required',  
  'days.*.end_time' => 'required',
  // more rules  
];

This would simplify the validation in the cases where mapping a list of validation rules, which would have the same rules for each entry, but we still want to assert that all the required data was present.

The rule could also be applied to a string field to make sure a part or parts of it given match a particular structure, for example, a form where you might enter an invoice "number" something like "INV:12345-WEB"

public function rules() {

return [
	'invoice_id' = [
		'required',
		'contains_all:INV,WEB',
	],
];

Would be grateful for any feedback or thought you have on this :)

Thanks guys.

@Drewdan Drewdan changed the title Feature/array has keys rule [8.x] Add contains validation rule Jan 31, 2022
@taylorotwell
Copy link
Member

taylorotwell commented Jan 31, 2022

I'm curious if we need any additional functionality in the framework for this. It seems you could do (note the array unpacking):

return [
  'name' => 'required',
  'description' => 'required',
  'days' => 'bail|array',
  ...BusinessHourDay::sameCompany()->get()->flatMap(function ($day) {
      return [
          'days.'.$day->day.'.start_time' => 'required',
          'days.'.$day->day.'.end_time' => 'required',
      ];
  }),
];

@Drewdan
Copy link
Contributor Author

Drewdan commented Jan 31, 2022

My intended approach here is to handle the times when you might have a some simple validation requirements, like just making sure it has some value using the required validation rule, without having to add any "complexity" to the code. Has the array got all of these keys? Yes, great, if not fail.

The approach you have suggested works nicely too, and it is definitely a pattern I'd reach for. I just really like the simplicity of a simple array in the form request validator rather than having to build one up.

I'd see the pattern you proposed being something I would more likely reach for if I had some admin interface to generate a dynamic form, where part of that generation was to also specify which validation rules would be used to validate it. I could then dynamically generate the rules to apply to each element in the array.

Something like:

return [
  'questions' => 'bail|array',
  ...$questionnaire->flatMap(function ($question) {
      return [
          'question.'.$question->slug => $question->rules,
      ];
  }),
];

If you don't feel it's a good fit, happy to close though :)

@taylorotwell
Copy link
Member

I think I'm a having a bit of a hard time following the rule. Can you give another example of using it - you also use contains_all and contains in your examples. I'm not sure if those are for the same rule?

My intended approach here is to handle the times when you might have a some simple validation requirements, like just making sure it has some value using the required validation rule, without having to add any "complexity" to the code. Has the array got all of these keys? Yes, great, if not fail.

Can you give an example of this? Ideally without any loops - just hard-coded strings to make it very clear.

@Drewdan
Copy link
Contributor Author

Drewdan commented Jan 31, 2022

Apologies, it should be contains_all in my examples - I originally named the rule contains and then realized that this was not clear, as it is checking to see if all keys are present, so changed it to contains_all and forgot to update my write-up.

I am just going to cook my kids dinner, and then I will write up a clearer example for you to review :)

@Drewdan
Copy link
Contributor Author

Drewdan commented Jan 31, 2022

If you had an array of questions which you wanted to be asked in a form, you might have something like this:

$questions = [
	'favourite_color' => 'What is your favorite color?',
	'favourite_animal' => 'What is your favorite animal?',
	'maiden_name' => 'What is your Mother\'s maiden name?',
	'favorite_php_framework' => 'What is your favorite PHP framework?',
];

(This would likely be database driven, or a static array on a model, but the method of which we get it is not really relevant to this.)

You could use these to generate a form in a blade file using the key as the input name in an array.

@foreach($questions as $question => $key)
<input type="text" name="questions[{{ $key }}] value="{{ old('questions.' . $key) }} ">
@endforeach

When it comes to validation, with these, I don't have to care much about the content, only that the user has answered something for them.

I would build a rule set in a request validator by looping through the array which would end up looking something like this:

public function rules() 
{
	return [
		'questions' => 'array',
		'questions.favorite_color' => 'required',
		'questions.favourite_animal' => 'required',
		'questions.maiden_name' => 'required',
		'questions.favorite_php_framework' => 'required',
	];
}

The rules are the same, and I would have had to flatMap or some other form of loop to build up this rule set. Whereas my approach with the contains_all would be to have a rule set looking like this:

public function rules()
{
	return [
		'questions' => [
			'array',
			'contains_all:favorite_color,favorite_animal,maiden_name,favorite_php_framework',
		],
		'questions.*' => 'required',
	];
}

The contents of the contains_all would be imploded from the array.

My belief here is this approach is easier when you are trying to keep validation really simple.

@Drewdan Drewdan changed the title [8.x] Add contains validation rule [8.x] Add contains_all validation rule Jan 31, 2022
@taylorotwell
Copy link
Member

taylorotwell commented Jan 31, 2022

Gotcha. I wonder if something like contains_keys would be a clearer name? Or maybe requires_keys so it's clear that all of them are required? Or honestly maybe just array_keys to compliment the existing array rule?

@Drewdan
Copy link
Contributor Author

Drewdan commented Jan 31, 2022

I feel like array_keys would make sense. I had it like this so the behaviour could be applied to a string as well, as most other rules seem to work across data types, but I am not sold on my string implementation. I feel pulling this back to just work with arrays seems like a sensible move.

@Drewdan
Copy link
Contributor Author

Drewdan commented Jan 31, 2022

I have pushed some new commits to rename the rule to array_keys and also makes the validation fail if the subject is not an array, also removing the validation rules for a string.

@Drewdan Drewdan changed the title [8.x] Add contains_all validation rule [8.x] Add array_keys validation rule Jan 31, 2022
@morloderex
Copy link
Contributor

@Drewdan @taylorotwell just to make sure you have not forgotten that the “array” rule can actually already take in parameters and if I am remembering correctly already check for precedence of keys in the array?

Could it be that the array rule could be helpful in this case as well, sense you already know the specific keys?

@Drewdan
Copy link
Contributor Author

Drewdan commented Feb 1, 2022

@morloderex - as I understand, the array validation method accepts a list of valid keys, but this does not validate that they are present, it only serves to prevent unacceptable keys being passed through. This rule differs in that it validates all keys are present in the array. Ideally, you would use these in tandem with each other.

@taylorotwell
Copy link
Member

Renamed to required_array_keys.

@taylorotwell
Copy link
Member

@morloderex ... @Drewdan is correct that array rule only accepts a list of valid keys but does not validation that they are present.

@Drewdan
Copy link
Contributor Author

Drewdan commented Feb 1, 2022

@taylorotwell renaming to required_array_keys makes sense, it is a much clearer rule name :)

@taylorotwell taylorotwell merged commit fec487c into laravel:8.x Feb 1, 2022
@Drewdan Drewdan deleted the feature/array_has_keys_rule branch February 1, 2022 16:15
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.

3 participants