Is urlFor() in HttpTestTrait a good idea?

[samuelgfeller]

Inside a test action method would you recommend creating the request with the hardcoded route or with the route name?

public function testAction(): void
{
    $request = $this->createRequest('GET', '/page-link');
    // or 
    $request = $this->createRequest('GET', $this->urlFor('page-name'));
    ...
}

If the second option is good, may I create a pull request to HttpTestTrait adding something like this:

/**
 * Build the path for a named route including the base path
 * @param string $routeName Route name
 * @param string[] $data Named argument replacement data
 * @param string[] $queryParams Optional query string parameters
 *
 * @return string route with base path
 */
protected function urlFor(string $routeName, array $data = [], array $queryParams = []): string
{
    return $this->app->getRouteCollector()->getRouteParser()->urlFor($routeName, $data, $queryParams);
}

[odan]

Yes, this sounds like a good idea. Please create a PR.

[samuelgfeller]

Awesome thank you!

[samuelgfeller]

I'd be interested in a comment on 446c8d09616b0bff501576c415f3a5bf771e161b. If the answer to the initial question of this issue changed, I'd also be interested.

[odan]

Good question. I still think this is a very useful Trait but there was a technical issue. Maybe we can find a working solution.

When I remember correctly the problem was that if you use another psr7 implementation in your project, e.g. nyholm/psr7 than you have two different PSR-7 implementations installed and the Slim Factory uses the slim/psr7 implementation and the one you want to and so on. It is also problematic for the IDE autocompletion if you have multiple Request/Response classes to select from.

I would like to add this Trait again, because it is useful. I guess the only issue was the slim/psr7 dev-dependency in composer.json which is not needed for this trait.

[samuelgfeller]

I switched to nyholm/psr7 too and could use the same trait, which worked. But I removed slim/psr7 when I installed nyholm/psr7.

Not sure to understand the why exactly, but that doesn't matter. If the problematic happens when you have two different PSR-7 implementations, would a word in the php-doc of that function saying to be careful to have only one PSR-7 implementation to use this function make things clear?

Anyway, if you add it again I'd use it, but I can have it in my project as well, that's perfectly fine too.

[samuelgfeller]

I switched to nyholm/psr7 too and could use the same trait, which worked.

Okay, probably I spoke too fast. When creating a request for an integration test with urlFor including query params (third argument)

$request = $this->createRequest('GET',  $this->urlFor('post-list-all', [], $queryParams)); // $queryParams -> ['user' => 1]

$request:

Nyholm\Psr7\ServerRequest::__set_state([
   'attributes' => [],
   'cookieParams' => [],
   'parsedBody' => NULL,
   'queryParams' => [], // Empty array here
   'serverParams' => [],
   'uploadedFiles' => [],
   'headers' => [],
   'headerNames' => [],
   'protocol' => '1.1',
   'stream' => NULL,
   'method' => 'GET',
   'requestTarget' => NULL,
   'uri' => Nyholm\Psr7\Uri::__set_state([
     'scheme' => '',
     'userInfo' => '',
     'host' => '',
     'port' => NULL,
     'path' => '/posts',
     'query' => 'user=1', // Value is here
     'fragment' => '',
  ]),
])

Now I wouldn't care too much, but my tests fail because in the action class, when I do $request->getQueryParams(); (which I thought is the official way) the result is an empty array.
This is the $request in the action class.

I guess I could do $request->getUri()->getQuery() and then transform this back into an assoc array but is it the way? In the slim doc its written

You can get the query parameters as an associative array on the Request object using getQueryParams().

Is that valid only for the slim/psr7 and is a limitation of the nyholm/psr7 or am I missing something.

Edit

Did some more digging and I think I have my answer: slim/psr7 ServerRequest.php (source)

    public function getQueryParams(): array
    {
        $queryParams = $this->serverRequest->getQueryParams();

        if (is_array($queryParams) && !empty($queryParams)) {
            return $queryParams;
        }

        $parsedQueryParams = [];
        parse_str($this->serverRequest->getUri()->getQuery(), $parsedQueryParams);

        return $parsedQueryParams;
    }

nyholm/psr7 ServerRequest.php

public function getQueryParams(): array
{
    return $this->queryParams;
}

They discussed it extensively in https://github.com/Nyholm/psr7/issues/181 as well. I hope it comes soon!

I posted this question here because I thought that it was related to what you said about using different PSR-7 implementations, but in reality it has only something to do with how the getQueryParams() function is written in the respective libraries, so let me know if I should delete this comment.

[odan]

Ok, this is very interesting.

I always use $request->getQueryParams(); to get the query string parameters as array. That's also the preferred way. I never use the $request->getUri()->getQuery() method for this use case.

[samuelgfeller]

I have a similar issue with getParsedBody(). The HttpJsonTestTrait function createJsonRequest adds the passed $data to the body stream with $request->getBody()->write((string)json_encode($data));.

The problem is that when calling $request->getParsedBody() in the action, the result is null. It only works when adding the body data with ->withParsedBody($data).

Something to have in mind and in my opinion relevant to mention in the test-trait docs and especially the php-docs of the methods that add query params (like urlFor()) and data (like createJsonRequest()) as you're using the nyholm/psr7 in your slim4 skeleton project.

Edit

Okay I figured it out, after adding $app->addBodyParsingMiddleware(); to middleware.php, getParsedBody() no longer returns null.

[odan]

The Slim 4 ServerRequestCreatorFactory detects the installed PSR-7 package. In case of nyholm/psr7 it uses the Nyholm\Psr7Server\ServerRequestCreator class of the nyholm/psr7-server packge to create fully loaded request object.

But the test-trait HttpTestTrait::createRequest method "just" uses the general purpose ServerRequestFactoryInterface object from the container. The ServerRequestFactoryInterface DI container definition then returns the Nyholm\Psr7\Factory\Psr17Factory instance from the nyholm/psr7 package. The Psr17Factory just creates a request object without fetching the context information from the server variables. For a test, it must be just a nearly "empty" request that contains only the HTTP method and the URI. See here.

This makes sense (and is a feature) because this is a "test-trait" that should be used only in the context of a test environment, where there is no real HTTP server request context. So to make your test requests work, you should build your test specific request object only "in-memory" for that specific test. It would be wrong to use the server variables from a real server. The assumption that a test reflects a "real" HTTP request from a real client is not correct.

[odan]

Update. I guess there is a strange behavior in both psr7 components, because they can return different value depending on how you pass the values. See this example:

$request = $this->createRequest('GET', '/api/users?id=1');

// Returns: "id=1"
$queryParams1 = $request->getUri()->getQuery();

$request = $request->withQueryParams(['id' => 2]);

// Returns: ["id" => 2]
$queryParams2 = $request->getQueryParams();

So I would recommend to not pass the query strings values directly as string. Instead use the withQueryParams method to add the query params and the getQueryParams to fetch the query params as array.

So I would write a test like this:

$request = $this->createRequest('GET', '/api/users')
    ->withQueryParams($queryParams);

In combination with urlFor it could be used like this:

$request = $this->createRequest('GET',  $this->urlFor('post-list-all'))
    ->withQueryParams($queryParams);

[samuelgfeller]

I appreciate the time you're taking to investigate with me.

So to make your test requests work, you should build your test specific request object only "in-memory" for that specific test. It would be wrong to use the server variables from a real server. The assumption that a test reflects a "real" HTTP request from a real client is not correct.

I added $app->addBodyParsingMiddleware(); and after that, everything worked. My request looks like this:

$request = $this->createJsonRequest(
            'PUT',
            // Request to change user with id 1
            $this->urlFor('user-update-submit', ['user_id' => 1]),
            [
                'first_name' => 'John',
                'surname' => 'Doe',
                'email' => 'edited_john.doe@example.com'
            ]
        );

Wouldn't it be useful to mention to not forget to add $app->addBodyParsingMiddleware(); when passing non-POST or GET data with createJsonRequest() ? Or maybe not, this is still a little blurry for me, I haven't taken the time to look at exactly how it's done behind the scenes.

So I would recommend to not pass the query strings values directly as string. Instead use the withQueryParams method to add the query params and the getQueryParams to fetch the query params as array.

Yes, that's what I ended up doing. I'd still like to see getQueryParams() return the uri query parameters by default if there isn't any data passed with withQueryParams() which would have the priority over the other like you showed in the example.

Thanks for adding the route trait. Maybe a comment in the spirit of the following could be useful?:

    /**
     * ...
     * @param string[] $queryParams Optional query string parameters. If you're using Nyholm/psr7, query parameters
     * MUST be added via $request->withQueryParams($queryParams) to be retrieved with $request->getQueryParams();
     *
     * @return string route with base path
     */
    protected function urlFor(string $routeName, array $data = [], array $queryParams = []): string
    // ...

At least it would have saved me a few hours of research.

[odan]

Thanks for your feedback. I add the BodyParsingMiddleware by default to my projects, so maybe this is the reason why I had no issue with it. I just added your comment to the DocBlock.