-
Notifications
You must be signed in to change notification settings - Fork 11k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[5.3] Fixed the validator bug in hydrateFiles when removing the files…
… array (#15663) * Update Validator.php If this value is an instance of the Array, failed when remove files from the data
- Loading branch information
1 parent
ab8da3a
commit 9900c9f
Showing
2 changed files
with
54 additions
and
4 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9900c9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chengguoqiang @taylorotwell This change broke a test in my app.
I am POSTing an array of images with field name
files[]
, likefiles[$file1, $file2]
. Specifically I am trying to validate that at least one image was POSTed.These request validation rules worked fine in 5.3.16:
With the changes in this commit, the validation now stops at
files => required
, since the field does not exist anymore. It is being stripped out inhydrateFiles
now. Not sure if this was intended? That you're not supposed to be able to validate that an array of files is required and is actually an array?9900c9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm Let me try to regenerate and get back to you.
9900c9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@themsaid A slight variation of the
testMultipleFileUploads
test above could probably test this:From the
testMultipleFileUploads
method in the commit above: I doubt therequired
rule has any effect other than checking for empty values:9900c9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any luck yet @themsaid ?
9900c9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@torkiljohnsen I was able to regenerate the problem yes but a bit confused about how to fix, a fix might introduce a breaking change :/ Will keep looking into that.
9900c9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! To me this was a breaking change to begin with :)
My objective is to validate that the variable
files
is an array that contains at least one uploaded file. The variable name might be confusing, could rename itimages
, orfoo
.9900c9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having the same problem as @torkiljohnsen. This validation is no longer working:
9900c9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@themsaid would the latest release fix @nhowell problem?
9900c9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened a PR that should fix this issue @nhowell @torkiljohnsen reported.
#16104
9900c9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@themsaid That PR looks like it should solve the issue. Thanks 👍