From e5fa30c97f8193c08ae990538234ee72ed75631d Mon Sep 17 00:00:00 2001 From: Erik Huelsmann Date: Mon, 23 May 2022 21:32:09 +0200 Subject: [PATCH 1/5] Remove internal parameter from public API The `autorun` parameter was documented to be internal. It's also no longer used due to migrating from recursion to a loop for the `execute_action` function. Therefore, remove all references. Also, Oliver Welter and I discussed the removal of the autorun parameter to the observer calls: as "autorunning" isn't a criterion to the state machine, it shouldn't be to an observer. So we decided to drop it there too. --- lib/Workflow.pm | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/lib/Workflow.pm b/lib/Workflow.pm index e227f61..eadad17 100644 --- a/lib/Workflow.pm +++ b/lib/Workflow.pm @@ -113,11 +113,11 @@ sub get_action_fields { } sub execute_action { - my ( $self, $action_name, $autorun ) = @_; + my ( $self, $action_name ) = @_; while ( $action_name ) { my $wf_state = - $self->_execute_single_action( $action_name, $autorun ); + $self->_execute_single_action( $action_name ); if ( not $wf_state->autorun ) { last; @@ -279,7 +279,7 @@ sub set { sub _execute_single_action { - my ( $self, $action_name, $autorun ) = @_; + my ( $self, $action_name ) = @_; # This checks the conditions behind the scenes, so there's no # explicit 'check conditions' step here @@ -336,12 +336,11 @@ sub _execute_single_action { croak $error; } - $self->notify_observers( 'execute', $old_state, $action_name, $autorun ); + $self->notify_observers( 'execute', $old_state, $action_name ); my $new_state_obj = $self->_get_workflow_state; if ( $old_state ne $new_state ) { - $self->notify_observers( 'state change', $old_state, $action_name, - $autorun ); + $self->notify_observers( 'state change', $old_state, $action_name ); } return $new_state_obj; @@ -873,11 +872,9 @@ No additional parameters. B - Issued after a workflow is successfully executed and saved. -Adds the parameters C<$old_state>, C<$action_name> and C<$autorun>. +Adds the parameters C<$old_state>, and C<$action_name>. C<$old_state> includes the state of the workflow before the action -was executed, C<$action_name> is the action name that was executed and -C<$autorun> is set to 1 if the action just executed was started -using autorun. +was executed, C<$action_name> is the action name that was executed. =item * @@ -885,10 +882,9 @@ B - Issued after a workflow is successfully executed, saved and results in a state change. The event will not be fired if you executed an action that did not result in a state change. -Adds the parameters C<$old_state>, C<$action> and C<$autorun>. +Adds the parameters C<$old_state>, and C<$action>. C<$old_state> includes the state of the workflow before the action -was executed, C<$action> is the action name that was executed and -C<$autorun> is set to 1 if the action just executed was autorun. +was executed, C<$action> is the action name that was executed. =item * @@ -935,13 +931,12 @@ than the entire system. =head2 Object Methods -=head3 execute_action( $action_name, $autorun ) +=head3 execute_action( $action_name ) Execute the action C<$action_name>. Typically this changes the state of the workflow. If C<$action_name> is not in the current state, fails one of the conditions on the action, or fails one of the validators on -the action an exception is thrown. $autorun is used internally and -is set to 1 if the action was executed using autorun. +the action an exception is thrown. After the action has been successfully executed and the workflow saved we issue a 'execute' observation with the old state, action name and @@ -950,7 +945,7 @@ So if you wanted to write an observer you could create a method with the signature: sub update { - my ( $class, $workflow, $action, $old_state, $action_name, $autorun ) + my ( $class, $workflow, $action, $old_state, $action_name ) = @_; if ( $action eq 'execute' ) { .... } } From 8288e47cb4ee48f738b2f18897dd01267a98c70d Mon Sep 17 00:00:00 2001 From: Erik Huelsmann Date: Tue, 14 Jun 2022 20:23:18 +0200 Subject: [PATCH 2/5] Pass action arguments to 'execute_action' By passing the arguments to 'execute_action', there's no need to directly manipulate the workflow context. This way, the context is treated as workflow internal state. --- lib/Workflow.pm | 26 +++++++++++++++++++------- lib/Workflow/Action.pm | 15 +++++++-------- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/lib/Workflow.pm b/lib/Workflow.pm index eadad17..c59c380 100644 --- a/lib/Workflow.pm +++ b/lib/Workflow.pm @@ -113,11 +113,12 @@ sub get_action_fields { } sub execute_action { - my ( $self, $action_name ) = @_; + my ( $self, $action_name, $action_args ) = @_; while ( $action_name ) { my $wf_state = - $self->_execute_single_action( $action_name ); + $self->_execute_single_action( $action_name, $action_args ); + $action_args = undef; if ( not $wf_state->autorun ) { last; @@ -279,7 +280,7 @@ sub set { sub _execute_single_action { - my ( $self, $action_name ) = @_; + my ( $self, $action_name, $action_args ) = @_; # This checks the conditions behind the scenes, so there's no # explicit 'check conditions' step here @@ -291,8 +292,15 @@ sub _execute_single_action { my ( $new_state, $action_return ); try { - $action->validate($self); + $action->validate($self, $action_args); $self->log->is_debug && $self->log->debug("Action validated ok"); + if ($action_args) { + # Merge the action args into the context + my $ctx = $self->context; + while (my ($k, $v) = each %{$action_args}) { + $ctx->param( $k, $v ); + } + } $action_return = $action->execute($self); $self->log->is_debug && $self->log->debug("Action executed ok"); @@ -517,10 +525,10 @@ This documentation describes version 1.57 of Workflow my $context = $workflow->context; $context->param( current_user => $user ); $context->param( sections => \@sections ); - $context->param( path => $path_to_file ); # Execute one of them - $workflow->execute_action( 'upload file' ); + $workflow->execute_action( 'upload file', + { path => $path_to_file }); print "New state: ", $workflow->state, "\n"; @@ -931,13 +939,17 @@ than the entire system. =head2 Object Methods -=head3 execute_action( $action_name ) +=head3 execute_action( $action_name, $args ) Execute the action C<$action_name>. Typically this changes the state of the workflow. If C<$action_name> is not in the current state, fails one of the conditions on the action, or fails one of the validators on the action an exception is thrown. +The C<$args> provided, are checked against the validators to ensure the +context remains in a valid state; upon successful validation, the C<$args> +are merged into the context and the action is executed as described above. + After the action has been successfully executed and the workflow saved we issue a 'execute' observation with the old state, action name and an autorun flag as additional parameters. diff --git a/lib/Workflow/Action.pm b/lib/Workflow/Action.pm index 2038142..4c6265b 100644 --- a/lib/Workflow/Action.pm +++ b/lib/Workflow/Action.pm @@ -66,7 +66,7 @@ sub get_validators { } sub validate { - my ( $self, $wf ) = @_; + my ( $self, $wf, $action_args ) = @_; my @validators = $self->get_validators; return unless ( scalar @validators ); @@ -75,17 +75,15 @@ sub validate { my $validator = $validator_info->{validator}; my $args = $validator_info->{args}; - # TODO: Revisit this statement it does not look right - # runtime_args becomes the WF object?? - my @runtime_args = ($wf); + my @runtime_args = (); foreach my $arg ( @{$args} ) { if ( $arg =~ /^\$(.*)$/ ) { - push @runtime_args, $context->param($1); + push @runtime_args, $action_args->{$1}; } else { push @runtime_args, $arg; } } - $validator->validate(@runtime_args); + $validator->validate($wf, @runtime_args); } } @@ -460,9 +458,10 @@ L object, while 'args' contains an arrayref of arguments to pass to the validator, some of which may need to be evaluated at runtime. -=head3 validate( $workflow ) +=head3 validate( $workflow, $action_args ) -Run through all validators for this action. If any fail they will +Run through all validators for this action, using the arguments +provided with the C call. If any fail they will throw a L, the validation subclass. =head3 execute( $workflow ) From bead05ea3cecf5e02d56671da1b955bcd86dfe40 Mon Sep 17 00:00:00 2001 From: Erik Huelsmann Date: Fri, 28 Jul 2023 22:58:20 +0200 Subject: [PATCH 3/5] Instead of using the current arguments, use the workflows state *after* merging arguments This allows passing in values earlier in the workflow and thereby decouples the workflow invokers from the workflow execution. Without this change, the workflow invoker would need to know exactly which step needs which parameters, which would be a case of tight integration. --- lib/Workflow/Action.pm | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/Workflow/Action.pm b/lib/Workflow/Action.pm index 4c6265b..2acf887 100644 --- a/lib/Workflow/Action.pm +++ b/lib/Workflow/Action.pm @@ -70,7 +70,11 @@ sub validate { my @validators = $self->get_validators; return unless ( scalar @validators ); - my $context = $wf->context; + $action_args //= {}; + my %all_args = ( + %{ $wf->context->param() }, + %{$action_args} + ); foreach my $validator_info (@validators) { my $validator = $validator_info->{validator}; my $args = $validator_info->{args}; @@ -78,7 +82,7 @@ sub validate { my @runtime_args = (); foreach my $arg ( @{$args} ) { if ( $arg =~ /^\$(.*)$/ ) { - push @runtime_args, $action_args->{$1}; + push @runtime_args, $all_args{$1}; } else { push @runtime_args, $arg; } From 42241de6993dc41e59cfe5a8b37d8c023ff46d9e Mon Sep 17 00:00:00 2001 From: Erik Huelsmann Date: Sun, 6 Aug 2023 12:48:31 +0200 Subject: [PATCH 4/5] Updating the context with an 'undef' value removes the parameter from the context --- lib/Workflow/Base.pm | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/Workflow/Base.pm b/lib/Workflow/Base.pm index a756a40..811fa71 100644 --- a/lib/Workflow/Base.pm +++ b/lib/Workflow/Base.pm @@ -38,7 +38,12 @@ sub param { if ( ref $name eq 'HASH' ) { foreach my $param_name ( keys %{$name} ) { - $self->{PARAMS}{$param_name} = $name->{$param_name}; + if (defined $name->{$param_name}) { + $self->{PARAMS}{$param_name} = $name->{$param_name}; + } + else { + delete $self->{PARAMS}->{$param_name}; + } } return { %{ $self->{PARAMS} } }; } From 284bfa59e6d4c04503bf719a8587b9c127a08040 Mon Sep 17 00:00:00 2001 From: Erik Huelsmann Date: Sun, 6 Aug 2023 13:00:33 +0200 Subject: [PATCH 5/5] Incorporate review from @oliwel --- lib/Workflow.pm | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/Workflow.pm b/lib/Workflow.pm index c59c380..2fa957d 100644 --- a/lib/Workflow.pm +++ b/lib/Workflow.pm @@ -296,10 +296,7 @@ sub _execute_single_action { $self->log->is_debug && $self->log->debug("Action validated ok"); if ($action_args) { # Merge the action args into the context - my $ctx = $self->context; - while (my ($k, $v) = each %{$action_args}) { - $ctx->param( $k, $v ); - } + $self->context->param( $action_args ); } $action_return = $action->execute($self); $self->log->is_debug && $self->log->debug("Action executed ok");