Skip to content

document options argument #62

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.

Sign up for GitHub

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

Merged
sagikazarmark merged 1 commit into from
Sep 30, 2015
Merged

document options argument #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/HttpMethodsClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@
*
* Use this interface when you do not have PSR-7 RequestInterface instances available.
*
* Implementations should be configured in their constructor. For runtime behaviour changes, there
* is an $options parameter. It SHOULD be validated by the client and an exception thrown if
* unknown options are passed.

@joelwurtz joelwurtz Sep 29, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

exception thrown if unknown options are passed.

How to handle specific options for an adapter then ?

@dbu dbu Sep 29, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

exception thrown if unknown options are passed.

How to handle specific options for an adapter then ?

only pass them when you know you are using that specific client/adapter.
in a library, such options - if needed at all - should be injected.

to be honest i am a bit unsure about this. it would be cleaner to not
have options at all. but it could lead to an excessive number of
adapter/client instances to have them with all the various options.
maybe we should still do it that way.

@dbu dbu Sep 29, 2015 via email

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sagikazarmark sagikazarmark Sep 29, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When we started this project we had a ConfigurableRequest, which wrapped the original request and had a getOptions method.

To be honest: how often do you need different timeout option in a client?

@joelwurtz joelwurtz Sep 29, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe not different timeout, but other behaviour may be needed, like sometimes you want an Async Request/Response and sometimes not.

@sagikazarmark sagikazarmark Sep 29, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, the SocketClient might be special from this point of view. These specific settings should probably be set in the constructor, or using specific setters.

@joelwurtz joelwurtz Sep 29, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't agree, we also need this settings to be set in the request call,

As an exemple, for our library (docker-php) we need to have this settings per request, as i want to have a logs response in a async manner (it's a chunked reponse which is populated from consecutive actions and stay alive "forever"), but we also want to have the stop / start calls in a sync manner (to be sure that it is correctly stop / start before doing anything else).

But i agree with @dbu and we should only inject these options when we know we use a specific client.

Another approach would be to have a "namespaced" option name for each adapter, where all the configuration under this namespace should only be handle by the adapter

// Global / Interoperable option
$options['timeout'] = 30;
// Specific option for socket adapter
$options['socket-adapter'] = [
    'specific-otption-for-socket' => 'value'
];
// Specific option for guzzle6 adapter
$options['guzzle6-adapter'] = [
    'specific-otption-for-guzzle6' => 'value'
];

@sagikazarmark sagikazarmark Sep 29, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would rather leave this to the code using the client and not add things like namespaces.

@dbu dbu Sep 30, 2015 via email

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

* Option that must be accepted by every client implementation (though it is free to ignore them
* if the underlying implementation does not support the option):
*
* - timeout (int): the timeout for the HTTP request in seconds. 0 means to use the default.
*
* @author Márk Sági-Kazár mark.sagikazar@gmail.com>
*/
interface HttpMethodsClient
Expand Down
8 changes: 8 additions & 0 deletions src/HttpPsrClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@
/**
* Sends one or more PSR-7 Request
*
* Implementations should be configured in their constructor. For runtime behaviour changes, there
* is an $options parameter. It SHOULD be validated by the client and an exception thrown if
* unknown options are passed.
* Option that must be accepted by every client implementation (though it is free to ignore them
* if the underlying implementation does not support the option):
*
* - timeout (int): the timeout for the HTTP request in seconds. 0 means to use the default.
*
* @author GeLo <geloen.eric@gmail.com>
*/
interface HttpPsrClient
Expand Down