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

[BREAKING CHANGE 🚨] Change arguments for authorize #489

Merged
merged 7 commits into from
Oct 20, 2019

Conversation

mfn
Copy link
Collaborator

@mfn mfn commented Oct 12, 2019

For background, see #481 and specifically #481 (comment) and further comments.

The basic change:

  • before: public function authorize(array $args): bool
  • after: public function authorize($root, array $args, $ctx, ResolveInfo $resolveInfo = null, Closure $getSelectFields = null): bool

The astute reader will recognize that these are exactly the same arguments passed to resolve, and this isn't a coincidence.

This is also made even clearer in the small refactor I did in the code regarding calling them:

            // Authorize
            if (call_user_func_array($authorize, $arguments) != true) {
                throw new AuthorizationError('Unauthorized');
            }

            return call_user_func_array($resolver, $arguments);

The only difference is that checking if the returned value and the (automatic) reaction to it (this fact was never different, but due to aligning the arguments it's now even more clear how this works).

Additional changes:

  • Don't merge $arguments[2] (context) into $arguments[1] (args), this seems like a strange thing to do, another one of those implicit behaviours without clear indication as to way (blame didn't offer me much more insights)
  • Remove method_exists for getRules, that method is already there and can never be "not there"

TODO

  • Add a test making use of all args

@mfn mfn self-assigned this Oct 12, 2019
@mfn
Copy link
Collaborator Author

mfn commented Oct 12, 2019

Pinging all involved in #481 for feedback @illambo @rebing @fvdung

FTR, I did not go down the suggestion with the trait, I simply couldn't see how this would improve things.

$arguments = func_get_args();

// Get all given arguments
if (! is_null($arguments[2]) && is_array($arguments[2])) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed, see PR desciption

// Validate mutation arguments
if (method_exists($this, 'getRules')) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -201,6 +199,11 @@ protected function getResolver(): ?Closure
};
}

// Authorize
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved down from before "selects and relations" block to include SelectFields too

Copy link
Owner

@rebing rebing left a comment

Choose a reason for hiding this comment

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

Makes sense to change it like this. How will we version it now - 3.0?

CHANGELOG.md Outdated Show resolved Hide resolved
@mfn
Copy link
Collaborator Author

mfn commented Oct 14, 2019

How will we version it now - 3.0?

Yes, absolutely has to be!

Actually also for #487 => changing the behaviour what kind of exception will be "reported" is also breaking semvar (IMHO / AFAIK)

@mfn mfn force-pushed the mfn-authorize-ctx branch 2 times, most recently from 1d08974 to 0dfe387 Compare October 20, 2019 17:51
@mfn mfn merged commit b4f8643 into rebing:master Oct 20, 2019
@mfn mfn deleted the mfn-authorize-ctx branch October 20, 2019 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants