Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Messaging for outdated environments #908

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

jeromegamez
Copy link
Member

Fixes #888

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

Impacted file tree graph

@@              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
Copy link
Member Author

@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
Copy link

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

@jeromegamez
Copy link
Member Author

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 🤞🏻

@jeromegamez jeromegamez force-pushed the fix-messaging-for-outdated-envs branch 3 times, most recently from 55aac0a to 67da676 Compare June 28, 2024 13:17
@jeromegamez jeromegamez force-pushed the fix-messaging-for-outdated-envs branch from 67da676 to 3dcd1c4 Compare June 28, 2024 14:01
Copy link

@rickclephas rickclephas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a dependency conflict for lcobucci/jwt that prevents me from doing a quick test. But based on the changes I am not expecting this to break our implementation/performance 👍🏻.

@rickclephas
Copy link

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
Copy link

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
Copy link
Member Author

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

I will merge this later and create a new release.

@jeromegamez jeromegamez marked this pull request as ready for review July 2, 2024 20:45
@jeromegamez jeromegamez merged commit e73e6b3 into 7.x Jul 2, 2024
2 checks passed
@jeromegamez jeromegamez deleted the fix-messaging-for-outdated-envs branch July 2, 2024 20:46
@jeromegamez
Copy link
Member Author

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
Copy link

Thanks @jeromegamez!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to send messaging after updating to 7.10.0
3 participants