Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Notifier] Add mercure bridge #39342

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
Jan 20, 2021
Merged

[Notifier] Add mercure bridge #39342

merged 1 commit into from
Jan 20, 2021

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Dec 5, 2020 •
edited
Loading

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #36481
License MIT
Doc PR symfony/symfony-docs#14840

Add a Notifier bridge for Mercure.
In this PR, Mercure is considered as a chatter (I'm still wondering if it's the most appropriate type).

The first approach for the DSN is mercure://jwtToken@host:port/hubPath?topic=/foo/1&secure=false with:

  • topic optional (defaults to null)
  • secure optional (defaults to true)

I'm not sure about the current way to deal with http/https. Maybe we can just replace the mercure scheme by http|https?

The notification representation is following Activity Streams

#SymfonyHackday

@mtarld mtarld force-pushed the feature/mercure-notifier branch from 57546cd to 7b886bf Compare December 5, 2020 21:29
jderusse
jderusse reviewed Dec 5, 2020
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.

Could you please add files

  • phpunit.xml.dist
  • README.md

src/Symfony/Component/Notifier/Bridge/Mercure/.gitattributes Show resolved Hide resolved
@mtarld mtarld force-pushed the feature/mercure-notifier branch 4 times, most recently from f366f12 to b1cb35d Compare December 5, 2020 21:53
@jderusse jderusse added this to the 5.x milestone Dec 5, 2020
@mtarld mtarld force-pushed the feature/mercure-notifier branch from b1cb35d to 7c4465b Compare December 5, 2020 22:04
@OskarStark
Copy link
Contributor

friendly ping @dunglas 🙂

dunglas
dunglas requested changes Dec 7, 2020
View reviewed changes
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Thank you very much for working on this! I think we can make this transport a bit more generic, but it's almost good!

src/Symfony/Component/Notifier/Bridge/Mercure/MercureTransport.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/Mercure/MercureTransport.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/Mercure/MercureTransport.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/Mercure/MercureTransport.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/Mercure/MercureTransport.php Outdated Show resolved Hide resolved
src/Symfony/Component/Notifier/Bridge/Mercure/MercureTransportFactory.php Show resolved Hide resolved
@mtarld mtarld mentioned this pull request Dec 8, 2020
Merged
@mtarld mtarld force-pushed the feature/mercure-notifier branch 3 times, most recently from faa0539 to f2af0e1 Compare December 8, 2020 12:39
@mtarld
Copy link
Contributor Author

mtarld commented Dec 8, 2020

In order to make it work, symfony/mercure-bundle#36 has to be merged.
Indeed, this PR adds a mercure.publisher tag on each publisher service so that they can be discovered.

@mtarld mtarld requested a review from dunglas December 8, 2020 12:47
@mtarld mtarld force-pushed the feature/mercure-notifier branch from f2af0e1 to 69d6bb3 Compare December 8, 2020 14:27
dunglas
dunglas reviewed Jan 3, 2021
View reviewed changes
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

for consistency

src/Symfony/Component/Notifier/Bridge/Mercure/MercureTransport.php Outdated Show resolved Hide resolved
@mtarld mtarld force-pushed the feature/mercure-notifier branch 2 times, most recently from 0ecb759 to f9ed6b8 Compare January 8, 2021 19:11
@mtarld mtarld force-pushed the feature/mercure-notifier branch 4 times, most recently from 9695088 to 564a77f Compare January 15, 2021 15:26
@mtarld mtarld force-pushed the feature/mercure-notifier branch from 564a77f to 19c6544 Compare January 16, 2021 09:47
@mtarld mtarld mentioned this pull request Jan 16, 2021
Merged
@chalasr chalasr requested a review from OskarStark January 19, 2021 11:07
@OskarStark OskarStark requested a review from derrabus January 19, 2021 13:26
@OskarStark
Copy link
Contributor

Thanks Mathias for working on this feature, this is much appreciated.

@OskarStark OskarStark merged commit c6100bc into symfony:5.x Jan 20, 2021
@mtarld mtarld deleted the feature/mercure-notifier branch January 20, 2021 09:09
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Jan 20, 2021
@OskarStark
feature #14840 [Notifier] Add Mercure notifier documentation (mtarld)
7c6c76e 
This PR was merged into the 5.3-dev branch.

Discussion
----------

[Notifier] Add Mercure notifier documentation

Documentation related to symfony/symfony#39342.

Commits
-------

475a87d [Notifier] Add Mercure notifier documentation
@mtarld mtarld mentioned this pull request Jan 21, 2021
Merged
@fabpot fabpot mentioned this pull request Apr 18, 2021
Merged
weaverryan added a commit to symfony/ux that referenced this pull request May 23, 2022
@weaverryan
feature #49 [Notify] Add Notify bundle (mtarld)
9b877d4 
This PR was merged into the 2.x branch.

Discussion
----------

[Notify] Add Notify bundle

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| Tickets       |
| License       | MIT

In Symfony 5.3, Notifier was shipped with a Mercure bridge (symfony/symfony#39342)

Then, with a simple `$this->notifier->send(new Notification('My message', ['chat/mercure']));` (and a running Mercure server), it'll be easy to create "Server-Sent-Events".

Therefore, the Notify bundle idea is to listen to these events using JavaScript event sourcing and convert them as native HTML5 notifications (using the `{{ stream_notifications() }}` Twig function).

(The "Notify" name was the first that came to my mind a could/should be challenged)

Commits
-------

e02a362 [Notify] Add Notify library
jameswebapp added a commit to jameswebapp/ux that referenced this pull request Aug 1, 2023
@jameswebapp
feature #49 [Notify] Add Notify bundle (mtarld)
aa59cf7 
This PR was merged into the 2.x branch.

Discussion
----------

[Notify] Add Notify bundle

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| Tickets       |
| License       | MIT

In Symfony 5.3, Notifier was shipped with a Mercure bridge (symfony/symfony#39342)

Then, with a simple `$this->notifier->send(new Notification('My message', ['chat/mercure']));` (and a running Mercure server), it'll be easy to create "Server-Sent-Events".

Therefore, the Notify bundle idea is to listen to these events using JavaScript event sourcing and convert them as native HTML5 notifications (using the `{{ stream_notifications() }}` Twig function).

(The "Notify" name was the first that came to my mind a could/should be challenged)

Commits
-------

e02a362 [Notify] Add Notify library
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

@stof stof stof left review comments

@jderusse jderusse jderusse left review comments

@chalasr chalasr chalasr approved these changes

@dunglas dunglas dunglas approved these changes

@OskarStark OskarStark OskarStark approved these changes

@derrabus derrabus Awaiting requested review from derrabus

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

Successfully merging this pull request may close these issues.

[Mercure] [Notifier] Add Mercure integration
9 participants
@mtarld @OskarStark @dunglas @nicolas-grekas @stof @jderusse @derrabus @chalasr @carsonbot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.