Add Pusher::forUrl static factory function about pusher-http-php

[leggetter]

Although in this comment we made a decision to support:

$pusherConfig = new Config('https://key:[email protected]/apps/1234');
$pusher = new Pusher($pusherConfig);

I don't think it's the right decision. I don't see the need to expose a Config class that doesn't add any value to the public API.

We could update the constructor to support:

$pusher = new Pusher('https://key:[email protected]/apps/1234');

But then where do options parameters get added since the 2nd and 3rd are usually the appKey and appSecret.

Thus, I would opt to keep the 1st parameter as the application ID only when using new Pusher and go with a dedicated factory function (this issue) when using this interesting URL initialisation:

$options = [];
$pusher = Pusher::forUrl('https://key:[email protected]/apps/1234', $options);

This also aligns with the Node library:
https://github.com/pusher/pusher-http-node/blob/master/lib/pusher.js#L62

Any objections or additional thoughts?

[tanuck]

Well there will be a Config class that's exposed, so whether we have the forUrl() method or not, that api will still exist. It's just a case of whether we advertise this in the documentation.

And obviously we've already regressed the new Pusher('https://key:[email protected]/apps/1234'); so it doesn't make sense to go back down that road. I'm happy for using this static factory method, but as long as it's not some other variation of the old singleton factory method (Pusher::instance();).

[leggetter]

I've had a dig into this and it's a bit of a rabbit hole. The Config class does a few things that probably need refactored:

1. You can't create a new Config instance with the key:secret URL and also pass in additional options that aren't represented in the URL e.g. debug/logger, proxy
2. It supports app key/secret pairs which we don't need. This was actually also in the Python library and we took it out before release

My personal preference right now would be to move this issue to a 3.1 milestone and create a fix issue (or re-open #66) for 3.0 in order to remove support for the first parameter of the Pusher constructor (appId) being a Config instance.

3.1 will then include:

1. support for Pusher::fromUrl($url, $options) (this issue)
2. remove key pair support from Config
3. Refactor URL parsing so it can be used outside of the Config class or maybe $config = Config.fromUrl($url)
4. Update Config constructor to only support new Config($options) or maybe it should align with the Pusher constructor: new Config($appId, $appKey, $appSecret, $options)

Putting all this together should allow a cleaner solution to this issue than the rabbit hole I mentioned. This is just an off the top of my head and there may be a more elegant solution:

class Pusher {

  public static function fromUrl($url, $options) {
    $config = Config::fromUrl($url);
    $pusher = new Pusher($config->appId, $config->appKey, $config->appSecret, $options);
    return $pusher;
  }

}

Does anybody object to removing support for new Pusher($configInstance) in 3.0? The main problem I can think of is that it supports Heroku's preferred key:secret URL method. But I don't believe too many developers a) use PHP on Heroku b) use a non-US cluster on Heroku c) Have needs for a & b

[zimbatm]

I had a look at this but am unsure in what file to put that function. If we're following PSR-4 the only way to have a Pusher::fromUrl() function to be auto-loaded would be to define a top-level Pusher class.

[zimbatm]

Nevermind, I'll follow what Guzzle does: https://github.com/guzzle/guzzle/blob/master/composer.json#L26