[Vortex] FCM: Update to new firebase API v1

Review Request #1088 — Created Jan. 11, 2024 and submitted

b.stoop
Lunr.Vortex
fcm/updateapi
1085
1089
7ab046e...
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.

  • 0
  • 0
  • 27
  • 1
  • 28
Description From Last Updated
There are no open issues
b.stoop
smillernl
  1. 
      
  2. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 2)
     
     
    The issue has been resolved. Show all issues

    Why not null?

  3. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 2)
     
     
    The issue has been resolved. Show all issues

    Wouldn't it be more convenient to have a path to the key?

    1. Firebase provides a file with a json object where the iss and private_key comes from. We could add that file somewhere and provide the path, or we could add the fields to the config files we have and provide the values like this. I choose the latter one as we do that for all the other notification providers too

  4. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 2)
     
     
    The issue has been resolved. Show all issues

    Can we split this up into steps so we can give better error messages?

  5. 
      
b.stoop
pprkut
  1. 
      
  2. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    This should work the same way WNS works. There's a get and set method, and a convenience method that combines the two.

  3. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3)
     
     
    The issue has been resolved. Show all issues

    what does iat mean?

    1. Issued at (time)

  4. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3)
     
     
    The issue has been resolved. Show all issues

    Seems presumptuous to hardcode that

  5. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3)
     
     
     
     
     
    The issue has been resolved. Show all issues

    I prefer the log message split out, rather than a multiline function call

  6. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3)
     
     
     
     
    The issue has been resolved. Show all issues

    ditto

  7. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3)
     
     
    The issue has been resolved. Show all issues

    int isn't nullable. Like this the default value is 0 because it will be cast to int.

    1. I don't think this is correct. With the = NULL php will see that the type of $http_code is int|null, so a NULL will not be cast to int unlike a string for example that will be cast to int if strict type is disabled

  8. 
      
b.stoop
pprkut
  1. 
      
  2. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revisions 3 - 4)
     
     
    The issue has been resolved. Show all issues

    @return $this

  3. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revisions 3 - 4)
     
     
    The issue has been resolved. Show all issues

    should probably be static instead of self

  4. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revisions 3 - 4)
     
     
    The issue has been resolved. Show all issues

    @return $this

  5. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revisions 3 - 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    I understand why we would want client_email and private_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.

  6. src/Lunr/Vortex/FCM/Tests/FCMDispatcherConfigureOAuthTokenTest.php (Diff revision 4)
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    try to not use withConsecutive in new code

    1. Should I use Mockery for this now, or rewrite the test we so don't use it at all?

    2. Either option is fine :)

  7. 
      
smillernl
  1. 
      
  2. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4)
     
     
    The issue has been resolved. Show all issues

    What format should I think of here? Timestamp?

    1. No, one of https://www.php.net/manual/en/datetime.formats.php#datetime.formats.relative, like '+30 minutes'. How do you suggest I can make that clear?

    2. relative time as a string for strtotime() to parse into an expiry timestamp

  3. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4)
     
     
    The issue has been dropped. Show all issues

    What is a client_email? Is it a project identifier? A username?

    1. Its an email from the client, that is provided by fcm in the file service-account-file.json in the field client_email

  4. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4)
     
     
     
    The issue has been resolved. Show all issues

    Won't these two show exactly the same in the log?

  5. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4)
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    I think this is safe to have a reasonable fallback.

  6. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4)
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    Why these?

    1. I thought it was something default we used for the fcm requests, but I'm not sure if we need it

  7. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    Can you group these with the other small functions?

  8. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4)
     
     
    The issue has been resolved. Show all issues

    Nope

  9. 
      
pprkut
  1. 
      
  2. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    The issue has been resolved. Show all issues

    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.

  3. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4)
     
     
    The issue has been resolved. Show all issues

    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);

  4. 
      
b.stoop
b.stoop
smillernl
  1. 
      
  2. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 5)
     
     
    The issue has been resolved. Show all issues

    Why not static like the others?

  3. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 5)
     
     
    The issue has been resolved. Show all issues

    @see might be better for this

  4. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 5)
     
     
    The issue has been resolved. Show all issues

    Can you make the time a constant? I'd never know to look for it here.

  5. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 5)
     
     
    The issue has been resolved. Show all issues

    Please use the constant here

  6. 
      
b.stoop
b.stoop
smillernl
  1. 
      
  2. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 6)
     
     
     
     
     
    The issue has been resolved. Show all issues

    Optionally, this could be:

    $http_response->status_code = $http_code ?? NULL;
    
    1. The ?? NULL would not be necessary as $http_code is always set as NULL or an int. The status_code property in the Response class is default false, but this would mean that that becomes NULL and I don't know if that has some side effects

    2. In that case ?? false would be the way to go.

  3. 
      
smillernl
  1. Ship It!
  2. 
      
b.stoop
b.stoop
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (634c179)
Loading...