-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: Allows Key instance to be used for encoding #575
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
base: main
Are you sure you want to change the base?
Conversation
Allows an instance of Key to be passed as the second argument to JWT::encode and JWT::sign, making $alg optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good, and I can see the logic in it... but I am not thrilled about having two ways of doing the same thing. It would be less confusing, looking at the phpdoc, to just have the one way (as it is today).
But I don't have a real issue with it. Could you add some unit tests as well?
string $keyId = null, | ||
array $head = null | ||
): string { | ||
if (is_a($key, Key::class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this library prefers the use of instanceof
if (is_a($key, Key::class)) { | |
if ($key, instanceof Key::class) { |
string $keyId = null, | ||
array $head = null | ||
): string { | ||
if (is_a($key, Key::class)) { | ||
$alg = $key->getAlgorithm(); | ||
$key = $key->getKeyMaterial(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would also want to throw an exception if $alg
isn't null
. Otherwise this would be possible and potentially cause confusion:
JWT::encode($payload, $key, 'FooAlg');
Allows an instance of Key to be passed as the second argument to
JWT::encode
andJWT::sign
, making $alg optional.This is more in line with how decoding works and as such it makes sense to streamline it.
This also generally makes the process easier in the use case where you get an instance of Key injected so you can just encode using
JWT::encode($payload, $key)
rather thanJWT::encode($payload, $key->getKeyMaterial(), $key->getAlgorithm())