Skip to content

Commit

Permalink
[5.3] Fixed the validator bug in hydrateFiles when removing the files…
Browse files Browse the repository at this point in the history 
… array (#15663)

* Update Validator.php

If this value is an instance of the Array, failed when remove files from the data
  • Loading branch information
@chengguoqiang @taylorotwell
chengguoqiang authored and taylorotwell committed Oct 5, 2016
1 parent ab8da3a commit 9900c9f
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 4 deletions.
16 changes: 12 additions & 4 deletions src/Illuminate/Validation/Validator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,17 +247,22 @@ protected function hydrateFiles(array $data, $arrayKey = null)
}

foreach ($data as $key => $value) {
$key = ($arrayKey) ? "$arrayKey.$key" : $key;
$new_key = ($arrayKey) ? "$arrayKey.$key" : $key;

// If this value is an instance of the HttpFoundation File class we will
// remove it from the data array and add it to the files array, which
// we use to conveniently separate out these files from other data.
if ($value instanceof File) {
$this->files[$key] = $value;
$this->files[$new_key] = $value;

unset($data[$key]);
} elseif (is_array($value)) {
$this->hydrateFiles($value, $key);
if (! empty($value)) {
$value = $this->hydrateFiles($value, $new_key);
if (empty($value)) {
unset($data[$key]);
}
}
}
}

Expand Down Expand Up @@ -334,7 +339,10 @@ public function sometimes($attribute, $rules, callable $callback)
*/
public function each($attribute, $rules)
{
$data = Arr::dot($this->initializeAttributeOnData($attribute));
$data = array_merge(
Arr::dot($this->initializeAttributeOnData($attribute)),
$this->files
);

$pattern = str_replace('\*', '[^\.]+', preg_quote($attribute));

Expand Down
42 changes: 42 additions & 0 deletions tests/Validation/ValidationValidatorTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,12 @@ public function testValidateRequired()
$file = new File(__FILE__, false);
$v = new Validator($trans, ['name' => $file], ['name' => 'Required']);
$this->assertTrue($v->passes());

$file = new File(__FILE__, false);
$foo = new File(__FILE__, false);
$v = new Validator($trans, ['name' => [$file, $foo]], ['name.0' => 'Required', 'name.1' => 'Required']);
$this->assertTrue($v->passes());
$this->assertEmpty($v->getData());
}

public function testValidateRequiredWith()
Expand Down Expand Up @@ -3199,6 +3205,42 @@ public function testValidMethod()
]);
}

public function testFilesHydration()
{
$trans = $this->getIlluminateArrayTranslator();
$file = new File(__FILE__, false);
$v = new Validator($trans, ['file' => $file, 'text' => 'text'], ['text' => 'Required']);
$this->assertEquals(['file' => $file], $v->getFiles());
$this->assertEquals(['text' => 'text'], $v->getData());
}

public function testArrayOfFilesHydration()
{
$trans = $this->getIlluminateArrayTranslator();
$file = new File(__FILE__, false);
$file2 = new File(__FILE__, false);
$v = new Validator($trans, ['file' => [$file, $file2], 'text' => 'text'], ['text' => 'Required']);
$this->assertEquals(['file.0' => $file, 'file.1' => $file2], $v->getFiles());
$this->assertEquals(['text' => 'text'], $v->getData());
}

public function testMultipleFileUploads()
{
$trans = $this->getIlluminateArrayTranslator();
$file = new File(__FILE__, false);
$file2 = new File(__FILE__, false);
$v = new Validator($trans, ['file' => [$file, $file2]], ['file.*' => 'Required|mimes:xls']);
$this->assertFalse($v->passes());
}

public function testFileUploads()
{
$trans = $this->getIlluminateArrayTranslator();
$file = new File(__FILE__, false);
$v = new Validator($trans, ['file' => $file], ['file' => 'Required|mimes:xls']);
$this->assertFalse($v->passes());
}

protected function getTranslator()
{
return m::mock('Symfony\Component\Translation\TranslatorInterface');
Expand Down

10 comments on commit 9900c9f

@torkiljohnsen
Copy link

@torkiljohnsen torkiljohnsen commented on 9900c9f Oct 14, 2016 •
edited
Loading

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[], like files[$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:

public function rules(): array
{
    return [
        'files' => 'required|array',
        'files.*' => 'file|image',
    ];
}

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 in hydrateFiles 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?

@themsaid
Copy link
Member

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.

@torkiljohnsen
Copy link

@torkiljohnsen torkiljohnsen commented on 9900c9f Oct 14, 2016 •
edited
Loading

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:

public function testValidationFailsWhenRequiredFilesAreMissing()
{
    $trans = $this->getIlluminateArrayTranslator();
    $v = new Validator(
        $trans, 
        ['files' => []],
        [
            'files' => 'required|array', // should fail here
            'files.*' => 'file' // should be ignored
        ]
    );
    $this->assertFalse($v->passes());
}

From the testMultipleFileUploads method in the commit above: I doubt the required rule has any effect other than checking for empty values:

$v = new Validator($trans, ['file' => [$file, $file2]], ['file.*' => 'Required|mimes:xls']);

@torkiljohnsen
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any luck yet @themsaid ?

@themsaid
Copy link
Member

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.

@torkiljohnsen
Copy link

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 it images, or foo.

@nhowell
Copy link
Contributor

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:

[
    'attachments' => 'required|array',
    'attachments.*' => 'file',
];

@taylorotwell
Copy link
Member

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?

@themsaid
Copy link
Member

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

@nhowell
Copy link
Contributor

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 👍

Please sign in to comment.