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

Add support for custom authorization message #578

Merged
merged 1 commit into from
Jan 19, 2020
Merged

Add support for custom authorization message #578

merged 1 commit into from
Jan 19, 2020

Conversation

Sh1d0w
Copy link
Contributor

@Sh1d0w Sh1d0w commented Jan 19, 2020

Summary

Currently it is not possible to override the authorization message in case the authorization message returns false.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

@Sh1d0w Sh1d0w requested review from rebing and mfn and removed request for rebing January 19, 2020 05:07
Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, rarely have a seen such a good "1st PR", thanks for covering everything!

I've only some minor feedback and I'm ready to merge afterwards. I would appreciate if you just rebase your first commit.

Thanks!

CHANGELOG.md Outdated
Comment on lines 6 to 9


2020-01-19, 4.0.0
-----------------
Copy link
Collaborator

@mfn mfn Jan 19, 2020

Choose a reason for hiding this comment

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

Please remove these lines, as the next release date isn't decided yet!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure will do. I haven't added this initially, but saw in the PR checklist it was required to amend CHANGELOG.md and wasn't sure what exactly to add. Are you happy to remove the whole entry I've added?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, please keep the changelog (it's a "change log", after all 😜) but only remove the lines I covered in github!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mfn Right, should be ok now I think :)

Comment on lines 293 to 295
/**
* @return string
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the phpdoc, doesn't "add" anything here, thanks!

/**
* Test query with authorize.
*/
public function testQueryAndReturnResultWithAuthorizeMessage(): void
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function testQueryAndReturnResultWithAuthorizeMessage(): void
public function testQueryAndReturnResultWithCustomAuthorizeMessage(): void

What do you think about this suggestion?

@Sh1d0w Sh1d0w requested a review from mfn January 19, 2020 07:16
@Sh1d0w
Copy link
Contributor Author

Sh1d0w commented Jan 19, 2020

Thanks for the feedback @mfn , I've updated my PR.

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

Thanks!

@mfn mfn merged commit 1498647 into rebing:master Jan 19, 2020
@mfn
Copy link
Collaborator

mfn commented Apr 3, 2020

https://github.com/rebing/graphql-laravel/releases/tag/5.0.0 has been released which includes this!

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.

None yet

3 participants