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

[Mailer] [Brevo] Check that tags is present in payload before calling setTags #54199

Merged
merged 1 commit into from Mar 15, 2024

Conversation

palgalik
Copy link
Contributor

@palgalik palgalik commented Mar 7, 2024

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #54197
License MIT

tags was removed as a mandatory parameter from Brevo remote event requests in this PR by me. Sadly, I wasn't wary enough, and I didn't added payload check before calling setTags in the PayloadConverter, and it causes TypeErrors.

  • With this fix, setTags won't be called anymore if tags are not present in the request
  • AbstractMailerEvent defaults tags as empty array, so there is no need to call the setter with empty array parameter

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "7.1" but it seems your PR description refers to branch "6.4".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@palgalik palgalik changed the base branch from 7.1 to 6.4 March 7, 2024 20:29
@GromNaN
Copy link
Member

GromNaN commented Mar 15, 2024

Could you update the PR title to describe the bug fix?

@palgalik palgalik changed the title Fix 54197 [Mailer] [Brevo] Check that tags is present in payload before calling setTags Mar 15, 2024
@palgalik
Copy link
Contributor Author

Could you update the PR title to describe the bug fix?

Sure, done. 👍

@stof
Copy link
Member

stof commented Mar 15, 2024

Please add a test covering the conversion of a payload that has no tags, to prevent regressions.

@palgalik
Copy link
Contributor Author

Please add a test covering the conversion of a payload that has no tags, to prevent regressions.

Just added a test case where there are no tag in the payload, but I only did it for one event type "request".

What do you think @stof , should I change the existing test cases to add the no tags variation for them also?
I think it's not needed, because the tags checking condition is common for each event type.

@fabpot fabpot modified the milestones: 7.1, 6.4 Mar 15, 2024
@fabpot
Copy link
Member

fabpot commented Mar 15, 2024

Thank you @palgalik.

@fabpot fabpot merged commit 4bd3b29 into symfony:6.4 Mar 15, 2024
7 of 10 checks passed
This was referenced Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BrevoPayloadConverter causes type error if tags are not present
7 participants