fix: SendMailJob returns incorrect errors#1146
Conversation
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 26371fc | Previous: a5e38a6 | Ratio |
|---|---|---|---|
Benchmark_DecryptString |
6430 ns/op 2032 B/op 16 allocs/op |
2148 ns/op 2032 B/op 16 allocs/op |
2.99 |
Benchmark_DecryptString - ns/op |
6430 ns/op |
2148 ns/op |
2.99 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes incorrect parameter ordering and error messages in the SendMailJob.Handle method. The changes ensure that argument validation error messages correctly identify the expected parameter types and that the SendMail function is called with the correct parameter mapping.
- Reorders argument parsing to match the expected
SendMailfunction signature - Updates error messages to reflect the correct parameter names and types
- Fixes the
SendMailfunction call to use the correctly parsed arguments
Comments suppressed due to low confidence (1)
mail/job.go:60
- The
replyToparameter is parsed fromargs[6]but is not being used in theSendMailfunction call on line 75. This suggests that either the parsing is incorrect or theSendMailcall is missing thereplyToparameter.
bcc, ok := args[6].([]string)
| } | ||
|
|
||
| return SendMail(r.config, from, subject, body, recipient, cc, bcc, replyTo, attachments, convertSliceHeadersToMap(headers)) | ||
| return SendMail(r.config, subject, body, fromAddress, fromName, to, cc, bcc, attachments, convertSliceHeadersToMap(headers)) |
There was a problem hiding this comment.
It may be better to pass a struct here.
There was a problem hiding this comment.
Yeah, I'll create a separate PR for these optimizations.
There was a problem hiding this comment.
Thanks, only for master should be fine.
There was a problem hiding this comment.
Yes, it’s a breaking change in some sense, at least since SendMail is an exposed method(not documented).
📑 Description
Closes https://github.com/goravel/goravel/issues/
✅ Checks