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
WNSworks. There's agetandsetmethod, 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) |
|---|
intisn't nullable. Like this the default value is0because 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
staticinstead ofself
| src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revisions 3 - 4) |
|---|
I understand why we would want
client_emailandprivate_keyproperties, 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
withConsecutivein 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) |