Clear entity-manager after each task to ensure clean environment #34
Conversation
wachterjohannes
force-pushed
the
bugfix/event-after
branch
from
dbcc770 to
52e62e2
Compare
| @@ -16,5 +16,11 @@ | |||
| <argument type="string">TaskBundle:TaskExecution</argument> | |||
| </service> | |||
| <service id="task.storage.task_execution" alias="task.repository.task_execution"/> | |||
|
|
|||
| <service id="task.event_listener.execution" class="Task\TaskBundle\EventListener\TaskExecutionListener"> | |||
There was a problem hiding this comment.
If this service is loaded or not should be configureable. It might be possible that the EntityManager should be cleared although the tasks are stored somewhere else.
| @@ -92,6 +92,22 @@ public function findPending(TaskInterface $task) | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function findByUuid($uuid) | |||
There was a problem hiding this comment.
couldnt you use the doctrine internal findOneByUuid function?
There was a problem hiding this comment.
tbh i dont like this "magic" internal methods.
wachterjohannes
force-pushed
the
bugfix/event-after
branch
4 times, most recently
from
806a917 to
8471371
Compare
wachterjohannes
force-pushed
the
bugfix/event-after
branch
from
8471371 to
266b2b5
Compare
| @@ -92,6 +92,22 @@ public function findPending(TaskInterface $task) | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function findByUuid($uuid) | |||
There was a problem hiding this comment.
Same opinion as @wachterjohannes. This way it will be easier to optimize if there come up performance problem at some time.
| /** | ||
| * Listens on task-execution events. | ||
| */ | ||
| class TaskExecutionListener |
There was a problem hiding this comment.
Shouldn't that be something like DoctrineTaskExecutionListener?
|
@danrot fixed |
| /** | ||
| * Listens on task-execution events. | ||
| */ | ||
| class DoctrineTaskExecutionListener |
There was a problem hiding this comment.
Sorry to mention that again, but I missed it the last time... Wouldn't it be better to make it a subscriber? Then it could handle all the doctrine specific settings, which might be added later. Otherwise this would have to be renamed to something to include clear.
There was a problem hiding this comment.
also like this we could handle everything for doctrine - i dont see the difference
There was a problem hiding this comment.
Not if you have to do some doctrine specific tasks on different events.
There was a problem hiding this comment.
thats configuration you can use multiple tags for different events.
There was a problem hiding this comment.
Ok, that's true, but I think it is a little bit hidden in the configuration... And since this class is already bound to an event I would also not fear implementing the EventSubscriberInterface... But well, I think that's just personal preference. It's also ok for me this way.
| /** | ||
| * Listens on task-execution events. | ||
| */ | ||
| class DoctrineTaskExecutionListener |
There was a problem hiding this comment.
Ok, that's true, but I think it is a little bit hidden in the configuration... And since this class is already bound to an event I would also not fear implementing the EventSubscriberInterface... But well, I think that's just personal preference. It's also ok for me this way.
| @@ -11,7 +11,7 @@ | |||
| ], | |||
| "require": { | |||
| "php": "~5.5 || ~7.0", | |||
| "php-task/php-task": "dev-master", | |||
| "php-task/php-task": "dev-bugfix/event-after", | |||
There was a problem hiding this comment.
As soon as you've removed this change this can be merged.
|
@danrot done |
Main PR: php-task/php-task#29