[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.

Github Actions run: https://github.com/brianstoop/lunr.vortex/actions/runs/7831880681

  • 0
  • 0
  • 27
  • 1
  • 28
Description From Last Updated
b.stoop
smillernl
  1. 
      
  2. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 2)
     
     

    Why not null?

  3. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 2)
     
     

    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)
     
     

    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)
     
     
     
     
     
     
     
     
     
     

    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)
     
     

    what does iat mean?

  4. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3)
     
     

    Seems presumptuous to hardcode that

  5. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3)
     
     
     
     
     

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

  6. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3)
     
     
     
     
  7. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 3)
     
     

    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)
     
     

    @return $this

  3. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revisions 3 - 4)
     
     

    should probably be static instead of self

  4. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revisions 3 - 4)
     
     

    @return $this

  5. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revisions 3 - 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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)
     
     
     
     
     
     
     
     

    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)
     
     

    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)
     
     

    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)
     
     
     

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

  5. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4)
     
     
     
     
     
     

    I think this is safe to have a reasonable fallback.

  6. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 4)
     
     
     
     
     
     

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Can you group these with the other small functions?

  8. 
      
pprkut
  1. 
      
  2. 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.

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

  4. 
      
b.stoop
b.stoop
smillernl
  1. 
      
  2. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 5)
     
     

    Why not static like the others?

  3. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 5)
     
     

    @see might be better for this

  4. 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.

  5. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 5)
     
     

    Please use the constant here

  6. 
      
b.stoop
b.stoop
smillernl
  1. 
      
  2. src/Lunr/Vortex/FCM/FCMDispatcher.php (Diff revision 6)
     
     
     
     
     

    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...