-
Notifications
You must be signed in to change notification settings - Fork 16
Add EventDispatcher to producer #45
base: master
Are you sure you want to change the base?
Conversation
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'm mostly ok with it, I've pointed out only minor changes. But can you provide any live example of usage? I'm just curious.
@@ -59,111 +59,128 @@ function it_publishes_product_created_events(ProducerInterface $producer) | |||
'data' => [['amount' => 20, 'currency' => 'USD']], | |||
], | |||
], | |||
], | |||
]; |
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.
Wrong indent
], | ||
]; | ||
|
||
|
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.
Too many blank lines
'code' => 'X_SELL', | ||
'labels' => [ | ||
'en_US' => 'Cross sell', | ||
'fr_FR' => 'Vente croisée', | ||
], | ||
], | ||
]; |
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.
Wrong indent
function it_should_be_constructed_correctly() | ||
{ | ||
$payload = [ | ||
'foo' => 'bar' |
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.
Could be fitted into one line
$producer->publish(json_encode([ | ||
'type' => 'message_type', | ||
'payload' => $item, | ||
'recordedOn' => (new \DateTime())->format('Y-m-d H:i:s'), |
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.
Just an idea. We could extract this format to some constant, WDYT?
|
||
use Symfony\Component\EventDispatcher\Event; | ||
|
||
class MessageEvent extends Event |
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.
Should this class be extendable?
|
||
/** | ||
* @param ProducerInterface $producer | ||
* @param $messageType |
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.
Missing typehint
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 looks OK, but this kind of support for events should be added in a ItemWriterInterface
decorator named like EventDispatchingItemWriter
- then we wouldn't need to modify all the specs and modify existing classes but just add this one and change dependency injection configuration, separating the concerns.
@lchrusciel, these events are useful when you want do have some additional logic after message was published. For example, Akeneo stores categories in a @pamil, OK, I'll create the decorator. Also, i see that different |
No description provided.