Message: support body to be a callback #65

petrkotek · 2016-09-28T22:12:28Z

What?

In Sabre\HTTP\MessageInterface (and Sabre\HTTP\Message), support $body to be a callback outputting the body into php://output

Why?

Some responses may be big, so it's not scalable to hold the whole response in a variable (in memory), see https://github.com/fruux/sabre-dav/pull/890.

Limitation

getBodyAsString() and getBodyAsStream() doesn't currently work with the callback (throws an exception). It's implementation seem to be a bit complicated - mostly because we are able to produce output only once, but i'm open to ideas.

Usage example from sabre/dav

Snippet from sabre/dav to make generateMultiStatus() method work as a callback:

    function generateMultiStatus($fileProperties, $strip404s = false) {

        $w = $this->xml->getWriter();
        return function() use ($fileProperties, $strip404s, $w) {
            $w->openUri('php://output');
            $w->contextUri = $this->baseUri;
            $w->startDocument();
            $w->startElement('{DAV:}multistatus');

            foreach ($fileProperties as $entry) {
                $href = $entry['href'];
                unset($entry['href']);
                if ($strip404s) {
                    unset($entry[404]);
                }
                $response = new Xml\Element\Response(
                    ltrim($href, '/'),
                    $entry
                );
                $w->write([
                    'name'  => '{DAV:}response',
                    'value' => $response
                ]);
                // maybe we should flush() from time to time here as well
            }

            $w->endElement();
            $w->endDocument();
            $w->flush();
        };

    }

…/output

staabm · 2016-09-29T07:42:34Z

lib/Sapi.php

@@ -71,6 +71,11 @@ static function sendResponse(ResponseInterface $response) {
        $body = $response->getBody();
        if (is_null($body)) return;

+        if (is_callable($body)) {
+            $body();


shouldn't we pass the $response object into the callback so it can handle the case of existing Content-Length header etc? (like wie do for non-callback bodies a few lines below)

i'm not sure if it's necessary - i think that the same layer which is setting the headers (e.g. content length) is also setting the body (callback), so it can also pass the content length to the callback (e.g. function() use ($contentLength) {...}

looking at your generateMultiStatus example tells me that this assumption doesnt work?

i can imagine getting around that (e.g. passing some more context in CorePlugin::httpPropFind() into the generateMultiStatus() method), however, i've added the $response as an argument to the body callback function (see 47c4b1f)

i usually prefer not to add features which are not immediately required, however i understand you want to make it simple for the callback to do the same as Sapi::sendResponse() does without any other "external information"

let me know what you guys think now :)

thanks!

I'm with @petrkotek on this. generateMultiStatus really should move out of Sabre\DAV\Server anyway. It's a leftover from the extremely early days.

Usually I think it's reasonable to assume that the thing that sets the body, also has the ability to access the response object.

petrkotek · 2016-10-03T14:10:38Z

ping @evert / @staabm: will you have a chance to look into this? thanks!

petrkotek · 2016-10-05T07:07:04Z

can i get some 👀 on this? thanks!

evert

Aside from those small suggestions, I'm happy with the change. I do think that its time to give the sabre/http package a major version bump though. So I might need some patience until a full release. That doesn't mean we can't have master of sabre/http and sabre/dav reflect these changes though, so hopefully you're able to use just that.

evert · 2016-10-05T22:51:29Z

lib/Message.php

@@ -74,6 +77,9 @@ function getBodyAsString() {
        if (is_null($body)) {
            return '';
        }
+        if (is_callable($body)) {
+            throw new \UnexpectedValueException('Callback to string not supported');


We can definitely support this with output buffering.

ob_start / ob_get_clean and stuff ;)

evert · 2016-10-05T22:54:05Z

lib/Message.php

@@ -53,6 +53,9 @@ function getBodyAsStream() {
            rewind($stream);
            return $stream;
        }
+        if (is_callable($body)) {
+            throw new \UnexpectedValueException('Callback to stream not supported');


This can also definitely be done, and would be nice to have! With output buffering this can be turned into a stream.

More than 'nice to have' actually ;) I think this needs to be an absolute requirement to not break existing stuff using the HTTP response.

…se when is a callback function

petrkotek · 2016-10-06T12:12:49Z

so in the last commit i implemented getBodyAsString() and getBodyAsStream() for the case when body is a callback (using ob_* functions as suggested).

I'm not sure if there is more effective way how to convert the php://output into a stream -- at the moment there is an intermediate string representation used

I also removed the commit 47c4b1f which was changing $body() to $body($response)

staabm · 2016-10-06T12:29:00Z

lib/Message.php

+            throw new \RuntimeException('Cannot start output buffering');
+        }
+        $callback();
+        $content = ob_get_contents();


ob_get_clean() would do the same in 1 step

yay, very good point. i don't use this ob_* stuff too much :) updated

(note that i can always rebase & squash the commits if needed)

evert · 2016-10-06T21:54:00Z

lib/Message.php

+    private function captureCallbackOutput($callback)
+    {
+        $success = ob_start();
+        if ($success === false) {


You don't really need this check. I've never encountered a situation where this can possibly fail.

evert · 2016-10-06T21:56:36Z

tests/HTTP/MessageTest.php

    {
-        return function() {
+        return function() use ($content) {
            $fd = fopen('php://output', 'r+');


This is a nitpick and not that important, but instead of opening a stream to php://output and writing the contents there, you can just do:

echo $content;.

And this will have an identical effect, and possibly even a bit faster. Not important for unittests though.

thanks, good suggestions; fixed ✅

… doesn't fail in common cases; MessageTest: use echo rather then complicated fwrite to php://output

Petr Kotek added 2 commits September 29, 2016 00:01

Message: support body to be a callback outputting the body into php:/…

0e79b79

…/output

Test coverage for callback as a body

161dd98

petrkotek force-pushed the support-callback branch from df35734 to 161dd98 Compare September 28, 2016 22:57

petrkotek mentioned this pull request Sep 29, 2016

Memory optimization: support responses bigger than max allowed memory sabre-io/dav#890

Closed

Fix typo and use single-quotes for consistency

a849fd6

staabm reviewed Sep 29, 2016

View reviewed changes

evert requested changes Oct 5, 2016

View reviewed changes

petrkotek force-pushed the support-callback branch 3 times, most recently from 6cbe577 to b907b64 Compare October 6, 2016 12:07

Message: implement getBodyAsString() and getBodyAsStream() for the ca…

a95cb49

…se when is a callback function

petrkotek force-pushed the support-callback branch from b907b64 to a95cb49 Compare October 6, 2016 12:08

staabm reviewed Oct 6, 2016

View reviewed changes

Message: replace ob_get_contents() & ob_end_clean() by ob_get_clean()

78c34ba

evert requested changes Oct 6, 2016

View reviewed changes

Message: simplification: don't check return value of ob_start() as it…

f065bd5

… doesn't fail in common cases; MessageTest: use echo rather then complicated fwrite to php://output

evert approved these changes Oct 7, 2016

View reviewed changes

evert added the enhancement label Oct 7, 2016

evert merged commit 283f33f into sabre-io:master Oct 7, 2016

petrkotek mentioned this pull request Oct 12, 2016

Sabre\DAV\Server: support large PROPFINDs sabre-io/dav#898

Closed

petrkotek deleted the support-callback branch January 3, 2017 02:03

This was referenced May 7, 2018

Apply support for large PROPFINDS to 3.2.2 bigcommerce/sabre-dav#4

Closed

Apply support for large PROPFINDS to 3.2. bigcommerce/sabre-dav#5

Merged

Add body as a callback support to 4.2.4. bigcommerce/sabre-http#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Message: support body to be a callback #65

Message: support body to be a callback #65

petrkotek commented Sep 28, 2016 •

edited

Loading

staabm Sep 29, 2016

petrkotek Sep 29, 2016

staabm Sep 29, 2016

petrkotek Sep 29, 2016

evert Oct 5, 2016

petrkotek commented Oct 3, 2016

petrkotek commented Oct 5, 2016

evert left a comment

evert Oct 5, 2016

evert Oct 5, 2016

evert Oct 5, 2016

evert Oct 5, 2016

petrkotek commented Oct 6, 2016 •

edited

Loading

staabm Oct 6, 2016

petrkotek Oct 6, 2016

evert Oct 6, 2016

petrkotek Oct 6, 2016

evert Oct 6, 2016

petrkotek Oct 6, 2016

Message: support body to be a callback #65

Message: support body to be a callback #65

Conversation

petrkotek commented Sep 28, 2016 • edited Loading

What?

Why?

Limitation

Usage example from sabre/dav

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrkotek commented Oct 3, 2016

petrkotek commented Oct 5, 2016

evert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrkotek commented Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrkotek commented Sep 28, 2016 •

edited

Loading

petrkotek commented Oct 6, 2016 •

edited

Loading