Allow modules to alter the list of possible next states

Thanks for the interesting idea muriqui.

What do you think is the use case for this hook? That will make the patch easier to test.

Thanks for the explanation muriqui. I like where you are going with this. Before releasing Workbench we discussed a similar use case. We discussed a hypothetical site that would need the states of "editorial review," "PR review" and "legal review." I pointed out that for some clients this might not be a linear process and that those three states do not follow a specific order. In some cases it may only matter that those three departments have reviewed a node and "signed off." I think with this hook a site could set up arbitrary rules like that so a node can't be published without entering those states or perhaps those are just separate checkboxes as fields.

I think I just rephrased everything you said but I want to make sure we're on the same page.

What I like about your approach is that the mechanism is arbitrary and as you said, OG, TAC, even Workbench Access could hook in.

However there could be a downside to the arbitrary nature. I would be concerned if all of those modules implemented completely separate UIs for controlling transition states. I'd like to look into using the access functionality in CTools allow or deny a transition. Attached is a screenshot that might help.

I have used CTools access plugins a lot with Panels. I have even written some custom ones. However, I don't know what is necessary to do this integration I'm suggesting.

One of the reasons I like this is that it requires Organic Groups to write no new code at all. OG already has CTools access plugins so if we got this working OG would integrate immediately with no unique attention.

How does this sound muriqui? Have I taken your idea in a completely different direction? I think if this hook got into WB Moderation then all the CTools integration could be a separate module if we want to keep WB Moderation simple.

Yes idea behind the CTools integration is that OG or any other module wouldn't need to do anything specific to Workbench. CTools access plugins accept arbitrary "contexts" and other variables if necessary and then return TRUE or FALSE. So there is an OG CTools access plugin (at least there was in D6) that takes as the "context" of a node, and I think the context of a user (or it just uses the current user), and then returns TRUE or FALSE for if that user is in that group node.

I have used that plugin to control access to a panels page. The path to that page was something like "example.com/foo/bar/%nid" So between Panels and CTools that %nid was converted to a node object and that node object was passed to an arbitrary number of CTools access plugins one of which evaluated whether the current user belonged to the group and returned TRUE or FALSE accordingly.

The important part is that the OG CTools access plugin doesn't need to know or care if it is controlling access to a page like foo/bar/%nid or is control access to the next state of workbench moderation.

So to answer your question, other modules could provide a CTools access plugins or they could implement the WB Moderation hook directly.

Hi again muriqui. The CTools plugin approach would depend on the hook so perhaps the hook first. I'd like the opinion of another Workbench dev.

We got a similar request in #1127524: New permissions

Testing the patch in #1

Working nicely for my custom access module

any news on this?

I'm sorry this issue hasn't gotten much attention lately. Bug fixes are getting prioritized over new features.

I just marked another issue as a dup of this one: #1190256: Extensibility: Collective /IMC /group moderation; rules for more editors having to decide together

Subscribe.

I want to get this hook in soon meet the functionality discussed in #1294880: Convert Workbench Moderation records to entities

Muriqui, just a few minutes ago I was moving an issue from Workbench to Workbench Moderation and the autocomplete widget brought up http://drupal.org/project/conditional_roles_workbench_moderation It looks like we're trying to solve a lot of same problems.

Perhaps we can discuss in #drupal-workbench soon.

#1: 1108502-hook_workbench_moderation_states_next_alter.patch queued for re-testing.

Hmm. Think I just found a bug in the test system. Trying to submit a completely different patch, somehow ended up here. Disregard.

#1: 1108502-hook_workbench_moderation_states_next_alter.patch queued for re-testing.

We're still having problems with the deployment of project_dependency on the testing infrastructure, and I suspect that workbench's dependency on views may have played a part in our most recent testing lock-up.

As a result, I'd like to try leaving this issue out of 'needs review' state until we get the project_dependency and pift patches deployed on d.o. (hopefully within a couple days) ... apologies in advance for the hassle, and thanks for your understanding.

Fingers crossed!

sorry to bother but... I'm wondering too :)

#21: 1108502-hook_workbench_moderation_states_next_alter-2.patch queued for re-testing.

+++ b/workbench_moderation.moduleundefined
@@ -1326,14 +1326,18 @@ function workbench_moderation_transition_delete($transition) {
+  global $user;

This addition of the global $user doesn't make sense to me.

I didn't look at the code just the patch. I don't see any use of the global $user object in the patch so I'm confused why it's been added here.

Besides that I really like this patch. I think it'll be a good addition it'll also help me solve some issues I'm running into.

+++ b/workbench_moderation.moduleundefined
@@ -1326,14 +1326,18 @@ function workbench_moderation_transition_delete($transition) {
+  $node_type = is_string($node) ? $node : $node->type;

I would also say lets get rid of this line. Lets just change it to $node_type = $node->type;

The one place left in the code that doesn't pass the $node object is in the workbench_moderation_form_node_form_alter function and that can easily be changes to pass in $form['#node'] instead of $form['type']['#value'].

The reason for this is that passing in the type will make us have to also use the same code in the alter hook and constantly check if we're getting the $node object of just a string. To prevent us from assuming the node is always being passed. Instead we should just fix all the places the node object isn't being passed in.

Anyone disagrees?

$node_type = is_string($node) ? $node : $node->type;

I've seen this pattern in node_access_hooks in other modules, and indeed, sometimes it is a string.
Don't know why drupal does this.

So before changing it, better make sure it wont break things.

Here's an updated patch with the above comments.

Here's an updated patch with the api example updated.

I really like this patch because for now it lets me at least fake different states for different node types :).

Thanks for working on these issues Eric. I'm sorry I've been slow to get them reviewed. I should be able to get to this one next week. Just reading through it, it looks good.

I worked on a module to integrate Workbench Moderation with OG (so that the change of state permissions can be set for OG roles), and in the process had to be able to hook into how WM check the permissions for the change of states.
I went a (very slightly) different way, adding the hook into workbench_moderation_state_allowed rather than workbench_moderation_states_next.
I realised it'd silly to have several patches to solve the same problem and would like your opinions on the advantages/drawbacks of the different approaches (to know if it'd be better to change my code to use the previously submitted patches rather than mine).

I used the last patch and it work well,

I think adding hooks to alter state_allowed is more successful from add hooks to states_next , becouse states_next relies on state_allowed.

just one comment to the patch: I think that the entire node object should pass to the state_allowed function and it's alter hooks, not just the id.

The patch in 31 works well for me. Thanks Eric! And thanks again to muriqui for starting this issue.

Thomas, I understand that altering on _allowed() also works. However I don't see an advantage to it since it is only called from _next(). So for me it boils down to one drupal_alter() call versus "n" calls with the _allowed() approach.

If there's not a strong argument to alter on _allowed(), I'll commit the _next() version.

That's independent of the node type vs. full object question that avinoam mentions. I agree that the full object is necessary.

Thanks for the hard work everyone.

This broke because I just committed a Rules patch which also modified the API page. I'll post a revised patch shortly.

This should pass tests. I'll then RTBC and commit.

Commit coming momentarily.

Thanks muriqui, Eric and everyone who helped and tested this.

http://drupalcode.org/project/workbench_moderation.git/commit/b04fbf7ccf...