Skip to content

[Notifier] Add Seven.io bridge #52936

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
Dec 29, 2023
Merged

[Notifier] Add Seven.io bridge #52936

merged 1 commit into from
Dec 29, 2023

Conversation

NeoBlack
Copy link
Contributor

@NeoBlack NeoBlack commented Dec 7, 2023 •
edited by OskarStark
Loading

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Docs symfony/symfony-docs#19351
Recipe symfony/recipes#1277
License MIT

Seven.io is the new name of SMS77, and they also changed the URL to the gateway. To reflect that change, this patch introduces the new Seven Notifier Bridge.

The current SMS77 Bridge should be deprecated in another patch.

@NeoBlack NeoBlack requested a review from OskarStark as a code owner December 7, 2023 23:14
@carsonbot carsonbot added this to the 6.4 milestone Dec 7, 2023
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot carsonbot changed the title [DEPRECATION][Notifier] Add Seven Notifier Bridge and deprecate SMS77 [Notifier] Add Seven Notifier Bridge and deprecate SMS77 Dec 7, 2023
@OskarStark OskarStark modified the milestones: 6.4, 7.1 Dec 8, 2023
OskarStark
OskarStark reviewed Dec 8, 2023
View reviewed changes
OskarStark
OskarStark reviewed Dec 8, 2023
View reviewed changes
@OskarStark OskarStark changed the title [Notifier] Add Seven Notifier Bridge and deprecate SMS77 [Notifier] Add Seven.io Notifier Bridge and deprecate SMS77 Dec 8, 2023
@OskarStark
Copy link
Contributor

Hi 👋

Please target 7.1 branch, as this is a new feature.

  1. Your new bridge need to require symfony/notifier ^7.1
  2. Does the old bridge including the old URL still work or did the revoke the old endpoint from SMS77? If this is the case we would need a bugfix PR against the lowest maintained branch for the SMS77 bridge to keep it compatible.
  3. Lets rename it to SevenIo bridge and sevenio for scheme and package name
  4. Please remove everything related to the deprecation of the old bridge for now, thanks

@OskarStark OskarStark changed the title [Notifier] Add Seven.io Notifier Bridge and deprecate SMS77 [Notifier] Add Seven.io Notifier Bridge Dec 8, 2023
@NeoBlack
Copy link
Contributor Author

NeoBlack commented Dec 8, 2023

Thanks for your quick feedback, I will take care of your feedback tomorrow.

@NeoBlack NeoBlack force-pushed the bugfix/sms77-seven-io branch from a6173cb to 72f6865 Compare December 9, 2023 11:43
fabpot
fabpot requested changes Dec 9, 2023
View reviewed changes
@fabpot
Copy link
Member

fabpot commented Dec 9, 2023 •
edited
Loading

Small note: this is a new feature, so it's going to be merged in 7.1, not 6.4.

OskarStark
OskarStark reviewed Dec 9, 2023
View reviewed changes
@NeoBlack
Copy link
Contributor Author

NeoBlack commented Dec 9, 2023

@OskarStark @fabpot, I am having trouble running the tests locally on my machine. I am trying to understand why the tests are failing in CI. how to enable the new package for the test?

OskarStark
OskarStark reviewed Dec 9, 2023
View reviewed changes
fabpot
fabpot requested changes Dec 9, 2023
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

I think this is why the tests fail.

@NeoBlack NeoBlack changed the base branch from 6.4 to 7.1 December 9, 2023 12:19
@NeoBlack NeoBlack requested a review from chalasr as a code owner December 9, 2023 12:19
@NeoBlack NeoBlack force-pushed the bugfix/sms77-seven-io branch from cbffdeb to a443c92 Compare December 9, 2023 12:25
@fabpot
Copy link
Member

fabpot commented Dec 9, 2023

Like the other day, your rebase looks wrong.

@OskarStark OskarStark changed the title [Notifier] Add Seven.io Notifier Bridge [Notifier] Add Seven.io bridge Dec 28, 2023
@OskarStark OskarStark force-pushed the bugfix/sms77-seven-io branch 3 times, most recently from 483651f to 3d8cf8a Compare December 28, 2023 20:41
@OskarStark
Copy link
Contributor

OskarStark commented Dec 28, 2023 •
edited
Loading

@fabpot I squashed and rebased the branch, created a docs and re recipe PR ✅

@OskarStark OskarStark requested a review from fabpot December 28, 2023 20:42
@OskarStark OskarStark mentioned this pull request Dec 28, 2023
Merged
@OskarStark OskarStark force-pushed the bugfix/sms77-seven-io branch 4 times, most recently from 6b781fb to 67a5120 Compare December 28, 2023 21:41
@OskarStark OskarStark mentioned this pull request Dec 28, 2023
Merged
@NeoBlack @OskarStark
[Notifier] Add Seven Notifier Bridge
83b2eb5 
Seven.io is the new name of SMS77, they changed also the URL
to the gateway. To reflect that change, this patch introduces the
new Seven Notifier Bridge.

The current SMS77 Bridge should be deprecated in another patch
@OskarStark OskarStark force-pushed the bugfix/sms77-seven-io branch from 67a5120 to 83b2eb5 Compare December 28, 2023 21:43
@fabpot
Copy link
Member

fabpot commented Dec 29, 2023

Thank you @NeoBlack.

@fabpot fabpot merged commit 167c90b into symfony:7.1 Dec 29, 2023
@fabpot fabpot mentioned this pull request May 2, 2024
Merged
@MrYamous MrYamous mentioned this pull request Sep 8, 2024
Merged
fabpot added a commit that referenced this pull request Mar 29, 2025
@fabpot
feature #58200 [Notifier] Deprecate sms77 Notifier bridge (MrYamous)
764fe52 
This PR was squashed before being merged into the 7.3 branch.

Discussion
----------

[Notifier] Deprecate sms77 Notifier bridge

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Issues        | Following #52936
| License       | MIT

#52936 introduced a new Seven.io bridge for Notifier component to replace sms77 but this one has not been deprecated

Commits
-------

006ea39 [Notifier] Deprecate sms77 Notifier bridge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark approved these changes

@chalasr chalasr Awaiting requested review from chalasr

@fabpot fabpot Awaiting requested review from fabpot

Assignees
No one assigned
Labels
Feature Notifier Status: Needs Review
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
4 participants
@NeoBlack @carsonbot @OskarStark @fabpot