Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Notifier] Fix thread key in GoogleChat bridge#57384

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

Merged
fabpot merged 1 commit intosymfony:5.4fromromain-jacquart:fix_57323
Jun 19, 2024

Conversation

romain-jacquart
Copy link
Contributor

@romain-jacquartromain-jacquart commentedJun 12, 2024•
edited
Loading

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
IssuesFix#57323
LicenseMIT

Hello,

Google Chat APIhas deprecated the use ofthreadKey query parameter in favor ofthread.threadKey in request's body.

In order for the thread key value to not be ignored, anew query parametermessageReplyOption has been added which controls the way the key is handled.
I chosenot to expose the values used by this new parameter as I wanted to focus this PR on the actual fix.
To do so, I've forced the value ofmessageReplyOption toREPLY_MESSAGE_FALLBACK_TO_NEW_THREAD which replicates the original behavior of this bridge.

@OskarStark
Copy link
Contributor

Please target 7.2 branch for your PR as this is a new feature. thanks

@romain-jacquart
Copy link
ContributorAuthor

Please target 7.2 branch for your PR as this is a new feature. thanks

Hello, thanks for your reply.

This actually is not a new feature, since the thread key in query parameters has no effect now on the new Google Chat API. Shouldn't this qualifies as a bug and the fix be backported to 5.4?

@OskarStark
Copy link
Contributor

OskarStark commentedJun 13, 2024•
edited
Loading

It introduces new code to handle the new threadKey . So this should go into 7.2 with the deprecation and the deprecation layer be removed in 8.0.

Someone who needs this should upgrade to 7.2 then

@fabpot
Copy link
Member

I agree that this should go in 7.2.

Copy link
Member

@GromNaNGromNaN left a comment

Choose a reason for hiding this comment

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

Thanks for catching this deprecation in the Google API.

This PR keeps the backward compatibility regarding the threadKey declaration (in DSN) and the GoogleChatOptions::setThreadKey() method.

I think this PR does too many things. We could just modify the HTTP request, and leave the options and methods untouched.

xabbuh reacted with thumbs up emoji
@romain-jacquart
Copy link
ContributorAuthor

This PR keeps the backward compatibility regarding the threadKey declaration (in DSN) and the GoogleChatOptions::setThreadKey() method.

I think this PR does too many things. We could just modify the HTTP request, and leave the options and methods untouched.

Thanks for your time reviewing this change.

I thought this was a good opportunity to update the options, but I guess this made too much noise around the actual fix.

GromNaN reacted with thumbs up emoji

@GromNaN
Copy link
Member

Thanks for simplifying this PR. I think we can now consider it as a bugfix to guarantee compatibility with the Google API in the future and merge in 5.4 @symfony/mergers?

yceruto reacted with thumbs up emoji

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

Works for me as a bug fix for 5.4.
@romain-jacquart Can you remove the CHANGELOG change and rebase on 5.4?

Copy link
Member

@GromNaNGromNaN left a comment

Choose a reason for hiding this comment

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

LGTM once rebased on 5.4

Google Chat API has deprecated the use of `threadKey` query parameterin favor of `thread.threadKey` in request's body.
@romain-jacquart
Copy link
ContributorAuthor

Works for me as a bug fix for 5.4.@romain-jacquart Can you remove the CHANGELOG change and rebase on 5.4?

The branch has been rebased on 5.4 and CHANGELOG changes have been removed 👍.

The rebase has introduced a slight change regarding a variable's name ($options vs$opts). Will it be a problem?

Thanks!

GromNaN reacted with thumbs up emoji

@fabpotfabpot modified the milestones:7.2,5.4Jun 19, 2024
@fabpot
Copy link
Member

Thank you@romain-jacquart.

romain-jacquart reacted with hooray emoji

@fabpotfabpot merged commit53a3024 intosymfony:5.4Jun 19, 2024
0 of 2 checks passed
This was referencedJun 28, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabpotfabpotfabpot approved these changes

@GromNaNGromNaNGromNaN approved these changes

@OskarStarkOskarStarkAwaiting requested review from OskarStarkOskarStark is a code owner

@lyrixxlyrixxAwaiting requested review from lyrixx

@ycerutoycerutoAwaiting requested review from yceruto

@chalasrchalasrAwaiting requested review from chalasr

@dunglasdunglasAwaiting requested review from dunglas

@xabbuhxabbuhAwaiting requested review from xabbuh

Assignees
No one assigned
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

Google Chat notifications not work properly with threads
5 participants
@romain-jacquart@OskarStark@fabpot@GromNaN@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp