search

php-http / message-factory

Abandoned: Httplug Factory interfaces for PSR-7 HTTP Message
http://php-http.org
MIT License
1.38k stars 10 forks source link

added new interface for creating multipart streams #30

Closed digitalkaoz closed 8 years ago

digitalkaoz commented 8 years ago

as first start for #29

dbu commented 8 years ago

thanks @digitalkaoz !

Nyholm commented 8 years ago

thanks @digitalkaoz !

Yes, indeed!

Have we looked att Guzzles implementation of Multipart stream? (ref). I cant find an implementation from diactoros...

digitalkaoz commented 8 years ago

create a multipart stream in guzzle:


private function createBody(array $params, array $files)
{
        $streams = [];

        foreach ($params as $k => $v) {
            $streams[] = ['name' => $k, 'contents' => $v];
        }

        foreach ($files as $k => $file) {
            $streams[] = ['name' => $k, 'contents' => new LazyOpenStream($file, 'r'), 'filename' => $file];
        }

        StreamFactoryDiscovery::find()->createStream(new MultipartStream($streams));
 }
Nyholm commented 8 years ago

Interesting. I do like this on my Mailgun PR:

    protected function send($method, $uri, $body = null, $files = [], array $headers = [])
    {
        $headers['User-Agent'] = Api::SDK_USER_AGENT.'/'.Api::SDK_VERSION;
        $headers['Authorization'] = 'Basic '.base64_encode(sprintf('%s:%s', Api::API_USER, $this->apiKey));
        if (!empty($files)) {
            $body = new MultipartStream($files);
            $headers['Content-Type'] = 'multipart/form-data; boundary='.$body->getBoundary();
        }
        $request = new Request($method, $this->getApiUrl($uri), $headers, $body);
        $response = $this->getHttpClient()->sendRequest($request);
        return $this->responseHandler($response);
    }

$files is an array with ['name'=>'foo', 'contents'=>'bar']

digitalkaoz commented 8 years ago

@Nyholm mh, you are overwriting the existing body in case there are any files...so you either have a stream, or a new stream with files (the old one ist lost). guzzle handles the boundary itself afaik...

LazyOpenStream could be replaced with a normal resource in my example...

we need to support something like that (sending both, files and parameters)

Nyholm commented 8 years ago

@Nyholm mh, you are overwriting the existing body in case there are any files...so you either have a stream, or a new stream with files (the old one ist lost). guzzle handles the boundary itself afaik...

Yes, that is correct. I know it is weird. But it makes (kind of) sense in the case of Mailgun. Either you have files or a body.

guzzle handles the boundary itself afaik...

Do you mean that I do not need this line?

$headers['Content-Type'] = 'multipart/form-data; boundary='.$body->getBoundary();

But what if I use the Curl client to send the request. Then I need to set this header, right?

digitalkaoz commented 8 years ago

so whats todo to get this merged? @Nyholm

sagikazarmark commented 8 years ago

I'll review this today or tommorrow. Thanks for the PR.

dbu commented 8 years ago

i still feel extending the simple StreamFactoryInterface would be more convenient for consumers of the interface. if you need both, you would usually have to expect two parameters and pass the same object twice... but if everybody else thinks differently, we can leave as is.

digitalkaoz commented 8 years ago

im on your side @dbu ;) all other felt different

sagikazarmark commented 8 years ago

I've gone through the PR, looks good. Added a few questions though which are yet unclear to me.

About interface extension: I kind of agree with both of you. I see the point that MultipartStreamFactory is probably not implemented without, but inheritance is kind of pointless here. However, this is probably the case when usability should win.

Nyholm commented 8 years ago

About interface extension: I kind of agree with both of you. I see the point that MultipartStreamFactory is probably not implemented without, but inheritance is kind of pointless here. However, this is probably the case when usability should win.

Lets make the MultipartStreamFactory implement the StreamFactory then. I just wanted to challenge that because the answer was not obvious.

sagikazarmark commented 8 years ago

Going through this PR it seems to me that only one open question remains: What do we accept in the files parameter?

I think we should at least support Streams, because files might not always be available from local paths (eg. reading file from a remote location or loading file "content" from database).

I also think that we are free to support file paths as well.

WDYT?

dbu commented 8 years ago

what does it take to open a file? fopen($path);? maybe we want to limit this to streams for simplification? on the other hand, allowing a string is convenient as in reality, its not only fopen but also error checking and throwing a readable exception.

there are some todos

digitalkaoz commented 8 years ago

im a bite lost everybody wants something slightly different ;)

i would support both streams and paths in the files array ?!

sagikazarmark commented 8 years ago

I proposed to support at least Streams, optionally paths.

@dbu proposes to support streams, but does not support paths. Too much hassle with error handling, and we lose the ability to pass strings to the factory. Am I right?

digitalkaoz commented 8 years ago

so $files will be @var StreamInterface[] $files ? do we need to ensure the streams are valid files?

if we only support streams, should we have a discoverable FileStreamFactory? im definitly for Streams and files, so the typehint would be @var array $files StreamInterface[] or string[] and nothing needs to be changed. i mean we are discussing about a docblock?!

dbu commented 8 years ago

ups, this was left lying around, sorry.

i am ok if we support opening a file. but in that case we need to have a well-documented exception if the file is not found or can't be read from. imo, the definition must be clear. if we support streams and file paths, every factory should support both, or we have little surprises between implementations that make them not interchangeable.

@digitalkaoz: note that there is https://github.com/php-fig/fig-standards/pull/759/ discussing about adding factories in the fig. more eyes looking at that proposal are always welcome.

lets try to wrap this PR up and get it merged. even if fig eventually has this, it might still take a while for it to happen.

Nyholm commented 8 years ago

Any updates on this? Are there any questions that needs to be resolved?

sagikazarmark commented 8 years ago

I think @dbu has been more involved in this PR, maybe he can tell us about the open questions.

dbu commented 8 years ago

the current FIG draft also does not want to open files for you, if i read https://github.com/php-fig/fig-standards/blob/master/proposed/http-factory/http-factory.md#24-streamfactoryinterface correctly. imo we should be as close to the FIG proposal as possible. there is no multipart but imo we should think how to keep the implementation as lightweight and simple. could we rely on the http StreamInterface as list of streams?

sagikazarmark commented 8 years ago

Files are not always an obvious source for a multipart stream IMHO, so I have no problem with NOT including that in the interface. We can have a helper class or something though in the message package.

Nyholm commented 8 years ago

@digitalkaoz Do you have time to make a last changes to this PR?

digitalkaoz commented 8 years ago

@Nyholm i would...but i really dont get it where are all your ideas heading to...i didnt thought a simple enhancement would lead to this discussion

Nyholm commented 8 years ago

I agree, there has been a lot of discussions for just 27 lines of code. Sorry for that. This thread as been a bit out of control. But one sure need discuss and rethink things thoroughly when introducing a new interface like this. What we should support and what we shouldn't and also considering various use cases.

I think there are not that much left open for discussion. I can do a summary later today if you want to. Or you may do the changes directly.

digitalkaoz commented 8 years ago

a summary would be nice :)

dbu commented 8 years ago

yeah sorry for the lengthy discussion. but indeed, this interface is really hard to change once people started using it, so we need to be clear what we want. and underspecified interfaces in a place like this would also be a problem.

Nyholm commented 8 years ago

Here is a summary of what we talked about so far.

1. Should MultipartStreamFactory extends StreamFactory?

No, because either you need a Stream or a MultiPartStream, a client would never need both. But it will be easer to use the MultipartStreamFactory if they extend each other. I suggest to introduce a third interface extending both StreamFactory and MultipartStreamFactory. Just like our existing interface MessageFactory.

2. What resources to support?

We all agree that StreamInterfaces should be valid. Guzzle supports StreamInterface, resource and string (not path). We been back and forth arguing if paths should be allowed. The issue is complex file handling and too many things that may go wrong when reading a file. What we have not mention is resources. 

$resource = fopen($path, 'r');

I suggest support for StreamInterfaces and resources. That would leave out the complexity and be similar to our own StreamFactory::createStream which supports string|resource|StreamInterface|null

3. Parameters

public function createMultiPartStream(array $parameters = [], array $files = []);

We have not talked about parameters a lot. It is only David that stated he wants to keep $files and $parameters apart. A parameter value could be 'filename'. We have not discussed if the 'formname' should be keys to $files or a parameter itself.

I see an issue with splitting them up because they are so tightly coupled. What if you have two $files but the first one does not have any parameters. The order of those array is obviously important and Im not sure that is good design.

Guzzle have one parameter that you may build up like this: 

foreach ($files as $k => $file) {
   $streams[] = ['name' => $k, 'contents' => new LazyOpenStream($file, 'r'), 'filename' => $file];
}

Mark suggests a MultipartStreamBuilder. Is that something we want to investigate further in?

4. Boundery

What about the boundary header? There must we a way to retrieve (or set) the boundary. Guzzle does this themselves like this: Ref.

I suggest that we should either have a MultipartStreamBuilder::getBoundery() or create a MultipartStream interface ourselves.

5. Other

Reviewing this thread of discussions I'm not sure a MultipartFactory is the right way to go. My reasons are:

It maybe better to create a new repo with a MultipartStreamBuilder because it is easier to use a fluid API to build the streams. The builder could return an normal stream form Guzzle/Zend/whatever. See example: https://github.com/Nyholm/MultipartStreamBuilder

dbu commented 8 years ago

thanks for this summary. i think your last point is most convincing to me: a multipart stream builder rather than an interface for something that only exists in guzzle. the builder should be in a way that it can use guzzle or implemented with a plain stream and fill it with multipart.

that would also solve the question how to handle parameters.

  • David wants streams and paths (but well documented)

i am actually not convinced about supporting file paths. i think stream interface and resources are what we should support, leaving file handling (and closing!) to the consumer. wrapping a string into a stream is also not hard to do for the consumer.

just saying, in case we end up sticking with the interface - and i guess its also relevant for the builder.

Nyholm commented 8 years ago

To sum up in this thread: Discussions on Slack resulted in #31. We ask for feedback on twitter but no real breakthrough was made.

We stopped the discussion because we did not want to define a MultipartStreamInterface (the return value of the factory). Instead we changed the way we look at multipart streams. If we consider them as normal streams that you create in a very specific way we would not need a MultipartStreamInterface.

We created a PSR-7 independent MultipartStreamBuilder to help us create Streams in this very specific way. Read about it in the docs: http://php-http.readthedocs.io/en/latest/components/multipart-stream-builder.html

I suggest we wait to create a MultipartStreamFactory until PHP-Fig has an interface for such implementation.

dbu commented 8 years ago

i suggest that we also close this pull request. discussion about standardization should happen in the FIG and until a standard is defined, the builder should cover the needs.

thank you @digitalkaoz for bringing this up, and thanks @Nyholm for the work on the multipart stream builder!