condition to filter by user role

yep, I also have already noticed that token doesn't support user roles.
I'll either include token support or write a condition so that it's possible to restrict actions by user role.

Here's a patch that will fulfill this requirement. It also:

• Provides actions to add/delete user roles.
• Adds numeric conditions.
• Adds a comma on the end of some array items.
• Rocks.

jeevesbond,
Checking your patch:
* Test if user has a role - rename to "Test certain role(s)" . (Since all users have a role, even if they are admin).
* Test if user has a role - can roles be multiple selection?
* Numeric comparison - Cool. maybe also add "Equal or greater"; "Equal or Less". It can be done with two conditions, but I think it is less user friendly.
* You rock! :)

Point by point:

• Good idea: I renamed it, can't remember what to now, but it makes more sense now.
• Multiple selection has been added, with the option in the criteria to match one or any of the selected roles.
• Not sure whether 'equal or greater' and such should be added. It complicates matters, and number of features is not proportional to user friendliness. :)
• Lol, thanks very much. :)

So: scratch that previous patch! Attached is newer, funkier one! ;)

great!

@numeric comparison
I wondered if we should cast the variables to floats (float)$var before doing the actual comparison - at least for == it wouldn't be a numeric comparison else. I don't know how php behaves with in case of strings, however a cast couldn't harm I think.

@equal or greater: Like you prefer, it can be done by combining some conditions nevertheless.

@language: "Test certain role(s)" - I don't think it's a good idea to include "test". Most conditions are tests so we would end with "Test content type", "Test .." - so I would prefer "User has role(s)".

@condition userhasrole:

• + $roles = (array)$settings['role_select'];
  Why the cast? And you could just call it $settings['roles'] - a bit cleaner I think.
• Instead of $role_found just return the result, as soon as you know it - then the variable is useless.
• "anyorall"
  Please use 'op' or better 'operation' as you have done with the numeric comparison. you could use values like
  array('OR' => t('any'), 'AND' => t('all')) - to get clear UI and code.
• $role_key
  In drupal this is usually called $rid, perhaps just use foreach ($roles as $rid => $role)..
• } elseif
  obey drupal code style, use

  }
  else if {
  ..

That's it! Let's clear this small issues out and get it committed :)

@numeric comparison
I wondered if we should cast the variables to floats (float)$var before doing the actual comparison - at least for == it wouldn't be a numeric comparison else. I don't know how php behaves with in case of strings, however a cast couldn't harm I think.

Sounds like a good idea. I've put a cast in front of each function call, it seems to be working: is this allowed in PHP though? I checked the online docs and they only show casting a variable. Just one of those things I've never had to do before!

@equal or greater: Like you prefer, it can be done by combining some conditions nevertheless.

Let's just commit as-is (if that's alright with you) and see what the demand to have >= etc. combined into the selection is.

@language: "Test certain role(s)" - I don't think it's a good idea to include "test". Most conditions are tests so we would end with "Test content type", "Test .." - so I would prefer "User has role(s)".

Yep, good point. That's been changed.

@condition userhasrole:
+ $roles = (array)$settings['role_select'];
Why the cast? And you could just call it $settings['roles'] - a bit cleaner I think.

The cast is there for when the user selects only one item, PHP returns a number instead of an array. Thought the code would be more readable with a cast instead of another if statement.

Instead of $role_found just return the result, as soon as you know it - then the variable is useless.

Good point, that code is about as readable as Ancient Egyptian at the moment. Have rewritten it as a switch case statement, it looks far better now.

"anyorall"
Please use 'op' or better 'operation' as you have done with the numeric comparison. you could use values like
array('OR' => t('any'), 'AND' => t('all')) - to get clear UI and code.

Changed.

$role_key
In drupal this is usually called $rid, perhaps just use foreach ($roles as $rid => $role)..

} elseif
obey drupal code style, use
}
else if {
..

Changed and will bear in mind for future. :)

That's it! Let's clear this small issues out and get it committed :)

Hell Yeah(tm)! I've attached another patch, haven't had time to test it as thoroughly as the last one so give it a try before committing it! Think I got everything on your list, if something is missing, let me know. :)

The anonymous problem: try using a different event, you're using: 'Invoked on event: Content view by a logged in user'. Which will not trigger the event for an anonymous user. Anonymous means not logged-in in Drupal. :)

I see it also shows the message for Admin, but it should show only to Anonymous and authenticate

I believe an Administrator is an authenticated user, that's why the message will display (in fact I believe anyone who's logged-in, ergo everyone but Anonymous, has the authenticated user role). You should probably pick a more specific role, or just live the odd message popping up. It's an advantage of Drupal that it has a role which covers every logged-in user in my opinion. :)

If you get any more problems do let us know though! It's better to be advised of a configuration issue than miss a bug. It's also cool to learn how people use stuff.

If I'm wrong about anything am certain fago will correct it, he's the final authority: I'm just his patch-tart. ;)

yes, indeed great respond speed!

regarding the anonymous issue, read http://drupal.org/node/164170

@the patch:
It already looks a lot cleaner! However, some points you mentioned that you have changed are still in there? Perhaps you have uploaded a wrong version of your patch? ($role_key, Test user roles).

Then your role selection form isn't #multiple as you have written above?
Probably this is because you need to cast the roles to an array - because if #multiple is set the value should be always an array.

jeevesbond,
Maybe for the 'user friendliness', it is possible to show the relevant options, according to the chosen event.

I'll do that, if you pay me lots of money. :) Seriously though, that's easy to say but the framework doesn't (and probably never will) support it. It's too much work for what we call a 'fringe case' I'm afraid. You could raise another bug for it though, it would mean the condition would somehow have to be aware of the nature of the event, any thoughts fago?

I've had another go at a patch fago. Am really tired at the moment so am making lots of mistakes, thanks for being patient. I literally fell asleep in my chair after uploading that last one! :D

#multiple in forms, didn't know about that (apart from when creating a select element), have used that instead of a cast, tested and all seems well.

Maybe a nice feature would be to allow to change the Event.

That would be desirable, are you going to create a new feature request? Personally I'd like to see multiple events for the same set of conditions/actions, I messed about with the code a little, but the system pretty-much assumes a single event per-item throughout; I gauged it not worth the effort (for the moment).

Whoah there Skippy! I've made another mistake! Didn't do a CVS update before creating that patch, here's a new one. :)

patch attached....

thanks, good work! -> committed :)

Sry for confusing you with #mulitple, I was wrong in this point. I've overlooked that you have used checkboxes, and not a select. I meant just the #multiple property of a select. Nevertheless the (array) cast wasn't needed for checkboxes too.
So the form #multiple property doesn't exist - I removed this line.

@event changing:
I thought about this, but this isn't practicable - because changing the event would change the available arguments. As an affect the configured actions/conditions aren't valid any more. So no way to do so - sry.

Configuring multiple events would be possible - however then only the intersection of the available arguments would be available for configuration. So I'm not sure if this would be so neat.