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 Component #33687

Merged
merged 1 commit into from Oct 5, 2019

Conversation

@fabpot
Copy link
Member

commented Sep 24, 2019

Q A
Branch? 5.0
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR not yet

Initial PR for the Notifier component. Tests missing for now.

@fabpot fabpot force-pushed the fabpot:notifier-feature-v1 branch from f4662b8 to a55762b Sep 24, 2019
@fabpot fabpot changed the title Notifier feature v1 Notifier Component Sep 24, 2019
Copy link
Member

left a comment

Just minor CS so far ;)

@yceruto yceruto added this to the next milestone Sep 24, 2019
@yceruto yceruto added the Notifier label Sep 24, 2019
@fabpot fabpot force-pushed the fabpot:notifier-feature-v1 branch 6 times, most recently from e25fd0f to 47c4b0a Sep 24, 2019
@fabpot fabpot requested review from dunglas, lyrixx, sroze and xabbuh as code owners Sep 25, 2019
@fabpot fabpot changed the base branch from 4.4 to master Sep 25, 2019
@fabpot fabpot removed this from the next milestone Sep 25, 2019
Copy link
Member

left a comment

First-round, looks simple :)

*
* @experimental in 5.0
*/
class FailoverTransport extends RoundRobinTransport

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 27, 2019

Member

is this legit? FailoverTransport instanceof RoundRobinTransport?
should the common logic be moved to a trait instead, or an abstract class?

@fabpot fabpot force-pushed the fabpot:notifier-feature-v1 branch from 3920681 to 084e9bc Oct 1, 2019
@fabpot fabpot force-pushed the fabpot:notifier-feature-v1 branch 7 times, most recently from 7dcb45b to 46939d9 Oct 3, 2019
Copy link

left a comment

25 cents from me 🙂 Looks promising. Probably we will use it when it become available.

Additional suggestions and/or thoughts:

  • There are places where phpDoc would be very helpful. For example Slack block concept is not intuitive, reading code I don't know what it is
  • There is some inconsistency because sometimes setters are prefixed (AbstractTransport::setHost()) and sometimes not (SlackOptions::channel() and other methods in this class)
  • RocketChat could be supported for text notifications
  • WhatsApp could be supported (through Twilio or natively?)
  • Signal could be supported for text notifications
@fabpot fabpot force-pushed the fabpot:notifier-feature-v1 branch 2 times, most recently from 5ccdc2c to af4fba6 Oct 4, 2019
@Wirone

This comment has been minimized.

Copy link

commented Oct 4, 2019

@fabpot @nicolas-grekas Side question: why are you amending changes all the time? In this case it's impossible to see changes introduced on each step of the review process, so when new version of a branch is pushed I don't know what should be reviewed again (Github marks whole files as "changed since last view", with large files it's difficult to read whole file every time). I recommend adding new commits and squash them just before merging, if you want to keep compact history.

Edit: ahh, Github generates compare view for those pushes, but they're linked under "force pushed" and it's not intuitive and transparent since text is not decorated in any way. In Gitlab there is "Compare with previous version" link, much easier to notice.

@fabpot

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2019

For some reasons, some of my comments were not published :(

@fabpot

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2019

@Wirone About providers that could be implemented: this initial PR provides just a few of the possible/popular providers to get us started. I hope that many other ones will be submitted over time.

Regarding setXxx() methos vs xxx() ones: I only used the xxx() styles for data objects.

@fabpot fabpot force-pushed the fabpot:notifier-feature-v1 branch 2 times, most recently from 2554b5c to 0b219b9 Oct 5, 2019
@fabpot fabpot force-pushed the fabpot:notifier-feature-v1 branch from 0b219b9 to 7f97a3f Oct 5, 2019
Copy link
Member

left a comment

Let's merge and accept followups to tweak the rest, add tests, etc.
Help wanted!

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 5, 2019

Thanks @fabpot for working on this feature, this is much appreciated.

nicolas-grekas added a commit that referenced this pull request Oct 5, 2019
This PR was merged into the 5.0-dev branch.

Discussion
----------

Notifier Component

| Q             | A
| ------------- | ---
| Branch?       | 5.0
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       |
| License       | MIT
| Doc PR        | not yet

Initial PR for the Notifier component. Tests missing for now.

Commits
-------

7f97a3f [Notifier] added the component
@nicolas-grekas nicolas-grekas merged commit 7f97a3f into symfony:master Oct 5, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Your code looks good.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.