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

[5.3] Fixed the validator bug in hydrateFiles when removing the files array #15663

Merged
merged 17 commits into from
Oct 5, 2016

Conversation

chengguoqiang
Copy link
Contributor

@chengguoqiang chengguoqiang commented Sep 29, 2016

fixed the bug of the method hydrateFiles when remove the files array.

@chengguoqiang
Copy link
Contributor Author

tests for #15660

@themsaid
Copy link
Member

themsaid commented Sep 29, 2016

The test you added is currently passing without the changes you are introducing, can you please describe what are you trying to fix?

@GrahamCampbell GrahamCampbell changed the title add tests [5.3] add tests Sep 29, 2016
@GrahamCampbell GrahamCampbell changed the title [5.3] add tests [5.3] Validator improvement Sep 29, 2016
@chengguoqiang
Copy link
Contributor Author

@themsaid Upload the files array , hydrateFiles can't remove it from data array

@themsaid
Copy link
Member

@chengguoqiang can you please add a failing test?

@taylorotwell
Copy link
Member

We also need a much better description of what this is attempting to solve / fix / do before merging.

@chengguoqiang
Copy link
Contributor Author

@themsaid I added a failing test, please check, thank you.

@chengguoqiang chengguoqiang changed the title [5.3] Validator improvement [5.3] Validator improvement - fixed the bug of the method hydrateFiles when remove the files array. Sep 29, 2016
@chengguoqiang
Copy link
Contributor Author

@taylorotwell the description was modified, would you please check the description for me

@themsaid
Copy link
Member

Well you just pointed out a bug, but the fix isn't actually complete. The validator works fine with this bug except for the part of separating data from files for array file uploads, means that now $this->data won't have these file uploads, and the each() method relys only on the data attribute, so you'll need to update the each() method and merge the files array to the $data array inside the method before starting the iteration.

To ensure everything is working, you may add these tests:

public function testFilesHydration()
{
    $trans = $this->getIlluminateArrayTranslator();
    $file = new File(__FILE__, false);
    $v = new Validator($trans, ['file' => $file, 'text' => 'text'], ['name' => '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'], ['name' => '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());
}

@chengguoqiang
Copy link
Contributor Author

@themsaid improvement

@themsaid
Copy link
Member

themsaid commented Sep 30, 2016

I'm sorry but your version of testMultipleFileUploads() doesn't really test anything, same for testFileUploads ().

These simple tests should pass:

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());
}

If you try it testMultipleFileUploads() will fail, which is the problem we need to solve, this test should pass.

@chengguoqiang
Copy link
Contributor Author

chengguoqiang commented Sep 30, 2016

I update the each() method and merge the files array to the $data array inside the method before starting the iteration. these simple tests what you given run fail:


if ($value != '__missing__') {
if (! is_null($value)) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this change? null is a value we still want to get.

@themsaid
Copy link
Member

In the each() method you need to have this:

$data = array_merge(
            Arr::dot($this->initializeAttributeOnData($attribute)), 
            $this->files
        );

so that it looks into the files array as well, if you have this the tests will pass.

@chengguoqiang
Copy link
Contributor Author

the tests pass, thank you very match

@GrahamCampbell GrahamCampbell changed the title [5.3] Validator improvement - fixed the bug of the method hydrateFiles when remove the files array. [5.3] Fixed the validator bug in hydrateFiles when removing the files array Oct 3, 2016
@GrahamCampbell
Copy link
Member

Please rebase and squash to one commtit.

@taylorotwell taylorotwell merged commit 9900c9f into laravel:5.3 Oct 5, 2016
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.

None yet

4 participants