CodeIgniter4
CodeIgniter4 copied to clipboard
codeigniter4
Validation: permit_empty, optional arguments
Hi,
i love the idea to specify that a given attribute is allowed to be empty by rule, but it would be great if you can specify an explicit type, or a subset. If the target data fileld can be an empty array, null, empty string there will be a database error for sure, if you try to insert an array for example in a boolean nullable column.
Greetings Tada5hi
I like the concept but it feels a bit weird to be a parameter to permit_empty. Why not just a new validation rule like is_type that takes a type name (or array of names) to check against is_a() or the relevant type? Then you could use this rule:
'colors' => 'permit_empty|is_type[array,ColorCollection]'
I think adding optional parameters to the permit_empty rule isn't such a bad idea. Right now, when this rule occurs and its assumptions are met, all other rules are ignored.
With optional parameters, both - implementation and rules behind it would be easier to understand for users. The alternative is to make this one exceptional rule, which will be executed alongside the permit_empty rule.
A separate rule might also cause some problems. What if I would like to validate the "type" only when there is a value (not empty) in the field? We would need yet another rule for the same thing?
Would love to see that all rules support empty values (or only get called if there is a value, but this wouldn't allow to build a required rule...) so only the rule required forces to have a value and there is no need for permit_empty anymore.
See #3025
This was closed but I don't think the issue was ever addressed. I ran into this probably again today - namely, that we don't have any way to validate nullable database fields.
Consider the following:
class WidgetModel extends Model
{
protected $table = 'widgets';
protected $returnType = Widget::class;
protected $allowedFields = ['name', 'cost'];
protected $validationRules = [
'name' => 'required',
'cost' => 'permit_empty|is_natural_no_zero',
];
}
Since cost is a nullable integer in the database (cost INT NULL DEFAULT NULL) the model should accept anything int|null (with the added benefit of enforcing positive integers). However, permit_empty will pass '' and false as well as null. Accepting form input like so:
// WidgetController.php
$data = $this->request->getPost(); // ['name' => 'My Widget', 'cost' => '']
if ($this->model->insert($data)) {
...
... actually causes a database error when it tries inserting '' for cost:
mysqli_sql_exception #1366 Incorrect integer value: '' for column 'cost' at row 1 SYSTEMPATH/Database/MySQLi/Connection.php at line 292
There are a few different ways we could implement this, but being able to define a validation for null|{rule} is pretty important.
Something like these?
permit_empty[null]
permit_empty[array,string,null,false] === permit_empty
My comment was about the ability to create a rule something like required_if_spacial_case where the $value can be null or a value and the rule checks if that is valid.
This is not possible since without using permit_empty the field fails when $value is null.
When using permit_empty the rule required_if_spacial_case is not executed when the value is null.
I don't know if that is still the case in dev but should be something to check while implementing.
@element-code Sorry, I don't get what you want. Is it another feature request?
I think what @element-code is suggesting is a different way of handling this, by making a new rule. I had imagined this as an option like this:
'cost' => 'nullable|is_natural'
Which basically expands any following rules to be '{type}|null` just like the database would accept.
None of these solve the example problem that submitting a form with <input type='text' name='cost'> will result in ['cost' => ''] which causes a database error. This might ultimately be beyond what Validation can handle without some pre-Validation transformations, but it is a common scenario.
For the original issue, I like the proposal that has been floating around of adding parameters to permit_empty. The only addition I would make to @kenjis' suggestion would be to add zero as a choice, as long as we can agree on handling for 0 versus '0'.
Oh, ~~'0' and 0 and 0.0 also pass permit_empty.~~
https://github.com/codeigniter4/CodeIgniter4/blob/7666380582e5722aaead0651ead1b4bd81a4676d/tests/system/Validation/RulesTest.php#L128
~~And also '-0', -0, '0.0', -0.0, '-0.0' also pass permit_empty.~~
(If the rule is only permit_empty, anything passes.)
Then
permit_empty[array,string,null,false,zero,minus_zero] === permit_empty ?
I don't feel the need to make a special case for -0; maybe we lump all the zeroes into one by checking is_numeric() and == 0 or something similar.
The test code is broken. all zeros do not pass
permit_empty.
🤦♂️ What a mess. Does this mean we have never allowed zero of any kind? Now we have to decide if we break people relying on this to match the User Guide or make this weird exception.
Or do we make the default parameters equivalent to your original suggestion: permit_empty[array,string,null,false]
... then add zero as an additional option?
See #6175 and https://github.com/codeigniter4/CodeIgniter4/blob/7666380582e5722aaead0651ead1b4bd81a4676d/system/Validation/Validation.php#L243-L244
permit_empty allows empty array or empty string cast to string.
@MGatner By the way, where did you get the idea that permit_empty allows zero?
I thought I read that in the User Guide, but that is not the case:
Allows the field to receive an empty array, empty string, null or false
Must have been from the tests? Which we know now are irrelevant. This is actually good, that zero isn't currently supported allows it to be a new parameter option without affecting current implementations.
This is actually good, that zero isn't currently supported allows it to be a new parameter option without affecting current implementations.
Why do you need to permit zero? Any use case?
"empty" is a bit of a code word in PHP. While I think the method with that name is a terrible design it sets some expectations: https://3v4l.org/BSN3G
If we don't include zero as a parameter I think we need to be very explicit that all the "zero variants" are considered acceptable input (I.e. this validation differs in behavior from empty()).
Some nice precedence from PHPStan on "non-empty" strings:
non-empty-stringis any string except''. It does not mean “empty” in the weird sense used byempty().
Bring up the subject. I came across this task, but initially on validation in the controller. I need to exclude some fields sometimes when registering a user (jobTitle, dateBirth, Captcha, etc). Since I use entities I have setDateBirth() methods I have no problem assigning null, 0, "" to the database.
public function setDateBirth($dateBirth): self
{
if (empty($dateBirth)) {
$dateBirth = null;
}
$this->dateBirth = DatetimeCast::get($dateBirth);
$this->attributes['date_birth'] = is_null($dateBirth) ? null : (string)$this->dateBirth;
return $this;
}
To avoid misunderstandings of the validator, I intentionally delete empty fields, which is guaranteed to process permit_empty
if (empty($input['dateBirth'])) {
unset($input['dateBirth']);
}
Otherwise, the workaround is the BeforeUpdate, before Insert, etc model events to set the desired values. I haven't tested it, but it's possible to try a personal NullCast class
Thanks for your feedback and solution @neznaika0 ! These days I am less inclined to solve all possible scenarios in the framework and expect developers to do some of their own handling, like you are here.