Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Notifier][Firebase] Add Firebase v1 API support#60205
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
[Notifier][Firebase] Add Firebase v1 API support#60205
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedApr 12, 2025
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
| return $sentMessage; | ||
| } | ||
| private function getJwt(): string |
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.
Generating JWT is quite a heavy operation and for example a messages worker dedicated to sending push notifications would perform this operation many times.
Can I add caching for the generated JWT? If so, should I addsymfony/cache dependency?
200ece1 toa83a711Comparea83a711 to1ea778bCompareSometests on Windows are failing. It seems like Rest of the failing tests seem to come from Doctrine bridge which should be unrelated to changes made in this PR. |
f213052 to4d52e8aCompare4d52e8a to0259dceCompareMethraen commentedMay 23, 2025
If im not wrong, only Symfony maintainers can modify the CI pipelines configuration, @nicolas-grekas would it be okay to enable the missing ext so that the PR can be completed? |
Adding the extension is possible by patching .github/workflow files as fit. |
New Firebase API uses JWT authorization and OpenSSL is needed for signing it.
…nion field from new APINew API changed the 'to' field into a union field with 3 different types. This adds backwards-compatible way to support targeting this union field in FirebaseOptions.
…the new v1 APIBackwards compatibility was maintained with the old DSN format. A deprecation is triggered when someone tries to create the transport with the old DSN format. Transport created using the old DSN will not be able to send messages and will throw exception for every sent message (this should be consistent with its previous behavior).
The private key used in tests was generated only for the purposes of these tests and is not connected to anything real.
… shapeShape of the endpoint input has changed with the new API. Separating options for messages based on the target platform no longer makes sense as all options for all platforms can now be set for the same message.AndroidNotification, IOSNotification and WebNotification were marked as deprecated and will be removed.FirebaseOptions is no longer abstract and should be used directly.
Reflects changes made by transferring to new v1 API.
Running tests there makes no sense since ext-openssl is needed.
0259dce to828c14cCompareI have rebased my branch and I have also modified All tests have passed now. @nicolas-grekas does this approach make sense? |
This PR adds support for the newv1 API.
The main changes were:
FirebaseOptionswere extended to hold params supported by new APIThis PR does not break backwards compatibility meaning that new Firebase bridge should behave the same way inside the Symfony app as the old one does right now.
Support for the old API was completely removed as the API has been shut down in 2024 already and attempting to use it throws exceptions.
Deprecations
$tokenparam inFirebaseTransport::__construct()was deprecated - the param no longer exists for the new APIAndroidNotification,IOSNotificationandWebNotificationclasses were deprecated - new Firebase API allows for all platform specific options to be specified for the message and having options separated by platform limits usabilityFirebaseTransportwith the old DSN format a deprecation notice is triggered to inform the user to upgrade (this has been done mainly to not break apps which have theFirebaseTransportconfigured with the old credentials format)