Allow MultipartStreamBuilder subclass to manipulate data

[yookoala]

• Change property $data visibility from "private" to "protected".
  Leaving more flexibilities for subclassing.

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets
Documentation	This is one simple signature change for subclassing.
License	MIT

What's in this PR?

Allow subclass of MultipartStreamBuilder to manipulate $data property.

Why?

I am recently working on Range Request support
for my application. For a range request with multiple specified ranges, I need to return a "multipart/byteranges" response.
The signature of MultipartStreamBuilder::addResource forces me to add a field name ($name), which is totally non-sense for
my use case.

I tried to extend the MultipartStreamBuilder with my own class and have an extra method that can properly add
the streams without the unnecessary extra handling, but since the visibility of MultipartStreamBuilder::data is "private",
I cannot do that.

Example Usage

class MyBuilder extends MultipartStreamBuilder
{
    public function addStream($resource, array $options = [])
    {
        $options += ['headers' => []];
        $this->data[] = [
            'contents' => $this->createStream($resource),
            'headers' => $options['headers'],
        ];
    }
}

// If you added new features, show examples of how to use them here
// (remove this section if not a new feature)
$response = $resposneFactory->createResponse(416);

// Assuming I have to deal with a range request "Range: bytes=0-8192, 65536-131072"
$steam = (new MyBuilder($streamFactory))
  ->addPart(custom_splice_resource_func(fopen('/path/to/file1), 0, 8192), ['Content-Range', 'bytes 0-8192/8192'])
  ->addPart(custom_splice_resource_func(fopen('/path/to/file1), 65536, 131072), ['Content-Range', 'bytes 65536-131072/65536']);

// Emit the response to client
$emitter->emit($response->withBody($stream));

Checklist

• Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
• Documentation pull request created (if not simply a bugfix)

To Do

• If the PR is not complete but you want to discuss the approach, list what remains to be done here

[dbu]

this sounds really cool and makes me wonder if it would make sense to add that functionality into this repository, if you are motivated to contribute it.

looking at how we put things into data in
https://github.com/php-http/multipart-stream-builder/pull/49/files#diff-28351bcab63ba2060821f4c2f4e0c12877a30426b7f6819c2faf72e6db1250c1R107 i wonder if instead it would make sense to have a protected method addData(stream $contents, array $headers, string $filename) and keep $data private? otherwise it is rather easy to mess things up.

hm, and it seems that the 'filename' in the $data is not used, only the one in the 'headers' seems relevant... so it might be just addData(stream $contents, array $headers)?

[yookoala]

I am currently writing a multipart stream implementation from ground up to fulfill my own needs. My approach is quite different than yours so I don't think it is suitable as a PR.

Instead of building the entire stream content string, I opt to dynamically seek and read the underlying resources on StreamInterface::read. This approach, although heavier in code (thus probably slower), would be more memory efficient for large file uploads / downloads.

I am still in the middle of my process. I can share more with you later.

[yookoala]

this sounds really cool and makes me wonder if it would make sense to add that functionality into this repository, if you are motivated to contribute it.

looking at how we put things into data in
https://github.com/php-http/multipart-stream-builder/pull/49/files#diff-28351bcab63ba2060821f4c2f4e0c12877a30426b7f6819c2faf72e6db1250c1R107 i wonder if instead it would make sense to have a protected method addData(stream $contents, array $headers, string $filename) and keep $data private? otherwise it is rather easy to mess things up.

hm, and it seems that the 'filename' in the $data is not used, only the one in the 'headers' seems relevant... so it might be just addData(stream $contents, array $headers)?

This approach makes sense to me. If you're OK, I can write a PR of it.

[dbu]

would be great if you can refactor it to have a clean, protected addData method. that would allow you to do the thing you want, right?

and i see, this different approach to a stream builder sounds interesting but can not replace the one here. in that case i think it is best to keep it separate. if you open source it, we'd be happy to link to it from the readme here, with the explanation that the php-http implementation does not support range requests, and people who need that should look at your implementation.

[yookoala]

Rewritten into a public addData function. Please check.

[dbu]

can we have addResource call the addData method? though there is some handling on the thing we created with getStream to determine the "filename" from the uri. would it make sense to have that filename header determined that way in addData as well, if it is not set?

[yookoala]

can we have addResource call the addData method? though there is some handling on the thing we created with getStream to determine the "filename" from the uri. would it make sense to have that filename header determined that way in addData as well, if it is not set?

If the 'filename' key is not used at all elsewhere, can I omit it in MultipartStreamBuilder::data? Seem to me it's only used in MultipartStreamBuilder::prepareHeaders.

[dbu]

If the 'filename' key is not used at all elsewhere, can I omit it in MultipartStreamBuilder::data? Seem to me it's only used in MultipartStreamBuilder::prepareHeaders.

i agree, i can't see it being used anywhere once it goes into data. i guess its a leftover after a previous refactoring. and with data being private, we can be sure there is no BC break if we drop it from the $data.

[yookoala]

Please see if 7323e85 (rewrite addResource with addData) is OK.

[dbu]

thank you, this looks good to me - i have one question to double-check i understood correctly.

and please keep the blank line before the return. i think if you rebase on master, the styleci run would work again and complain about them too ;-)

[yookoala]

Updated. Please see if 0db0d5c works.

[dbu]

great, looks good to me!

can you please change the branch-alias in composer.json to 1.x-dev (it is currently 1.1-dev but what you do should go into 1.2. with the 1.x-dev notation we avoid having to update it on each minor version upgrade). and please add a note in the CHANGELOG.md describing the new feature.

[yookoala]

Updated. Please check.

[dbu]

i added a changelog entry in 9addfcf

thanks a lot! can you rebase your other MR on master to get the latest changes in?