fixes issue #13 with passing variables from and to sub-workflows #14

sgehrig · 2018-01-17T08:24:13Z

This pull request fixes the issues described in #13.

…workflows

derickr

I've had a few comments, but more importantly, this misses a test case, or test cases.

derickr · 2018-01-17T10:19:16Z

src/nodes/sub_workflow.php

 *   http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *


Please split white space changes out in a separate commit.

Oops. Sure.

derickr · 2018-01-17T10:19:56Z

src/nodes/sub_workflow.php

+                $subVariables = [];
+                foreach ( $execution->getVariables() as $variableName => $data ) {
+                    if ( isset($this->configuration['variables']['in'][$variableName]) ) {
+                        $subVariables[$this->configuration['variables']['in'][$variableName]] = $data;


Coding standards want the { on a new line on its own, please.

Overlooked that. Uncommon coding style for me. Will be fixed.

derickr · 2018-01-17T10:20:10Z

src/nodes/sub_workflow.php

@@ -188,9 +195,10 @@ public function execute( ezcWorkflowExecution $execution )
        }

        // Execution of Sub Workflow has been suspended.
+        $reverseInVariableMap = array_flip( $this->configuration['variables']['in'] );


Why does this need to be flipped?

Given an interactive sub-workflow node which maps x on the parent workflow to y on the sub-workflow. The workflow used as sub-workflow now waits for its variable y to be set.
If sub-workflow is waiting for y the parent workflow is also waiting for a variable. But the parent is not waiting for y but for x because of the mapping. y is not even a variable known to the parent workflow.
I hope this is somehow understandable...

sgehrig · 2018-01-17T10:46:17Z

Regarding the failing test cases: regardless of running against master or the new fix I always get 26 errors and 24 warnings.

The warnings are due to deprecations in the mocking API in PHPUnit (when running on PHPUnit 5.7.26) and the errors are:

17 x missing hard dependency on WorkflowEventLogTiein being installed parallel to Workflow
9 x Illegal string offset 'operand' in Workflow/src/interfaces/node_arithmetic_base.php:114

I didn't see any tests that used interactive sub-workflows.

derickr · 2018-01-19T10:39:21Z

Hmm. OK. Would you be interested in making the test suite up to par first then? And hook them up to Travis as well? I know little about this component, I am not sure whether @sebastianbergmann is still interested in maintaining it either, so I am going to need some help with this one :-)

sebastianbergmann · 2018-01-19T10:41:15Z

I am not interested in this anymore.

sgehrig · 2018-01-19T12:23:44Z

@derickr I'll fix the test suite sometime next week. I'm not very familiar with the contribution guidelines for zetacomponents, so: is it allowed to add e.g. the WorkflowEventLogTiein as a dev-dependency and remove this old hard dependency on the installation path?

What's the required PHP version compatibility for zetacomponents?

derickr · 2018-01-19T12:39:23Z

Curious, when I run the tests, I get this — nothing about WorkflowEventLogTiein:

derick@singlemalt:~/dev/zeta-new/Workflow $ php ~/phpunit-5.7.22.phar 
PHPUnit 5.7.22 by Sebastian Bergmann and contributors.

............................W..................................  63 / 283 ( 22%)
..............................WWWWEEWWWWWWWWEWWWWWWEEWWWWWWWWWW 126 / 283 ( 44%)
WWWWEEWW.E................WEE..........................EE....WW 189 / 283 ( 66%)
............................................................... 252 / 283 ( 89%)
...............................                                 283 / 283 (100%)

Time: 13.39 seconds, Memory: 14.00MB

There were 12 errors:

1) ezcWorkflowExecutionListenerTest::testEventsForIncrementingLoop
Illegal string offset 'operand'

/home/derick/dev/zeta-new/Workflow/src/interfaces/node_arithmetic_base.php:114
/home/derick/dev/zeta-new/Workflow/src/interfaces/execution.php:510
/home/derick/dev/zeta-new/Workflow/src/interfaces/execution.php:275
/home/derick/dev/zeta-new/Workflow/tests/execution_listener_test.php:99

This is with PHP 7.2.0, and PHPUnit 5.7.22.

I don't mind if you add WorkflowEventLogTiein as a dev-dependency, as long as the component can normally work without it being there.

I would suggest you look at the Travis config for Base: https://github.com/zetacomponents/Base/blob/master/.travis.yml

And as the lowest version we test on Travis is currently PHP 5.6 (I think because of PHPUnit version requirements), I'm OK with that being the lowest version. If there is no problem to make it work with PHP 5.3, then it would be good if you can at least manually test that once (or at least lint).

sgehrig · 2018-01-19T13:57:41Z

@derickr Do you have the WorkflowEventLogTiein installed in parallel to Workflow (/home/derick/dev/zeta-new/WorkflowEventLogTiein )? That's where the tests are looking for it.

derickr · 2018-01-19T14:07:52Z

Oh yes, I have that checked out next to it.

sgehrig · 2018-01-19T15:31:39Z

@derickr There's now a second PR (#16) that fixes the unit tests and adds the correct .travis.yml to setup Travis CI. See https://travis-ci.org/ipoint-bis/Workflow

fixes issue zetacomponents#13 with passing variables from and to sub-…

3196e1e

…workflows

derickr requested changes Jan 17, 2018

View reviewed changes

Stefan Gehrig added 2 commits January 17, 2018 11:32

fixes { placement according to coding style

25c1f69

fixes whitespace

7595a2a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes issue #13 with passing variables from and to sub-workflows #14

fixes issue #13 with passing variables from and to sub-workflows #14

sgehrig commented Jan 17, 2018

derickr left a comment

derickr Jan 17, 2018

sgehrig Jan 17, 2018

derickr Jan 17, 2018

sgehrig Jan 17, 2018

derickr Jan 17, 2018

sgehrig Jan 17, 2018

sgehrig commented Jan 17, 2018 •

edited

Loading

derickr commented Jan 19, 2018

sebastianbergmann commented Jan 19, 2018

sgehrig commented Jan 19, 2018

derickr commented Jan 19, 2018

sgehrig commented Jan 19, 2018 •

edited

Loading

derickr commented Jan 19, 2018

sgehrig commented Jan 19, 2018 •

edited

Loading

fixes issue #13 with passing variables from and to sub-workflows #14

Are you sure you want to change the base?

fixes issue #13 with passing variables from and to sub-workflows #14

Conversation

sgehrig commented Jan 17, 2018

derickr left a comment

Choose a reason for hiding this comment

derickr Jan 17, 2018

Choose a reason for hiding this comment

sgehrig Jan 17, 2018

Choose a reason for hiding this comment

derickr Jan 17, 2018

Choose a reason for hiding this comment

sgehrig Jan 17, 2018

Choose a reason for hiding this comment

derickr Jan 17, 2018

Choose a reason for hiding this comment

sgehrig Jan 17, 2018

Choose a reason for hiding this comment

sgehrig commented Jan 17, 2018 • edited Loading

derickr commented Jan 19, 2018

sebastianbergmann commented Jan 19, 2018

sgehrig commented Jan 19, 2018

derickr commented Jan 19, 2018

sgehrig commented Jan 19, 2018 • edited Loading

derickr commented Jan 19, 2018

sgehrig commented Jan 19, 2018 • edited Loading

sgehrig commented Jan 17, 2018 •

edited

Loading

sgehrig commented Jan 19, 2018 •

edited

Loading

sgehrig commented Jan 19, 2018 •

edited

Loading