Skip to content

[Notifier] Remove deprecation in slack-notifier #41298

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 23, 2021
Merged

[Notifier] Remove deprecation in slack-notifier #41298

merged 1 commit into from
May 23, 2021

Conversation

jschaedl
Copy link
Contributor

Q A
Branch? 6.0
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

@jschaedl jschaedl requested a review from OskarStark as a code owner May 19, 2021 21:43
@carsonbot carsonbot added this to the 6.0 milestone May 19, 2021
@jschaedl jschaedl force-pushed the notifier-deprecations branch from 93d20c5 to 3566574 Compare May 19, 2021 21:54
derrabus
derrabus reviewed May 19, 2021
View reviewed changes
jderusse
jderusse reviewed May 19, 2021
View reviewed changes
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 wonder if we should also remove this code

symfony/src/Symfony/Component/Notifier/Bridge/Slack/SlackTransportFactory.php

Lines 34 to 36 in a2c8e84

if ('/' !== $dsn->getPath() && null !== $dsn->getPath()) {
throw 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).');
}

@jschaedl
Copy link
Contributor Author

I wonder if we should also remove this code

symfony/src/Symfony/Component/Notifier/Bridge/Slack/SlackTransportFactory.php

Lines 34 to 36 in a2c8e84

if ('/' !== $dsn->getPath() && null !== $dsn->getPath()) {
throw 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).');
}

I think that make sense. This is missing an entry in the changelog of 5.2 too. Shall I add that entry in a separate PR against 5.2?

@jschaedl jschaedl force-pushed the notifier-deprecations branch from 190b4b7 to 4bf9461 Compare May 20, 2021 05:56
@jschaedl
Copy link
Contributor Author

I wonder if we should also remove this code

symfony/src/Symfony/Component/Notifier/Bridge/Slack/SlackTransportFactory.php

Lines 34 to 36 in a2c8e84

if ('/' !== $dsn->getPath() && null !== $dsn->getPath()) {
throw 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).');
}

I think that make sense. This is missing an entry in the changelog of 5.2 too. Shall I add that entry in a separate PR against 5.2?

I checked it again. It's probably this entry: https://github.com/symfony/symfony/blame/4bf9461d9b0d2ee6840f4e3873542f8b54a5f8f4/src/Symfony/Component/Notifier/Bridge/Slack/CHANGELOG.md#L21

@jschaedl jschaedl mentioned this pull request May 20, 2021
Merged
@jschaedl jschaedl force-pushed the notifier-deprecations branch from 9bb67b0 to bb4e119 Compare May 20, 2021 19:22
nicolas-grekas added a commit that referenced this pull request May 21, 2021
@nicolas-grekas
minor #41309 [Notifier] Add missing deprecation entry (jschaedl)
1769685 
This PR was merged into the 5.2 branch.

Discussion
----------

[Notifier] Add missing deprecation entry

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       |  <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | <!-- required for new features -->

follow-up on #41298

Commits
-------

257d9ba Add missing deprecation entry
@jschaedl jschaedl mentioned this pull request May 23, 2021
Merged
nicolas-grekas
nicolas-grekas reviewed May 23, 2021
View reviewed changes
src/Symfony/Component/Notifier/Bridge/Slack/CHANGELOG.md Outdated
6.0
---

* Remove `SlackOptions::channel()`, use `SlackOptions::recipient()` instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no final dot please

@nicolas-grekas nicolas-grekas force-pushed the notifier-deprecations branch from 3abdf63 to 1c7d5b9 Compare May 23, 2021 09:45
@nicolas-grekas
Copy link
Member

Thank you @jschaedl.

@nicolas-grekas nicolas-grekas merged commit 5344dc2 into symfony:6.0 May 23, 2021
@jschaedl jschaedl mentioned this pull request May 23, 2021
Merged
derrabus added a commit that referenced this pull request May 23, 2021
@derrabus
minor #41387 [Mime] Remove symfony/deprecation-contracts (jschaedl)
fa9fc7f 
This PR was merged into the 6.0 branch.

Discussion
----------

[Mime] Remove symfony/deprecation-contracts

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | - <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | - <!-- required for new features -->

As commented here #41298 (comment) I removed `symfony/deprecation-contracts` as follow-up on #41364

If the removal of `symfony/deprecation-contracts` is not important, I am happy to close this PR.

Commits
-------

4a52a9b Remove symfony/deprecation-contracts
@fabpot fabpot mentioned this pull request Nov 5, 2021
Merged
@xabbuh xabbuh mentioned this pull request Feb 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@jderusse jderusse jderusse left review comments

@derrabus derrabus derrabus requested changes

@OskarStark OskarStark OskarStark approved these changes

Assignees
No one assigned
Labels
Notifier Status: Reviewed
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.
6 participants
@jschaedl @nicolas-grekas @jderusse @OskarStark @derrabus @carsonbot