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

[Notifier] Introduce LengthException #39493

Merged
merged 1 commit into from Dec 17, 2020

Conversation

OskarStark
Copy link
Contributor

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets ---
License MIT
Doc PR ---

Follows https://github.com/symfony/symfony/pull/39444/files#r542298086

Copy link
Member

@jderusse jderusse left a 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.

@derrabus
Copy link
Member

I tend to agree with @jderusse. Can you elaborate why we need a dedicated exception here?

@nicolas-grekas nicolas-grekas added this to the 5.x milestone Dec 14, 2020
@OskarStark
Copy link
Contributor Author

It was proposed by @chalasr and confirmed by @nicolas-grekas and I felt it makes sense... so... 🤔

@chalasr
Copy link
Member

chalasr commented Dec 15, 2020

Specific exceptions allow for more fine-grained error handling. But both are fine here, I don't really care tbh :)

@ogizanagi
Copy link
Member

ogizanagi commented Dec 15, 2020

Specific exceptions for logic exceptions don't really make sense to me, appart from normalizing error messages across the codebase.
Actually, I think most of the LogicException in components aren't useful when it doesn't perform any common logic to format the message, nor brings more semantics in the class name, if no native PHP exception matches . I guess we could just use native \LengthException here.

@stof
Copy link
Member

stof commented Dec 15, 2020

@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.

@OskarStark
Copy link
Contributor Author

@stof its not about an invalid DSN in this case, its about a dynamic subject length of a message

@OskarStark
Copy link
Contributor Author

Specific exceptions for logic exceptions don't really make sense to me, appart from normalizing error messages across the codebase.

@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.

Copy link
Member

@nicolas-grekas nicolas-grekas 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.

@ogizanagi
Copy link
Member

Thank you @OskarStark.

@ogizanagi ogizanagi merged commit 928594e into symfony:5.x Dec 17, 2020
@OskarStark OskarStark deleted the length-exception branch December 17, 2020 16:07
@fabpot fabpot mentioned this pull request Apr 18, 2021
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.

None yet

8 participants