support for looping and data lists

I'm here, but like you I'm waiting for Fago to say his say :)
I'll look in the code some more to see if i have something smart to say, but anyway this feature is long time needed, so good job in doing it!

For the first issue - #260617: Expose hidden actions/ conditions

Second, why not something like this:
1. declare 'array of node' in the rule set itself.
2. The argument declaration will be something like this (need's adding a setting to rules API):

...
    'arguments' => array(
      'node_array' => array(
        'type' => 'node_array',
        'label' => t('Content which term will be assigned to'),
        'base_type' => 'node', // The 'base_type' will make sure all actions/ conditions of node arguments will be visible.
      ),

First off sry for chiming so late - I'm quite busy today. I've already thought about this feature - it's important. However I've not planned to add it for the 1.0 release - but of course, if you are willing to work on it, we could do that.. :)

First off, I'd like to get it *right* in the first place. So we don't need an "array of nodes" and "array of users" we need the general concept of an "array" or perhaps call it "list" as it might be more intuitive for users. Or "set"? So if we do it in a generic way, rules should automatically support lists of every supported data type.

Then the question is, how this should be usable. Your approach is interesting, but I think the variables availability problem can't be solved easily that way as you embed the logic in the action and is so unavailable to rules.

So to make this all work properly in rules, we have to integrate with rules variable system. So this is how I was thinking about a possible implementation:

* Do it like condition groups. Loops are handled (create, edit, delete) and represented like condition groups. I think of adding another point "Add a loop", if a list is available. Then the user selects the variable to loop over.
* For actions in the loop we get an additional variable. I'd reuse the 'new variable' property for that as far as possible - but of course the variable availability has to restricted to inside the loop.
* But it should be also possible to use the lists directly as required argument. But we need to find a notation to specify what is expected to be inside the list.
* Also internally loops work like condition groups. I don't think it's hard to adjust the internal rules storage and evalution to support them, but we have to adapt some variable parsing code - as then actions might be nested in loops. That's probably the most difficult part - adapting the code that lists available variables.

@notation:

We could borrow the syntax from java and use "set" or "list" as 'type' and implement it in a single data type related class set/list. So we get compatibility for set == set for free. So we would have to add only support for the case something provides or needs a mixed set and we have a specific set and the other way round - which is easy to do.

I agree, list is probably the better choice.

--> So we could use list, list or just 'list' to deal with lists with unknown content.

Perhaps we should implement this in two steps?

First implement it in the engine, let's add the list data type and its special treatment (<> support). Then add a new element for the representation of configured rules of #type 'loop' and an execution callback that adds, sets and removes the loop variable properly. Then test and verify it's working by adding a simpletest for a rule that makes use of it. Once this is working, we can commit part #1.

Then as part #2 I'd tackle the changes UI, which should no big deal either once #1 is done.

Starting to review. Here's the testing module Gzipped for convenience.

Your programing Kung-fu is strong, and it makes me want to practice :) It's good to have you in the Rules issue queue!

rules.admin.inc
- When adding a loop it asks for the loop list ("List over which to loop"), but then the same list is presented in the form as an argument. So i think the first step isn't needed. so maybe we can get rid of rules_admin_form_add_loop().
- Out of scope of this patch, but related - You are using, as fago suggested, the 'new variables' to be able to identify the looped actions. I think we should add a 'hidden' => TRUE to 'new variables' for two reasons:
1. The new loaded variable isn't important to the user, only the argument is.
2. It creates another token group.

I hope the rest of my review process will be quicker, this one took me 1.5 hour, but hey I'm at least trying ;)

I understand the logic, one question is why do we need two variables "random list of users" and "content loop variable".Can't we let the user to create her actions using the on the "random list of users" argumen?

Minor about rules.admin_rule_proxy.inc
- Maybe instead of 'starts scope' we can use 'starts loop'. I think scope might be out of context.
- Typo - "The scenrario is determined by the $id parameter" => scenario.

Ok thanks, all my questions were answered. sorry about the typo thingy :P
Now let's see what fago has to say.

wow, this is really cool! Great work... You even already did views support, awesome!

Anyway, let's discuss the details:

>(The "scalar" and "list" terminology is borrowed form Perl.)
I like that. :)

Phase 1

>As can be seen, I didn't pick this route. A "list notation" isn't extensible: tomorrow a programmer would want his argument to match either foo or bar types. How would the "list of xyz" >notation serve him? Instead of notation, we can put flags on the type-info. A 'list' => TRUE flag identifies all list types. (This scheme can be generalized.)

Hm, I think I don't understand you right here. Perhaps you can try to explain it better for me?
Let me point out my idea:
* The notation I proposed earlier is: list, list, list<..> or even just list.
* Whether we call the lists user_list or list doesn't make a big difference, but the difference is that we need no special property list => TRUE. So 'list' can be treated as usual data type, allowing to do even list
.
* Having support for just the type "list" also enables us to do actions like "Get me the first entry of the list", "Get the size of the list" and so on.
* We can easily enhance the type matching to let list match list - this enables to write actions that operate on lists in a generic way.
* Later on I can think of introducing a kind of type inheritance. So we could use this to let list inherit from 'list' - this would just be applied when doing type-checks. E.g. use case: "Organic groups nodes" could get the data type "group" inheriting from "node" - that way we can write actions that work only for group nodes while usual node actions still are available for groups.
* Adding a 'plural label' to data types and automatically providing generating list .. types looks good to me.

Phase 2: Forms for adding/editing a loop

agreed

Phase 3: Variables scope

First off, why do you think we should define the scope explicit with "#starts scope" ? Couldn't we just go with the simple rule: Nested elements have their own scope. So if their is an element, which has several child elements, the child elements operate in their own scope. This would apply to loops and condition groups too, but as conditions can't add new variables it doesn't matter.

@proxy code: I had no close look at you variable parsing code in the proxy class yet. Indeed you've found a bug there.. :) Does your code deal with nested lists too?
However I don't understand what you mean with the two use cases of get_available_variables(). It's used to get all variables that are available to an element - for getting all defined elements to check for variable name clashes get_defined_variables() is used.

Phase 4: Executing a loop

You use rules_save_variables() although you don't want to save anything - so you get the troubles with 'changed'. I think instead of that, we should factor the code of adding new variables out into an own function, which then gets called from rules_save_variables() and the loop-execution.
Update: see comment below

I agree that new variables introduced in the scope shouldn't be available afterwards. So to do that, best unset() the variable. I'd just go and unset the loop variable after each iteration, then you also don't get into troubles with saving and we don't need that '_changed' flag.

@loading identifiable vars:
This is really similar to rules_pack_variables(). Best refactor that to rules_unpack_variable() to get the data of a single variable for a given info + data. Again both rules_pack_variables() and your loop execution could then rely on that.

Phase 5: UI

fine :)

I still think that this are quite a lot changes, so please let us do two steps:
First off let us concentrate on phase 1 + 4 and add a simple test to make sure it's working. Once we have that we can commit this and proceed with the second step:
Concentrating on the UI and the necessary changes to the variable parsing system.

@list generator:
Doing views integration is just awesome. I think we should add support for mapping views base tables to rules data types - so then we can write the action "Produce a lost of something" in a generic way :)

@Phase 4:
rules_save_variables() isn't so bad, perhaps it's better to just stay with that, but clear the old loop variable before calling it again. This makes sense as two different nodes need to be treated as two different variables. Then also saving is fine, if an action inside modifies the node.

I thought a bit more about the looping support. I think this issue is about two things: loops and lists. While it makes sense to have a loop to loop over the list, there are other types and possible applications of loops.

So I think we should implement the loop a bit more generic. Add it as an element on the same level as condition and action and so also add hook_rules_loop_info() to allow adding further type of loops. So we can add add a foreach loop, but also a for() and a while() loop.

Then the execution could work like that:

function pseudo_execute_loop(..) {
  while (($return = $loop_callback()) !== FALSE) {
     // add in loop variables
     rules_save_variables($return);

     // execute children elements

     //handle each loop variable
     save_changes
     unset($state['loop_variables']);
  }
}

then the foreach callback implementation could look like

function pseudo_foreach_loop($list, $settings) {
  $settings += array('counter' => 0);
  $settings['counter']++;
  if (count($list) >= $settings['counter']) {
     return array('loop_variable' => $list[ $settings['counter']-1 ]);
  }
  return FALSE;
}

This way we could easily do a loop like for($i = 1; i < 10, $i++) or even while() loops.. :)
Furthermore we could support adding #conditions inside of loops - which shouldn't no big deal either for the evaluation code nor for the UI.

What do you think about that? :)

@#32: that makes perfectly sense.. Indeed this is a new case we need to consider now. thanks for your explanation

@#33: yep, however this will be only a problem with big loops. It's not possible to deal with this in a general way in rules, but we could over a different type of "loop", which makes use of batching or something similar. Of course this would only work right, if the rule is invoked repeatedly.

@#29:

The current mechanism, of hook_elements(), seems more generic than a possible hook_rules_loop_info(), doesn't it? It gives the programmer the most freedom (having your own #execute and #theme). If anything, while writing the patch it's hook_rules_element_info() I was thinking about (but I think this too isn't needed, once all hardcoded 'action' (as in the proxy code) are removed).

Indeed, however then we would have a different type of element. This would require new code for handling the UI - so we can't add this so easy and modular. But I think all different type of loops should be represented the same way in the UI - so doing a single #type "loop" with different implementations would fit good. Different types of loops mostly differ in the loop variable the offer and the condition they evaluate over. I think even we could even do a generic while() which supports selection an arbitrary #condition that way.

If we need different theming and UI code a different element would suit yes, but for different loops I think it's not necessary.

(That's why I originally entitled this issue "foreach", not "loops". The element should really be named "foreach".)
agreed.

We my also want an 'if' element.
hm, I'm not sure about that. Somehow a whole rule is an "if element".

We probably don't need a "for(;;)" loop. Scripting languages nowadays "prove" that a "for(;;)" loop isn't crucial. Lanuguages like Python and Ruby have only "foreach". However, they do have "while".

Hm, of course when there is a while it's not necessary to have a for. But it might still be useful - as it would be easier to do by just adding a for loop as doing it with a while(), introducing a count variable, incrementing it...

@#30:

Couldn't we just go with the simple rule: Nested elements have their own scope.

In our case, we can't go with this proposed simple rule. Because:

"#starts scope" has one additional purpose besides scoping the child element's variables. It also scopes its own "new variables". Actually, I'd say that it changes the meaning of its own "new variables": instead of them functioning as a return value (to borrow terminology from programming languages), these "new variables" are now functioning as local variables accessible to children only.

The downside is that a loop cannot return a value. (Because elements following it don't "see" its "new variables").

hm, no I think the different behaviour of "new variables" is already caused by the different element type - it's an loop and no action. So the new variables are loop variables. Doing so we could go with the nesting rule.

but stating it explicitly couldn't harm, so we could use a flag. Perhaps #new scope would fit better - as it's both the start and the end of the scope..

@#30:

You could design some notation to answer the "inheritance" need, but this notation may not answer other needs.
No I wouldn't encode this information in the type string. That would be a separate thing which could types state, like 'parent' => 'node'. Anyway, inheritance would be optional, we could just start with 'list' and hardcode the typematch of list to list.
Inheritance would be just be a generic approach to do the typematch. As of now I think of "inheritance" just as "type matching" - I wouldn't inherit any properties or so. That could be covered by real-php-inheritance of the data type classes anyway.. ;)

For example, what if tomorrow a programmer would want his argument to match either a node or a user? How would you express that using your string notation?
That's already possible by using array('user','node') as type. But I don't think it makes sense to support list - so I think it's ok to not support this.

@predicate:
This is an interesting idea, but it has some drawbacks:
* It's not known by the people so difficult to grasp.
* It looks like it is complicated to implement. Let's don't over-complicate and over-power rules, but do that what's needed to get a powerful, but not too complicated module.. :) I think that type-inheritance would suffice for type-matching and it's quite easy to implement.

@inheritance, list - list match supprt:
Basically changing rules_admin_is_valid_data_type() would suffice here. When we would do inheritance we would have to parse the parent-child relation and build a simple compatibility look-up-array - statically cache that and use it for the matches. That should do it.. :)

marked #331612: Load user reference fields with multiple values as a duplicate.

Hey there, this seems really promising, is there any chance this can be added to a release so more people can test it? I have the need to load several referenced nodes with cck node reference, I think this module would solve my issue no? Do we still need patch reviews here?'

Patchak

Just to be clear : I tried to test a patch, but I realised I don't have any idea which one to test now, seems this seems to be in flux. is there any way to provide a patch will all new modifs or to add it to the dev release so more people can play with this?

thanks a lot

Patchak

@patchak,
Indeed this feature seems promising, but it still needs some work.

Marking this issue status as per Amitabu's post.

If anyone is currently using this code, please post your experiences. I'd be very interested, and I bet the developers would get some use from the feedback.

I'm going to set up a test env this weekend and hopefully I can put everything together into one easy patch or tarball; or if it works well, I could make a separate project.

@fago, Amitaibu: would a public demo be helpful in getting this back on your radar screens?

>if it works well, I could make a separate project.
Please don't do that, this can't be split out in a separated module well.

If you want to get this in let us first get the engine modification + simpletests in - then proceed with UI and variable parsing. This is no simple task, though - the more important it is that it's done properly. If you are willing to work on it, I can summarize the outcome of the discussions of this thread.

For me it's now priority to release 1.0 and get the basics right, so for now as nobody works on it, it is postponed. This is on my TODO list for later though.

Any updates on this? Is it possible now to load multiple referenced nodes?

Please see #371728: Load Multiple Nodes to perform actions on them for a very similar code snippet.

subscribing

fago,

Interesting.

I wrote this other node: #490920: Actions, conditions and order... and looking at this long post (#49 here!) it looks to me that this is definitively needed, too.

My idea was to have actions & conditions mixed and possibly "branches" (if condition then actions [else actions] end if).

What I see you saying in #8 is something I like. Have a foreach() type of instruction that repeats a set of actions with a given variable. The fact that some actions could support the list as is... neat, but it means adding support that is not really required. On the other hand, it could be a lot faster for large sets (Oh! Sorry! large lists... 8-) .)

Having the conditions & actions mixed in a single long list would solve several problems in the foreach() that you could write in some other way: Repeat[define a list], Actions, Loop[back to the corresponding repeat]. The Loop is a condition!

Thank you.
Alexis Wilke

fago,

What's the status of this work?

It sounds like you were hashing out some of the design principles to get it done "right", with mooffie and then he dropped out.

I'd like to help with getting items #1-#4 done, as per your plan. Can you provide an update on the current status, provide any sandbox code that may exist, and lay out your design principles?

I've got a need for this functionality and I've got time to work on it. I just need to know where you are in terms of design so I can contribute!

This will be supported with rules2, which I'm currently working on. I will probably open the devel code for public review soon, but it's not ready for that yet. In the meantime you can only help, by helping out in the rules 1.x queue so I have more time to work on rules 2.x ... ;)

Glad to hear this will be supported soon!

I am wondering if the issue I have is a totally different request. Here's the pseudo code of what I'm trying to do:

if node a of type A is created
for each value v in the node a's multi-valued date cck field
       1. create a node b of type B
       2. set node b's cck field to to v 
       3. set node b's nodereference to point to a
end

It works great with a single value. Seems that it shouldn't be as difficult as the "loop through nodes" situation since there is not the issue of which node to refer to, but it still requires the "for each" logic.

(in plain english here is the full description: I am creating projects, each of which have any number of "budget years ". Each budget year corresponds to a budget node that is linked to the project through a nodereference. I want the budget nodes to be created automatically when someone creates a project and adds one or more budget years in the project's "budget years" date field.)

Any insight on whether there would be a way to implement this?
Thanks!

@fago
Hi!
Speaking of rules2, I was wondering if we can expect a testable release anytime soon, so we could use and test this great feature?

Thanks for your work!
Patchak

Subscribing...

subscribing

subscribing...

Any news?

#57: http://drupal.org/cvs?commit=311226

Yep, it's already implemented in rules 7.x-2.x.. :)

A lot of thingie are moved to D7 :-(

Is this also being added to the 6.x version as well?

subscribing

subscribing

subscribing

subscribing

i would also like to see this for 6.x

nuooooooooooo not Rules 7.x! Looks like I'll just have to make my own hook_cron. Ah well...it's not that hard anyway. I mean I guess I could write my own rule to loop through some certain nodes, but my hook_cron is better than my Rules skills.

Yet another vote for this to be included in 6.x

subscribing :)

Adding this to 6.x would make me a very happy person :)

please add!

Will this ever be added to 6.x
any update is helpful so we can move on...

Another vote for this in 6 please. Seems backporting to 6 is low on the agenda for a lot of modules including this one - and yet 7 is immature and has many modules not yet properly implemented. Which leaves a lot of folk in no mans land.

One more for backporting to D6 !