ServerRequest and UploadedFIle Implementation #54

ivank · 2015-12-23T14:00:31Z

This is an implementation of ServerRequest and UploadedFIle and closes #13.

I realize there is already a PR for that (#32) but it seams sort-of abandoned. I was waiting on that functionality as I need it in one of my projects, so seeing this PR stagnate I decided to reimplement it myself.
It has almost full test coverage, the $_FILES array conversion handles more cases, the moveTo method is according to spec and I try to use the already available functionality as much as possible

Merry Christmas 🎄 :)

ivank · 2016-01-06T13:39:53Z

@mtdowling post holiday ping, hope you guys are doing well :)

mtdowling · 2016-01-07T21:14:12Z

Thanks for putting this together!

I haven't reviewed this yet, but one thing I want to ensure is that if this is work based on #32, then I'd prefer it if you include the original author's commits in the history so we can ensure credit is given in the log. If that is not the case, then no worries.

jmatosp · 2016-01-07T23:50:19Z

src/ServerRequest.php

+
+        return $uri
+            ->withScheme(isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on' ? 'https' : 'http')
+            ->withHost($_SERVER['SERVER_NAME'])


Sometimes this is empty, throwing an PHP Notice undefined index

ivank · 2016-01-08T13:01:32Z

@mtdowling I haven't based this on @SaphirAngel pull request, rather copy-pasted small amounts of code here and there, then heavily modified, I thought basing it on his work and then radically changing everything will make little sense to future reviewers of the code, (since this is not how this code came to be) but if you prefer I could certainly do it like that.

Another approach would be to rewrite the commits and mention him explicitly if you think that will be more appropriate.

10x @jmatosp will certainly fix those

jmatosp · 2016-01-08T14:10:23Z

I haven't seen the code, but keep an eye on basic benchmarks... so far so good

diactoros psr7 from globals: 22,249 reqs/s
symfony request from globals: 46,943 reqs/s
Guzzle psr7 from globals: 219,159 reqs/s

This is just instance creation

https://gist.github.com/jmatosp/bbb33c8121e6cc106615

ivank · 2016-01-12T12:04:38Z

Well they do quite a lot of additional processing to make the data appear in a unified interface, while I just stick to the PSR-7 standard and don't add any more stuff than necessary. I think this is a good approach as anything more can be built on top of this afterwards.

ivank · 2016-01-22T09:44:38Z

@mtdowling @jmatosp any idea when you guys could take a look at this and tell me if you need something else improved or you think its a good enough MVP? I know you are probably very busy people and hate to nag but it would be really great to not need to rely on composer repository hacks for a project I'm working on (https://github.com/spiderling-php more specifically - https://github.com/spiderling-php/kohana-driver/blob/master/src/Loader.php and some other repos in this project too).

webda2l · 2016-01-22T16:46:46Z

src/UploadedFile.php

+     */
+    private function isStringNotEmpty($param)
+    {
+        return is_string($param) && false == empty($param);


strict comparison ===

...or !empty($params)?

webda2l · 2016-01-22T16:48:41Z

Hope a merge too.
I'll just move static functions of ServerRequest class after __construct, more usual way.

jeremeamia · 2016-01-22T17:02:08Z

src/ServerRequest.php

+            $uri = $uri->withScheme($_SERVER['HTTPS'] == 'on' ? 'https' : 'http');
+        }
+
+        if (isset($_SERVER['SERVER_NAME'])) {


When $_SERVER['HTTP_HOST'] is defined, I think it contains better information. I think Diactoros and Symfony both prioritize using that value.

Yeah the long SO question has all of those issues explained. I read a bit deeper and indeed due to some bugs in PHP, HTTP_HOST is what we need. And as you said it is true that others already prioritize it, and it must be working for them.

IMO relying on the Host header is way more insecure than server variables. Anyone can modify the host header. But if the server variables are broken then it's more likely a broken webserver setup. So the host header, if used at all, should have lower priority than the SERVER_NAME.

Also the implementation using the host header is broken as the host header can also include the port. So it needs to be excluded before calling ->withHost.

jeremeamia · 2016-01-22T17:07:09Z

Any advantages or disadvantages to using filter_input vs. directly accessing the super globals?

ivank · 2016-01-22T22:13:51Z

To tell you the truth never heard of "filter_input" till now :) It does look neat, but it might be harder to write tests for. I can certainly rewrite it using filter_input instead of direct superglobal access, but I don't really see what's the difference, apart from longer code ...

jeremeamia · 2016-01-22T22:20:06Z

It actually makes it impossible to test as far as I can tell, and there doesn't seem to be any strong benefits. So, we'll table that.

aurimasniekis · 2016-01-23T08:47:47Z

Any progress on this? Would like to use this instead own implementation on my http server

ivank · 2016-01-26T14:28:51Z

Well I hope I've addressed all the comments from the team, waiting for the next wave :)

webda2l · 2016-01-29T15:34:01Z

src/ServerRequest.php

+     */
+    private $uploadedFiles;
+
+    /**


Outdated phpdoc

@webda2l sorry didn't get this one, how should this be documented?

__construct params don't matches phpdoc.

@param string $method @param string|Uri $uri ....

ivank · 2016-02-13T07:42:40Z

@webda2l, @GCDS updated the phpdocs, you guys should really look into scrutinizer as it handles all of these psr standards checking, phpdoc updating and code coverage automatically. I have it on for most of my OSS projects and I've kind of don't really pay that much attention to it anymore, sorry.

Example: https://scrutinizer-ci.com/g/clippings/composer-init/ more specifically: https://scrutinizer-ci.com/g/clippings/composer-init/inspections/73324ada-d30e-4d37-aba7-0fef471ba659/patches

webda2l · 2016-02-14T23:04:36Z

@ivank Yes true. This is not my project here, just a developer who use your fork and hope a merge. But for my own projects and projects clients, I use scrutinizer too, it ease this kind of review, yes.

aurimasniekis · 2016-02-15T02:58:07Z

src/ServerRequest.php

+    /**
+     * @var array
+     */
+    private $serverParams = [];


Does not require initial value as constructor passes default value []

Took some idaes and code snippets from guzzle#32

Took ideas and some code snippets from guzzle#32

Since this is almost completely modified

This value is set by the default argument in the constructor

ivank · 2016-02-27T08:33:08Z

@mtdowling, @jeremeamia I've rebased this with latest master. It would be fantastic if you guys could take a look and tell me what else needs to be done to get this thing merged?

webda2l · 2016-03-14T14:45:58Z

@guzzle Will nice to merge this PR while there is no conflicts.
Other PSR-7 like SLIM one, progress http://www.slimframework.com/2016/03/10/slim-3.3.0.html

webda2l · 2016-03-16T16:47:14Z

Ẁill be better to adding this patch before merge I think

diff --git a/src/functions.php b/src/functions.php
index 921a5a8..65ee687 100644
--- a/src/functions.php
+++ b/src/functions.php
@@ -234,6 +234,19 @@
         $uri = $uri->withQuery($changes['query']);
     }

+    if ($request instanceof ServerRequest) {
+        return new ServerRequest(
+            isset($changes['method']) ? $changes['method'] : $request->getMethod(),
+            $uri,
+            $headers,
+            isset($changes['body']) ? $changes['body'] : $request->getBody(),
+            isset($changes['version'])
+                ? $changes['version']
+                : $request->getProtocolVersion(),
+            $request->getServerParams()
+        );
+    }
+
     return new Request(
         isset($changes['method']) ? $changes['method'] : $request->getMethod(),
         $uri,

mtdowling · 2016-03-17T21:59:46Z

@ivank Thanks for all the work you put into this. Can you comment on if you would be able to help me and the rest of the Guzzle team to help support and maintain this functionality?

ivank · 2016-03-21T12:39:00Z

@mtdowling sure I can help, though as of last month I don't really work with PHP all that much so might be a bit slow in participating.

mtdowling · 2016-03-21T19:59:27Z

Thanks @ivank! I like that most of the implementation details are private, which will make this easier to maintain IMO. 🎈

ServerRequest and UploadedFIle Implementation

Tobion · 2016-06-03T21:05:44Z

src/ServerRequest.php

+     * @throws InvalidArgumentException for unrecognized values
+     * @return array
+     */
+    public static function normalizeFiles(array $files)


I don't think this should be public.

barryvdh · 2016-06-04T12:47:41Z

@ivank Could you update the readme with docs/examples for the new methods?

Tobion · 2016-06-05T16:33:02Z

How to determine the request uri is also standarized: http://tools.ietf.org/html/rfc7230#section-5.5
They use the Host header as well. So I assume the webserver must really be configured to not just blindly accept any host header.

iotch · 2016-07-29T18:49:21Z

Hello.
I think apache_request_headers()/getallheaders() polyfill should be provided for this:
https://github.com/guzzle/psr7/blob/master/src/ServerRequest.php#L169
as those functions are only available when running PHP as an Apache module.

jmatosp reviewed Jan 7, 2016
View reviewed changes

ivank force-pushed the server-request branch from a172569 to f494712 Compare January 8, 2016 13:18

webda2l reviewed Jan 22, 2016
View reviewed changes

jeremeamia reviewed Jan 22, 2016
View reviewed changes

webda2l reviewed Jan 29, 2016
View reviewed changes

caleblloyd mentioned this pull request Feb 12, 2016

Abstract Super Globals such as $_SERVER for each request into \yii\web\Request yiisoft/yii2#10824

Closed

aurimasniekis reviewed Feb 15, 2016
View reviewed changes

ivank added 13 commits February 27, 2016 10:05

Add uploaded file and tests

e4a804a

Took some idaes and code snippets from guzzle#32

Add ServerRequest and tests

2b282b9

Took ideas and some code snippets from guzzle#32

Use more accurate test data

93f4583

Fix getUriFromGlobals to work with CLI

8d93b1c

Move static methods below constructor

4ca5ac8

Use stricter style

a73f6c6

Prioritize HTTP_HOST before SERVER_NAME

29fa3d6

Remove obsolete copyright

efa4c6f

Since this is almost completely modified

Use a stricter style

9e75fe3

Update dockblock consistency

cd25a41

Update phpdocs

d320ba3

Remove redundant initial value

264386e

This value is set by the default argument in the constructor

Make sure getUploadedFiles always returns array

69765f4

ivank force-pushed the server-request branch from db61652 to 69765f4 Compare February 27, 2016 08:30

mtdowling added a commit that referenced this pull request Mar 21, 2016

Merge pull request #54 from ivank/server-request

ee6a9b7

ServerRequest and UploadedFIle Implementation

mtdowling merged commit ee6a9b7 into guzzle:master Mar 21, 2016

mtdowling mentioned this pull request Mar 21, 2016

Missing implementation #32

Closed

webda2l mentioned this pull request Mar 24, 2016

[RFR] Extend modify_request support to ServerRequest #75

Merged

ivank deleted the server-request branch March 27, 2016 08:38

Tobion reviewed Jun 3, 2016
View reviewed changes

Tobion mentioned this pull request Nov 1, 2016

SeverRequest::fromGlobals only works on Apache #126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ServerRequest and UploadedFIle Implementation #54

ServerRequest and UploadedFIle Implementation #54

ivank commented Dec 23, 2015

ivank commented Jan 6, 2016

mtdowling commented Jan 7, 2016

jmatosp Jan 7, 2016

ivank commented Jan 8, 2016

jmatosp commented Jan 8, 2016

ivank commented Jan 12, 2016

ivank commented Jan 22, 2016

webda2l Jan 22, 2016

jeremeamia Jan 22, 2016

webda2l commented Jan 22, 2016

jeremeamia Jan 22, 2016

ivank Jan 22, 2016

Tobion Jun 3, 2016

jeremeamia commented Jan 22, 2016

ivank commented Jan 22, 2016

jeremeamia commented Jan 22, 2016

aurimasniekis commented Jan 23, 2016

ivank commented Jan 26, 2016

webda2l Jan 29, 2016

ivank Feb 10, 2016

webda2l Feb 10, 2016

ivank commented Feb 13, 2016

webda2l commented Feb 14, 2016

aurimasniekis Feb 15, 2016

ivank commented Feb 27, 2016

webda2l commented Mar 14, 2016

webda2l commented Mar 16, 2016

mtdowling commented Mar 17, 2016

ivank commented Mar 21, 2016

mtdowling commented Mar 21, 2016

Tobion Jun 3, 2016

barryvdh commented Jun 4, 2016

Tobion commented Jun 5, 2016

iotch commented Jul 29, 2016

ServerRequest and UploadedFIle Implementation #54

ServerRequest and UploadedFIle Implementation #54

Conversation

ivank commented Dec 23, 2015

ivank commented Jan 6, 2016

mtdowling commented Jan 7, 2016

Choose a reason for hiding this comment

ivank commented Jan 8, 2016

jmatosp commented Jan 8, 2016

ivank commented Jan 12, 2016

ivank commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webda2l commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremeamia commented Jan 22, 2016

ivank commented Jan 22, 2016

jeremeamia commented Jan 22, 2016

aurimasniekis commented Jan 23, 2016

ivank commented Jan 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivank commented Feb 13, 2016

webda2l commented Feb 14, 2016

Choose a reason for hiding this comment

ivank commented Feb 27, 2016

webda2l commented Mar 14, 2016

webda2l commented Mar 16, 2016

mtdowling commented Mar 17, 2016

ivank commented Mar 21, 2016

mtdowling commented Mar 21, 2016

Choose a reason for hiding this comment

barryvdh commented Jun 4, 2016

Tobion commented Jun 5, 2016

iotch commented Jul 29, 2016