Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V4: routes can no longer return null? #2777

Closed
mnapoli opened this issue Aug 5, 2019 · 22 comments

Comments

@mnapoli
Copy link
Contributor

commented Aug 5, 2019

I had this route:

        $app->get('/', function () {
            // nothing
        });

It doesn't seem to work anymore with v4.0.0:

TypeError : Return value of Slim\Handlers\Strategies\RequestResponse::__invoke() must implement interface Psr\Http\Message\ResponseInterface, null returned

Is this something that changed in v4?

I read both the documentation and the upgrade guide and couldn't find any mention of this.

@shadowhand

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

The Response docs are the only mention I could find:

Your Slim app’s routes and middleware are given a PSR-7 response object that represents the current HTTP response to be returned to the client.

@adriansuter

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Yes, every route handler has to return a response.

@shadowhand

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

This should probably documented as a side effect of adopting PSR-15, in which all request handlers must return a PSR-7 ResponseInterface.

@mnapoli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

OK thanks. This should indeed be mentioned in the upgrade guide as this is a major BC break.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

That is because we type hinted the return value of the strategy. I think in Slim 3 it was not.

Either way, why is this necessary? Just return a blank response object if you really need to.

@mnapoli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Or yes maybe we could re-instate the previous behavior that allowed this? That would be one less BC break for users, which is good.

@shadowhand

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Was the fact that handler could return nothing documented in v3? Unless that was documented behavior, I feel like that was probably not meant to be a feature.

@shadowhand

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

I guess the handling of echo implicitly allows for returning nothing, per the v3 callable docs:

There are two ways you can write content to the HTTP response. First, you can simply echo() content from the route callback. This content will be appended to the current HTTP response object. Second, you can return a Psr\Http\Message\ResponseInterface object.

@shadowhand

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

The solution for RequestResponse and RequestResponseArgs is pretty simple, just add:

return /* ... */ ?? $response;

I do not think that should be added to RequestHandler because RequestHandlerInterface requires a ResponseInterface return value.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

@shadowhand we’d need to check and see if the return from $callable($request, ...) is a ResponseInterface from within RequestResponse also I’m not sure if we may need to modify the ResponseEmitter in order to accommodate users who want to use “echo”.

@shadowhand

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

@l0gicgate the /* ... */ in my example is the existing $callable invocation. If it returns null or void, then the existing $response is returned; for example RequestResponse would be:

return $callable($request, $response, $routeArguments) ?? $response;
@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

@shadowhand how do we deal with the logic in ResponseEmitter? Imagine we return an empty $response but the user used echo within the route callable, then ResponseEmitter will assume that the $response object is empty when it theoretically isn't. This is one reason why we type hinted the strategy.

@shadowhand

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

I don't see why that would matter. The difference between a callable handler that returns nothing and one that returns an empty response is:

 function ($request, $response) {
-    return $response;
 }

The empty response detection is exactly the same in either situation.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

That's exactly my point, if someone uses echo "content outside response object"; from within a route callable, isResponseEmpty() only checks the Response object and not the actual output buffer which is where the content from the echo statement would appear.

There's logic associated with a response actually being empty (which it wouldn't be if someone used echo from within a route callable):

if ($isEmpty) {
$response = $response
->withoutHeader('Content-Type')
->withoutHeader('Content-Length');
}

@mnapoli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@l0gicgate

how do we deal with the logic in ResponseEmitter?

I would say the same way it was dealt with in Slim 3?

To me the question boils down to: should this BC break be kept or reverted?

@shadowhand

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

@l0gicgate isn't echo'd content handled by the output buffer middleware?

ob_start();
/** @var ResponseInterface $response */
$response = $handler->handle($request);
$output = ob_get_clean();

In which case, the response will already have been filled with echo'd content by the time it reaches the ResponseEmitter ?

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

@shadowhand OutputBufferMiddleware is an optional middleware it’s not used internally.

@mnapoli I don’t personally like enabling a null return without justification. If it’s just for convenience, I don’t see the purpose.

@mnapoli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@mnapoli I don’t personally like enabling a null return without justification. If it’s just for convenience, I don’t see the purpose.

This isn't for convenience, this is a major BC break. At the moment this BC break isn't documented or acknowledged.

Limiting the amount of additional BC breaks to this release (that is already quite huge) could be a good thing.

@l0gicgate l0gicgate added the Slim 4 label Aug 6, 2019

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

@mnapoli I'm fine with acknowledging it. I'm also fine with reverting it if you can provide a solid argument as to why we should enable users to return a null response.

@mnapoli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Understood.

if you can provide a solid argument as to why we should enable users to return a null response.

It is a BC break. V4 is a big BC break. Being able to remove one BC break from V4 is a good thing. That's all I've got :)

To be clear I don't like returning null as well :)

@shadowhand

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

@l0gicgate I realize it is optional, but what difference does that make? Using echo and not using the output buffer middleware is supposed to do... what exactly? How does that impact this issue?

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

To be clear I don't like returning null as well :)

I guess we're agreeing then. I'm closing this as won't fix, I will address the BC break in the 4.1 Release so it's on record.

@l0gicgate l0gicgate closed this Aug 6, 2019

@l0gicgate l0gicgate added the wontfix label Aug 6, 2019

@l0gicgate l0gicgate referenced this issue Aug 6, 2019

Merged

4.1 - Release #2782

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.