Fix issue using optional() on a RemoteUser in Laravel 5.6+. #129
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Because we wrap checks for the `role` attribute in an `optional()` in Rogue, we need to support this object's unique field behavior.
weerd
reviewed
Jul 20, 2020
Comment on lines
+52
to
+54
| if (in_array($key, ['id', 'northstar_id', 'role'])) { | ||
| return true; | ||
| } |
weerd
approved these changes
Jul 20, 2020
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's this PR do?
This pull request fixes an issue where checking
optional(auth()->user())->rolewould throw an error in Laravel 5.6.This started happening because Laravel 5.6's
optionalhelper now checks if the requested field is set using the null-coalescing operator (which, behind the scenes runs anisset()check on that field). Because these fields aren't really set on theRemoteUserclass (which is just a convenience wrapper for accessing the token or Northstar user), it'd always be null!How should this be reviewed?
I extracted a
loadAttributesmethod in 30b6141 so I could use it in this new__issetmethod, added in f302997.I also cleaned up a reference to
$this->relationsfromHasAttributesin 97016a7. I think this was mistakenly copy-pasted when I extracted this logic from Laravel'sHasAttributesto use here.Checklist