Skip to content

Add Message cloner #49

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

Closed
wants to merge 2 commits into from
Closed

Add Message cloner #49

wants to merge 2 commits into from

Conversation

joelwurtz
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets mentioned in #46
Documentation Does the phpdoc is enough ?
License MIT

What's in this PR?

Add a class to copy a request including it's body

Why?

IMO cloning a request with its stream his something that is shared by some concepts (like logging, caching, ....)

Example Usage

$cloner = new MessageCloner();

$clonedResponse = $cloner->cloneMessage($response);

// If original stream is not seekable, cloned it a second time to ensure that we can read it
if (!$response->getBody()->isSeekable()) {
    $response = $cloner->cloneMessage($clonedResponse);
}

Checklist

  • Updated CHANGELOG.md to describe new feature
  • Documentation pull request created
  • Add a plugin "EnsureSeekable" : Goal is to have a plugin that ensure that the body of a request or response is always seekable.

{
const COPY_BUFFER = 8192;

private $resource;
Copy link
Member

Choose a reason for hiding this comment

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

Can you add type hints here too for internal usage?

@sagikazarmark
Copy link
Member

What about memory usage in case of huge bodies? We should add a warning somewhere about that.

@joelwurtz
Copy link
Member Author

It's in the header of the MemoryClonedStream, by default it use a php://temp stream with a 2mb memory buffer, so the memory footprint of a cloned stream will, by default, have a max memory size of 2mb, when the body is higher, all data over the limit is written into a file.

@joelwurtz joelwurtz mentioned this pull request Jul 31, 2016
@Nyholm
Copy link
Member

Nyholm commented Aug 1, 2016

I like this. It could help on all the places we do some ugly hack to rewind streams and make sure they are seekable.

@joelwurtz could you address the comments?


private $resource;

private $size;
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@joelwurtz
Copy link
Member Author

Finally i'm not a fan of this implementation, message cloner is useless and the cloned stream enforce stream reading where i'm not a fan, will do a better implementation for the wanted use case.

@joelwurtz joelwurtz closed this Aug 4, 2016
@joelwurtz joelwurtz deleted the feature/message-cloner branch August 4, 2016 20:18
@joelwurtz joelwurtz mentioned this pull request Aug 4, 2016
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

Successfully merging this pull request may close these issues.

3 participants