Skip to content
This repository has been archived by the owner on Nov 2, 2022. It is now read-only.

Add hosting provider support (Provider) #10

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

msaladna
Copy link

@msaladna msaladna commented Feb 9, 2016

This pull adds support for hosting providers (API doc) on top of your existing code:

Usage would be something along these lines to create a new user and provision a zone under that account:

$api = new Cloudflare\Provider($providerkey);
$key = $api->create_user("[email protected]", "somepasswd")->user_key;
$api->zone_set($key, "somedomain.com", "mycname.somedomain.com");
  • Also fixed User::change_email() - fix typo in local variable name

User::change_email() - fix typo in local variable name
@jamesryanbell
Copy link
Owner

jamesryanbell commented Jul 17, 2016

Hi

Thanks for this, just a couple of points that need addressing:

  • The endpoint for the hosting API is different to the client API, https://api.cloudflare.com/host-gw.html, this would need to be factored into the package
  • Is there a specific reason you are using Doctrine\Instantiator\Exception\InvalidArgumentException? This is included into the package as part of PHPUnit but that's only a dev requirement for testing purposes, anyone who installed the package without the dev dependencies would not get this.

Thanks,
James

User::change_email() - fix typo in local variable name
PR comment fixes: correct InvalidArgumentException ref, commit rest of changes
@msaladna
Copy link
Author

Not sure why PHPStorm settled on importing a reference from Doctrine...

Pushed the missing changes. I'll leave formatting and style considerations to you.

@andrewklau
Copy link

Is there anything holding up getting this merged? Happy to help, would like to use this.

@jamesryanbell
Copy link
Owner

I've set aside some time this weekend to review this and hopefully merge.

Thanks,
James

Copy link
Owner

@jamesryanbell jamesryanbell left a comment

Choose a reason for hiding this comment

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

Just one small change to the authentication, once that's sorted I am happy to merge.

Thanks,
James

*/
public function __construct()
public function __construct($arg1, $arg2 = null, $arg3 = null)
Copy link
Owner

@jamesryanbell jamesryanbell Oct 1, 2016

Choose a reason for hiding this comment

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

Given that the provider authentication method is only used with the Provider class, I feel it would be simpler to revert this method back and just add another condition to the if statement for 3 arguments being passed in.

Then in the provider constructor you can do this:

parent::__construct($key, null, 'host');

I know it's a bit of a workaround but it saves having variable such as $arg1, $arg2 etc.. and reduces the complexity of the conditions. It also means we aren't introducing the breaking change of the constructor requiring at least one argument.

Copy link
Author

Choose a reason for hiding this comment

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

My argument to that is that the method signature now becomes ambiguous. Formal parameters are supposed to lend some insight as to what the method accepts and also work in conjunction with docblock commenting.

Not withholding, it also breaks reflection on the method. You'll get inconsistent results with what actually needs to be passed, e.g.

    class Foo
    {
        public function __construct() {
            $args = func_get_args();
            var_dump($args);
        }
    }

    $rfxn = new \ReflectionMethod('Foo', '__construct');
    var_dump($rfxn->getNumberOfRequiredParameters());

Alternatively, we can drop support for pre-PHP 5.6 and use the variadic notation, i.e. function foo(...$args) { ... }. Ultimately it's your project, so let me know the direction you want to proceed. That's just my two cents on the matter.

Copy link
Owner

Choose a reason for hiding this comment

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

Hi

Just holding off on this now as CloudFlare have added a third parameter to their authentication methods: User Service Key (https://api.cloudflare.com/#endpoints). So I need to think about how this factors into this work.

Thanks,
James

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants