[Notifier] [Mercure] Add options #54961
Conversation
|
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:
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! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
this PR connect to this PR symfony/ux#1853 |
ernie76
changed the title
OskarStark
changed the title
Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
|
I just noticed that |
you need this option in google chrome |
Might be the case, couldn't find any example/issue that this is the case, but why excluding other browsers and not use just a simple array to dynamically pass the possible options? My recommendation:
It also looks like the resulting activity stream doesn't fit the spec: https://www.w3.org/TR/activitystreams-vocabulary/#dfn-announce. This might fit the spec more: {
"@context":"https://www.w3.org/ns/activitystreams",
"type":"Announce",
"summary":"subject",
"mediaType": "application/json",
"content": "{\"body\":null,\"icon\":null,\"tag\":null,\"renotify\":false}"
} |
|
@chapterjason it's a good idea |
|
@ernie76 You don't need to create new ones, just modify it, that's what PRs are about, they are evolving. I really like the initial idea, as it gives more control without creating any JS. |
add "mediaType" => "application/json" to MercureOptions.php
OskarStark
changed the title
body, icon, tag ,renotify
ernie76
changed the title
body, icon, tag ,renotify
|
@chapterjason is this so ok now? |
| $this->topics = $topics ?? 'https://symfony.com/notifier'; | ||
|
|
||
| parent::__construct($client, $dispatcher); | ||
| } | ||
|
|
||
| public function __toString(): string | ||
| { | ||
| return sprintf('mercure://%s%s', $this->hubId, '?'.http_build_query(['topic' => $this->topics], '', '&')); | ||
| return sprintf('mercure://%s%s', $this->hubId, null !== $this->topics ? '?'.http_build_query(['topic' => $this->topics], '', '&') : ''); |
There was a problem hiding this comment.
Why checking for null if constructor always sets a topic in that case?
| @@ -30,18 +30,17 @@ | |||
| */ | |||
| final class MercureTransport extends AbstractTransport | |||
| { | |||
| private HubInterface $hub; | |||
There was a problem hiding this comment.
This should be reverted, we already had promoted properties
| @@ -30,18 +30,16 @@ | |||
| */ | |||
| final class MercureTransport extends AbstractTransport | |||
| { | |||
| private string $hubId; | |||
| public function __construct( | ||
| private HubInterface $hub, | ||
| private string $hubId, | ||
| string|array|null $topics = null, | ||
| ?HttpClientInterface $client = null, | ||
| ?EventDispatcherInterface $dispatcher = null, | ||
| ) { |
| ]); | ||
| } | ||
|
|
||
| public function testConstructWithParameters() | ||
| { | ||
| $options = (new MercureOptions('/topic/1', true, 'id', 'type', 1)); | ||
| $options = (new MercureOptions('/topic/1', true, 'id', 'type', 1, null)); |
There was a problem hiding this comment.
I would provide a text here, rather than null
optionstest with options
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/Tests/MercureOptionsTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Notifier/Bridge/Mercure/Tests/MercureOptionsTest.php
Outdated
Show resolved
Hide resolved
…ionsTest.php Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
…ionsTest.php Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
| @@ -1,6 +1,9 @@ | |||
| CHANGELOG | |||
| ========= | |||
|
|
|||
| * Add content to MercureOptions.php for Symfony UX Notify | |||
There was a problem hiding this comment.
Please add a new 7.2 section and add only
* Add `content` option
….php Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
….php Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
| @@ -77,6 +77,8 @@ protected function doSend(MessageInterface $message): SentMessage | |||
| '@context' => 'https://www.w3.org/ns/activitystreams', | |||
| 'type' => 'Announce', | |||
| 'summary' => $message->getSubject(), | |||
| 'mediaType' => 'application/json', | |||
| 'content' => $options->getContent(), | |||
There was a problem hiding this comment.
To make it fit correctly it should be serialized to json here. (Only the value of the content entry)
For now I think it is enough to just inject the Serializer and always encode to json. To keep this PR simple.
I would also add an test to cover this case.
|
Hey @ernie76 a bit busy the last days, only one thing, everything else looks good. 👍🏼 |
Added body, icon, tag ,renotify for desktop notifications in Symfony UX
This pull-request are need changes in src/Notify/assets/dist/controller.js I have made a pull request there too.