[Mailer][Resend] Fix compatibility with resend template #63196
Conversation
2295aec to
45b42ba
Compare
src/Symfony/Component/Mailer/Bridge/Resend/Transport/ResendApiTransport.php
Outdated
Show resolved
Hide resolved
45b42ba to
8b21821
Compare
|
@nicolas-grekas do I need to create an issue to not forget to remove unused / deprecated params for 8.4 and 9.0 ? |
src/Symfony/Component/Mailer/Bridge/Resend/Transport/ResendApiTransport.php
Outdated
Show resolved
Hide resolved
8b21821 to
c47347d
Compare
| } | ||
|
|
||
| if ('params' === $name) { | ||
| trigger_deprecation('symfony/resend-mailer', '7.3', 'Usage of params is deprecated use variables instead.'); |
There was a problem hiding this comment.
| trigger_deprecation('symfony/resend-mailer', '7.3', 'Usage of params is deprecated use variables instead.'); | |
| trigger_deprecation('symfony/resend-mailer', '7.3', 'The "params" header name is deprecated, use the "variables" header name instead'); |
There was a problem hiding this comment.
No deprecation can be added here, these must be postponed to 8.1
There was a problem hiding this comment.
You're right, my bad.
The bug fix is only about transforming ['templateId'] into ['template']['id'] and params into ['templates']['variables'] in this PR, targeting 7.3. Users can still continue to call headers templateId and params
Then another PR, targeting 8.1, to add the deprecation trigger when usage of params header name is detected, to inform users to use variables header name instead.
Am I right?
There was a problem hiding this comment.
I think the bug can allow using variables headers too to match with the documentation
There was a problem hiding this comment.
Accepting the new variables header name is considered as a new feature from Symfony user point of view. The bug fix is limited to match what Resend API expect, not to introduce something new to Symfony users. In other words, fixing the bug must be transparent to Symfony users in 7.3. Then for 8.1, you can introduce the new variables header name and deprecate the params one to align naming between Resend API documentation and Symfony Resend Bride source code.
There was a problem hiding this comment.
Ok I understand so I should make an other PR with this with target branch symfony 8.1 right ?
There was a problem hiding this comment.
Exactly.
This one targeting 7.3, only to transform header names to match what Resend API expect.
Another one targeting 8.1, to introduce params header name deprecation and add support to variables header name instead.
|
-1 for this. This PR is adding new features to give a special meaning to some header names in the ResendTransport, which are not part of the symfony/mailer abstraction. We already rejected similar attempts in the past for other bridges when trying to turn them into SDKs supporting all features of the API:
|
|
Thus, this PR is not actually fixing a bug. It implements a new feature by adding a special meaning for some header names that were handled as normal headers until now. |
|
I'm sorry but for me it's a bug fix, the tests assume that there is a That's why for me it's a bugfix because providing them do nothing |
|
The tests don't assume that those work to configure the There has never been a version of Symfony shipping a working way to configure the |
|
@stof Ok I understand, so maybe it could be interesting to rename the name of thoses parameters to avoid misunderstanding from the I can make a PR to change that |
|
I suggest changing the test to use names than don't cause confusion about whether this feature is supported or no indeed. This is a test about covering custom headers, as said by the test name. |
Implementation of ReSend template is not compatible with ReSend API : https://resend.com/docs/dashboard/templates/introduction
I used the same way that it's handle for Brevo