Change Summary:
Rebased on master
Description: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||
Commit: |
|
||||||||||||||||||
Diff: |
Revision 2 (+385 -47) |
Review Request #1088 — Created Jan. 11, 2024 and submitted
Information | |
---|---|
b.stoop | |
Lunr.Vortex | |
fcm/updateapi | |
|
|
1089 | |
7ab046e... | |
Reviewers | |
lunr | |
Based on master
FCM: Update to new firebase API v1
https://firebase.google.com/docs/cloud-messaging/migrate-v1?hl=en&authuser=0#update-authorization-of-send-requests, We now need a OAuth token to send notifications to firebase.
Github Actions run: https://github.com/brianstoop/lunr.vortex/actions/runs/7831880681
Rebased on master
Description: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||
Commit: |
|
||||||||||||||||||
Diff: |
Revision 2 (+385 -47) |
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 2) |
---|
Wouldn't it be more convenient to have a path to the key?
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 2) |
---|
Can we split this up into steps so we can give better error messages?
Fix issues
Testing Done: |
|
||||||
---|---|---|---|---|---|---|---|
Commit: |
|
||||||
Diff: |
Revision 3 (+634 -47) |
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3) |
---|
This should work the same way
WNS
works. There's aget
andset
method, and a convenience method that combines the two.
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3) |
---|
I prefer the log message split out, rather than a multiline function call
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3) |
---|
int
isn't nullable. Like this the default value is0
because it will be cast to int.
Testing Done: |
|
||||||
---|---|---|---|---|---|---|---|
Commit: |
|
||||||
Diff: |
Revision 4 (+1698 -47) |
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revisions 3 - 4) |
---|
should probably be
static
instead ofself
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revisions 3 - 4) |
---|
I understand why we would want
client_email
andprivate_key
properties, since that is static info that won't change (most likely). But the oauth lifetime seems flexible. I think we can leave that as parameter to the method.
src/Lunr/Vortex/FCM/Tests/FCMDispatcherConfigureOAuthTokenTest.php (Diff revision 4) |
---|
try to not use
withConsecutive
in new code
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4) |
---|
What format should I think of here? Timestamp?
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4) |
---|
What is a
client_email
? Is it a project identifier? A username?
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4) |
---|
Won't these two show exactly the same in the log?
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4) |
---|
I think this is safe to have a reasonable fallback.
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4) |
---|
Can you group these with the other small functions?
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4) |
---|
I don't think we need to log this. The exception is good enough here, it just shouldn't be an
UnexpectedValueException
. We already use that below and the case here would need to be handled differently.
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4) |
---|
That's not "good form". If you need info from the previous exception, you should attach the previous exception, not concatenate the messages.
new RuntimeException('message', 0, $e);
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+1574 -49) |
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 5) |
---|
Can you make the time a constant? I'd never know to look for it here.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+1586 -49) |
src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 6) |
---|
Optionally, this could be:
$http_response->status_code = $http_code ?? NULL;
Testing Done: |
|
||||||
---|---|---|---|---|---|---|---|
Commit: |
|
||||||
Diff: |
Revision 7 (+1583 -49) |