-
-
Notifications
You must be signed in to change notification settings - Fork 914
Automatically dispatch updates to clients using the Mercure protocol #2282
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
Conversation
src/Bridge/Doctrine/EventListener/PublishMercureUpdatesListener.php
Outdated
Show resolved
Hide resolved
|
||
public function __construct(ResourceClassResolverInterface $resourceClassResolver, IriConverterInterface $iriConverter, ResourceMetadataFactoryInterface $resourceMetadataFactory, SerializerInterface $serializer, MessageBusInterface $messageBus = null, callable $publisher = null, ExpressionLanguage $expressionLanguage = null) | ||
{ | ||
if (null === $messageBus && null === $publisher) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly ensure only 1 is provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would make the wiring a bit harder.
|
||
private function reset(): void | ||
{ | ||
$this->createdEntities = new \SplObjectStorage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the same as removeAll
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removeAll
takes a mandatory argument as parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, $this->createdEntities->removeAll($this->createdEntities)
.
I was wondering if there would be any different from a memory/gc point of view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea :) I would keep it as is for now because it's "safer", but if your suggestion improve GC or something similar, let me know and I'll update!
|
||
if (\is_string($value)) { | ||
if (null === $this->expressionLanguage) { | ||
throw new RuntimeException('The Expression Language component is not installed. Try running "composer require symfony/expression-language".'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the proper place to throw this, or can it be done in the metadata factory somehow?
} | ||
|
||
if (!\is_array($value)) { | ||
throw new InvalidArgumentException(sprintf('The value of the "mercure" attribute of the "%s" resource class must be a boolean, an array of targets or a valid expression, "%s" given.', $resourceClass, \gettype($value))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above, proper place for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently don't handle metadata validation in the factory, but it would definitely be a better place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice huh!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'll keep it as is because it out of this PR's scope, and I'll try to add a proper validator (for all existing attributes) in a subsequent PR. Is it ok for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of course! sorry, my ?!
wasn't meant to come off as rude or aggressive, but enthusiastic!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood it like that no worries :)
@@ -15,7 +15,7 @@ | |||
|
|||
<tag name="doctrine.event_listener" event="preUpdate" /> | |||
<tag name="doctrine.event_listener" event="onFlush" /> | |||
<tag name="doctrine.event_listener" event="postFlush" /> | |||
<tag name="kernel.event_listener" event="kernel.terminate" method="postFlush" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, wow ok. has your thinking changed since #1286?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, you're right, I'll revert this change as it's not related to this PR. (I forgot about this discussion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the discussion point is similar, i guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, to me it's a big deal if a cache is not flushed because you'll serve stale data. Not dispatching a Mercure update is less a problem, because it breaks nothing, and it's explicitly stated in the Mercure protocol that the client MUST call the original API if it wants to be sure to have fresh data. So we can use kernel.terminate
safely for Mercure, I'm still unsure for Varnish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nicely argued.
@@ -145,7 +145,10 @@ private function storeEntityToPublish($entity, string $property): void | |||
} | |||
|
|||
if ('deletedEntities' === $property) { | |||
$this->deletedEntities[$this->iriConverter->getIriFromItem($entity, UrlGeneratorInterface::ABS_URL)] = $value; | |||
$this->deletedEntities[(object) [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we transform this to a real class? We had issues in the past with StdClass and it's better for debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it worth it? It's a purely internal state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/api-platform/core/pull/1780/files this was kinda the same, internal state but eventually it hit a bug :p. As you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooo. mine! that was for anonymous classes, not stdClass
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was in the serialization context, and indeed an anonymous class. \stdClass
is serializable: https://3v4l.org/2VWST
…pi-platform#2282) * Automatically dispatch updates to clients using the Mercure protocol * Allow to use different URL to publish and subscribe * Fix PHPStan * Use public repos * Remove useless arg * Review * Revert unrelated change * Don't deal with absolute IRIs in documents for now
This PR adds support for the Mercure Protocol to API Platform.
It introduces a simple new attribute called
mercure
:When set, every time an object of this type is created, updates or deleted through the API, the new version is sent to all connected clients through the Mercure hub
Checkout this YouTube video to see how it works with the API Platform client generator: http://www.youtube.com/watch?v=JdlAcfTJsOI
The code of I used for this video is available here: api-platform/demo#66
It's also possible to send the update to users having some
targets
only:Finally, it's possible to execute an expression (using the Expression Language component), to retrieve a dynamic list of targets:
To enable Mercure support, you must install the brand new Symfony Mercure Bundle, that comes with the Symfony Mercure Component.
Take a look to my Forum PHP's slide deck for further information about Mercure and its support in Symfony and API Platform.