Add AddPathPlugin and BaseUriPlugin #48

soullivaneuh · 2016-10-14T12:36:41Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no since this is a kind of bug regarding the situation
Deprecations?	no
Related tickets	N/A
Documentation	php-http/documentation#163
License	MIT

What's in this PR?

This bugfix permits to keep the base path of an URI on the AddHost plugin.

Why?

With the following PHP code example:

$httpClient = new HttpMethodsClient(
    new PluginClient(HttpClientDiscovery::find(), [
        new AddHostPlugin(UriFactoryDiscovery::find()->createUri('https://logs.domain.com/api')),
        new ErrorPlugin(),
    ]),
    MessageFactoryDiscovery::find()
);
$httpClient->get('/users');

The https://logs.domain.com/users URL will be called instead of /api/users.

The base path should be kept on this plugin.

Checklist

Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
Update the tests

dbu · 2016-10-15T11:08:11Z

as far as i remember, @sagikazarmark had a good argument why this should be a separate plugin. i can't find the place we discussed this anymore. mark would you rembember?

doing this like you do here would be a BC break-ish thing as people that now use a domain with path and rely on the path not being appended would see their applications stop working. and its a new feature, not an actual bugfix so i think we should not do a BC break in a minor version for this.

for the content, question is whether we treat /path/something and path/something the same or only prepend a path when there is no leading /? depending on the scenario, both can make sense.

sagikazarmark · 2016-10-15T11:17:17Z

This one? php-http/plugins#42 (comment)

dbu · 2016-10-15T11:27:12Z

exactly, thanks!

to sum it up:

separate plugin for path:

+ clearly separated logic
+ mix and match with replace host or add host
- two plugins to configure
- unexpected behaviour when specifying path to AddHostPlugin => should we forbid that?

if we would mix it together, would we only add the path if the host is missing, or always even if the host exists? should we check if the path already starts with the configured path and if so, skip adding it again (probably not)?

@soullivaneuh what is your opinion on the other thread? would you agree that we move this to a separate plugin?

sagikazarmark · 2016-10-15T11:37:54Z

AddHostPlugin only sets the host, scheme and port, ommits the path at the moment.

joelwurtz · 2016-10-15T13:08:13Z

Yes we talk about this and say it must be in a separate plugin like an AddPathPlugin, then we could imagine a third one using these two (like BaseUriPlugin).

soullivaneuh · 2016-10-17T12:42:22Z

doing this like you do here would be a BC break-ish thing as people that now use a domain with path and rely on the path not being appended would see their applications stop working. and its a new feature, not an actual bugfix so i think we should not do a BC break in a minor version for this.

@dbu This is technicaly a BC break, and you know how I'm pernickety with this for Sonata. 😉
But for me it's not really a BC break because who will setup http://domain.com/api on this plugin if the /api append does not work?

But I agree with your point of view about the goal of the plugin, I hesitated for this too.

I'll rework it for a AddPathPlugin then.

then we could imagine a third one using these two (like BaseUriPlugin).

@joelwurtz What do you mean by "using"? I don't see how expect rewrite the logic of both on this class.

joelwurtz · 2016-10-17T13:03:56Z

@joelwurtz What do you mean by "using"? I don't see how expect rewrite the logic of both on this class.

Something like this :

class BaseUriPlugin
{  
    private $addHostPlugin;

    private $addPathPlugin;

    public function __construct($options = [])
    {
        // @TODO deal with options
        $this->addHostPlugin = new AddHostPlugin($options);
        $this->addPathPlugin = new AddPathPlugin($options);
    }

    public function handleRequest(RequestInterface $request, callable $next, callable $first)
    {
        $addHostNext = function (Request $request) use ($next, $first) {
             return $this->addHostPlugin->handleRequest($request, $next, $first);
        };

        return $this->addPathPlugin->handleRequest($request, $addHostNext, $first);
    }
}

dbu · 2016-10-17T13:36:28Z

i like joel's proposal. the coupling of BaseUriPlugin will be a bit tight like this. Maybe use two option arguments, one for host and one for path? that leaks the abstraction but is clean and future compatible

soullivaneuh · 2016-10-17T13:38:11Z

Maybe use two option arguments, one for host and one for path?

My AddPathPlugin does not need options ATM.

soullivaneuh · 2016-10-17T13:59:05Z

Code updated, thanks for reviewing.

joelwurtz

Need to be fixed (see my comments) otherwise LGTM

joelwurtz · 2016-10-17T14:01:41Z

src/Plugin/BaseUriPlugin.php

+            return $this->addPathPlugin->handleRequest($request, $addHostNext, $first);
+        }
+
+        return $addHostNext;


Should be return $addHostNext($request);

joelwurtz · 2016-10-17T14:02:50Z

spec/Plugin/BaseUriPluginSpec.php

+
+        $this->beConstructedWith($host, ['replace' => true]);
+        $this->handleRequest($request, function () {}, function () {});
+    }


Can you add test when there is no path ? (with my previous comment it should have failed :) )

xabbuh · 2016-10-18T06:44:56Z

src/Plugin/AddPathPlugin.php

+    public function __construct(UriInterface $host)
+    {
+        if ($host->getPath() === '') {
+            throw new \LogicException('Host path can not be empty');


xabbuh · 2016-10-18T06:48:29Z

src/Plugin/AddPathPlugin.php

+    /**
+     * @param UriInterface $host
+     */
+    public function __construct(UriInterface $host)


By the way, what is the advantage of passing an UriInterface instance here instead of a plain string?

i guess the advantage is that we have validation. otherwise we would need to discover the uri factory or use parse_url to get the path part and make sure we have a path part.

Do we really? the only with we do with $host is to convert it to a string..

Well, actually we don't need a host here at all. The user just has to pass a path (not a host) and I am not sure that actually needs any validation at all.

no, we do $host->getPath() once here to validate, then again where we use it to just get the path part. if we would accept any string, you could create this plugin with "foo bar baz" and we would try to change the request uri to that. or we start to parse urls in here (which might have been validated and parsed before being passed), which is what the Uri model would free us from.

I agree with @dbu.

This also make an easier argument passing for BaseUriPlugin.

After that, it's as you want. I can pass a string instead if you prefer.

Nyholm · 2016-10-17T16:20:50Z

src/Plugin/AddPathPlugin.php

+    /**
+     * @param UriInterface $host
+     */
+    public function __construct(UriInterface $host)


Does this need to be a URIInterface? It will always be converted to a string.

Discussed here: #48 (comment)

dbu · 2016-10-18T07:31:11Z

src/Plugin/BaseUriPlugin.php

+    private $addHostPlugin;
+
+    /**
+     * @var AddPathPlugin


this is AddPathPlugin|null

I do not think the docblock here is necessary at all. IDEs should be able to infer the type themselves (the same applies to the $addHostPlugin property).

we are using these blocks throughout the code, imho its consistent to keep it. with the |null and a text saying this is the plugin for adding the path and only set if there is a path to add at all, it would be valuable

dbu · 2016-10-18T07:49:30Z

src/Plugin/BaseUriPlugin.php

+    {
+        $this->addHostPlugin = new AddHostPlugin($host, $hostConfig);
+
+        if ($host->getPath()) {


we should rtrim($path, '/') here. or throw an exception on http://domain.com/ (it makes no sense to prepend just / to a path)

dbu · 2016-10-18T07:52:50Z

src/Plugin/AddPathPlugin.php

+    public function handleRequest(RequestInterface $request, callable $next, callable $first)
+    {
+        $request = $request->withUri($request->getUri()
+            ->withPath($this->host->getPath().$request->getUri()->getPath())


what about anything ending on /? notably http://domain.com/ but also http://domain.com/path/ - both would end up in double leading slash, which is very likely not what the user wanted.

this is a rather annoying edge case. i think we should somehow handle it (maybe validate and InvalidArgumentException) to avoid wtf moments (some servers will handle the example.com//something paths, others wont). if somebody actually wants to create double slashes, they can write their own plugin, its a rare edgecase.

I will add a test for that and handle it.

soullivaneuh · 2016-10-18T08:10:35Z

Requested changes was made.

Except if you want me to change something on https://github.com/php-http/client-common/pull/48/files/2aac11b11bfd025c9d6082e722a1408139ab36f8#diff-44d3f10fc0543e373abc05223f369e01, this PR should be LGTM.

dbu

went over this and think we should throw an exception rather than eat away offending / characters. and some minor codestyle requests.

dbu · 2016-10-18T08:13:48Z

CHANGELOG.md

@@ -8,7 +8,8 @@
 - Fix Emulated Trait to use Http based promise which respect the HttpAsyncClient interface
 - Require Httplug 1.1 where we use HTTP specific promises. 
 - RedirectPlugin: use the full URL instead of the URI to properly keep track of redirects
-
+- Add AddPathPlugin for API URLs with base path
+- Combine AddHostPlugin and AddPathPlugin onto BaseUriPlugin


Lets say "Add BaseUriPlugin that combines ..."

dbu · 2016-10-18T08:16:35Z

src/Plugin/AddPathPlugin.php

+     */
+    public function __construct(UriInterface $host)
+    {
+        if (rtrim($host->getPath(), '/') === '') {


i think i would check '' or last character is '/' and throw an exception when it is. silently fixing things makes harder debugging. after doing example.com/ once or twice, people will learn to use example.com

dbu · 2016-10-18T08:16:37Z

src/Plugin/AddPathPlugin.php

+    public function handleRequest(RequestInterface $request, callable $next, callable $first)
+    {
+        $request = $request->withUri($request->getUri()
+            ->withPath(str_replace('//', '/', $this->host->getPath().$request->getUri()->getPath()))


this feels odd. i would rtrim '/' again. if somebody puts double slashes elsewhere, they probably wanted that. but see above...

If we throw an exception like asked in #48 (comment), we don't need to rtrim anything IMHO.

dbu · 2016-10-18T08:17:08Z

src/Plugin/AddPathPlugin.php

+    /**
+     * @var UriInterface
+     */
+    private $host;


lets call this $uri, instead of $host. also in the constructor argument.

dbu · 2016-10-18T08:17:34Z

src/Plugin/BaseUriPlugin.php

+     * @param UriInterface $host
+     * @param array        $hostConfig Config for AddHostPlugin. @see AddHostPlugin::configureOptions
+     */
+    public function __construct(UriInterface $host, array $hostConfig = [])


s/$host/$uri/

dbu · 2016-10-18T08:18:03Z

src/Plugin/BaseUriPlugin.php

+    private $addPathPlugin = null;
+
+    /**
+     * @param UriInterface $host


we should say that this must contain a host name and may contain a path that must not end on /

soullivaneuh · 2016-10-18T09:43:36Z

@dbu All requested changes was made.

dbu · 2016-10-18T10:54:13Z

LGTM

Nyholm · 2016-10-18T10:53:18Z

src/Plugin/AddPathPlugin.php

+use Psr\Http\Message\UriInterface;
+
+/**
+ * Add base path to the request URI. Useful for base API URLs like http://domain.com/api.


Maybe be more clear here:

"Prepend a base path to..."

Nyholm · 2016-10-18T10:54:14Z

src/Plugin/BaseUriPlugin.php

+use Psr\Http\Message\UriInterface;
+
+/**
+ * This plugin combines the host and path plugins.


Maybe:
This plugin combines the AddHostPlugin and AddPathPlugin.

sagikazarmark

Nice work, thanks!

sagikazarmark · 2016-10-18T11:06:57Z

spec/Plugin/AddPathPluginSpec.php

+
+    function it_is_initializable(UriInterface $uri)
+    {
+        $uri->getPath()->shouldBeCalled()->willReturn('/api');


I think this could be moved to the let method. That would simplify things.

sagikazarmark · 2016-10-18T11:07:31Z

spec/Plugin/AddPathPluginSpec.php

+    }
+
+    function it_throws_exception_on_empty_path(
+        UriInterface $host


Why multiline?

sagikazarmark · 2016-10-18T11:07:35Z

spec/Plugin/AddPathPluginSpec.php

+    }
+
+    function it_throws_exception_on_trailing_slash(
+        UriInterface $host


Why multiline?

Well, a copy/paste with parameters removal... 😛

sagikazarmark · 2016-10-18T11:07:50Z

spec/Plugin/BaseUriPluginSpec.php

+
+    function it_is_initializable(UriInterface $uri)
+    {
+        $uri->getHost()->shouldBeCalled()->willReturn('example.com');


Same here: can these be moved to let?

I'll look at this, I'm more familiar with PHPUnit than PHPSpec. 😉

I tried this:

function let(UriInterface $uri) { $uri->getPath()->shouldBeCalled()->willReturn('/api'); $this->beConstructedWith($uri); }

But got this error:

Http/Client/Common/Plugin/AddPathPlugin 47 ✘ throws exception on trailing slash some predictions failed: Double\UriInterface\P114: No calls have been made that match: Double\UriInterface\P114->getPath() but expected at least one. Http/Client/Common/Plugin/AddPathPlugin 56 ✘ throws exception on empty path some predictions failed: Double\UriInterface\P116: No calls have been made that match: Double\UriInterface\P116->getPath() but expected at least one.

I don't think this can be refactored on the let method because the return value changes on some cases.

AFAIK, let will set default config, so you can set the default input in the let method and override it where it needs (when there is an empty path by example)

soullivaneuh · 2016-10-18T12:03:53Z

@Nyholm @sagikazarmark Requested changes made, except #48 (comment) for the moment.

Nyholm

Awesome. I'm happy with these changes.

I'll let Mark decide on the PHPSpec comments.

dbu · 2016-10-18T12:10:53Z

great! there is one thing left to do: the documentation pull request so that these plugins are documented as well. can you please do that?

soullivaneuh · 2016-10-18T12:18:22Z

Sure! Shall I just add two files on this part? https://github.com/php-http/documentation/tree/master/plugins

Nyholm · 2016-10-18T12:29:38Z

Yes, I've added an issue for that. See php-http/documentation#162

soullivaneuh · 2016-10-18T15:37:57Z

Documentation added: php-http/documentation#163

dbu · 2016-10-19T07:01:28Z

thanks @soullivaneuh !

sagikazarmark · 2016-10-19T07:04:26Z

Thank you @soullivaneuh

soullivaneuh · 2016-11-04T08:59:13Z

Thank you too! Could we please have a new release for this feature? Thanks.

sagikazarmark · 2016-11-04T09:12:47Z

Done: https://github.com/php-http/client-common/releases/tag/v1.3.1

xabbuh · 2016-11-04T09:16:06Z

Shouldn't that have become 1.4.0?

And by the way, could you fix the link to the changelog for the 1.3.0 release? :)

soullivaneuh · 2016-11-04T09:16:34Z

Shouldn't that have become 1.4.0?

I agree, it's a new feature. It should be a new minor if you follow semver ofc.

sagikazarmark · 2016-11-04T09:18:18Z

Well, it didn't seem to be that huge change, but you are right.

sagikazarmark · 2016-11-04T09:21:59Z

Done

soullivaneuh · 2016-11-04T09:22:07Z

Thank you!

soullivaneuh force-pushed the add-host-path branch 2 times, most recently from 1781c76 to 3c94066 Compare October 14, 2016 12:41

joelwurtz closed this Oct 15, 2016

joelwurtz reopened this Oct 15, 2016

soullivaneuh force-pushed the add-host-path branch 2 times, most recently from a179c23 to 1b62dc0 Compare October 17, 2016 13:58

soullivaneuh changed the title ~~The AddHost plugin should take care of the base path~~ Add AddPathPlugin and BaseUriPlugin Oct 17, 2016

joelwurtz requested changes Oct 17, 2016

View reviewed changes

soullivaneuh force-pushed the add-host-path branch from 1b62dc0 to 2aac11b Compare October 17, 2016 14:11

joelwurtz approved these changes Oct 17, 2016

View reviewed changes

soullivaneuh mentioned this pull request Oct 17, 2016

Client setup with Symfony integration nexylan/graylog-sdk#1

Merged

xabbuh reviewed Oct 18, 2016

View reviewed changes

Nyholm reviewed Oct 18, 2016

View reviewed changes

dbu reviewed Oct 18, 2016

View reviewed changes

soullivaneuh force-pushed the add-host-path branch from 2aac11b to f7bfea9 Compare October 18, 2016 08:08

dbu requested changes Oct 18, 2016

View reviewed changes

soullivaneuh force-pushed the add-host-path branch 2 times, most recently from 36c470d to de9bff6 Compare October 18, 2016 09:43

dbu approved these changes Oct 18, 2016

View reviewed changes

Nyholm reviewed Oct 18, 2016

View reviewed changes

sagikazarmark requested changes Oct 18, 2016

View reviewed changes

soullivaneuh added 2 commits October 18, 2016 14:01

Added AddPathPlugin

f4e245d

Add BaseUriPlugin that combines AddHostPlugin and AddPathPlugin

2c2e86d

soullivaneuh force-pushed the add-host-path branch from de9bff6 to 2c2e86d Compare October 18, 2016 12:01

Nyholm approved these changes Oct 18, 2016

View reviewed changes

sagikazarmark approved these changes Oct 18, 2016

View reviewed changes

Nyholm mentioned this pull request Oct 18, 2016

Add documentation for AddPathPlugin and BaseUriPlugin php-http/documentation#162

Closed

soullivaneuh mentioned this pull request Oct 18, 2016

Request URI Manipulations php-http/documentation#163

Merged

dbu merged commit c14fdb6 into php-http:master Oct 19, 2016

soullivaneuh deleted the add-host-path branch November 4, 2016 08:58

Add AddPathPlugin and BaseUriPlugin #48

Add AddPathPlugin and BaseUriPlugin #48

Conversation

soullivaneuh commented Oct 14, 2016 • edited Loading

What's in this PR?

Why?

Checklist

dbu commented Oct 15, 2016

sagikazarmark commented Oct 15, 2016

dbu commented Oct 15, 2016

sagikazarmark commented Oct 15, 2016

joelwurtz commented Oct 15, 2016

soullivaneuh commented Oct 17, 2016 • edited Loading

joelwurtz commented Oct 17, 2016

dbu commented Oct 17, 2016

soullivaneuh commented Oct 17, 2016

soullivaneuh commented Oct 17, 2016

joelwurtz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soullivaneuh commented Oct 18, 2016

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soullivaneuh commented Oct 18, 2016

dbu commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelwurtz Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

soullivaneuh commented Oct 18, 2016

Nyholm left a comment

Choose a reason for hiding this comment

dbu commented Oct 18, 2016

soullivaneuh commented Oct 18, 2016

Nyholm commented Oct 18, 2016

soullivaneuh commented Oct 18, 2016

dbu commented Oct 19, 2016

sagikazarmark commented Oct 19, 2016

soullivaneuh commented Nov 4, 2016

sagikazarmark commented Nov 4, 2016

xabbuh commented Nov 4, 2016

soullivaneuh commented Oct 14, 2016 •

edited

Loading

soullivaneuh commented Oct 17, 2016 •

edited

Loading

dbu Oct 18, 2016 •

edited

Loading

joelwurtz Oct 18, 2016 •

edited

Loading