Show lines of code, psr7 compatibility (percentage) and psr17 compatibility

Why is https://github.com/Nyholm/psr7/blob/master/src/Factory/Psr17Factory.php#L69 the factory not initialized with the body?

I figured out that there is a separate package (Nyholm\Psr7Server). OK, But why!?

Now I have to add another dependency and I basically have to copy and paste the code from the docs into my own ServerRequestFactory? Why?

I had the hope to just use this package and start off with a working server request by using the factory included in this package.

<?php
declare(strict_types=1);

namespace App\Infrastructure\Http;

use Nyholm\Psr7\Factory\Psr17Factory;
use Nyholm\Psr7\ServerRequest;
use Nyholm\Psr7Server\ServerRequestCreator;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestFactoryInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Message\UriInterface;

class ServerRequestFactory implements ServerRequestFactoryInterface
{
    public function createServerRequest(string $method, $uri, array $serverParams = []): ServerRequestInterface
    {
        $psr17Factory = new Psr17Factory();

        $creator = new ServerRequestCreator(
            $psr17Factory, // ServerRequestFactory
            $psr17Factory, // UriFactory
            $psr17Factory, // UploadedFileFactory
            $psr17Factory  // StreamFactory
        );

        return $creator->fromGlobals();
    }
}

Hi,

Very nice piece of code.
Just a silly question, and i want your insights.

why not have the factories in a separate project ? factories are not (yet) official psr specs and the fact to mix some strict psr implementation and some interop implementation in the same project seems a bit strange in my opinion.
Is it an anticipation of the standards ?

Hi, first off, great job, the library is great, love it.
One question though - why is the Request class final? The guzzle implementation isn't final and I see very little reason this one should be...
Is there an actual reason for it or was it just a choice you've made?
I'm using this library to create a compatible request via the interface but now instead of just doing new WhateverRequest($data) I have to do basically (new WhateverRequest($data))->getRequest() or WhateverRequest::create($data) to achieve the same effect.

See if we can remove features and add type hints

I need to extend the class

@Zegnat what do you think? Release 0.2.0?

I have the following library code, which is implemented in another Symfony application with httplug and auto discovery.

// Both `$requestFactory` and `$streamFactory` are the same instance of `Nyholm\Psr7\Factory\HttplugFactory` (through auto discovery).

$request = $requestFactory->createRequest($method, $url);

$body = json_encode(['test' => 'test']);
$request->withBody($streamFactory->createStream($body));

echo (string)$request->getBody(); // empty string "" (!)

I've traced this problem to the MessageTrait, the withBody method (line 126), does this:

        $new = clone $this;
        $new->stream = $body;  // Note: `stream` is a PRIVATE property and that's probably why this does nothing.

If I change the code to this, it works as expected:

        $this->stream = $body;
        $new = clone $this;

What I don't understand is that nobody else runs into this problem ;)

My version of nyholm/psr7 is 1.2.1

Using this lib with kriswallsmith/Buzz

$request = new Request('GET', 'robu.in');
$this->client->sendAsyncRequest($request,$callback);

//Callback
$uri = $request->getUri(); 
$uri->host // is empty string
$uri->path // is 'robu.in' 

Not sure if this is expected behavior. But I was expecting the host to be filled in .

I am trying to get the request's parsed body using ServerRequest::getParsedBody method, but it always returns an empty array. I've checked the code and do not find the code related to parsing request's body.
Is this the intended behaviour?

I don't see any explicit examples for POST requests - particularly anything to show the correct usage for setting headers or the body. The *factory methods don't seem to expose anything for manipulating the headers or body. What I have below seems to function - but I'm certain this isn't correct form:

<?php
use Buzz\Client\Curl;
use Nyholm\Psr7\Factory\Psr17Factory;
use Nyholm\Psr7\Request;

function Push_SSE( $channel, $message ) {
    $server_url = 'http://' . SSE_SERVER . "/pub?id=ip$channel";
    try {
        $psr17Factory = new \Nyholm\Psr7\Factory\Psr17Factory();
        $psr18Client = new \Buzz\Client\Curl( $psr17Factory );

        $pheaders = array(
                'Cache-Control' => 'no-cache',
                'Content-type' => 'text/event-stream; charset=utf-8'
                );

        $request = new \Nyholm\Psr7\Request( 'POST', $server_url, $headers = $pheaders, $body = $message );
        $response = $psr18Client->sendRequest( $request );
    } catch ( Exception $e ) {
        echo 'Error: ' . $e->getMessage();
    }
}
?>

Can you please show me the proper method?

Do you think some class variables like "private headers[]" or "private statusCode" actually marked as private, will be changed as protected ?

I will try to extend you Response classe to add some helpers, so a direct acces to the variables could make the dev more smooth.

What are your opinion on this subject ?

PS : After looking at guzzle and diactoros implementation, the first lib stick to private variables, the second is a mix with private+protected.

Tracking here so I do not forget.

I have asked about clarification of PSR-17 on the PHP-FIG mailing list, [PSR-17] Document what must and mustn’t be set by createServerRequestFromArray?:

Hi,

I was recently looking at the latest changes to the PSR-17 proposal made in commit 3166325 and while I agree with the change itself (splitting into two separate methods), my main concern is about the complete removal of the following sentence:

The method and URI of the request SHOULD be derived from the given server parameters.

This was previously the only guidance in the PSR as to what property values should be assigned to the ServerRequest by the factory. Leaving this completely open would, I believe, make different factory implementations incompatible with each other.

Implementations might make #createServerRequestFromArray() follow #createServerRequest() and only set the method and URI based on the array. Others might assign the server parameters, which I would argue is the expected (but undocumented) behaviour of createServerRequestFromArray versus createServerRequest. Library users switching implementation could run into this difference when #getServerParams() suddenly starts returning unexpected different results.

As the server parameters are read-only, a more dangerous implementation difference could be the assignment of headers to the ServerRequest based on what they find in the provided array. ($_SERVER includes HTTP headers as HTTP_* values.) As the documentation does not mention headers, implementations can make their own choice on what they will do. If a library user is used to having to add the headers themselves, they might loop over some soft of header collection and call #withAddedHeader() on the ServerRequest. In the case of an implementation that does not add headers, this will work fine, while the user otherwise ends up with a load of duplicated headers on their ServerRequest.

I would propose clarifying the documentation on #createServerRequestFromArray() to specify what must and mustn’t be set on the newly created ServerRequest. The following things come to mind:

• Specify that the passed $server MUST or MUST NOT be set as the result for #getServerParams(). Do not leave this up to the interpretation of the implementer.
• Specify that the results for #getMethod() and #getUri() MUST be set based on $server. This is currently implied by the documented @throws only.
• Specify what other information MUST or MUST NOT be extracted. E.g. header information, which is available in $_SERVER but should either always be added by implementers or never as it otherwise makes for different ServerRequest instances that need different follow-up.

I found this because of php-http/client-common#186
In my application, I had a 200 with an empty body 🤯

IMHO, doing this is wrong as it hides the root cause.

More over, in PHP 7.4 it's allowed to throw exception.
And for PHP 7.3-, it prefer a fatal error than something wrong.

WDYT?

We should bump to PHP 7.1 so we can use nullable operator.

Hey there,

FYI, php enforce type hinting on a few native functions from 7.2. Thus, it is required to cast before sending values.

I've just got that exception when upgrading PHP today, as this project is a guzzle dependency.

Have a great day,
Alain

A while ago, the createServerRequestFromGlobals method was dropped from the PSR-17 factory interfaces. The current ServerRequestFactoryInterface only includes a single method (createServerRequest) rather than the four separate ones in our package.

Should we move to more strictly following PSR-17 again by dropping the 3 extra methods currently supplied?

Getting the implementation synced with PSR-17’s new wording will mostly solve #8 as well. The current wording says to specifically not parse or process the $serverParams parameter.

Some people might still have expectations of automatically filling a ServerRequest instance with the current HTTP request being handled by the server (cf. #42). Maybe it is an idea to spin this off into a separate class? That will be its own issue.

ServerRequest::getBody method returns an empty stream, while running file_get_contents("php://input") actually returns the correct POST body. I cannot find anywhere how the body resource is set.

The related place is: https://github.com/Nyholm/psr7/blob/master/src/MessageTrait.php#L160

Suppose we need to read the raw input here?

Is it an idea to make all of the final objects implement Serializable?

One use-case is for HTTP caches to store request/response objects easily. I ran into this today and ended up requiring zend-diactoros for its Serialization, which just means I end up with two entire PSR-7 implementations loaded at the same time. Any unserialised Nyholm object becomes a Zend object.

The drawback being that we would be adding methods that aren’t specifically defined by PSR-7.

Alternatively this could be spun of just like psr7-server. Which I might work on any way. But did want to throw this up for discussion in the mean time.

I have an HTML form with an optional <input type="file"> field.

There is no problem with PHP 7.1-7.4, but with PHP 8.0 I get the following error.

Uncaught ValueError: Path cannot be empty in /Users/gr4376/Projects/webtrees/vendor/nyholm/psr7/src/Factory/Psr17Factory.php:40
Stack trace:
#0 .../vendor/nyholm/psr7/src/Factory/Psr17Factory.php(40): fopen('', 'r')
#1 .../vendor/nyholm/psr7-server/src/ServerRequestCreator.php(216): Nyholm\Psr7\Factory\Psr17Factory->createStreamFromFile('')
#2 .../vendor/nyholm/psr7-server/src/ServerRequestCreator.php(188): Nyholm\Psr7Server\ServerRequestCreator->createUploadedFileFromSpec(Array)
#3 .../vendor/nyholm/psr7-server/src/ServerRequestCreator.php(98): Nyholm\Psr7Server\ServerRequestCreator->normalizeFiles(Array)
#4 .../vendor/nyholm/psr7-server/src/ServerRequestCreator.php(71): Nyholm\Psr7Server\ServerRequestCreator->fromArrays(Array, Array, Array, Array, Array, Array, Resource id #7)
#5 .../index.php(55): Nyholm\Psr7Server\ServerRequestCreator->fromGlobals()

This is because fopen('') gives a warning in PHP <=7.4, but a fatal error in PHP 8.0.

See https://3v4l.org/cgYav

As I documented in #43, there isn’t an automatic way (defined by PSR-17) to take the current HTTP request as seen by PHP (combination of superglobals, php://input, etc.) and make it a ServerRequest instance. This is by design.

I’d propose building a separate helper class following an idea initially mentioned on the PSR-17 repo:

use ServerRequestCreator\Creator;

$creator = new Creator(new PsrServerRequestFactory());

$serverRequest = $creator->fromGlobals([
    'useMethodOverride' => true,
    'useIISRewriteModule' => true
]);

I am not so sure about the options array used there, or even the names, but it would make sense to me to offer this as part of the package. Simply because people expect it. Maybe something akin to:

use Nyholm\Psr7\Factory\ServerRequestFactory;
use Nyholm\Psr7\Factory\StreamFactory;
use Nyholm\Psr7\Helper\ServerRequestCreator;

$creator = new ServerRequestCreator(
    new ServerRequestFactory(),
    new StreamFactory()
);

$serverRequest = $creator->fromGlobals();

If I request a file from AWS I get an empty header:

x-amz-id-2: mhBUIOWKJ*()Z=*U?IPZJ))(J=OetWk=
x-amz-meta-orientation:
x-amz-request-id: 1ZUWIODKLD

which leads to an exception:

throw new \InvalidArgumentException('Header values must be non-empty strings');

in the MessageTrait. So I have no chance to use this library with servers who may return an empty header in their response.

Why is this psr7 implementation so strict with this? Can we change the behavior? Like for example just ignore empty headers and not throw an exception?

Currently the constructor of ServerRequest looks like this:

The PSR says (here) about queryParams:

They MAY be injected during instantiation, such as from PHP's $_GET superglobal, or MAY be derived from some other value such as the URI.

Would it be a good idea to allow them to be injected via the constructor?

Before we release the next version, I would like to discuss one thing. The discussion started here between @Zegnat, @dbu and myself. A short summery is:

$s = \Nyholm\Psr7\Stream::create('Hello');
$s->write(' W');
echo "getContents(): \n";
echo $s->getContents();
echo "\n\n";
echo "toString(): \n";
echo $s->__toString();

In 1.4.0:

getContents(): 


toString(): 
Hello W

In master:

getContents(): 
llo

toString(): 
 Wllo

If we dont use ->write()

$s = \Nyholm\Psr7\Stream::create('Hello');

echo "getContents(): \n";
echo $s->getContents();
echo "\n\n";
echo "toString(): \n";
echo $s->__toString();

In 1.4.0 (last release):

getContents(): 


toString(): 
Hello

In master:

getContents(): 
Hello

toString(): 
Hello

Yes the PSR-7 getContents() method is confusing and should never be used. But the issue Im most concerned with is if this is a BC break or not. We have never specified where the read pointer should be after you run Stream::create() and we have always said that you never should assume anything. Ie, when you get a stream, you place the pointer yourself so you know where it is.

This functionality has also never been tested (until now). The reason for making the change is because we have got so many confused users and complaints about this... so we are rather pragmatic than 100% strict/correct.

So we can break BC in the future.

I just noticed that Response class is final, but it would be really useful to be able to extend from it, for example, to create custom error responses or JSON response.

Is there any specific reason why not to allow this, especially as the class has all of those HTTP status phrases in it?

EDIT: Same question really applies to all the classes.

Here are three ways to create a Nyholm\Psr7\Stream that I would expect to yield the same result:

This one works (because there's a type hint that casts to string):

$psr17Factory = new \Nyholm\Psr7\Factory\Psr17Factory();
$psr17Factory->createStream(3);

This one gives Uncaught InvalidArgumentException: First argument to Stream::create() must be a string, resource or StreamInterface.:

Stream::create(3);

This one gives Uncaught TypeError: fwrite() expects parameter 2 to be string, int given:

$stream = Stream::create('');
$stream->write(3);

Not a big deal, but I thought you might want to know. I encountered it when I implemented an API endpoint that was trying to return just the number of search results (3).

I really like this library as it's small and fast. While it's not popular as Zend Diactoros or Guzzle, it strictly follows the PSR standards, which I really appreciate.

However, one thing that makes me wondering from use it or not is the documentation. There's nothing, even a "hello world" example. Other libraries do have. So, I'd suggest adding some basic usage in the README to let developers how to use it.

Thanks

I think your readme file needs an update. 😉

Version: 1.2.1

Steps to recreate:
$response = new \Nyholm\Psr7\Response(203, [], 'hello world'); var_dump($response->getBody()->getContents()); var_dump((string)$response);

Expectation:
Both var_dump calls dump "hello world"

Actual result:
Both var_dump calls dump "" (an empty string)

PSR17's createResponse

public function createResponse(int $code = 200, string $reasonPhrase = ''): ResponseInterface;

means, that there is never a null value on $reasonPhrase

this means that within the Response implementation, using the factory is always empty, cause the null check https://github.com/Nyholm/psr7/blob/master/src/Response.php#L49.

what is OK, but dangerous is also line https://github.com/Nyholm/psr7/blob/master/src/Response.php#L50 cause its checks against the property and the assignment agains the variable within the method.

The ServerRequest constructor (deliberately?) fails to initialize with an empty body:

        if ('' !== $body && null !== $body) {
            $this->stream = Stream::create($body);
        }

Why the '' !== $body condition?

I can understand defaulting to null meaning "no body". (even though I'm unsure as to the point of partially initializing a Response instance that violates it's own internal constraints, but...)

If I explicitly specify the empty string "" as the body, why wouldn't you initialize with an empty stream?

It's inconsistent with the Stream constructor, which explicitly permits the empty string and defaults to it.

Having to explicitly initialize every created ServerRequest instance with Stream::create() or StreamFactoryInterface is a bit impractical (especially in test-suites) and creates unnecessary coupling.

Can we remove this condition, please?

I suggest version 0.4.0 should be the last release before 1.0.

Anything else we need to add or remove?

From guzzle/psr7#158 (comment)

I'm pretty confident that I will propose making all message classes in this project final if we ever hit a 2.0 version.

Reason: HTTP messages are represented as value objects, implementing an interface. From this point I don't see any valid use cases for extending the message classes.

FYI @sagikazarmark

Detailed description

I keep getting this error:
InvalidArgumentException: Header values must be RFC 7230 compatible strings.

It seems like I'm getting this error because of a Null value in the access-control-allow-origin header of the baseResponse. Is this supposed to happen and/or am I missing something?

Here's the trace:

/var/www/vendor/nyholm/psr7/src/MessageTrait.php:201
/var/www/vendor/nyholm/psr7/src/MessageTrait.php:80
/var/www/vendor/symfony/psr-http-message-bridge/Factory/PsrHttpFactory.php:163
/var/www/vendor/osteel/openapi-httpfoundation-testing/src/HttpFoundation/HttpFoundationResponseAdapter.php:33
/var/www/vendor/osteel/openapi-httpfoundation-testing/src/ResponseValidator.php:57
/var/www/tests/Feature/SensorAPI/GatewayControllerTest.php:113

This is an example of the test I am trying to run:

        $response = $this->get(
            'api/v1/sensor/gateways/gateway_name',
            [
                'Accept' => 'application/json',
                'Content-Type' => 'application/json',
                'Authorization' => 'Bearer token'
            ]
        );

        $validator = ResponseValidatorBuilder::fromJson(storage_path('api-docs/api-docs.json'))->getValidator();

        $result = $validator->validate('/v1/sensor/gateways/gateway_name', 'get', $response->baseResponse);

        $this->assertTrue($result);

The baseResponse header looks like this:

(
    [headers] => Symfony\Component\HttpFoundation\ResponseHeaderBag Object
        (
            [computedCacheControl:protected] => Array
                (
                    [no-cache] => 1
                    [private] => 1
                )

            [cookies:protected] => Array
                (
                )

            [headerNames:protected] => Array
                (
                    [cache-control] => Cache-Control
                    [date] => Date
                    [content-type] => Content-Type
                    [access-control-allow-origin] => Access-Control-Allow-Origin
                    [vary] => Vary
                    [access-control-allow-credentials] => Access-Control-Allow-Credentials
                )

            [headers:protected] => Array
                (
                    [cache-control] => Array
                        (
                            [0] => no-cache, private
                        )

                    [date] => Array
                        (
                            [0] => Wed, 06 Jan 2021 11:07:53 GMT
                        )

                    [content-type] => Array
                        (
                            [0] => application/json
                        )

                    [access-control-allow-origin] => Array
                        (
                            [0] => 
                        )

                    [vary] => Array
                        (
                            [0] => Origin
                        )

                    [access-control-allow-credentials] => Array
                        (
                            [0] => true
                        )

                )

        )
)

The $header and $values I get when I dump them in the validateAndTrimHeader() in MessageTrait class are:

string(13) "cache-control"
array(1) {
  [0]=>
  string(17) "no-cache, private"
}
string(4) "date"
array(1) {
  [0]=>
  string(29) "Wed, 06 Jan 2021 11:07:53 GMT"
}
string(12) "content-type"
array(1) {
  [0]=>
  string(16) "application/json"
}
string(27) "access-control-allow-origin"
array(1) {
  [0]=>
  NULL
}

My environment

PHP 7.3.20
Laravel 8
nyholm/psr7 : ^1.3

When I was checking \ prefixes for #67 I saw the factory uses \UPLOAD_ERR_OK:

Every other time constants are used, we seem to leave it up to PHP to figure out we want them from outside our own namespace. Here is UPLOAD_ERR_OK:

Is there some way to make StyleCI check this? Is it even important?

Use Psr17Factory and try to upload a huge file.

You will get an error:
ServerRequestCreator::createUploadedFileFromSpec() throws RuntimeException: The file cannot be opened. in vendor/nyholm/psr7/src/Factory/Psr17Factory.php on line 39 if file upload failed for any reason.

Uploading a bad file should never cause the whole ServerRequest to fail to be created as the error needs to be handled by the PSR-15 handler.

In Nyholm\Psr7\Stream, the method read must return only string but fread can be return a boolean
The StreamInterface does not force return type

    public function read($length): string
    {
        if (!$this->readable) {
            throw new \RuntimeException('Cannot read from non-readable stream');
        }

        return \fread($this->stream, $length);
    }

A super lightweight PSR-7 implementation. Very strict and very fast.

Can you provide benchmarks comparison with other implementations?

I'm wondering if you would consider removing the final modifier from Nyholm\Psr7\Request.

I totally agree in making most classes final by default, but in the case of PSR-7 requests I have a tendency to extend them to model certain API requests, overriding the constructor so that the object knows how to construct itself from domain objects rather than HTTP specifics (such as verb, URI, headers etc.) Here's an example: https://github.com/1ma/dca/blob/master/src/Bitstamp/Request/BuyOrder.php

What are your thoughts regarding this use case?

In PHP8, https://github.com/Nyholm/psr7/blob/master/src/Stream.php#L128 causes an issue with mockery:

Fatal error: Declaration of Mockery_0_Nyholm_Psr7_Stream_Nyholm_Psr7_Stream_Psr_Http_Message_StreamInterface::close() must be compatible with Nyholm\Psr7\Stream::close(): void

This issue may be a bit controversial, because the spec (PSR-17) appears to be incomplete on this point.

The issue is with the state of the internal stream produced by createStreamFromFile() and createStream() respectively.

While createStreamFromFile() produces a stream that points to the start of the file, createStream() produces a stream that points to the end of the file.

Producing a stream that points to the end of the file is problematic, since the most likely use of these streams is in the body of a Response - and you end up producing an empty response.

It's arguably a bit inconsistent and surprising, as these are just two different ways to create a stream from some pre-existing content - I think it's reasonable to expect them to work the same?

Diactoros is a popular source of reference for this, and it does rewind the stream when it creates a stream from a string.

Creating a stream from a string with the intent of writing to it is a much less likely scenario - if I was doing that, I'd probably start with createStream("") anyhow, and it would work either way, since the file pointer would be at 0 either way.

This has already lead to some pretty serious bugs while switching from Diactoros to this package.

Can we change it please?

Let me know if you'd accept a PR.

Hi,

Shouldn't we cast the header value as string in the trimHeaderValues() function ?

A few http headers represent a number of seconds or bytes, and the user could easily does something like this :
new Response(200, ['Age' => 60]);

instead of using a string for the value. In this case the trimHeaderValues() will crash trying to da a trim on a non string.

It seems more logical to cast the value as string before trying to do the trim, no ? or perhaps doesn't apply the trim if the value is not a string ? or just let it crash ?

what is you opinion ?

Hi all

The PSR7-specification (see PR php-fig/fig-standards#1194) as well as the RFC 2616 define a comma, not a comma with white-space, as separator.

We should probably update the following method:

There is a small typo in this example code:

$response = (new Psr17Factory())->createReponse('200', 'Hello world');

It should be:

$response = (new Psr17Factory())->createResponse(200, 'Hello world');

Or even better to emit a real Hello World text:

$response = (new Psr17Factory())->createResponse(200);
$response->getBody()->write('Hello World');
(new SapiEmitter())->emit($response);

I understand the goal of the project is to be compliant with the psr spec, but i dont see why that would enforce your implementations to be final?

Being spec compliant, nothing less and nothing more is perfect for use as a parent class in projects where you want to extend the functionality to include your own little helpers here and there and not have to write your own implementation, or deal with the baggage of implementations that add their own methods outside of the spec.

Is this something you would consider changing?

Right now the implementation prevents me from using unofficial HTTP status codes, like Laravels 419 Page Expired.

https://en.wikipedia.org/wiki/List_of_HTTP_status_codes

I think those should be allowed if the $reason parameter in the constructor is set.

Can you add the .github folder to .gitattributes export-ignore?

This also means bumping PHP to 7.1

See #131 and #133 for a problem with strtolower that never before showed up on tests.

While working on #133 I also discovered that testUriComponentsGetEncodedProperly is locale dependent and started failing with en_US.UTF-8:

Tests\Nyholm\Psr7\UriTest::testUriComponentsGetEncodedProperly with data set #1 ('/€?€#€', '/%E2%82%AC', '%E2%82%AC', '%E2%82%AC', '/%E2%82%AC?%E2%82%AC#%E2%82%AC')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'/%E2%82%AC'
+'/%E2_%AC'

/psr7/tests/UriTest.php:406

Though I am unsure why this goes wrong. rawurlencode seems pretty clear in what it should output. More research necessary to fix that one.

This seems to highlight that PSR7 was developed to run only for 1 locale, and deviations may break. How do we best detect this with our tests?