8000 [HttpFoundation] Return value of BinaryFileResponse::getContent() and StreamedResponse::getContent() by niklasf · Pull Request #5864 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] Return value of BinaryFileResponse::getContent() and StreamedResponse::getContent() #5864

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
Closed
wants to merge 1 commit into from
Closed

[HttpFoundation] Return value of BinaryFileResponse::getContent() and StreamedResponse::getContent() #5864

wants to merge 1 commit into from

Conversation

niklasf
Copy link
Contributor
@niklasf niklasf commented Oct 29, 2012

While reviewing #4546 @stof found an inconsistency:

    public function setContent($content)
    {   
        if (null !== $content) { // <--- Here we only allow only null
            throw new \LogicException('The content cannot be set on a StreamedResponse instance.');
        }   
    } 
    public function getContent()
    {   
        return false; // <-- Here we return false
    }

That means for example, that $streamedResponse->setContent($streamedResponse->getContent()) will throw an exception.

Should we (or can we even, without breaking backwards compability) change the return value of StreamedResponse::getContent() and BinaryFileResponse::getContent() to null?

Edit: For BinaryFileResponse we could also consider returning the real content. That might however be a huge string. For StreamedResponse we have no guarantees that we can do that and get the same result when we do it again.

@fabpot
Copy link
Member
fabpot commented Dec 11, 2012

After thinking more about this, I think the current behavior is correct. When calling getContent, false means that you cannot get the content, and as such, setting it again throws an exception.

@fabpot fabpot closed this Dec 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@niklasf @fabpot
0