Make UploadedFile Extend SplFileInfo #378
Conversation
|
@mattsah we need to add some unit tests for these changes |
|
Could you expand on what sorts of tests might be meaningful here? Happy to add them, but I'm a bit at a loss of what precisely to unit test as SplFileInfo is part of PHP itself. I don't know that I fully understand what case needs to be covered, but I may be less familiar with all the ways in which UploadedFile is used. |
In this case, a test that verifies that an |
|
@mattsah I've pushed a change containing a test for you. 😄 |
|
@weierophinney I would expect more tests here, as we call parent constructor with different values depends on some conditions. And there is a case when we do not call the parent constructor. I believe all these should be covered in tests. |
| } | ||
| if (is_resource($streamOrFile)) { | ||
| $this->stream = new Stream($streamOrFile); | ||
|
|
||
| parent::__construct($this->stream->getMetaData('uri')); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
So I am not sure if this is a very good change.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
Post-release, users discovered problems with `UploadedFile` extending `SplFileInfo`, particularly when a stream path is used to create the instance. Additionally, there are some differences with how `UploadedFileInterface` and `SplFileInfo` each define the return type for `getSize()`; the former allows nullification, while the latter does not, but allows raising an exception (something explicitly disallowed in PSR-7). This patch reverts the code and documentation changes associated with the patch.
Post-release, users discovered problems with `UploadedFile` extending `SplFileInfo`, particularly when a stream path is used to create the instance. Additionally, there are some differences with how `UploadedFileInterface` and `SplFileInfo` each define the return type for `getSize()`; the former allows nullification, while the latter does not, but allows raising an exception (something explicitly disallowed in PSR-7). This patch reverts the code and documentation changes associated with the patch.
This is a new feature to make UploadedFile extend SplFileInfo which enables you to pass UploadedFile instances in places where SplFileInfo typehints are required and use SplFileInfo based methods to access information about the file while still having access to the additional UploadedFile interface.
This is primarily useful for populating models/entities directly from requests wherein a supplied value needs to be an instance of SplFileInfo and wherein a service or hook later may move the files into more appropriate places at time of persistence.