-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Notifier] Introduce LengthException #39493
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
Conversation
7426084
to
6aa6458
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the benefit over an \InvalidArgumentException
. :/
IMHO not all checks require a dedicated exception class.
I tend to agree with @jderusse. Can you elaborate why we need a dedicated exception here? |
It was proposed by @chalasr and confirmed by @nicolas-grekas and I felt it makes sense... so... 🤔 |
Specific exceptions allow for more fine-grained error handling. But both are fine here, I don't really care tbh :) |
Specific exceptions for logic exceptions don't really make sense to me, appart from normalizing error messages across the codebase. |
@chalasr but invalid DSNs are not things that you should handle by catching exceptions IMO. You should instead fix your config to provide a valid DSN. |
@stof its not about an invalid DSN in this case, its about a dynamic subject length of a message |
81e5ecc
to
1384222
Compare
@ogizanagi please keep in mind, that new bridges will come from different people and I want to make it more and more easier to implement new ones. |
1384222
to
437cad7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me.
Thank you @OskarStark. |
Follows https://github.com/symfony/symfony/pull/39444/files#r542298086