Create PSR-7 messages using PSR-17 factories #43

ajgarlag · 2018-08-01T10:54:31Z

Use any implementation of the justs approved PSR-17 to build PSR-7 HTTP messages.

ajgarlag · 2018-08-01T12:18:55Z

Tests/Factory/AbstractHttpMessageFactoryTest.php

+        $this->assertEquals(0, $file->getSize());
+        $this->assertEquals(UPLOAD_ERR_NO_FILE, $file->getError());
+        $this->assertFalse($file->getSize(), 'SplFile::getSize() returns false on error');
+        $this->assertInternalType('integer', $file->getClientSize());


Since Symfony 4.1 the UploadedFile::getClientSize method is deprecated and it just call SplFile::getSize, so the tests are failing when PHP >= 7.1 because of the symfony/http-foundation package version.

I think that this assertion can be removed without any problem, and IMHO all these assertion about the UploadedFile instance should be removed too.

What do you think?

I removed that in #45

ajgarlag · 2018-08-01T12:33:54Z

Ready for review.

There are two different errors when testing in PHP >= 7.1, but they are not related to this PR.

jakzal · 2018-08-02T15:49:34Z

.travis.yml

@@ -20,9 +20,13 @@ matrix:
        - php: 5.6
          env: COMPOSER_OPTIONS="" SYMFONY_DEPRECATIONS_HELPER=weak
        - php: 7.0
+        - php: 7.1
+        - php: 7.2
+        - php: nightly
        - php: hhvm


hhvm can be dropped.

Nyholm

Great! I requested some changes.

Lets wait until #45 is merged.

Nyholm · 2018-08-04T17:09:12Z

Factory/PsrHttpFactory.php

+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpFoundation\StreamedResponse;
+
+


Remove extra space

Nyholm · 2018-08-04T17:09:16Z

Factory/PsrHttpFactory.php

+ */
+class PsrHttpFactory implements HttpMessageFactoryInterface
+{
+


Remove extra space

Nyholm · 2018-08-04T17:11:11Z

Factory/PsrHttpFactory.php

+        $response = $response->withBody($stream);
+
+        $headers = $symfonyResponse->headers->all();
+


Remove this empty line. (Just my preference)

Nyholm · 2018-08-04T17:11:26Z

Factory/PsrHttpFactory.php

+        }
+
+
+


Remove extra empty lines.

Nyholm · 2018-08-04T17:11:52Z

Factory/PsrHttpFactory.php

+        }
+
+        $protocolVersion = $symfonyResponse->getProtocolVersion();
+        if ('1.1' !== $protocolVersion) {


Remove this if. I think we should always set protocol version

Nyholm · 2018-08-04T17:13:59Z

Factory/PsrHttpFactory.php

+            ->withCookieParams($symfonyRequest->cookies->all())
+            ->withQueryParams($symfonyRequest->query->all())
+            ->withParsedBody($symfonyRequest->request->all())
+            ->withRequestTarget($symfonyRequest->getRequestUri())


This line is not really needed. PSR7 implementation will fetch the request target from the URI

Nyholm · 2018-08-04T17:15:08Z

Tests/Factory/PsrHttpFactoryTest.php

+{
+    protected function getHttpMessageFactory()
+    {
+        if (!class_exists('Http\Factory\Diactoros\ServerRequestFactory')) {


Is this really a PSR17 implementation? I did not know they already tagged it.

Hehe. Oh sorry. That class if from the http-interop/http-factory-diactoros package.... I created that package a few years back =)

ajgarlag · 2018-08-06T06:43:45Z

Thanks @Nyholm for the review. I'll wait for #45 to be merged.

Nyholm · 2018-08-08T14:43:19Z

Could you rebase on master now? The master branch is green on travis.

ajgarlag · 2018-08-08T21:17:50Z

@Nyholm Travis is happy with this PR

Nyholm

Good. I like this. I just have some minor questions.

Nyholm · 2018-08-08T21:24:38Z

.travis.yml

        - php: 5.6
+          env: DEPENDENCIES="zendframework/zend-diactoros:^1.4.1"


Are these three lines needed? Global value is fine, right?

Global value now includes http-interop/http-factory-diactoros:^1.0 which requires psr/http-factory:^1.0 which requires PHP >= 7.

Nyholm · 2018-08-08T21:25:26Z

.travis.yml

        - php: 7.2
-          env: DEPENDENCIES="symfony/lts:^3 symfony/force-lowest:~3.4.0 zendframework/zend-diactoros:^1.4.1"
+          env: DEPENDENCIES="$DEPENDENCIES symfony/lts:^3 symfony/force-lowest:~3.4.0"


Are these two changes needed?

I needed to add http-interop/http-factory-diactoros:^1.0, so I prefer to make explicit that we are using here default dependencies combined with the symfony metapackages to force testing with LTS versions.

But http-interop/http-factory-diactoros is already in the global. Right?
So no need to add it.

But if we overwrite the dependencies here without including the previous value, this configuration will mark DiactorosFactoryTest and PsrHttpFactoryTest as skipped because zendframework/zend-diactoros and http-interop/http-factory-diactoros won't be installed.

Hehe. I’ve been reviewing this PR multiple times but I’ve always read this change wrong.

Thank you for you explanation and your patience. Of course, this change is valid.

ajgarlag · 2018-08-09T06:49:39Z

Factory/PsrHttpFactory.php

@@ -75,7 +75,6 @@ public function createRequest(Request $symfonyRequest)
            ->withCookieParams($symfonyRequest->cookies->all())
            ->withQueryParams($symfonyRequest->query->all())
            ->withParsedBody($symfonyRequest->request->all())
-            ->withRequestTarget($symfonyRequest->getRequestUri())


@Nyholm After removing this line, Travis is unhappy. The RequestInterface::getRequestTarget() method should return the query string raw value or the urldecoded value?

See https://travis-ci.org/symfony/psr-http-message-bridge/jobs/413915386#L518

Just viewed your comment in the test. Thanks.

Nyholm · 2018-08-09T06:49:55Z

Tests/Factory/AbstractHttpMessageFactoryTest.php

+        $this->assertEquals('42', $queryParams['bar']['baz']);
+
+        $requestTarget = $psrRequest->getRequestTarget();
+        $this->assertEquals('/testCreateRequest?foo=1&bar[baz]=42', $requestTarget);


You must “urldecode($requestTarget)”

It is a valid response from the psr 7 implementation.

Nyholm

Thank you for this PR. I like it.

ajgarlag · 2018-08-22T10:23:13Z

Is there anything I can do to get this PR merged?

nicolas-grekas

LGTM thanks (with some minor comments)

nicolas-grekas · 2018-08-30T11:47:43Z

Factory/PsrHttpFactory.php

+    private $uploadedFileFactory;
+    private $responseFactory;
+
+    public function __construct(


the symfony code-style is to use single lines signatures

nicolas-grekas · 2018-08-30T11:48:19Z

Factory/PsrHttpFactory.php

+
+        foreach ($uploadedFiles as $key => $value) {
+            if (null === $value) {
+                $files[$key] = $this->uploadedFileFactory->createUploadedFile(


I think we prefer a long line here

nicolas-grekas · 2018-08-30T11:49:21Z

Factory/PsrHttpFactory.php

+                ob_start(function ($buffer) use ($stream) {
+                    $stream->write($buffer);
+
+                    return false;


this should return the empty string instead I suppose (please update DiacrotosFactory also it has the same "issue")

Nyholm

Excellent. Thank you!

fabpot · 2018-08-30T16:24:38Z

Thank you @ajgarlag.

This PR was squashed before being merged into the 1.0-dev branch (closes #43). Discussion ---------- Create PSR-7 messages using PSR-17 factories Use any implementation of the justs approved PSR-17 to build PSR-7 HTTP messages. Commits ------- dd81b4b Create PSR-7 messages using PSR-17 factories

stof · 2018-08-31T11:06:21Z

Factory/PsrHttpFactory.php

+            $request = $request->withHeader($name, $value);
+        }
+
+        if (PHP_VERSION_ID < 50600) {


the constant should be fully qualified, to allow the compiler to optimize this condition

and do we even care about PHP 5.5 here ? AFAICT, the PSR factory does not support it

ajgarlag force-pushed the feature/psr17-http-factories branch 5 times, most recently from e59727a to a6d4b9c Compare August 1, 2018 12:12

ajgarlag commented Aug 1, 2018

View reviewed changes

ajgarlag changed the title ~~[WIP] Create PSR-7 messages using PSR-17 factories~~ Create PSR-7 messages using PSR-17 factories Aug 1, 2018

jakzal reviewed Aug 2, 2018

View reviewed changes

This was referenced Aug 4, 2018

Supprt for ANY psr7 implementation #46

Closed

Fixed broken build #45

Merged

Nyholm requested changes Aug 4, 2018

View reviewed changes

ajgarlag force-pushed the feature/psr17-http-factories branch 2 times, most recently from 75dc839 to 756cf14 Compare August 8, 2018 15:54

ajgarlag changed the title ~~Create PSR-7 messages using PSR-17 factories~~ [WIP] Create PSR-7 messages using PSR-17 factories Aug 8, 2018

ajgarlag force-pushed the feature/psr17-http-factories branch from 756cf14 to 3635172 Compare August 8, 2018 21:12

ajgarlag changed the title ~~[WIP] Create PSR-7 messages using PSR-17 factories~~ Create PSR-7 messages using PSR-17 factories Aug 8, 2018

Nyholm reviewed Aug 8, 2018

View reviewed changes

ajgarlag commented Aug 9, 2018

View reviewed changes

Nyholm reviewed Aug 9, 2018

View reviewed changes

Nyholm approved these changes Aug 9, 2018

View reviewed changes

nicolas-grekas approved these changes Aug 30, 2018

View reviewed changes

dunglas approved these changes Aug 30, 2018

View reviewed changes

Nyholm approved these changes Aug 30, 2018

View reviewed changes

Create PSR-7 messages using PSR-17 factories

dd81b4b

fabpot force-pushed the feature/psr17-http-factories branch from 8aab743 to dd81b4b Compare August 30, 2018 16:24

fabpot merged commit dd81b4b into symfony:master Aug 30, 2018

stof reviewed Aug 31, 2018

View reviewed changes

		use Symfony\Component\HttpFoundation\Response;
		use Symfony\Component\HttpFoundation\StreamedResponse;

		$response = $response->withBody($stream);

		$headers = $symfonyResponse->headers->all();

		- php: 5.6
		env: DEPENDENCIES="zendframework/zend-diactoros:^1.4.1"

Uh oh!

Create PSR-7 messages using PSR-17 factories #43

Create PSR-7 messages using PSR-17 factories #43

Uh oh!

Conversation

ajgarlag commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajgarlag commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajgarlag commented Aug 6, 2018

Uh oh!

Nyholm commented Aug 8, 2018

Uh oh!

ajgarlag commented Aug 8, 2018

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

ajgarlag commented Aug 22, 2018

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm left a comment