#2317193 | Port "Add an item to a list" action to D8 #102

stephenpurkiss · 2014-08-09T15:41:12Z

Initial port

fago · 2014-08-21T14:20:50Z

tests/src/Action/ListAddItemTest.php

+  public function testActionExecution() {
+
+    // Test adding to array of string values
+    $list = array('One','Two','Three');


misse spaces between the items; ie.after the comma

stephenpurkiss · 2014-08-28T12:03:17Z

Sorted (I think!) the spacing issue, having issues working out the git branch/merging thing hence the references to temp branch etc. Looking into context value issue next.

klausi · 2014-09-19T11:10:27Z

Do you want to continue working on this, Steve? Any help needed?

stephenpurkiss · 2014-09-19T11:16:30Z

hmm I'd completely forgotten about this, sorry! Sure, I think it may just need squashing but will check

stephenpurkiss · 2014-09-19T11:26:22Z

Oh hang on, forgot about the unit test changes. brb

stephenpurkiss · 2014-09-19T11:46:01Z

Oh, OK, it's a bit further behind than I thought - pointers welcome if you have time and I'm not slowing the process down too much. I've got to figure out how to set up a test for a list, code doesn't seem to be there at the moment (my understanding has increased in the last 22 days!).

Will take a look over the weekend, and I'll be in Amsterdam this time next week so around for the first lot of extended sprints but then flying back the next friday - no doubt say hi if you're going too - not long now yay!

klausi · 2014-09-19T12:02:41Z

tests/src/Action/ListAddItemTest.php

+ * Contains \Drupal\rules\Tests\Action\ListAddItemTest.
+ */
+
+namespace Drupal\rules\Tests\Action;


Namespace should be moved to the "Unit" subfolder, please rebase your work on the 8.x-3.x branch and move the test.

stephenpurkiss · 2014-09-19T12:37:13Z

Managed a bit of time on it, now stuck at "PHP Fatal error: Call to a member function get() on a non-object in /Users/steve/Sites/d8dev/core/lib/Drupal.php on line 428" when I run the tests locally :(

Will continue

klausi · 2014-09-19T13:38:34Z

tests/src/Unit/Action/ListAddItemTest.php

+
+    // Test that the list contains 'Four'.
+    $this->action
+      ->setContextValue('list', $list)


I think you need to use $this->getMockTypedData($list) here, take a look at the other unit tests. Maybe also $this->action->setTypedDataManager($this->getMockTypedDataManager()); in setUp().

Nope, let's use RulesIntegrationTestBase now and the real typed data manager. So we can save converting this test later on :)

stephenpurkiss · 2014-09-27T10:59:05Z

Hi - am stuck on two issue on this one, just spoke to fago who didn't know what the issue is either so posting here whilst trying to debug too, although may move to something else whilst I'm stuck!

I'm setting a provided value but it says the context is not valid. Also I'm having to force values for 'unique' and 'pos' even though I say they're not required. Here's the output of my test:

There was 1 error:

Drupal\Tests\rules\Unit\Action\ListAddItemTest::testActionExecution
Drupal\Component\Plugin\Exception\ContextException: The outputlist provided context is not valid.

/Users/steve/Sites/d8dev/modules/rules/src/Context/RulesContextTrait.php:61
/Users/steve/Sites/d8dev/modules/rules/src/Context/RulesContextTrait.php:50
/Users/steve/Sites/d8dev/modules/rules/src/Context/RulesContextTrait.php:40
/Users/steve/Sites/d8dev/modules/rules/src/Plugin/Action/ListAddItem.php:90
/Users/steve/Sites/d8dev/modules/rules/tests/src/Unit/Action/ListAddItemTest.php:66
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:179
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:132

There was 1 failure:

Drupal\Tests\rules\Unit\Condition\UserHasEntityFieldAccessTest::testConditionEvaluation
Failed asserting that false is true.

/Users/steve/Sites/d8dev/modules/rules/tests/src/Unit/Condition/UserHasEntityFieldAccessTest.php:123
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:179
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:132

FAILURES!

klausi · 2014-09-27T11:19:05Z

The UserHasEntityFieldAccessTest.php should be fixed in 8.x-3.x, please merge your branch.

klausi · 2014-09-27T11:25:21Z

tests/src/Unit/Action/ListAddItemTest.php

+    $this->action = new ListAddItem([], '', ['context' => [
+      'list' => new ContextDefinition('list'),
+      'item' => new ContextDefinition('string')
+    ]]);


The outputlist provided context is missing here? Same for the other contexts in the annotation?

The UserHasEntityFieldAccessTest.php should be fixed in 8.x-3.x, please merge your branch.

Ah - I think the issue here is I fetched the upstream and rebased to my local 8.x-3.x but of course this is a branch so I'll need to now rebase that branch on the latest 8.x-3.x.

The outputlist provided context is missing here? Same for the other contexts in the annotation?

The 'pos' and 'unique' contexts aren't required - I take it then I still have to declare them here. Thanks for the pointer.

stephenpurkiss · 2014-09-27T11:39:08Z

So I've updated my branch, added values for 'unique' and 'pos', but still getting provided context is not valid :(

stephenpurkiss · 2014-09-27T12:22:39Z

I'm just print_r'ing out the input and output to ensure they are arrays, seems to be all ok in that respect:

..............EinputArray
(
[0] => One
[1] => Two
[2] => Three
)
resultArray
(
[0] => One
[1] => Two
[2] => Three
[3] => Four
)
.................................................. 65 / 92 ( 70%)
...........................

Time: 664 ms, Memory: 21.00Mb

stephenpurkiss · 2014-09-27T12:36:34Z

So through debugging I can see it's just not returning a definition of the context 'provides' in the getProvidedDefinition function, it's just blank. My declaration seems ok so not sure what the issue is, keeping hunting...

klausi · 2014-09-27T13:28:08Z

tests/src/Unit/Action/ListAddItemTest.php

+      'item' => new ContextDefinition('string'),
+      'unique' => new ContextDefinition('boolean'),
+      'pos' => new ContextDefinition('string')
+    ]]);


'provides' is missing here.

ah ok cool thx

stephenpurkiss · 2014-09-27T13:48:22Z

Hmmm... OK so I fixed that last issue and managed to make it pass the tests but only by returning a value as well as setting the provided value:

$this->setProvidedValue('outputlist', $list);
return array('list' => $list);

stephenpurkiss · 2014-09-27T13:50:07Z

...which makes me think I don't need the setProvidedValue line then, I'll take that out & resubmit

stephenpurkiss · 2014-09-27T13:59:03Z

OK, so that worked ;)

stephenpurkiss · 2014-09-27T14:07:13Z

Forgot to remove my forcing values test code, removed now.

stephenpurkiss · 2014-09-27T14:13:10Z

Added two more tests, one to check 'position' option works, one to check 'unique' option works.

stephenpurkiss · 2014-09-27T14:19:07Z

I put all three tests in testActionExecution - not sure if they should be split up to a function for each? I see it kinda auto-finds tests as long as I have the @Covers::execute there so presume I can do things like testActionExecutionWithPosition, testActionExecutionUnique, etc.?

stephenpurkiss · 2014-09-27T15:19:40Z

Oh hang on, wrote a test for unique but not not checking unique. On way back to hotel so will write it there.

There's always something... ;)

stephenpurkiss · 2014-09-27T16:03:06Z

Non-unique test added.

fago · 2014-10-03T10:36:53Z

src/Plugin/Action/ListAddItem.php

+ * Provides an 'Add a list item' action.
+ *
+ * @Action(
+ *   id = "rules_action_data_list_add",


Should be rules_list_add.

changing to rules_list_item_add to keep in toe with rules_list_item_remove which has already been committed to 8.x-3.x

stephenpurkiss · 2014-10-10T19:35:04Z

Wondering if this is still wanted to be done:

@todo: Replace in_array() with \Drupal\rules\Plugin\Condition\ListContains

klausi · 2014-10-11T21:10:23Z

src/Plugin/Action/DataListItemAdd.php

+          array_unshift($list, $item);
+          break;
+
+        case 'end':


So the default branch and the "end" branch do the same? Then we can just use "default" alone.

Changed to an if..else

…17193 Issue #2317193 by stevepurkiss: Port "Add an item to a list" action to D8

fago reviewed Aug 21, 2014
View reviewed changes

klausi reviewed Sep 19, 2014
View reviewed changes

klausi reviewed Sep 27, 2014
View reviewed changes

fago reviewed Oct 3, 2014
View reviewed changes

klausi reviewed Oct 11, 2014
View reviewed changes

2317193 | Port "Add an item to a list" action to D8

1e071e6

klausi added a commit that referenced this pull request Oct 17, 2014

Merge pull request #102 from stevepurkiss/port-list-add-item-to-D8-23…

5217f8e

…17193 Issue #2317193 by stevepurkiss: Port "Add an item to a list" action to D8

klausi merged commit 5217f8e into fago:8.x-3.x Oct 17, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2317193 | Port "Add an item to a list" action to D8 #102

#2317193 | Port "Add an item to a list" action to D8 #102

stephenpurkiss commented Aug 9, 2014

fago Aug 21, 2014

stephenpurkiss commented Aug 28, 2014

klausi commented Sep 19, 2014

stephenpurkiss commented Sep 19, 2014

stephenpurkiss commented Sep 19, 2014

stephenpurkiss commented Sep 19, 2014

klausi Sep 19, 2014

stephenpurkiss commented Sep 19, 2014

klausi Sep 19, 2014

fago Sep 19, 2014

stephenpurkiss commented Sep 27, 2014

klausi commented Sep 27, 2014

klausi Sep 27, 2014

stephenpurkiss Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

klausi Sep 27, 2014

stephenpurkiss Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

fago Oct 3, 2014

stephenpurkiss Oct 10, 2014

stephenpurkiss Oct 10, 2014

stephenpurkiss commented Oct 10, 2014

klausi Oct 11, 2014

stephenpurkiss Oct 12, 2014

#2317193 | Port "Add an item to a list" action to D8 #102

#2317193 | Port "Add an item to a list" action to D8 #102

Conversation

stephenpurkiss commented Aug 9, 2014

Choose a reason for hiding this comment

stephenpurkiss commented Aug 28, 2014

klausi commented Sep 19, 2014

stephenpurkiss commented Sep 19, 2014

stephenpurkiss commented Sep 19, 2014

stephenpurkiss commented Sep 19, 2014

Choose a reason for hiding this comment

stephenpurkiss commented Sep 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenpurkiss commented Sep 27, 2014

klausi commented Sep 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

stephenpurkiss commented Sep 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenpurkiss commented Oct 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment