Support swoole_http_request #304

ezimuel · 2018-05-30T15:24:16Z

This PR adds a ServerRequestFactory::fromSwoole static function to transform a Swoole request (swoole_http_request) into a PSR-7 request.

Moreover, it provides a Zend\Diactoros\Response\SwooleEmitter to convert a PSR-7 response to swoole_http_response.

The idea is to offer the possibility to use zend-diactoros (and Expressive) with Swoole out-of-the-box.

Ocramius · 2018-05-30T21:37:52Z

src/ServerRequestFactory.php

@@ -114,10 +114,10 @@ public static function fromSwoole(swoole_http_request $request)
        if (! extension_loaded('swoole')) {
            throw new Exception\RuntimeException('Swoole extension is not installed!');
        }
-        $get    = $request->get ?? [];


AAAAAAAAAAAAAAAAA WE STILL SUPPORT PHP 5?!

@Ocramius In 1.x, yes. For 2.0, we'll drop 5.6 and 7.0 support. I'm waiting on PSR-17 to finalize first, though.

weierophinney · 2018-05-30T22:32:09Z

src/ServerRequestFactory.php

+                                 $request->header['connection'] : '',
+            'HTTP_CACHE_CONTROL' => isset($request->header['cache-control']) ?
+                                    $request->header['cache-control'] : '',
+        ];


I'd argue we shouldn't populate most of the above under Swoole; most of these are things that an SAPI populates because there is no other way to get at headers, the request URI parts, etc.. Since we do have a way to get at those, I'd argue we should only populate the SERVER_* and REMOTE_* values, and maybe the GATEWAY_INTERFACE and REQUEST_TIME.

Also, please align these as follows:

'key' => isset(...) ? ... : ...,

When it comes to marshaling the URI, we have the various parts in the Swoole request, and can pass a smaller array in situ when calling marshalUriFromServer():

static::marshalUriFromServer([ /* values we need here */ ], $headers)

This is the var_dump of a Swoole HTTP request, using this simple test example.

object(Swoole\Http\Request)#7 (9) { ["fd"]=> int(1) ["header"]=> array(5) { ["host"]=> string(14) "localhost:9501" ["user-agent"]=> string(12) "HTTPie/0.9.8" ["accept-encoding"]=> string(13) "gzip, deflate" ["accept"]=> string(3) "*/*" ["connection"]=> string(10) "keep-alive" } ["server"]=> array(11) { ["request_method"]=> string(3) "GET" ["request_uri"]=> string(1) "/" ["path_info"]=> string(1) "/" ["request_time"]=> int(1527775742) ["request_time_float"]=> float(1527775742.4968) ["server_port"]=> int(9501) ["remote_port"]=> int(32922) ["remote_addr"]=> string(9) "127.0.0.1" ["master_time"]=> int(1527775742) ["server_protocol"]=> string(8) "HTTP/1.1" ["server_software"]=> string(18) "swoole-http-server" } ["request"]=> NULL ["cookie"]=> NULL ["get"]=> NULL ["files"]=> NULL ["post"]=> NULL ["tmpfiles"]=> NULL }

I think we should include all these values, what do you think?

weierophinney · 2018-05-30T22:33:03Z

src/ServerRequestFactory.php

+            static::normalizeFiles($files),
+            static::marshalUriFromServer($server, $headers),
+            $server['REQUEST_METHOD'],
+            $request->rawContent(),


Is there a possibility to retrieve a stream resource, by any chance? If so, we could decorate that in a Stream instance, which would be better.

It seems that swoole_http_request contains only the body message as string.

This definitely won't work as-is. ServerRequestFactory::initialize() accepts the following types of $body arguments:

A StreamInterface instance.

A string representing a stream URI to open.

A PHP resource.

Passing just the contents will not work; it needs to be wrapped in a Stream instance first.

ezimuel · 2018-05-31T15:21:38Z

@weierophinney I reduced the parameters for converting a Swoole request to PSR-7. I followed the var_dump output (see my previous comment) of a Swoole request.

ezimuel · 2018-06-05T15:37:20Z

@weierophinney, @Ocramius I also added the Swoole emitter. We can now convert a swoole_http_request to PSR-7 and a PSR-7 response to swoole_http_response.

`ServerRequestFactory` was incorrectly passing the body to the new request constructor. The content being passed was string content, and needs to be either a stream URI, resource, or `StreamInterface` instance. Test was updated to demonstrate this, and the factory was updated to create the stream. A number of consistency updates were also made: - License annotation in headers of affected files. - Flow of mock expectations.

But only in PHP 7 environments. Since the package has required prompts, this uses a script that has values for each prompt.

weierophinney · 2018-06-05T21:53:15Z

@ezimuel I figured out how to get Swoole installed on Travis, and pushed the changes.

Only thing remaining at this point is documentation; can you create and register a "Swoole" chapter in the docs for me, please? I can then finalize the merge.

Thanks!

ezimuel · 2018-06-06T13:12:41Z

@weierophinney Just added the documentation.

weierophinney · 2018-06-06T14:08:15Z

doc/book/swoole.md

@@ -0,0 +1,54 @@
+# Swoole
+use Zend\Diactoros\ServerRequestFactory;


I think this line belongs above line 15?

weierophinney · 2018-06-06T14:14:52Z

src/ServerRequestFactory.php

+            $headers[str_replace('-', '_', $name)] = $value;
+        }
+
+        $body = new Stream('php://temp', 'wb+');


I've been thinking about this, and I'm worried this could be a performance issue and/or security vector in some cases; in particular, requests that include file uploads and/or requests with large requests bodies.

As such, I'm thinking we should create a proxy decorator class for the swoole request body that extends the current default Stream implementation:

It would receive the request to the constructor.

Any method that would normally access the underlying stream resource would:

check if the request body has been requested before

if not, it would create the php://temp stream and write the request body to it, and assign it via a call to parent::construct().

otherwise, it would proxy to the parent method

In this way, we prevent duplication of the request contents until they are accessed, which alleviates the majority of issues (particularly since the file uploads will normally already be handled).

ezimuel · 2018-06-12T14:19:14Z

@weierophinney I created a SwooleStream class implementing the StreamInterface with the rawcontent() proxy usage that you proposed. I also improved ServerRequestFactory::fromSwoole() with additional checks.
Moreover, I removed the SwooleEmitter class in favor of a new component zendframework/zend-expressive-swoole.

malukenho · 2018-06-14T13:53:06Z

doc/book/swoole.md

+
+This method is as follows:
+
+```


malukenho · 2018-06-14T13:53:34Z

src/ServerRequestFactory.php

+     * @return ServerRequest
+     * @throws RuntimeException if Swoole is not installed
+     */
+


Remove empty line

malukenho · 2018-06-14T13:55:30Z

src/SwooleStream.php

+use RuntimeException;
+use swoole_http_request;
+
+class SwooleStream implements StreamInterface


Why is it not final?

malukenho · 2018-06-14T13:57:17Z

test/SwooleStreamTest.php

+<?php
+/**
+ * @see       https://github.com/zendframework/zend-diactoros for the canonical source repository
+ * @copyright Copyright (c) 2015-2017 Zend Technologies USA Inc. (http://www.zend.com)


malukenho · 2018-06-14T13:57:40Z

test/SwooleStreamTest.php

+{
+    const DEFAULT_CONTENT = 'This is a test!';
+
+    public function setUp()


Shouldn't it be protected to respect the interface?

Moln · 2018-06-20T00:53:48Z

Uh..... split to zend-diactoros-swoole, please.

Ocramius · 2018-06-20T06:14:01Z

src/ServerRequestFactory.php

+
+    public static function fromSwoole(swoole_http_request $request)
+    {
+        if (! extension_loaded('swoole')) {


Doesn't look like a good idea: as commented by @Moln, this should be a hard requirement, with an adapter package enforcing it

@Ocramius, @weierophinney maybe zend-expressive-swoole is a better fit for this PR?

ezimuel · 2018-06-26T13:35:17Z

@weierophinney, @Ocramius, @Moln I already prepared #306 to move the Swoole outside zend-diactoros (e.g. in zend-expressive-swoole).

ezimuel · 2018-06-26T15:18:05Z

Closing this PR and moving it to zend-expressive-swoole, waiting for #306 merge.

Imports the changes proposed in zendframework/zend-diactoros#304, updating the code to work with zend-diactoros 1.8.0. This includes: - Using appropriate Diactoros functions to create `ServerRequest` artifacts within the `ServerRequestSwooleFactory`. - Updating the `SwooleStream` to follow all PSR-7 semantics, including the ability to `getContents()` from the current `$index` position.

ezimuel added 2 commits May 30, 2018 17:17

Support swoole_http_request

6113356

Remove ?? usage for PHP 5.6 support

a51f767

Ocramius reviewed May 30, 2018

View reviewed changes

weierophinney suggested changes May 30, 2018

View reviewed changes

Reduced the params conversion from Swoole to PSR-7

6ffa7f6

Added Swoole emitter

08a46aa

ezimuel changed the title ~~Support swoole_http_request~~ Support swoole_http_request and swoole_http_response Jun 5, 2018

ezimuel and others added 2 commits June 5, 2018 18:07

Added the test for swoole extension in test

2cecc83

weierophinney force-pushed the feature/swoole branch 2 times, most recently from ba59d32 to 6266af0 Compare June 5, 2018 21:19

Install swoole extension in CI environment

56ae7d2

But only in PHP 7 environments. Since the package has required prompts, this uses a script that has values for each prompt.

weierophinney force-pushed the feature/swoole branch from 6266af0 to 56ae7d2 Compare June 5, 2018 21:46

weierophinney approved these changes Jun 5, 2018

View reviewed changes

Added the documentation for Swoole

c557ee6

weierophinney suggested changes Jun 6, 2018

View reviewed changes

Added a SwooleStream class and removed Swoole Emitter

cb21c21

ezimuel added 3 commits June 12, 2018 16:21

Fixed the Swoole docs

9dda5f9

Fixed PHP 5.6 compatibility

66c56a0

Added the Swoole skip test

9262404

ezimuel changed the title ~~Support swoole_http_request and swoole_http_response~~ Support swoole_http_request Jun 13, 2018

malukenho reviewed Jun 14, 2018

View reviewed changes

Ocramius suggested changes Jun 20, 2018

View reviewed changes

ezimuel mentioned this pull request Jun 26, 2018

Static functions to public in ServerRequestFactory #306

Closed

ezimuel closed this Jun 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support swoole_http_request #304

Support swoole_http_request #304

ezimuel commented May 30, 2018 •

edited

Loading

Ocramius May 30, 2018

weierophinney May 30, 2018

weierophinney May 30, 2018

ezimuel May 31, 2018

weierophinney May 30, 2018

ezimuel May 31, 2018

weierophinney Jun 5, 2018

ezimuel commented May 31, 2018

ezimuel commented Jun 5, 2018

weierophinney commented Jun 5, 2018

ezimuel commented Jun 6, 2018

weierophinney Jun 6, 2018

weierophinney Jun 6, 2018

ezimuel commented Jun 12, 2018 •

edited

Loading

malukenho Jun 14, 2018

malukenho Jun 14, 2018

malukenho Jun 14, 2018

malukenho Jun 14, 2018

malukenho Jun 14, 2018

Moln commented Jun 20, 2018

Ocramius Jun 20, 2018

ezimuel Jun 26, 2018

Ocramius Jun 26, 2018

ezimuel commented Jun 26, 2018

ezimuel commented Jun 26, 2018

		@@ -0,0 +1,54 @@
		# Swoole
		use Zend\Diactoros\ServerRequestFactory;

Support swoole_http_request #304

Support swoole_http_request #304

Conversation

ezimuel commented May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezimuel commented May 31, 2018

ezimuel commented Jun 5, 2018

weierophinney commented Jun 5, 2018

ezimuel commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezimuel commented Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Moln commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezimuel commented Jun 26, 2018

ezimuel commented Jun 26, 2018

ezimuel commented May 30, 2018 •

edited

Loading

ezimuel commented Jun 12, 2018 •

edited

Loading