search

Nyholm / psr7

A super lightweight PSR-7 implementation
MIT License
1.17k stars 77 forks source link

Fix null reason phrase resulting from Response constructor #120

Closed Zegnat closed 5 years ago

Zegnat commented 5 years ago

This addresses the issue noticed in the discussion around #117. This is purely to fix that one bug, I have not tested other usage of $reasonPhrase, not do I make any steps on resolving the core discussion of #117 and #118.

We may need to revise our thinking around $reasonPhrase for a 2.0 release (if we want to strictly adhere to semver).

mahagr commented 5 years ago

I ran into the exception as well -- to me it's a bug in the library (bad return value), so the only 2 ways to fix it is either: 1) throw an exception instead of returning null 2) make the reason phrase empty

The second one is tricky, though, because it's not really valid response either. On the other hand, doing something like converting the response into 500 isn't optimal either, mostly because of it is not what I would expect to get.

To fix the issue and keep the compatibility, I would just throw an error until a decision can be made.

Zegnat commented 5 years ago

The second one is tricky, though, because it's not really valid response either.

What makes you say that? RFC 7220 specifically allows zero length reason phrases, and empty string is completely fine to have.

In addition, we replace null with the default reason phrase for the status code. It seems acceptable to me that the default reason phrase for something like error code 567 would be blank, as there is no registered default. Even if we do not want to presume that empty strings are a good default, PSR-7 defines that we “must return an empty string if [no reason phrase is] present”.

[…] I would just throw an error until a decision can be made.

Throwing exceptions from getReasonPhrase() is unacceptable. PSR-7 does not document a @throws for the method. It simply MUST return a string to be valid within the context of the interface.

This is indeed a bug in the library code. And the way to handle it is to issue a bugfix. The library should not be causing exceptions there, so this PR makes it so there are no more exceptions (at least on that one specific point). Not sure what needs to be decided on that front?

mahagr commented 5 years ago

@Zegnat You are right, I somehow missed the return doc comment when reading the RFC:

@return string Reason phrase; must return an empty string if none present.

Above commit is the only valid way to fix the bug. I am sorry about causing the fix to be delayed; I mainly commented it because of I hit the same bug yesterday and it was long and busy day for me.

Zegnat commented 5 years ago

@mahagr no problem at all! This is still pending review anyway. And getting more eyes on a PR is just good for the overall health of the product 😄