Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[HttpKernel] Handle nullable callback ofStreamedResponse#51972

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:6.3fromelementaire:fix-streamed-response
Oct 12, 2023
Merged

[HttpKernel] Handle nullable callback ofStreamedResponse#51972

nicolas-grekas merged 1 commit intosymfony:6.3fromelementaire:fix-streamed-response
Oct 12, 2023

Conversation

@elementaire
Copy link
Contributor

@elementaireelementaire commentedOct 10, 2023•
edited
Loading

QA
Branch?6.3
Bug fix?yes
New feature?no
Deprecations?no
Tickets-
LicenseMIT

This PR fixes a bug introduced by#51396. Having a callback inStreamResponsed is not a mandatory by design.

So the methodgetCallback must check it likesendContent does.

When we are dealing with cacheable ressource, only headers are sent fromStreamResponsed, for example:

#[Route(path:'/download')]publicfunctiondownload(Request$request):StreamedResponse{// Get the last modified of a file.$lastModified = \DateTime::createFromFormat('d/m/Y H:i:s','10/10/2023 13:22:00');$response = (newStreamedResponse())->setLastModified($lastModified);if ($response->isNotModified($request)) {return$response;    }// Get the stream of a file and attach it to the callback.// ...return$response;}

HttpKernel class have to handle it else if you obtainHTTP 500 with messageValue of type null is not callable.

cc@nicolas-grekas

EDIT: found by@alexismarquis

@carsonbotcarsonbot added this to the6.3 milestoneOct 10, 2023
@OskarStarkOskarStark changed the title[HttpKernel] Handle nullable callback of StreamedResponse[HttpKernel] Handle nullable callback ofStreamedResponseOct 10, 2023
@nicolas-grekas
Copy link
Member

Thank you@elementaire.

@nicolas-grekasnicolas-grekas merged commitdd6342a intosymfony:6.3Oct 12, 2023
publicfunctiongetCallback():\Closure
publicfunctiongetCallback():?\Closure
{
if (!isset($this->callback)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would usenull === $this->callback for consistency with our other null checks

Choose a reason for hiding this comment

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

isset() is what we use on 7.0 because the property is not declared as nullable

Copy link
Member

Choose a reason for hiding this comment

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

but this whole ticket is about fixing the fact that the callback can be null. So to me, the 7.0 type should also be changed as part of this fix.

elementaire reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

btw, as the property is protected and not private, this is a BC break to turn it into a non-nullable potentially-uninitialized property as the way to represent an existing state.

elementaire reacted with thumbs up emoji
nicolas-grekas added a commit that referenced this pull requestOct 21, 2023
…with null (xabbuh)This PR was merged into the 7.0 branch.Discussion----------[HttpFoundation] initialize protected callback property with null| Q             | A| ------------- | ---| Branch?       | 7.0| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Issues        |Fix#51972 (comment)| License       | MITCommits-------b780c12 initialize protected callback property with null
@fabpotfabpot mentioned this pull requestOct 21, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@OskarStarkOskarStarkOskarStark left review comments

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

Assignees

No one assigned

Projects

None yet

Milestone

6.3

Development

Successfully merging this pull request may close these issues.

5 participants

@elementaire@nicolas-grekas@stof@OskarStark@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp