Problem/Motivation

I'd like to be able to pop off items one-by-one from a token list (without using custom events).

Steps to reproduce

Proposed resolution

  • Add an action "Remove item from list" that pops off the last item of a list and stores it as a new token. The action may provide an option to either pop from the end or the beginning of the list, or event may provide a key input that allows removing a certain delta / index.
  • Add a condition that checks whether a list is empty (or just use the existing list count action, guess that one is already fine)

Remaining tasks

Discuss details, implement, test.

User interface changes

API changes

Data model changes

Issue fork eca-3300517

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mxh created an issue. See original summary.

mxh’s picture

Assigned: Unassigned » mxh

Think I can just try realizing the suggested action.

mxh’s picture

Issue summary: View changes
mxh’s picture

Issue summary: View changes

mxh’s picture

Assigned: mxh » Unassigned
Status: Active » Needs review

Added two new actions for list operations, and a new condition plugin "Token: exists". Using these components, I'm able to iterate over a list without using custom events.

Had to refator the DataTransferObject a bit and also fixed a possible infinite loop for dependency calculation when creating a cyclic graph representing a loop in BPMN.

mxh’s picture

Assigned: Unassigned » mxh
Status: Needs review » Needs work

I've found another possible action plugin that may be useful regarding list operations: Putting an entity onto a list. Will add that into this MR.

mxh’s picture

Assigned: mxh » Unassigned
Status: Needs work » Needs review
mxh’s picture

The only thing that could be cherry-picked for 1.0.x from this MR is the following commit:
https://git.drupalcode.org/project/eca/-/merge_requests/209/diffs?commit...
That includes the prevention of an infinite loop during dependency calculation.

All others are feature additions and therefore belong into 1.1.x.

jurgenhaas’s picture

Version: 1.0.x-dev » 1.1.x-dev
Status: Needs review » Needs work

This is great stuff, well done!

Just a couple of comments that need to be addressed/discussed.

mxh’s picture

Assigned: Unassigned » mxh

Will have a look

mxh’s picture

Assigned: mxh » Unassigned
Status: Needs work » Needs review

Setting to review for continuation discussing the last note.

jurgenhaas’s picture

Assigned: Unassigned » mxh
Status: Needs review » Needs work

Added another comment to the discussion.

mxh’s picture

Assigned: mxh » Unassigned
mxh’s picture

Status: Needs work » Needs review
jurgenhaas’s picture

I have now added test cases that should demonstrate the use case I wanted to describe. It's in fact a great idea to do that with tests instead, that should be much more clear.

mxh’s picture

That's great, thanks for that. Tests are green on my system, which looks like both your and my expectations are met by the current implementation - unless the test coverage is not complete yet (?)

I think you need to enable test runs for the 1.1.x branch.

jurgenhaas’s picture

Sorry, my bad. My test case was still incomplete. The intent was NOT to test for existence of placeholder but to test for the token name that [plaveholder] represents. I've now added 2 more tests and one of them should assert False but does assert True.

Regarding the test runs, I'll add them right away.

mxh’s picture

Status: Needs review » Needs work

Setting to needs work, now that a test is failing.

Let's look at the test that is currently failing:

<?php
$condition = $this->conditionManager->createInstance('eca_token_exists', [
      'token_name' => '[placeholder]',
    ]);
    $this->assertFalse($condition->evaluate(), "Placeholder should not exist");
    $this->tokenServices->addTokenData('placeholder', 'mytoken1');
    $this->assertTrue($condition->evaluate(), "Placeholder now should exist");
    $this->tokenServices->addTokenData('placeholder', 'mytoken3');
    $this->assertFalse($condition->evaluate(), "Placeholder should not exist"); // <- This one is failing
?>

The reason why this is failing, is because

<?php
$this->tokenServices->addTokenData('placeholder', 'mytoken3');
?>

is adding the string "mytoken3" as token data for token name "placeholder". The condition then says "Yes, it exists" because there is token data, in this case a DTO holding "mytoken3" as string representation value, and token replacement would replace "Thevalue: [placeholder]" to "Thevalue: mytoken3".

mxh’s picture

Would changing the above mentioned line to

$this->tokenServices->addTokenData('placeholder', $this->tokenServices->getTokenData('mytoken3'));

do what you want to reflect here as a use case? As an alternative, you could also make use of an action that represents your case, like "Token: set value". But I currently doubt that the current test case is properly reflecting your targeted use case, as it's adding a string to the Token stack as described above, instead of representing another token name.

jurgenhaas’s picture

I'll explain my use case in a second, but before that, I try again explaining why I feel the changed treatment of the value in token_name feels like the natural one. As a user, when I have to provide a value in a field that asks for a name, I can either provide a static string or I can provide a variable which contains the name. And that's what I think this field needs to do: if the given value is a token (i.e. the variable), then that token's string value is the name; otherwise the given string is the name. And once we have the name, we can then check if a token with that name exists.

Now, talking about a use case: we have a model with lots of different events and processes, each of which working with different tokens like node, taxonomy term, comment, etc. All those tokens are named according to their entity type for better clarity and to avoid conflicts if one of the processes deals with more than one. Each "branch" in that somehow bigger model defines a token called type which declares the entity type it deals with, it's the same as the token name it deals with.

Each of those "branches" calls the same single action at the end, which starts evaluating if user messages have to be produces and what type of message was required. This wants to test if that token is available, either the node, the term or the comment. We could check that with [type].

mxh’s picture

Reading #21, this looks like you expect the "token_name" input to support tokens. Currently, none of our plugins supports token replacement in that input, and the new condition in this MR would be the very first one supporting it.

I'd recommend to see this as a separate feature request, because for that we'd have to look at all other plugins making use of "token_name" and change the overall behavior to keep consistency accross all plugins. Cleary an issue on its own.

jurgenhaas’s picture

Status: Needs work » Reviewed & tested by the community

Agreed, let's address this in #3302569: Add token support to fields "token_name". Removed my extra test, which should now allow all tests to pass again. Will merge this, when the tests completed successfully.

  • jurgenhaas committed 4616617 on 1.1.x authored by mxh
    Issue #3300517 by mxh, jurgenhaas: Provide a mechanic for popping off...

  • jurgenhaas committed b5daf04 on 1.0.x
    Issue #3300517 by mxh, jurgenhaas: Provide a mechanic for popping off...
jurgenhaas’s picture

Status: Reviewed & tested by the community » Fixed

Merged the MR into 1.1.x and cherry-picked the dependency calculation bug fix into 1.0.x

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.