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

Render now returns ResponseInterface instead of string #54

Merged
merged 5 commits into from Feb 14, 2020

Conversation

xepozz
Copy link
Contributor

@xepozz xepozz commented Feb 13, 2020

Q A
Is bugfix?
New feature?
Breaks BC?
Tests pass?
Fixed issues #52

@samdark samdark added the status:code review The pull request needs review. label Feb 13, 2020
@samdark samdark requested a review from a team February 13, 2020 15:06
Copy link

@schmunk42 schmunk42 left a comment

Choose a reason for hiding this comment

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

Looks much cleaner!

@roxblnfk
Copy link
Member

I think it is wrong that the render method returns a ResponseInterface.
I suggest considering another way.
Let the ActionCaller wrap the string value in a ResponseInterface.

@samdark
Copy link
Member

samdark commented Feb 13, 2020

I think it is wrong that the render method returns a ResponseInterface.

I was thinking the same before taking into account that it's controller's render method, not a method of a View.

@roxblnfk
Copy link
Member

I think it is wrong that the render method returns a ResponseInterface.

I was thinking the same before taking into account that it's controller's render method, not a method of a View.

It is also worth considering that this controller does not implement MiddlewareInterface, therefore it is not required to return a ResponseInterface. This also applies to other Actions passed to ActionCaller.

@samdark
Copy link
Member

samdark commented Feb 13, 2020

Let the ActionCaller wrap the string value in a ResponseInterface.

What if you need headers as well? In case of render() returning ResponseInterface you can add headers after the rendering.

@roxblnfk
Copy link
Member

What if you need headers as well? In case of render() returning ResponseInterface you can add headers after the rendering.

Just like now, when we can manually create a Response.

But what if the Action specified in the ActionCaller returns a string rather than ResponseInterface?
What if I want to treat the rendered page as a string? Should I unpack it from Response? (I remind you that ResponseFactory writes a line to a tmp file when creating a stream)
I would be glad to see a separate method like wrapToResponse (), but not so that render transformed it as Response by default

@yiiliveext
Copy link
Contributor

What if I want to treat the rendered page as a string? Should I unpack it from Response? (I remind you that ResponseFactory writes a line to a tmp file when creating a stream)

You still can do it, just render it manually

$content = $this->view->render($view, $parameters, $this);
$bodyText = $this->renderContent($content);

@samdark samdark merged commit 3a917b7 into yiisoft:master Feb 14, 2020
@samdark
Copy link
Member

samdark commented Feb 14, 2020

Let's have it. We're likely refactor base controllers in something else anyway: #53

@xepozz thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review The pull request needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants