Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Notifier] Add exception for deprecated slack dsn#39310
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Notifier] Add exception for deprecated slack dsn#39310
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/Symfony/Component/Notifier/Bridge/Slack/SlackTransportFactory.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
derrabus commentedDec 4, 2020
Thank you for working on this issue! |
| } | ||
| if ('/' !==$dsn->getPath()) { | ||
| thrownewInvalidArgumentException(sprintf('Invalid "%s" notifier DSN: Using a dsn for slack webhooks has been removed (maybe you haven\'t update the DSN when upgrading from 5.1).',$dsn->getOriginalDsn())); |
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.
Maybe better to usethrow new IncompleteDsnException('Support for slack webhook dsn has been dropped since 5.2 (maybe you haven\'t updated the DSN when upgrading from 5.1).');?
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 agree.
Please, change "webhook dsn" to "webhook DSN".
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.
But Incomplete... is missleading, it's a wrong DSN... 🧐
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.
@OskarStark That was also my first thought but i wasn't sure if it is okay if i a add a new exception.
jderusse left a comment
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.
LGTM. Just the small wording suggested by@evertharmeling
fabpot left a comment
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.
Could you also add the format DSN information in the README? We are doing this for almost all other providers and we forgot to do it here.
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| if ('/' !==$dsn->getPath()) { | ||
| thrownewInvalidArgumentException(sprintf('Invalid "%s" notifier DSN: Using a dsn for slack webhooks has been removed (maybe you haven\'t update the DSN when upgrading from 5.1).',$dsn->getOriginalDsn())); |
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 agree.
Please, change "webhook dsn" to "webhook DSN".
fabpot commentedDec 8, 2020
Thank you@malteschlueter. |
Uh oh!
There was an error while loading.Please reload this page.
The DSN for the Slack integration changed again from 5.1 to 5.2. There was the idea to output an exception for that.