AddHostPlugin

[Nyholm]

This will implement baseUri functionality. It copies Guzzle's behaviour when the input starts with a slash.

// base uri is http://foo.com/api/

Input	Output
http://baz.com/biz	http://baz.com/biz
foo	http://foo.com/api/foo
/foo	http://foo.com/foo

[joelwurtz]

Nice, do you think it's possible to remove the UriFactory dependancy and only accept a UriInterface for this Plugin ?

IMO would have a better reusability and less complexity for user experience

[Nyholm]

Yeah, you are probably right. That would be possible. I agree that it wound be more reusable but I wound also decrease the developer experience. We just push the responsibility to convert a uri string to an object away from us.

But I agree with your suggestion. I'll update shortly.

[Nyholm]

There you go. No UriFactory needed!

[joelwurtz]

We just push the responsibility to convert a uri string to an object away from us.

Yes that's something that i'm considering by adding some sort of factory around plugins

[joelwurtz]

Not possible to provide a base uri without host ? (Like we just want to add /api path for our url and keep current host ?)

[joelwurtz]

Hum nevermind my comment didn't read well, instead it's not possible to change the host with the base uri ?

For me base uri has 2 roles :

• Adding some sort of base path if needed
• Changing host if needed (very useful when in a developement environment or staging, so we can call a service on another url just by adding this plugin)

[Nyholm]

Hm, I've looked at Guzzle's implementation and copied their functionality. Which means that if we have the following:

Base path	Input	Output
http://foo.com/api/	http://baz.com/biz	http://baz.com/biz
http://foo.com/api/	foo	http://foo.com/api/foo
http://foo.com/api/	/foo	http://foo.com/foo

Do I understand you correctly when you suggest the base path should always replace the available parts of the input?
It is a great idea but I can see a problem with that. How do you know what to replace?

Base path	Input	Output
http://foo.com/api/	http://baz.com/biz/bar	http://foo.com/api/biz/bar, http://foo.com/api/bar or http://foo.com/api/?

I believe that the one that writes the api client should use this plugin to allow users to replace the base path with whatever.

I would suggest a ReplacePath plugin to implement your functionality.

[dbu]

i would expect a base uri plugin to only trigger if my uri has no domain in it, and never replace an explicitly specified domain. agree with tobias on this one. if there is a real use case to replace domains, we can do a plugin, but it seems like a rather weird edge case where we might better let people write a custom plugin to handle their case. as in replace only some domains or whatever.

@Nyholm another question on this line: what about an uri like ://example.com/foo, so one that is missing just the protocol part? should we handle that? i tend to say no, but we might want to be explicit in the documentation on that.

[joelwurtz]

Don'tt think it's possible to have something like ://example.com/foo since it's coming from an uri object. So it would be a defectuous implementation and not our part IMO

[dbu]

@joelwurtz ://example.com/foo is a valid uri afaik. you use that for example to include css/js/images on pages that are served both over http and https, to avoid security warnings by the browser. (though most situations https inside a http page would be fine - but not the other way round)

[sagikazarmark]

Isn't it //example.com/foo? At least I always ommited the colon.

[joelwurtz]

I think it's like @sagikazarmark said //exemple.com/foo

BTW guzzle and diactoros use parse_url as the parser for the string, so this string would recognize as the path :

$ php -r "var_dump(parse_url('://exemple.com/test'));" 
array(1) {
  'path' =>
  string(19) "://exemple.com/test"
}

$ php -r "var_dump(parse_url('//exemple.com/test'));"
array(2) {
  'host' =>
  string(11) "exemple.com"
  'path' =>
  string(5) "/test"
}

[dbu]

ups, you are right. the correct format without schema is // not ://

so do we agree that the code is correct?

[joelwurtz]

Hum after all i'm not sure about this i don't see the value of a plugin like this.

Maybe we should separate this in 2 plugins :

• PrefixPathPlugin: to change the path after host, it will simply take a string and add it in every request call
• BaseHostPlugin: change / add host for a request (this plugin can have an option to only set host if not defined)

So the current functionallity can be achieved by a combination of this 2 plugins, also it allow better control for the user and a better understanding on what it's going on.

WDYT ?

[Nyholm]

i like the idea is splitting it. But I also think we should be very close to Guzzles functionality and change if we really have a good reason for it.

If we look at the LinkedIn API there are two endpoints. One to change a Oauth code for a token and one to make API requests. With guzzle I (API developer) can instansiate a client with a base uri like "https://api.linkedin.com/api/" and do all my request with ->get("resource").
When I'm changing code for a token I would do ->get("https://www.linkedin.com/api/token")
The point is that I (API developer) can choose when to use the base uri and when to not use it.

A plugin like this does not, however, replace the functionality for always changing the host and prefixing the path.

[joelwurtz]

With your exemple i understand more the use case, didn't think of that :)

However i think this can be achieve with options to this 2 plugins

• Only add host if no host present (in the BaseHost plugin)
• Only add prefix if no trailing slash in path (in the PrefixPath plugin)

[dbu]

imho, we should not provide a plugin to rewrite urls. chances are high you are doing some very specific hackery when needing this, and in that situation you better write your own plugin tailered to the case you need to fix.

i would stick with the cases guzzle covers for what we provide, it seems to me the least astonishing behaviour.

[sagikazarmark]

I think we should keep single responsibilities and provide plugins for the most common use-cases. If someone needs some more functionality, writting plugins is not that hard.

[joelwurtz]

For me the BaseHost plugin is a must have for everyone and a very common use case, as when you develop something or are in production you want to use different host for your api.

So having a BaseHost plugin is something needed, also your staging host may be shared with others applications (or you local) so you want to add a prefix in the path in this case.

I think it would be very helpful to have this plugins given this use case, and by adding these 2 plugins with the corresponding options we can cover this use case and the guzzle one.

[sagikazarmark]

Note:
http://foo.com/api/ foo http://foo.com/api/foo

This scenario is not possible with PSR-7. The interface requires a starting / if no domain is added, which transforms foo to /foo.

[dbu]

ah, good catch mark. i guess that makes this discussion simpler: whatever the base uri is is prepended to the path. the "foo" case is thus leftover in guzzle from their own request, or guzzle psr7 request is not complying with the spec. imho we should not cater to specific implementation glitches. if somebody needs to "absolutize" paths, they can do that in their own library, e.g. in a wrapper around the request factory - we did that for FOSHttpCache.

what we want to be sure is that the base uri does not end on /, to avoid google.com//foo situations.

[dbu]

@Nyholm do you have time to wrap this one up? is everything clear or do we have uncertainties about what can be supported / should be supported?

[Nyholm]

@Nyholm do you have time to wrap this one up? is everything clear or do we have uncertainties about what can be supported / should be supported?

I will do that this afternoon.

[Nyholm]

So, what I have heard there are quite a few scenarios we would like to solve. I've listed them below. Some facts:

• We should not try to solve everything in one plugin.
• We can not copy the behavior form Guzzle because restrictions in PSR-7.

The use cases we want to have support for is (no particular order):
A) PrefixPathPlugin - Adding/Prefixing a base path
B) ReplaceHostPlugin - Changing host (useful in development)
C) BaseHostPlugin - Adding host if none available

As in the role of a API client developer I would like to use a BaseHostPlugin for two reasons. (1) To avoid write the host all the time and (2) to allow users to change the host where I allow them by using a BaseHostPlugin themselves.

As an application developer I would like to have the possibility replace all calls to production APIs. This is useful in staging environments to make sure we control where we send HTTP requests.

Im not sure about the use case for a PrefixPathPlugin. @joelwurtz you might want to help here?

---

As an API client developer Im likely to write something like this:

public function __construct(HttpClient $client = null) {
  if ($client === null) {
    $client = HttpClientDiscovery::find();
  }

  $this->client = new PluginClient($client, [BaseHostPlugin('http://example.com')]);
}

public function foo() {
  return $this->client->sendRequest(new Request('/api/resource'));
}

And when I use that API client I should do:

$client = new PluginClient(new Http\Adapter\Guzzle6\Client(), [BaseHostPlugin('http://staging.com')]);
$api = new Api($client);

There is an issue here; http://staging.com will never be used because it belongs to the second plugin chain. At the time the second plugin chain is being evaluated there will always be a host.

So instead I should do something like this:

public function __construct(HttpClient $client = null) {
  if ($client === null) {
    $client = new PluginClient(HttpClientDiscovery::find(), [BaseHostPlugin('http://example.com')]);
  }

  $this->client = client;
}

... and then tell my users that they have to use the base plugin if they provide a new client.

---

Would it make sense to have a BaseHostPlugin and a ReplaceHostPlugin or should their functionality be merged to one plugin?

class BaseHostPlugin {
  public function __construct($host, $replace = false) {}
}

[joelwurtz]

i prefer the option way : BaseHostPlugin and ReplaceHostPlugin merged with an override option

Im not sure about the use case for a PrefixPathPlugin. @joelwurtz you might want to help here?

It's the same use case as the ReplaceHostPlugin as maybe my dev environnement use subpath resolution instead of domain name resolution

[joelwurtz]

It is also useful with a versioned api in the url, like /v1.1/articles, so people can write /articles and easily replace the version number with this plugin

[Nyholm]

Are we talking about a prefixing, replacing and appending the URI?

[sagikazarmark]

Not sure if it makes sense here, but there are some header plugins and those are prefixed with Header in the class name. Would it make sense to do the same here?

[joelwurtz]

Only prefixing, also maybe we don't want to bother with that for the moment and think more about this use case in another PR / Issue

[Nyholm]

Okey, I'll update this PR to be the BaseHostPlugin that will change the schema and domain.
I think the default action should be "add if no host" and you have to choose if you want to force a replace.

class BaseHostPlugin {
  public function __construct($host, $replace = false) {}
}

What do you think?

[joelwurtz]

👍 Please use an options array with symfony/options-resolver for the constructor as it's done in every plugin now

[Nyholm]

👍 Please use an options array with symfony/options-resolver for the constructor as it's done in every plugin now

Thank you for the reminder.

The PR is updated

[joelwurtz]

Look at https://github.com/phpDocumentor/fig-standards/blob/master/proposed/phpdoc.md#examples-12 on how the intent is for config array parameters in phpdoc, even if the standard is not yet accepted the format for config is pretty stable

[Nyholm]

Yeah, I saw that but I choose not to include it in this PR because I wanted to be consistent with other plugins and then send a PR to fix them all.

I'll make sure this plugin is doing it the correct way now already then. =)

[dbu]

i would inline this in the constructor, but thats really a matter of taste :-)

[Nyholm]

Yeah, I saw your changes on the other plugins. I was suggested to do like this on the CachePlugin and I feel it is easer to read this way. But as you say, it is a matter of taste. We use both ways at the moment.

[dbu]

sorry if you already answerd, but i can't find my question anymore. should we call this $host? baseUri was correct for the original way this was going, but now its different.

[Nyholm]

Ive must have missed that. Sorry.

[Nyholm]

@dbu @joelwurtz Did you mean like this?

[dbu]

👍

[joelwurtz]

Could you fix the styleci issue like #51 ? and then go merge :)

[Nyholm]

Updated

[sagikazarmark]

Cool. Ready to merge and release?

[Nyholm]

Yes

[sagikazarmark]

@joelwurtz whenever you feel. 😄

[joelwurtz]

I'm feeling it right now ^^

[joelwurtz]

Thanks @Nyholm !

[Nyholm]

Thank you all for the feedback and discussions!