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

ResourceObject should be render (__toString) before looking up headers #1

Closed
sasezaki opened this issue Oct 3, 2015 · 10 comments
Closed

Comments

@sasezaki
Copy link

sasezaki commented Oct 3, 2015

current ResourceHandler seems have issue for header setting.

Expected result

$ php -r 'var_dump(get_headers("http://localhost:8888/", 1)["Content-Type"]);'
string(16) "application/json"

Actual result

$ php -r 'var_dump(get_headers("http://localhost:8888/", 1)["Content-type"]);'
string(24) "text/html; charset=UTF-8"

Because, handle() method sets header such as below

        $response = $response->withStatus($resourceObject->code);
        foreach ($resourceObject->headers as $name => $value) {
            $response = $response->withHeader($name, $value);
        }
        $response->getBody()->write((string) $resourceObject);

I think fix needed like..

        $responseBody = (string) $resourceObject;
        $response = $response->withStatus($resourceObject->code);
        foreach ($resourceObject->headers as $name => $value) {
            $response = $response->withHeader($name, $value);
        }
        $response->getBody()->write($responseBody);
@sasezaki
Copy link
Author

sasezaki commented Oct 3, 2015

But sorry, at first, I don't prefer __toString for Render for PSR-7. 😠
(reason is wrote at http://sasezaki.hatenablog.com/entry/2015/03/07/195908 )

@koriym
Copy link
Member

koriym commented Oct 5, 2015

Agreed. Handler should be handle stream not body string.

Your code seems works fine. Would you send PR ?
https://gist.github.com/sasezaki/68670975fbb5ca5114ed
Then we can leave your contribution on repository.

Or I can fixed it later. Thank you for making the issue.

@sasezaki
Copy link
Author

sasezaki commented Oct 6, 2015

ok. I would try at this weekend.

@koriym
Copy link
Member

koriym commented Oct 6, 2015

Awesome !

@sasezaki
Copy link
Author

sorry for late. I couldn't take a time to catch up lack of BEAR.Sunday knowledge.
so I want someone working on this.

@koriym
Copy link
Member

koriym commented Oct 12, 2015

no problem, I will do it later with your code.

koriym added a commit that referenced this issue Oct 16, 2015
koriym added a commit that referenced this issue Oct 16, 2015
@koriym
Copy link
Member

koriym commented Oct 16, 2015

quick fix for header now.

@koriym
Copy link
Member

koriym commented Oct 18, 2015

@sasezaki

ResourceObject should be render (__toString) before looking up headers

Fixed !

But sorry, at first, I don't prefer __toString for Render for PSR-7. 😠

Now RequestHandler returns 100% genuine stream response.

$response = $response->withBody(new Stream($stream));

But things are not easy as first glance. The current implementation has limitation. This version can't handle with your blog post example. (view template produce huge text).

Instead, we can assign stream resource at ResourceObject

    public function onGet()
    {
        $this['msg'] = 'hello world';
        $this['stream'] = fopen(__DIR__ . '/message.txt', 'r');
        return $this;
    }

hello world text and /message.txt stream are placed at their place folder in layout file, then text parts(hello world and layout template) is converted to stream to merge native stream (message.txt). You can assign multiple stream in any place. All stream will be merged and streamed.

https://github.com/bearsunday/BEAR.Middleware/blob/1.x/src/Module/StreamRenderer.php#L117

If you want single stream without any fixed text, you simple assign stream to entire resource body.

    public function onGet()
    {
        $this->body = fopen(__DIR__ . '/message.txt', 'r');
        return $this;
    }

@koriym
Copy link
Member

koriym commented Oct 18, 2015

@sasezaki I tested with huge text stream, It succeeded but need to change output procedure as you mentioned.

mwop-q

@koriym
Copy link
Member

koriym commented Oct 18, 2015

@sasezaki your blog post are SUPER helpful. (I knew very little about it)
Huge thanks !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants