Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

PSR-7 file validation implemented #251

Merged
merged 6 commits into from
Jan 29, 2019
Merged

PSR-7 file validation implemented #251

merged 6 commits into from
Jan 29, 2019

Conversation

icetee
Copy link
Contributor

@icetee icetee commented Jan 22, 2019

This commit allows validation files with $request->getUploadedFiles().

But I got stuck in testing. I want to be able to use a $upload->getStream()->getMetadata('uri') and return example /tmp/abcd.

$stream = $this->prophesize(StreamInterface::class);
$upload = $this->prophesize(UploadedFileInterface::class);

$stream->getMetadata('uri')->willReturn($testFile);

$upload->getClientFilename()->willReturn('csgo.mo');
$upload->getClientMediaType()->willReturn('mo');
$upload->getError()->willReturn(0);
$upload->getStream()->willReturn($stream->reveal());
$upload->reveal();

$upload->getStream()->getMetadata('uri'); // Call to undefined method Prophecy\Prophecy\MethodProphecy::getMetadata()   

@weierophinney
Copy link
Member

@icetee we already have support for validating PSR-7 uploaded files as of version 2.11.0; the support is even documented.

@froschdesign
Copy link
Member

froschdesign commented Jan 22, 2019

@weierophinney
But this support will not work for (file) validators like Size, MimeType and so on.

@froschdesign
Copy link
Member

Related to zendframework/zend-form#227

@weierophinney weierophinney reopened this Jan 22, 2019
Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

I've left a number of comments at this point with directions on changes necessary for us to merge.

With regards to the testing: it sounds like perhaps the PSR-7 library is not installed locally. Run composer list | grep http-message and check to ensure it's present. That method is definitely part of the spec and the interfaces, so that's where I'd start.

$file = $value;
$filename = basename($file);
}
extract($this->getFileInfo($value, $file));
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use extract(). Assign the return value to a variable, and pull elements out it.

$file = $value;
$filename = basename($file);
}
extract($this->getFileInfo($value, $file));
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here.

$filename = basename($file);
$filetype = null;
}
extract($this->getFileInfo($value, $file, true));
Copy link
Member

Choose a reason for hiding this comment

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

etc.

Please review all code for elements where you have done this and update.

* @link http://github.com/zendframework/zf2 for the canonical source repository
* @copyright Copyright (c) 2005-2015 Zend Technologies USA Inc. (http://www.zend.com)
* @license http://framework.zend.com/license/new-bsd New BSD License
*/
Copy link
Member

Choose a reason for hiding this comment

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

Use the compact form of the license, please:

/**
 * @see       https://github.com/zendframework/zend-validator for the canonical source repository
 * @copyright Copyright (c) 2019 Zend Technologies USA Inc. (https://www.zend.com)
 * @license   https://github.com/zendframework/zend-validator/blob/master/LICENSE.md New BSD License

(note that since this is a new file, the copyright date is only this year.)
*/

$fileInfo['file'] = $value;
$fileInfo['filename'] = basename($fileInfo['file']);
$fileInfo['filetype'] = null;
}
Copy link
Member

Choose a reason for hiding this comment

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

Please extract the different cases into separate private methods; doing so will make it easier to read the method and understand where the individual pieces come from. It will also allow you to return early, removing the elseif/else conditions:

if (is_string($value && is_array($file)) {
    return $this->getLegacyFileInfo($file, $hasType);
}

if (is_array($value)) {
    return $this->getSapiFileInfo($file, $hasType);
}

if ($value instanceof UploadedFileInterface) {
    return $this->getPsr7FileInfo($value, $hasType);
}

return $this->getFileBasedFileInfo($file, $hasType);

These methods can then be re-used in the getFileInfoExists() method, allowing you to act based on the return values.

$fileInfo['file'] = $file['tmp_name'];
$fileInfo['filetype'] = $file['type'];

$this->setValue($fileInfo['filename']);
Copy link
Member

Choose a reason for hiding this comment

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

This method call is a problem, because setValue() does not exists in the current trait and that results in a false dependency.

@icetee
Copy link
Contributor Author

icetee commented Jan 23, 2019

I pushed the changes.

@weierophinney I have http-message package composer show -i | grep http-message.

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

The strategy of testing the FileInformationTrait is great, as then the tests for the individual validators do not need to change, and we can still be assured that they will operate regardless of the file upload SAPI in place. Nice work!

It looks like the various test failures reported by Travis are not your fault; they appear to be timing issues. I'll try rebasing your PR against current develop to see if recent changes make those go away before I merge.

Fatal error: Default value for parameters with a class type hint can only be NULL
src/File/FileInformationTrait.php Outdated Show resolved Hide resolved
src/File/FileInformationTrait.php Outdated Show resolved Hide resolved
src/File/FileInformationTrait.php Outdated Show resolved Hide resolved
src/File/FileInformationTrait.php Outdated Show resolved Hide resolved
src/File/FileInformationTrait.php Outdated Show resolved Hide resolved
test/File/FileInformationTraitTest.php Outdated Show resolved Hide resolved
test/File/FileInformationTraitTest.php Outdated Show resolved Hide resolved
test/File/TestAsset/FileInformation.php Outdated Show resolved Hide resolved
@icetee
Copy link
Contributor Author

icetee commented Jan 29, 2019

Maybe related build failed - #257

@weierophinney weierophinney merged commit 8d9eb43 into zendframework:develop Jan 29, 2019
@weierophinney
Copy link
Member

@icetee Yes, and I just pushed a fix for that to develop. 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants