Add support for caching the underlying stream

[akrabat]

While php://input is seekable, you can't re-read it a second time. This is problematic when you want to parse the body for getParsedBody(), but also allow subsequent uses of getBody() to work.

To address this, we now allow Stream to optionally cache the data it reads (via read() or getContents(). Then, once the eof is detected, we return the cached data on subsequent getContents() calls so that the stream is not read a second time.

Also, update ServerRequestFactory so that when it creates a stream for php://input, it enables the caching mechanism.

[coveralls]

Coverage increased (+0.005%) to 99.734% when pulling 754b85b on akrabat:cached-stream into 78ff6b8 on slimphp:master.

[anfly0]

@akrabat Hi, by now you know I like my streams! 😊

Me thinking out loud:
Would it be reasonable to use a stream instead of a string as the cache?
My thinking is that if the cache is a stream, it would be possible to retain all of the functionality of the StreamInterface (seek, rewind, tell, etc.) when performing operations on the cached data.

If we wanted to, this could even be expanded on to let the user of the SteamInterface object set there own cache stream and in that way get fine-grained control of the cache behavior.

An example would be a memory-constrained system that maybe would use fopen("php://temp/maxmemory:1024", 'r+').

[mapogolions]

@akrabat why you decide to add additional flag to add cache ability? The Stream class has become smarter. What if in the future we will need yet another option? We will create yet another flag. Why not a decorator?

[akrabat]

@akrabat Hi, by now you know I like my streams! 😊

I do :) I think it's great that you're thinking about this stuff, thanks.

Would it be reasonable to use a stream instead of a string as the cache?

Very possibly. I'll change it and let's see what it looks like.

[akrabat]

Why you decide to add additional flag to add cache ability? The Stream class has become smarter. What if in the future we will need yet another option? We will create yet another flag. Why not a decorator?

In general in Slim, we haven't tended to create a new class and or interface for every little thing that can be decoupled as that seems to be overwhelming for beginners who are using Slim as their first framework and maybe don't have the strong foundation in design patterns and OO as more experience developers do. If a class does become overwhelming, then you do tend to see it being split up appropriately as we then have more knowledge.

However, as always, I'll take direction from @l0gicgate and the maintainers as I think the fundamentals of this feature are important and want to see it in the codebase!

[l0gicgate]

In general in Slim, we haven't tended to create a new class and or interface for every little thing that can be decoupled as that seems to be overwhelming for beginners

This is a hard pill to swallow for advanced developers like most of us maintaining the project and commenting on these PRs but it's the cold hard truth.

We need to keep simplicity in mind. That being said though, I'm just curious as to why we're giving the option to cache the stream and not make it just default internal behavior without exposing the $cacheStream option at all?

[akrabat]

That being said though, I'm just curious as to why we're giving the option to cache the stream and not make it just default internal behavior without exposing the $cacheStream option at all?

For a seekable stream (that isn't php://input), it doubles memory usage for no benefit. Imagine a 200MB PPT file for instance…

[l0gicgate]

@akrabat okay that makes sense then. So just so we're clear though, by default we are creating an automatically caching stream when using ServerRequestFactory::createFromGlobals()?

$body = (new StreamFactory())->createStreamFromFile('php://input', 'r', true);

[akrabat]

@akrabat okay that makes sense then. So just so we're clear though, by default we are creating an automatically caching stream when using ServerRequestFactory::createFromGlobals()?

$body = (new StreamFactory())->createStreamFromFile('php://input', 'r', true);

Yeah. php://input claims that it's seekable, but you can't actually read the data more than once, so the common case needs it enabled. I'm not sure if we should make it a parameter to ServerRequestFactory::createFromGlobals() though that defaults to true.

It's a toss-up. How many apps send a binary directly into a web app without using multipart/form-data (which would then use $_FILES)?

[l0gicgate]

I think we should just leave it as a true default. We can change the method signature and expose that if needed later. I don’t think it’s necessary at the moment.

[l0gicgate]

Seems like there's only some additional tests to be written before I merge this. I'd like to tag a release soon. This looks good to me.

[akrabat]

@anfly0 I've re-worked to use an instance of StreamInterface for the $cache in Stream. Works well and simplifies the code as I don't need both a flag and a string property in the class. 👍

[akrabat]

@l0gicgate This is ready for review from my point of view. I've added the tests that I could think of and for the Stream, StreamFactory & ServerRequestFactory classes coverage is 100%.

[akrabat]

Looking at the coveralls results, it doesn't seem to detect testCachedStreamsFillsCacheOnRead() has run?

[akrabat]

That's better! It seems at PHP 7.1 doesn't like something about testCachedStreamsFillsCacheOnRead().