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.

Sign up for GitHub

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

Jump to bottom

[11.x] Fixes Illuminate\Http\Response to output empty string if $content is set to null #53872

Draft
wants to merge 3 commits into
base: 11.x
Choose a base branch
from
Draft

[11.x] Fixes Illuminate\Http\Response to output empty string if $content is set to null #53872

wants to merge 3 commits into from
+9 −0

Conversation

crynobone
Copy link
Member

  1. Symfony set the content to null for response()->noContent() https://github.com/symfony/http-foundation/blob/e88a66c3997859532bc2ddd6dd8f35aba2711744/Response.php#L246-L249
  2. This cause issue with PSR-7 HTTP Stream in Octane

fixed laravel/octane#972

@crynobone
[11.x] Fixes Illuminate\Http\Response to output empty string if
70607d1 
`$content` is set to `null`

1. Symfony set the content to `null` for `response()->noContent()` https://github.com/symfony/http-foundation/blob/e88a66c3997859532bc2ddd6dd8f35aba2711744/Response.php#L246-L249
2. This cause issue with PSR-7 HTTP Stream in Octane

fixed laravel/octane#972

Signed-off-by: Mior Muhammad Zaki <[email protected]>
@crynobone
wip
e0953e4 
Signed-off-by: Mior Muhammad Zaki <[email protected]>
@crynobone
wip
a7d3f5f 
Signed-off-by: Mior Muhammad Zaki <[email protected]>
@crynobone crynobone marked this pull request as ready for review December 13, 2024 01:20
@crynobone crynobone marked this pull request as draft December 13, 2024 01:21
@crynobone crynobone mentioned this pull request Dec 13, 2024
Open
rodrigopedra
rodrigopedra reviewed Dec 14, 2024
View reviewed changes
src/Illuminate/Http/Response.php
#[\Override]
public function getContent(): string|false
{
return transform(parent::getContent(), fn ($content) => $content, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just?

return parent::getContent() ?? '';

@rodrigopedra
Copy link
Contributor

Also, for the same hash you linked the $this->setContent(null);, setContent() actually uses the null-coalescing operator to convert null to an empty string:

https://github.com/symfony/http-foundation/blob/e88a66c3997859532bc2ddd6dd8f35aba2711744/Response.php#L418-L423

Maybe the problem is something else?

@rodrigopedra
Copy link
Contributor

Does octane calls Illuminate\Http\ResponseTrait@getOriginalContent(), the Response::$original property is stored unchanged, that might be the issue.

@crynobone
Copy link
Member Author

I'm a little hesitant to follow Symfony style since their phpdoc indicate string|null while ours is mixed

Also with mixed we could potentially introduce breaking changes for application that expected that they can directly return null (maybe 🤔)

@rodrigopedra
Copy link
Contributor

I agree, but our override at the end defers to theirs

From our current code null should end up being handled by parent::setContent($content); which already coalesces null to an empty string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rodrigopedra rodrigopedra rodrigopedra left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sync_worker_exec_payload: TypeError: fwrite(): Argument #2 ($data) must be of type string, null given
2 participants
@crynobone @rodrigopedra