kreait / firebase-php

Unofficial Firebase Admin SDK for PHP
https://firebase-php.readthedocs.io/
MIT License
2.28k stars 433 forks source link

Fix Messaging for outdated environments #908

Closed jeromegamez closed 5 months ago

jeromegamez commented 5 months ago

Fixes #888

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.10%. Comparing base (c2bfad3) to head (3dcd1c4).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/kreait/firebase-php/pull/908/graphs/tree.svg?width=650&height=150&src=pr&token=Azl8QCvbTQ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kreait)](https://app.codecov.io/gh/kreait/firebase-php/pull/908?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kreait) ```diff @@ Coverage Diff @@ ## 7.x #908 +/- ## ============================================= + Coverage 74.42% 91.10% +16.68% - Complexity 1585 1588 +3 ============================================= Files 156 157 +1 Lines 4422 4429 +7 ============================================= + Hits 3291 4035 +744 + Misses 1131 394 -737 ```
jeromegamez commented 5 months ago

@Daniel-Cong @david-windsock I started with doing the 'CURL_HTTP_VERSION_2* constant check - even though we already know that this doesn't seem to be the problem, according to several comments in #888. Please test the PR anyway, so we can work towards finding the real culprit.

@rickclephas @tonysilva16 @woodongwong Since you participated in #874, I'd greatly appreciate if you could follow this PR and test it, to make sure nothing breaks for you (break = the performance gain is gone). Thank you!

david-windsock commented 5 months ago

Thanks @jeromegamez, will try to test it on monday.

jeromegamez commented 5 months ago

Upon further research, I stumbled upon https://github.com/microsoftgraph/msgraph-sdk-php/issues/854 and https://github.com/microsoftgraph/msgraph-sdk-php/pull/1120

I will implement this, let's hope that this is the way to check for HTTP/2 support 🤞🏻

rickclephas commented 5 months ago

But based on the changes I am not expecting this to break our implementation/performance 👍🏻.

To confirm this I have tested the environmentSupportsHTTP2 function in our project, and it correctly returns true in our Google App Engine environment.

david-windsock commented 5 months ago

Upon further research, I stumbled upon microsoftgraph/msgraph-sdk-php#854 and microsoftgraph/msgraph-sdk-php#1120

I will implement this, let's hope that this is the way to check for HTTP/2 support 🤞🏻

We tested it on our CentOS server and worked without problem. We faked the composer dependency to use this branch as the 7.9.1 version that is working for us and the notifications are sent OK, without any change on our code.

Thanks for your quick response!

jeromegamez commented 5 months ago

That's great to hear, thank you for the feedback! 🙏🏻

I will merge this later and create a new release.

jeromegamez commented 5 months ago

It's still curious - some have tried just commenting out the $request = $request->withProtocolVersion('2.0'); line and it didn't seem to have worked for them. Let's hope for the best 😅

david-windsock commented 5 months ago

Thanks @jeromegamez!