-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
Can UploadedFile extend SplFileInfo ? #3
Comments
@mattsah Send a pull request, and we can evaluate! Originally posted by @weierophinney at zendframework/zend-diactoros#377 (comment) |
#378 Originally posted by @mattsah at zendframework/zend-diactoros#377 (comment) |
@weierophinney
Long story short: Reference:
Originally posted by @froschdesign at zendframework/zend-diactoros#377 (comment) |
Another notice from @michalbundyra: (https://3v4l.org/DnSXf) class A {
public function getSize(): int {
return 2;
}
}
class B extends A {
public function getSize(): ?int
{
return null;
}
}
$b = new B;
var_dump($b->getSize()); This results in:
At the moment Originally posted by @froschdesign at zendframework/zend-diactoros#377 (comment) |
I'm not sure I understand the first point regarding StreamInterface. That is, I don't see the relevance with respect to UploadedFile. UploadedFile appears to return ?int (from your own link), null if unknown. If a stream is passed to UploadedFile, even if it called getSize() directly on the stream, if the return typeof that is always ?int, then it falls within the appropriate return type for UplaodedFile. Regarding SplFileInfo. Per PSR-7 spec, the getSize() is intended to return the size as actually transferred:
Assuming this is accurate, the size transferred, could always be 0 or > 0 -- which means, while UploadedFile may not throw an exception the same as SplFileInfo, it will always report the actual size of an uploaded file... which just happens to be 0 in the event of failure or if not reported by SAPI. I think the case I'm not understanding is where UploadedFile::getSize() would return NULL -- this would certainly conflict with SplFileInfo. But under what condition does this occur? In either case, neither UploadedFile or SplFileInfo actually typehint return. If PHP is willing to break backwards compatibility, I'd think all bets are off for libraries and you're in a position to address BC breaks regardless. If PSR-7 changes the actual programmatic interface, presumably that would require a potentially significant update anyway, including requiring newer versions of PHP. Originally posted by @mattsah at zendframework/zend-diactoros#377 (comment) |
I have raised other concerns on the related pull request zendframework/zend-diactoros#378 (comment). Originally posted by @ADmad at zendframework/zend-diactoros#377 (comment) |
FIG is actually currently voting on by-laws that will allow adding typehints that conform with the already declared signatures (this would be accomplished via a two-tiered approach to allow libraries to create first a forwards-compatible version, and then break BC to adopt all typehints). As such, libraries that provide implementations MUST follow the specified contract. Originally posted by @weierophinney at zendframework/zend-diactoros#377 (comment) |
I've submitted a patch to revert the change in #379 Originally posted by @weierophinney at zendframework/zend-diactoros#377 (comment) |
Sure, but adding a typehint of With respect to other issues raised. SplFileInfo does not go "kaboom" -- it does exactly what it does in any case where the file does not exist as can easily be demonstrated by the following:
SplFileInfo is not intended to only function on existing files. You can add any path you like to it in order to try and grok information. If someone is trying to get the m time of a file they are going to have to make sure the file exists first anyhow using SplFileInfo::isFile() or something similar. It seems unfortunate that this was reverted for SplFileInfo responding as it should. Originally posted by @mattsah at zendframework/zend-diactoros#377 (comment) |
@mattsah Originally posted by @ADmad at zendframework/zend-diactoros#377 (comment) |
@ADmad I guess people better stop using PHP has gone through a long process of progressively adding stream support and stream wrappers to places where previously only normal files could be used. Pointing out that PHP is inconsistent and/or doesn't provide proper/meaningful support for streams in all such places doesn't seem like a very good argument against this feature. A stream is a non-existent file, because it's not a file... it's a stream. Originally posted by @mattsah at zendframework/zend-diactoros#377 (comment) |
PSR-7 UploadedFile and SplFileInfo represent very different things. For UploadedFile emphasis is on upload while for SplFileInfo emphasis is on the file. The fact that SAPI creates temporary files to store uploads is mostly an implementation detail. Most of the added methods make no sense for upload either. Not to mention that extending |
Is there a particular reason that UploadedFile does not extend SplFileInfo. The only conflicting interface, which isn't even really a conflict so much is
getSize()
-- would there be any interest in this? I can do a PR if the interest is there.I need this feature like yesterday and without it I'm either gonna need to look for an alternative or fork.
Originally posted by @mattsah at zendframework/zend-diactoros#377
The text was updated successfully, but these errors were encountered: