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

Make UploadedFile Extend SplFileInfo #378

Merged
merged 4 commits into from
Nov 12, 2019
+42 −3
Merged

Make UploadedFile Extend SplFileInfo #378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bfe7a22
Making UploadedFile extend SplFileInfo
mattsah Nov 8, 2019
d4dec99
Changelog + doc changes
mattsah Nov 8, 2019
f38c319
qa: adds tests for #378
weierophinney Nov 12, 2019
b6d4ed1
docs: adds CHANGELOG entry for #378
weierophinney Nov 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,36 @@

All notable changes to this project will be documented in this file, in reverse chronological order by release.

## 2.2.0 - TBD
## 2.2.1 - TBD

### Added

- [#376](https://github.com/zendframework/zend-diactoros/pull/376) adds support for using the X-Forwarded-Host header for determining the originally requested host name when marshaling the server request.

### Changed

- [#378](https://github.com/zendframework/zend-diactoros/pull/378) updates the `UploadedFile` class to extend `SplFileInfo`, allowing developers to make use of those features in their applications.

### Deprecated

- Nothing.

### Removed

- Nothing.

### Fixed

- Nothing.

## 2.2.0 - 2019-11-08

### Added

- [#377](https://github.com/zendframework/zend-diactoros/issues/377) enables UploadedFile to stand in and be used as an SplFileInfo object.

### Changed

- Nothing.

### Deprecated
Expand Down Expand Up @@ -641,7 +663,7 @@ All notable changes to this project will be documented in this file, in reverse

- [#293](https://github.com/zendframework/zend-diactoros/pull/293) updates
`Uri::getHost()` to cast the value via `strtolower()` before returning it.
While this represents a change, it is fixing a bug in our implementation:
While this represents a change, it is fixing a bug in our implementation:
the PSR-7 specification for the method, which follows IETF RFC 3986 section
3.2.2, requires that the host name be normalized to lowercase.

Expand Down
2 changes: 2 additions & 0 deletions docs/book/v2/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,4 +194,6 @@ In most cases, you will not interact with the Stream object directly.
and provides abstraction around a single uploaded file, including behavior for interacting with it
as a stream or moving it to a filesystem location.

Additionally, it extends PHP's build in `SplFileInfo` object in order to provide a compatible interface wherever such an object is needed.

In most cases, you will only use the methods defined in the `UploadedFileInterface`.
7 changes: 6 additions & 1 deletion src/UploadedFile.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

namespace Zend\Diactoros;

use SplFileInfo;
use Psr\Http\Message\StreamInterface;
use Psr\Http\Message\UploadedFileInterface;

Expand All @@ -35,7 +36,7 @@
use const UPLOAD_ERR_OK;
use const UPLOAD_ERR_PARTIAL;

class UploadedFile implements UploadedFileInterface
class UploadedFile extends SplFileInfo implements UploadedFileInterface
{
const ERROR_MESSAGES = [
UPLOAD_ERR_OK => 'There is no error, the file uploaded with success',
Expand Down Expand Up @@ -102,9 +103,13 @@ public function __construct(
if ($errorStatus === UPLOAD_ERR_OK) {
if (is_string($streamOrFile)) {
$this->file = $streamOrFile;

parent::__construct($this->file);
}
if (is_resource($streamOrFile)) {
$this->stream = new Stream($streamOrFile);

parent::__construct($this->stream->getMetaData('uri'));
Copy link

@ADmad ADmad Nov 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Does SplFileInfo work with stream paths? PHP docs say the argument for SplFileInfo::__construct() should be a file path while the value of $this->stream->getMetaData('uri') could be a stream path like 'php://memory'.

As commented by @michalbundyra this patch should have more tests.

Copy link

@ADmad ADmad Nov 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

As suspected things go kaboom when trying to use some (or many) of the SplFileInfo methods if the instance is created with a stream path.

https://3v4l.org/klK0X

So I am not sure if this is a very good change.

Copy link
Member

Choose a reason for hiding this comment

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

@ADmad
Thanks for your comment! 👍

We have another discussion in the related issue report: #377

Copy link
Contributor Author

@mattsah mattsah Nov 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@ADmad now run getMTime() on a file that doesn't exist... SplFileInfo is doing exactly what it is supposed to do here. This is not going "kaboom" -- this is how it works. It's not intended to provide information about files that it can't get information for -- which includes any non existent file. Your test seems to be based on a false assumption about SplFileInfo that it requires an actually existing file... it doesn't. SplFileInfo is designed to grok information about files, which means the path you pass it MAY not exist, and it's up to a developer to use it properly.

There may, of course be edge cases that we'd want to handle differently. But the example provided is just SplFileInfo working as it does.

Copy link

@ADmad ADmad Nov 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

SplFileInfo is designed to grok information about files

Exactly, "about files", not streams. Treating a valid stream path like php://memory as non existent file path is wrong.

Copy link
Member

Choose a reason for hiding this comment

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

@mattsah —

Having the class extend SplFileInfo implies that the item returned has a relationship with the filesystem, and can be operated as if it were part of the filesystem. It also implies that users should be able to safely call its various methods.

Considering the major use case is to use a temporary stream (Diactoros creates temporary streams by default), we absolutely need stream support to work without errors. Since it wasn't, we had to revert.

Copy link
Contributor Author

@mattsah mattsah Nov 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@weierophinney

Is there a single case where SplFileInfo acts differently on a stream than it does on a non-existent file? It seems quite strange to me that SplFileInfo suggests a relationship with the filesystem while UploadedFile does not. A stream has the same relationship to the filesystem as a non-existent file. Again, the real implication (in comments) here seems to be that SplFileInfo is not about grokking information.

Additionally, if one is truly concerned with supporting streams more fully, it would be perfectly fine, for example, to overload getMTime() later and call parent if it's a file or examine stream meta data if it's a stream.

PHP uses stream URLs in places of "files" in all sorts of cases... the implication you suggest doesn't seem well supported.

Regarding major use cases and defaults... given that this did not previously extend it, I don't see any case where anyone would be relying on SplFileInfo methods... stream support would absolutely continue working as it has historically and could be improved upon to offer even more info via overloads as mentioned above in the future.

This is non-breaking, working as expected, and providing a useful feature. It may not provide all the features you want out of the gate, e.g. functional mtime on stream, but that'd make for a nice 2.2.1 -- awell.

Very disappointed here.

}

if (! $this->file && ! $this->stream) {
Expand Down
10 changes: 10 additions & 0 deletions test/UploadedFileTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use PHPUnit\Framework\TestCase;
use ReflectionProperty;
use RuntimeException;
use SplFileInfo;
use Zend\Diactoros\Stream;
use Zend\Diactoros\UploadedFile;

Expand Down Expand Up @@ -325,4 +326,13 @@ public function testMoveToRaisesExceptionWithAppropriateMessageWhenUploadErrorDe
$this->expectExceptionMessage($message);
$uploadedFile->moveTo('/tmp/foo');
}

/**
* @see https://github.com/zendframework/zend-diactoros/pull/378
*/
public function testExtendsSplFileInfo()
{
$uploaded = new UploadedFile(fopen('php://temp', 'wb+'), 0, UPLOAD_ERR_OK);
$this->assertInstanceOf(SplFileInfo::class, $uploaded);
}
}