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

[Turbo] Add expression language to topics #327

Merged

Conversation

rskuipers
Copy link
Contributor

@rskuipers rskuipers commented May 29, 2022

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

Why

With the current implementation of the Broadcast attribute there's no way to scope newly added entities. A newly added entity will be broadcasted to anywhere that uses turbo_stream_listen("App\\Entity\\Book"). But there are use-cases where you want to limit this.

For one, imagine having a template author/show.html.twig where you want to show all the books belonging to a specific author. A newly added Book entity would show up on all the author pages because there's no way to supply a scope to turbo_stream_listen.

Secondly, there's currently no way to limit access based on the logged in user. For simplicity's sake let's imagine every author belongs to a publisher. The publisher can login and add/edit/remove authors that belong to them. Now let's say we have a template called author/list.html.twig on which we use turbo_stream_listen("App\\Entity\\Author"). In the current implementation, this would also show all newly created Authors created by other Publishers.

So adding a scope is both useful for showing specific listings as well as listings that should only be shown to a specific user.

Adding expression language support to topics makes it possible to scope these topics based on a relationship or some other context.

How

We can achieve the above by changing two points in the code.

https://github.com/rskuipers/ux/blob/feature/add-scope-option-to-broadcast/src/Turbo/Tests/app/Entity/Book.php#L23

Expanding the Broadcast attribute with a scope option that supports Expression Language. This will be executed when an entity is created/changed typically in order to retrieve an ID of a parent entity that will function as a scope.

Note: this expression has null checking because the relationship is optional. Also in Expression Language 6.1 you can use the null-safe operator: entity.author?.id

https://github.com/rskuipers/ux/blob/feature/add-scope-option-to-broadcast/src/Turbo/Twig/TwigExtension.php#L44

Adding a third parameter $scope = null here so that you can provide a value used to add a scope to the topic you're listening to.

https://github.com/rskuipers/ux/blob/feature/add-scope-option-to-broadcast/src/Turbo/Twig/ScopedTurboStreamListenRendererInterface.php

Adding a separate interface with a method supporting a third parameter ?string $scope = null here so that this can be integrated into the topic to listen for a scoped topic.

@rskuipers rskuipers force-pushed the feature/add-scope-option-to-broadcast branch from 9744b04 to 25fe6c3 Compare May 29, 2022 19:21
@rskuipers
Copy link
Contributor Author

This would obviously still need documentation, but I'm just awaiting confirmation before I start writing that.

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

A few questions - but I think the approach is solid.

@dunglas - what do you think about this idea? tl;dr

  1. Optional scope option added using expression language - e.g. [Broadcast(scope: 'entity.author.id')]
  2. This causes the topic to change to https://symfony.com/ux-turbo/App%5CEntity%5CBook_15/7 where 15 is the entity.author.id value and 7 is the main Book object's value that was just added/updated.

src/Turbo/Bridge/Mercure/Broadcaster.php Outdated Show resolved Hide resolved
src/Turbo/Bridge/Mercure/TurboStreamListenRenderer.php Outdated Show resolved Hide resolved
src/Turbo/Twig/TwigExtension.php Outdated Show resolved Hide resolved
@dunglas
Copy link
Member

dunglas commented Jun 3, 2022

Nice idea!

We have a similar feature in API Platform: https://github.com/api-platform/core/blob/main/src/Doctrine/EventListener/PublishMercureUpdatesListener.php#L225-L244

Basically, the idea is to let the user generate the whole topic using Expression Language. This has the benefit of giving full control to the user and to leverage all the capabilities of the underlying Transport.

What do you think about following this approach?

@rskuipers
Copy link
Contributor Author

@dunglas That's brilliant! Looks like it will fit here as well, definitely adds even more flexibility and it solves the problem with the current implementation where it's limited to 1 scope.

If @weaverryan agrees then I'll go ahead and reimplement it using your suggestion.

@rskuipers rskuipers force-pushed the feature/add-scope-option-to-broadcast branch from 7c03f4c to 4df3c9b Compare June 5, 2022 15:54
@rskuipers rskuipers changed the title [Turbo] Add scope option to broadcast [Turbo] Add expression language to topics Jun 5, 2022
@rskuipers
Copy link
Contributor Author

rskuipers commented Jun 5, 2022

I have updated this PR to the method suggested by @dunglas.

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Minor comments - but this is just about ready for merge - thanks!

src/Turbo/Tests/app/templates/authors.html.twig Outdated Show resolved Hide resolved
src/Turbo/Tests/app/templates/books.html.twig Outdated Show resolved Hide resolved
src/Turbo/composer.json Outdated Show resolved Hide resolved
@weaverryan
Copy link
Member

Friendly ping @rskuipers - I'm hoping to tag all of UX this week, it'd be great if this got in there :)

@rskuipers
Copy link
Contributor Author

rskuipers commented Jun 14, 2022 via email

@rskuipers rskuipers force-pushed the feature/add-scope-option-to-broadcast branch 3 times, most recently from 6efedb7 to 05f2e3f Compare June 16, 2022 14:56
@rskuipers
Copy link
Contributor Author

@weaverryan I've reverted the existing books test and created separate entities to test the expression language feature as advised by @dunglas. I've also made expression language optional and changed object to entity to be more in-line with the rest of the code. I've also added a small bit of documentation. Not sure if you want to dedicate an entire chapter to it or if this is enough.

@weaverryan weaverryan force-pushed the feature/add-scope-option-to-broadcast branch from 05f2e3f to 957c556 Compare June 16, 2022 17:06
@weaverryan
Copy link
Member

Thank you Rick!

@weaverryan weaverryan merged commit 83a2885 into symfony:2.x Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants