Comments (12)
@mattsah Send a pull request, and we can evaluate!
Originally posted by @weierophinney at zendframework/zend-diactoros#377 (comment)
from laminas-diactoros.
#378
Originally posted by @mattsah at zendframework/zend-diactoros#377 (comment)
from laminas-diactoros.
@weierophinney
The pull request do not pay attention to the different behaviour:
Psr\Http\Message\StreamInterface
defines the method getSize()
with a return value of ?int
and SplFileInfo::getSize
with int
. And additionally SplFileInfo::getSize
: "An exception is thrown if the file does not exist or an error occurs."
Long story short: ?int
vs. int
and exception
Reference:
- https://www.php-fig.org/psr/psr-7/#36-psrhttpmessageuploadedfileinterface
- https://www.php.net/manual/en/splfileinfo.getsize.php
Originally posted by @froschdesign at zendframework/zend-diactoros#377 (comment)
from laminas-diactoros.
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:
Fatal error: Declaration of B::getSize(): ?int must be compatible with A::getSize(): in
At the moment SplFileInfo
does not define an explicit return type for getSize
method but it could become a problem in the future.
Originally posted by @froschdesign at zendframework/zend-diactoros#377 (comment)
from laminas-diactoros.
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:
* Implementations SHOULD return the value stored in the "size" key of
* the file in the $_FILES array if available, as PHP calculates this based
* on the actual size transmitted.
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)
from laminas-diactoros.
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)
from laminas-diactoros.
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.
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)
from laminas-diactoros.
I've submitted a patch to revert the change in #379
Originally posted by @weierophinney at zendframework/zend-diactoros#377 (comment)
from laminas-diactoros.
Sure, but adding a typehint of ?int
to getSize()
will not actually cause an issue when extending SplFileInfo as it predates strict interfaces, that is SplFileInfo says it returns int or throws exception, but it does not have a typehint that would require compatibility in the first place and is implicitly a mixed result in terms of actual code.
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:
matt@havana:~$ php -r "(new SplFileInfo('/tmp/test'))->getMTime();"
PHP Fatal error: Uncaught RuntimeException: SplFileInfo::getMTime(): stat failed for /tmp/test in Command line code:1
Stack trace:
#0 Command line code(1): SplFileInfo->getMTime()
#1 {main}
thrown in Command line code on line 1
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)
from laminas-diactoros.
@mattsah SplFileInfo
is meant for file paths (whether they exist or not). It's not meant to be used for stream paths. Treating stream paths as non existent file paths is simply not right (especially when the stream path is valid).
Originally posted by @ADmad at zendframework/zend-diactoros#377 (comment)
from laminas-diactoros.
@ADmad I guess people better stop using rename()
for streams then, since according to the documentation "rename — Renames a file or directory" and a stream is neither of those.
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)
from laminas-diactoros.
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 UploadedFile
from SplFileInfo
will add a lot of interface surface that would need to be maintained for very little benefit that would not align with the interchangeable goals of PSR-7.
from laminas-diactoros.
Related Issues (20)
- `FilterUsingXForwardedHeaders` should correctly deal with `<host>:<port>` pair in `X-FORWARDED-HOST` header HOT 9
- [RFC]: Allow better constraint handling for PHP HOT 2
- Could ServerRequestFactory::marshallUriFromSapi() be made public? HOT 4
- Update to PSR-7 1.1/2.0 HOT 2
- Remove image stream compatibility from `Stream`
- CVE-2023-29530: Fix For PHP 7.4 HOT 16
- CLI command to register diactoros as pinned for `php-http/discovery`
- Drop deprecated function marshalUriFromSapi
- PhpInputStream::getContent() inconsistency HOT 9
- RFC: Read php input stream content into php temp stream to allow all stream features in PhpInputStream HOT 1
- Numeric header names handling in PSR-7 message objects
- V3 getBody()->getContents() no longer returns full stream on second call HOT 3
- `composer.json` provides non-existing versions of `psr/http-factory`
- security vonerability HOT 1
- Plus signs in cookie data get converted to space.
- marshal_headers_from_sapi.php line 29 HOT 2
- `Uri::__toString()` can yield malformed URIs HOT 2
- 2.x series does not support PHP 8.3 HOT 20
- [RFC]: Remove `scheme` filtering HOT 3
- Malformed request causes 500 response HOT 7
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from laminas-diactoros.