Movatterモバイル変換

epitre force-pushed theeventWithContext branch from33cc8f2 to82b8a82Compare

July 9, 2020 10:47

epitre commented

src/Symfony/Bridge/Twig/Extension/WorkflowExtension.phpShow resolvedHide resolved

ro0NL reviewed

src/Symfony/Component/Workflow/Event/Event.php OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

epitre commentedJul 9, 2020

appveyor is not happy but I believe it has nothing to do with me. Does it ?

epitre force-pushed theeventWithContext branch 2 times, most recently fromee4d388 to348cb02Compare

July 9, 2020 11:07

epitre changed the title~~Added Context to Workflow Event~~[Workflow] Added Context to Workflow Event

epitre commented

src/Symfony/Component/Workflow/WorkflowInterface.php OutdatedShow resolvedHide resolved

pluk77 reviewed

src/Symfony/Component/Workflow/Workflow.php

		*/
		class Workflow implements WorkflowInterface
		{
		public const DISABLE_ANNOUNCE_EVENT = 'workflow_disable_announce_event';
		public const DEFAULT_INITIAL_CONTEXT = ['initial' => true];

Copy link

Contributor

pluk77Jul 9, 2020

Choose a reason for hiding this comment

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

Is there a reason you made this an associative array and not an indexed one?
public const DEFAULT_INITIAL_CONTEXT = ['initial'];

I see in one of your tests you did use an indexed array as $context.

Are yuo

Copy link

ContributorAuthor

epitreJul 9, 2020

Choose a reason for hiding this comment

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

this way, you can do :
if ($context['initial']??false)

lyrixx added the Workflow label

lyrixx reviewed

Copy link

Member

lyrixx left a comment

Choose a reason for hiding this comment

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

Thanks for your PR but I already rejected in the past

src/Symfony/Component/Workflow/WorkflowInterface.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Workflow/Workflow.php OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

epitre commentedJul 9, 2020

Thanks for your PR but I already rejected in the past

O_O Does it mean you are rejecting this one ?

What I want, is to be able to differentiate transitions that will be done automatically from others that will be done manually.
If not with the context, how do you suggest to do that ?

epitre force-pushed theeventWithContext branch from348cb02 to6ece94aCompare

July 9, 2020 14:06

Copy link

ContributorAuthor

epitre commentedJul 9, 2020•
edited
Loading

Thanks for your PR but I already rejected in the past

I found an oldPR and realized the TransitionEvent knows the context ^^

I'll use this event, then. But I really don't understand why the other events do not deserve to know the context ^^

Copy link

Member

lyrixx commentedJul 9, 2020

Oups, I write the comment but I forgot to remove it 👍

I'm against to put the context in the guard event but for other event I guess this is fine 👍

O_O Does it mean you are rejecting this one ?

nope, it's OK 👍

Sorry for my bad comment

epitre force-pushed theeventWithContext branch from6ece94a tof53d995Compare

July 9, 2020 15:09

Copy link

ContributorAuthor

epitre commentedJul 9, 2020

Oups, I write the comment but I forgot to remove it +1
I'm against to put the context in the guard event but for other event I guess this is fine +1
O_O Does it mean you are rejecting this one ?
nope, it's OK +1
Sorry for my bad comment

Great ! So, GuardEvent is not aware of the context anymore. I simplified TransitionEvent. And TransitionEvent is still the only event which can change the context.

epitre force-pushed theeventWithContext branch 3 times, most recently from7756024 to2b1651cCompare

July 9, 2020 15:23

nicolas-grekas added this to thenext milestone

lyrixx reviewed

src/Symfony/Component/Workflow/Workflow.php

src/Symfony/Component/Workflow/Event/Event.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Workflow/Workflow.php OutdatedShow resolvedHide resolved

		@@ -395,34 +400,38 @@ private function entered(object $subject, ?Transition $transition, Marking $mark
		foreach ($transition->getTos() as $place) {
		$this->dispatcher->dispatch($event, sprintf('workflow.%s.entered.%s', $this->name, $place));
		}
		} elseif (!empty($this->definition->getInitialPlaces())) {

Copy link

Member

lyrixxJul 9, 2020

Choose a reason for hiding this comment

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

This is not related to the current PR. Could you revert it? thanks

Copy link

ContributorAuthor

epitreJul 9, 2020

Choose a reason for hiding this comment

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

without that, the DEFAULT_INITIAL_CONTEXT makes no sense.

it's a little far fetched, but I believe adding this context enhancement is a good moment to add this default context

Unless you don't want a DEFAULT_INITIAL_CONTEXT ?

Added Context to Workflow Event

bfe07dd

There's also a default context for the initial marking event.

epitre force-pushed theeventWithContext branch from2b1651c tobfe07ddCompare

July 9, 2020 22:09

Copy link

ContributorAuthor

epitre commentedJul 20, 2020

@lyrixx up !

lyrixx approved these changes

carsonbot removed the Status: Needs Review label

carsonbot added the Status: Reviewed label

Copy link

Member

lyrixx commentedAug 12, 2020

I'm gonna merge your PR, but I also gonna tweak 2/3 things

Thanks for your work and your patience

Copy link

Member

lyrixx commentedAug 12, 2020

Thank you@epitre.

lyrixx merged commit25095d8 intosymfony:master

[Worfklow] Add 'initiated' event which is dispatched upon entering inital place#37481

This was referencedAug 12, 2020

Closed

[Workflow] Simplify code + Updated README.md#37813

Merged

fabpot added a commit that referenced this pull request