Regression in FileValidator since Yii 2.0.50 when validating required files

marcovtwout commented 1 month ago

What steps will reproduce the problem?

Create a form with a validation rule like for a required upload field:
```
['example', 'file', 'skipOnEmpty' => false],
```
Set enableClientValidation = false in the ActiveForm. (client side validation still works as expected and will prevent reproduction of the issue)
Post the form without selecting any file.

What is the expected result?

A validation error 'Please upload a file'.

What do you get instead?

The validation passes.

Additional info

The regression is introduced by the changes to validateAttribute() in https://github.com/yiisoft/yii2/pull/20167 which were released with Yii 2.0.50. It still works as expected on 2.0.49.

@bizley Since skipOnEmpty is set to false, validateOnAttribute() is called and should still handle the case where no file has been uploaded. The validation should be executed even if $minFiles is still set to the default of 0. Previously, this happened here, but this code is not reached anymore in the given scenario: https://github.com/yiisoft/yii2/blob/7c5b21355724659ac4ed39099a73ef30eeb4455c/framework/validators/FileValidator.php#L258

Q	A
Yii version	2.0.50
PHP version	-
Operating system	-

bizley commented 1 month ago

I'm a bit confused because I totally missed that logic. Is it stated only in this vardoc? To be honest I think this is very unclear for the users (= "I require a file and the required number of files must be at least 0"). I would only add upgrade note for this one. @samdark what do you think?

marcovtwout commented 1 month ago

@bizley It's part of the main example in the definitive guide as well: https://www.yiiframework.com/doc/guide/2.0/en/input-file-upload#creating-models

Here's the client side validation for comparison https://github.com/yiisoft/yii2/blob/master/framework/assets/yii.validation.js#L404

Requiring to set minFiles might also confuse some users as its related to functionality you only need when you want the input to deal with multiple files (giving a specific $tooFew error message instead of the $uploadRequired message).

If you go looking in the API for required field functionality without knowing the design, the $uploadRequired parameter documents clearly how to set it. While I agree its not the best design and a bit implicit from the code alone, I'd favor backward compatibility in this case.

bizley commented 1 month ago

I see your point 👍🏻 Do you think changing line 212 to

if ($filesCount === 0 && $this->uploadRequired !== null) {

would be enough?

marcovtwout commented 1 month ago

@bizley I think if ($filesCount === 0) would be enough. The field is defined as string only and similar code that uses validation texts does not check for null either.

yiisoft / yii2