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

BUGFIX: Null coalesce content type in ActionResponse getter #2458

Merged
merged 1 commit into from Apr 27, 2021

Conversation

albe
Copy link
Member

@albe albe commented Apr 12, 2021

The current PHP typehint of : string will cause this method to throw an error when setContentType() was not called before with a valid string. In Flow 7 we lifted the typehint to ?string, but IMO that does only complicate the API unnecessarily, because '' is not a valid content type any way and hence indistinguishable from "did not set content type" for any useful means and purposes.
Hence I suggest using null coalescing instead (and changing the 7+ typehint back to string, though that would be breaking).

See also discussion in #2180 (comment)

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for extracting this to a separate PR.
By silently converting the never set content type to an empty string, we assume that the consuming side caters for that case.
Throwing an exception would be a more explicit alternative IMO

Neos.Flow/Classes/Mvc/ActionResponse.php Show resolved Hide resolved
@albe
Copy link
Member Author

albe commented Apr 24, 2021

ping @bwaidelich I think we should decide on this the next days before 6.0-6.2 run into security-only mode. I consider this method seriously broken right now and would like to find a good solution from the DX PoV. Maybe we can have a quick call monday or tuesday? 15-30 min max.

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@albe I'm happy to do a quick call with you today or tomorrow, but I'm also happy with the fix as is. I just wanted to point out some alternative solution but it has its drawbacks too

@albe
Copy link
Member Author

albe commented Apr 26, 2021

I'd love to talk about this quickly regarding throwing exception or not, but more about how to deal with 7 and it's nullable return type.

@bwaidelich
Copy link
Member

I'd love to talk about this quickly regarding throwing exception or not

OK, sure, anytime!

@albe
Copy link
Member Author

albe commented Apr 26, 2021

As discussed, we'll keep it like this without throwing an exception in the getter. While that would be super safe to not forget handling the unset case (and it should maybe then even cover the empty case), it is pretty drastic if all you do is e.g. if ($this->getContentType() !== '') { ... } which would be totally sane. We'll document to use the hasContentType() method instead again, but won't dictate how not to write safe code.

The nullable return type will stay in 7.0.x, but I'll prepare another bugfix for 7.1 to revert that back to non-nullable. This is only breaking if you'd have code in place that did only cover the unset case by checking for null and ignores empty string.

@albe albe merged commit fb4b64f into 6.0 Apr 27, 2021
@albe albe deleted the get-contenttype-null branch April 27, 2021 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants