Skip to content
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.

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

Argument order of "Api::api" method is not correct #116

Open
dereuromark opened this issue Jul 13, 2016 · 6 comments
Open

Argument order of "Api::api" method is not correct #116

dereuromark opened this issue Jul 13, 2016 · 6 comments
Labels
Milestone

Comments

@dereuromark
Copy link
Contributor

dereuromark commented Jul 13, 2016

The order as per clean code should not have optional values prior to required ones:

    public function api(
        $method = self::REQUEST_GET,
        $url,
        $data = array(),
        $return_as_json = false,
        $is_file = false,
        $debug = false
    )

Instead if should be

    public function api(
        $url,
        $method = self::REQUEST_GET,
        $data = array(),
        $return_as_json = false,
        $is_file = false,
        $debug = false
    )

So you can actually do

    ->api($url)

and the others are actually optional.

@aik099
Copy link
Collaborator

aik099 commented Jul 13, 2016

Yeah, I know. This is the problem with several other methods as well. PhpStorm always complains about it to me.

Unfortunately we can't change argument order because that would be a BC break.

@aik099
Copy link
Collaborator

aik099 commented Jul 13, 2016

What we can do is to remove default value for $method argument. That sound like BC break, but in fact it is not, because when manually calling Api::api method you have to specify $method all the time and trick like specifying null instead to get default value used won't work currently.

@aik099 aik099 changed the title Argument order of Api::api() is not correct. Argument order of "Api::api" method is not correct Jul 13, 2016
@dereuromark
Copy link
Contributor Author

I agree.
And maybe in a future major bump we can fix it.

@aik099
Copy link
Collaborator

aik099 commented Jul 13, 2016

So, if you'd like you can send PR that would remove default value for optional method arguments, that come (in method declaration) before non-optional arguments. There are several such methods for sure.

@aik099
Copy link
Collaborator

aik099 commented Jul 13, 2016

And maybe in a future major bump we can fix it.

I don't think, that changing argument order is a way to go, because in API calls the result changes greatly based on the request method (get/post/etc) so specifying it first and then url makes sense actually.

@aik099
Copy link
Collaborator

aik099 commented Aug 1, 2016

@dereuromark , should I expect PR for review soon?

@aik099 aik099 added this to the v2.0.0 milestone Aug 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants