Provide workflow-ng integration

now this is a GHOP task
http://code.google.com/p/google-highly-open-participation-drupal/issues/...

Great idea, thanks!

Ok, I've checked a bit more and I saw no one claimed this from GHOP, so I decided to give it a shot.
If it does have a problem with the GHOP policy then I guess you can remove this...

I've implemented the required actions.
two (related) comments:
1. AFAIK, currently in ACL there are no functions to check if node/ user is already in ACL.
2. When 1 will be implemented, it will be also nice to add conditions to check if user/ node is in ACL.
3. I didn't understand the "acl_enabled()", so I guess I need some tips about this part.

small fix

I've pretty much copied from content_access module the acl_enabled() . I admit I don't really get this part, but I'm working on it ;)

I didn't try patch yet, but will following change work with table prefixes or at all?

I had an idea that I've implemented in the 'add user to ACL' action, which is adding another field for arbitrary user.
Maybe we can also enter radio buttons that user will select if input is name, uid or email.

If you like/ approve the idea I can implement it for the other actions as well.

Sorry for comment #6, I found where this change comes from (http://drupal.org/node/168750).

There is action "delete whole ACL(s) from node" which will delete node from all ACLs.

It will be good to have action: "delete a whole ACL" which will completely delete ACL specified by name. Not only remove node from ACL.

Otherwise patch from #8 works for me. Thanks.

I've added 'Delete ACL' action. Not sure if I used the node_access_rebuild correctly there.

fixed Typo.

When using action 'add user to ACL' and 'action add node to ACL' with latest patch, ACL name in table {acl} is empty and node is not added to ACL.

Had a typo there, should be ok now.

This patch includes feature to Add arbitrary user to ACL (in 'Add user to ACL' action). Admin can decide if the input type is UID/ name/ Email.

Deleting of ACL works good.

It seems that your patch is overwriting changes from versions 1.1.2.12 to 1.1.2.14. (http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/acl/acl.mod...)

In that way there is for example missing warning when module is disabled. (#150106: tell the user to rebuild permissions after disabling ACL)

Oh man, that's embarrassing for me - I patched it at the wrong release. Sorry!
Here's the code against 5.x-1.x-dev

Sorry, but patch is still removing changes from latest version.
If I understand correctly, in acl.module file you have to add only code for include of acl_workflow_ng.inc file.

Did you patch against DEV version (not 5.x-1.5)?

Attached patch against latest cvs code (DRUPAL-5).
I removed following functions from file 'acl_workflow_ng.inc':
- acl_enabled()
- acl_disable()
- acl_disabling()

"acl 5.x-1.x-dev" and "acl 5.x-1.5" are identical. (http://drupal.org/node/96991)

Did I mention that I'm a newbie developer ;)

FYI, Seems to be some problem in the 'Add node to ACL' with the access permissions checkboxes. I just can't understand why sometimes the form isn't updated correctly (i.e. it's updated with old values)

Yet another try. Fixed the incorrect adding of a user due to last feature I've added.

I didn't try yet, but it looks good now.

What functions acl_enabled() and acl_disabling() do?

ok, I did a quick review.

I think this is often a useful piece of code:
+ $acl_id = acl_get_id_by_name('acl', $token['acl']);
+ if (!$acl_id) {
+ $acl_id = acl_create_new_acl ('acl', $token['acl']);
+ }

Could you add this as a new ACL API function to the acl module and then make use of that function? I think of a get_acl funcition, that creates the acl if doesn't exist yet.

* Spelling:
Please start labels with a capital letter, but don't use capitals inside a sentence (user not User)

* Don't use node_save() in actions. return array('node' => $node) instead so workflow-ng will save it for you, but only when it is really needed =)

Apart from that, the code looks already good.

Amitaibu I wish you hadn't done this work, because now we have had to withdraw it from the GHOP project. :-( In the future, if you see a project marked for GHOP, PLEASE don't work on it and leave it for a student. (The GHOP tasks may be claimed by students until the last week of January so waiting 2 days wasn't a fair opportunity for someone to claim it.)

Changing title.

ah damn cross post that changed status. ;(

@add1sun,
Sorry about that, I've actually started working on ACL integration it a bit before the GHOP. Anyway, I'll make sure not to repeat the mistake. Hope the community will forgive me this time for being too entheuastic ;)

I can't over-emphasize how short we are on high-quality GHOP tasks, especially for the upcoming holidays. So even though I admire your enthusiasm, this is really disappointing that the clear reservation of this task was ignored, and that now we're yet another task short on an already dwindling list of available tasks for students to claim. :(

However, something that would help make up for it would be to propose and mentor an alternate task (or two). Or, if you're not able, finding someone else to do it. See instructions for submitting task proposals at http://drupal.org/node/add/project-issue/ghop.

Ok, after some bad Karma, here's how I repent :P

Back to this issue:
@Fago,
What about places such as 'Delete ACL', where I've used node_access_rebuild(); :
1. Is that the correct thing to do?
2. Won't it be better to have in the ACL API acl_get_acl_nodes which will return a list of assigned nodes, and return all those nodes to WF-ng?

and here's the patch.

This one allows adding/removing arbitrary user/nodes to ACL.
Like this user/ nodes can be added from nodereference/ usererference CCK fields as well.

Works for me.

What about to change "delete whole ACL(s) from node" to just "Delete ACL(s) from node"
and "delete ACL" to "Delete whole ACL"?

fago at #26:

* Spelling:
Please start labels with a capital letter, but don't use capitals inside a sentence (user not User)

@anantagati,
Please direct me to the incorrect labels, I though I covered them already.

There is another spelling issue: "Delte" - please fix.

Then yes, don't use node_access_rebuild() - that's will break on big sites and is not necessary. Only rebuild node access for nodes, that were in the ACL which is to be deleted.

I'm not sure how we should deal with the "use arbitrary user/node" feature. I plan to make it possible for actions to load further arguments, so then this should be a separate action. "Load arbitrary node" which then could be used by the ACL action. So perhaps we better leave this feature out for now, and I'll try to implement it in workflow-ng asap.

fago,
How to get the nodes from the ACL?

--edited--
Maybe acl_delete_acl should return an array of the deleted nodes?

All starts with small letter in latest patch from #33:
- add user to ACL
- remove user from ACL
- add node to ACL
- remove node from ACL
- delete whole ACL(s) from node
- delete ACL

1. Corrected labels
2. I've changed the acl_delete_acl to return an array of removed nodes, so delete_acl actions don't need to rebuild access.

ah, the patch of course...

hm, changing acl_delete_acl() doesn't makes much sense imho.

I think, it would be better to add another optional parameter to it, which if activated writes the node grants of the updated nodes. Use http://api.drupal.org/api/function/node_access_acquire_grants/5 for that. Be sure, to don't change the default behaviour of acl_delete_acl().

reverted acl_delete_acl behavior, but added option for it do node_access_acquire_grants

From patch 'ACL_Workflow_ng_integrate_5.x-1.5_try4.patch' function acl_add_user() has been called with parameter $author instead of $author->uid.

Result was that when user was added to ACL instead of his user id it was added with id 1.

Attached patch should fix it.

I've actually noticed that for example in function acl_workflow_ng_add_user_to_acl($author, $settings, &$arguments, &$log) the $author isn't coming properly as a user object. Not sure why...

--EDIT--
This seems to be another issue. Anyway, patch still needs review after some small fixes.

Just some quick comments about the acl.module part of the patch:
-- the default value of $write_grants should be FALSE, not NULL
-- $removed_nodes is always NULL and undocumented
-- every function that potentially loads a large number of nodes is problematic, see http://drupal.org/node/196002
-- rather than introducing a new function acl_get_acl($acl_name) I'd prefer to extend
function acl_get_id_by_name($module, $name, $auto_create = FALSE)
-- please run http://drupal.org/project/coder on your code

If you get an array rather than an object, then you should conditionally cast to object; this will make your code more robust.

Also, please provide Doxygen comments.

Slavis,
Thank you for the comments.

1. Instead of returning the nodes objects, function acl_delete_acl($acl_id, $write_grants = FALSE) returns the nids - I guess a more sensible solution.
2. Extended, acl_get_id_by_name($module, $name, $auto_create = FALSE)
3. Fixed according to coder.

I'm afraid I didn't get 'the cast to object' remark, do you mean I should do
return (object)$removed_nodes; ?

The "cast to object" bit was aimed at #45. If the incoming parameter is an array and you need an object, then just cast it to object.

Doxygen comments?

Ok,
currently returning $removed_nodes[] = $data->nid; so no need to cast.
Oops forgot to add Doxygen comments for acl_get_id_by_name. fixed.

My comment about casting has NOTHING to do with $removed_nodes! NOTHING!!!

It is the answer to your comment #45 above and to http://drupal.org/node/203297: If acl_workflow_ng_add_user_to_acl($author, $settings, &$arguments, &$log) is called with an $author array, then cast $author inside acl_workflow_ng_add_user_to_acl() to object, if that's what you need.

Please add Doxygen comments to the functions in the new include file as well.

@acl_delete_acl: First of it can't work that way, as $node is undefined. Use node_load.. Then I think it's not ideal, that the function returns $removed_nodes only, in the case the grants are update. So better remove that feature (as you don't use it anymore) or use db_affected_rows().
Please test the function, so we make sure that the grants are updated properly. (devel module's node access module is a nice helper).

@acl_add_user TODO: testing the user isn't necessary, acl_add_user covers that already. Similar for remove user there is no need to check before.

@other TODOs, check if node were in the acl (acl_node_remove_acl and acl_node_clear_acls)
We should implement this immediately, otherwise the node might be saved for nothing. I suggest we do so by modifying the API functions, so that they return the whether there was an entry removed or not -> so we could only then update the node grants.

For modifying the API functions, db_affected_rows() or db_num_rows() might be useful again.

@Salvis,
I thought you meant line 45 in the code, sorry. Regarding this I've opened an issue in WF-ng, as it seems to me to be a bug.
1. Added doxygen to inc file. Hope it's ok

@fago,
2. fixed node_load.
3. removed todo from user_add/ user_remove
4. acl_node_remove_acl, acl_node_clear_acls now return TRUE if db changed.

Better doxygen for the inc file

imho the patch looks good now.

Thanks! I have some comments on acl.module:

-- clarify comment and add a warning to acl_delete_acl:

-- fix commment:
Return TRUE if there was actaully a node deleted from ACL.
TRUE if the ACL was removed.

-- fix comment:
TRUE if there was actaully ACL(s) deleted from a node.
TRUE if there were any ACL's deleted from the node.

-- remove excessive parentheses:
if (($auto_create) && (!($acl_id))) {
if ($auto_create && !$acl_id) {

... and some more on the new acl_workflow_ng.inc:

-- fix comment
* It adds some actions which allows adding and removing users and nodes into arbitraty ACLs.
* It adds some actions that allow adding and removing users and nodes into arbitrary ACLs.

-- fix string
Enter the name of the ACL. You may have a single user/ node assigned to several ACL lists
Enter the name of the ACL. You may have a single user/node assigned to several ACL lists.

-- fix comment
* Form builder for action - Add a node from ACL
???

-- fix string
Select the operations users will have access rights to, when added to the ACL.
Select the operations that users will have access rights to, when added to the ACL.
Does this need to be so complicated? It's unlikely that it'll be translated correctly. How about
Select the operations that this ACL will control.

-- there's at least one TODO

-- I find several '#description' => t('Enter the name of the ACL.'), lines -- is it correct that these don't specify the action that will be carried out on the given ACL?

-- what's the purpose of acl_enabled() and acl_disabling()?

@fago: Are you aware of the fact that the strings in the ACL module are unlikely to ever get translated? I'm a bit surprised to see all those form definitions in here -- shouldn't these find a home inside the workflow-ng distribution?

This will need to be ported to Drupal 6. Who will do it? Who will test it against workflow-ng? In what time frame?

Porting acl_delete_acl() will not be trivial. It will need to use the new batch api (http://drupal.org/node/114774#batch). Will you do this, fago?

I intend to release ACL 6.x-1.0 soon, probably before workflow-ng is ready for testing, so I'll release it based on the current 5.x-1.5, and the workflow-ng additions will have to come later. Is that ok?

puh, good that you had such a close look.

I agree with your points. Indeed #description isn't optimal. The code should be changed, to show this in the configuration form.

@disabling:
Usually this code is used to prevent writing node access grants when the module is disabled and the grants updated. in the ACL case disabling isn't need and so can be removed. However enabled() is need by the acl module itself to return TRUE.

@TODO:
yes, imho it's only a nice to have, but not really necessary.

I can port the workflow-ng integration to d6 when workflow-ng has been ported - no big deal.
@acl_delete_all: Hm, I don't think it's necessary to go with batching as there needn't be so many nodes in one ACL. However, I agree it would be good to have it as option.

Anyway, I must admit that I'm currently really busy and I've not much time for this. Unfortunately amitaibu has left, perhaps you could help a bit?

I still don't understand what acl_enabled() does in this particular case (especially after removing acl_disabling()), and why it's needed -- ACL has worked fine without it so far.

If there's a TODO, that usually means something is unfinished and someone will need to finish it. Who will do it and when?

You're right, there needn't be so many nodes in one ACL, but there's nothing to stop a site from creating one ACL (or even multiple ACLs!) and assigning all of their nodes to it (them) -- especially with workflow-ng! -- and then we'd be exactly where we are today with D5. This must not happen. Core is now offering the means to do it properly, and ACL needs to play by the rules and not cut any corners.

Are you sure that these form definitions should go in here and not somewhere into workflow-ng, where they'd have a chance to get translated along with the bulk of workflow-ng?

@Slavis,
I'm back for only a few days but i wanted to try and finish this task. I've created a patch based on your comments and new issues from wf_ng:
* Fixed strings
* Fixed descriptions. Added a "Beware" also in the wf_ng action itself.
* removed t() from module name
* removed acl_enabled and acl_disabled - but i admit I'm not sure if it should or should not be there - fago?

I've attached also a suggestion for test (not SimpleTest, though):
1. Import the wf_ng_acl_test.txt into wf_ng import.
2. Since custom content links can't be exported you will have to do it manually.
3. Attached JPG show's how it looks in the end.

@Fago,
I have used Content access dev version, but it has no effect (see the screen shot). I've checked the ACL tables - they worked properly.

Last, sorry for the way my patch diff looks. I'm having problems for some reason with the creation of the patch (diff -urpN old new > patch_name.patch).

You seem to have the wrong type of newlines. That makes it a bit difficult to see what your patch does...

Changed lines.

Please answer my question in the other thread — I need to get a clearer picture of what we're doing here...

here's the same patch divided into two, for better readability

Fixed a mistake with the write grants in the delete_acl
Again, uploading 2 patches for better readability.

[EDIT] I keep having problems with the patch of the ACL.module. When I'll solve it I'll re-post. Sorry.

Ok, after a lot of head banging about the patch - it turned out file was with UNIX end line while mine DOS. when will computers stop being so annoying?! :P

#64: That's what I meant with "wrong type of newlines" in #59.

Please see the discussion in #234303: Remove ACL(s) from node - from all modules. I'm only now starting to realize that a function like acl_node_return_existing_acl($nid) should probably not be created.

Please fix #226090: Content Access disables other node access modules, review how wf_ng can work if CA behaves properly, think about cooperative use of ACL, and then let's discuss it.

To salvis' request I've changed 'module name' for the ACL table to acl_wfng

Oops, I missed one acl_wfng before. [Edit: updated file]

(subscribe)

I would prefer to see the module column filled with 'workflow_ng' to match the 'content_access' rows from the CA module. Also your labels 'User which will be added/removed' should be shortened to 'User to be added/removed' (more correct English would be 'User who will be ...' as they are generally people, but not necessarily).

However, as Workflow-ng doesn't have a form-based user interface to modify the acls going into this table, I expect this patch will lead to a lot of confusion. People will be asking "I've revoked all access from the CA module but my users can still see the content. WHY?" - because there was some workflow event that triggered an action to grant access but nobody can see where it is stored without diving into the database.

Should Workflow-ng be integrated with Content Access as well (and as a preferred interface)?

This patch should come with serious warnings about not revoking all access when you think you may have, and also a note to use the Devel Node Access module's block to verify what's going on when a user displays the content.

#244355: Provide Workflow-ng integration created for CA integration with Workflows-ng.

Thank you for your comments.

The idea of having grants that don't belong to a certain module is confusing, indeed.

I'm currently working on a patch to Devel Node Access at #241060: Provide the gory details of node access. It should help us understand what's going on when multiple modules provide grants, but I haven't looked at this patch here through the new magnifying glass yet...

This patch is a different, more open approach.
First, code wise, it uses the #new_arguments feature in wf_ng, which means you load an ACL as a new argument, for other actions to use. Less lines of code…

But the big change is that it allows user to manipulate all existing ACL, even from different modules. IMO This makes more sense as ACL is an API that allows other modules to use it. So with this patch I can for example create an action to add a user to Forum access module, or create an event that will add a node to content access ACL.

Otherwise, for previous patch I can not find many use cases.

Suggestion how to test:
1. Enable custom content links (CCL) from the wf_ng package.
2. Create custom links like in snap2.png
3. Import the wf_ng.txt to workflow_ng.
4. Assign the actions to CCL. So every time a CCL is clicked an event is triggered.

I am not a native speaker, so please review also my grammar …

Some fixes to Load action.

No, I won't accept any patch that tries to do something like that.

You can talk to other module's maintainers to create APIs for you (which they will then have to maintain forever!), but you must not start to manipulate their data. That would cause incredible headaches for everyone.

@salvis

I have to agree. We're talking about patching ACL to allow it to modify rows created by modules that called the ACL module's own functions? That doesn't make any sense. ACL should remain a glue module only, not a swiss army knife. Security is far too important for that.

The more I think about this, the more I think this issue belongs in the Workflow-NG issue queue, and the code should exist as an include under workflow_ng/workflow_ng/modules/ where it can create its own acls and not interfere with any other modules. It should extend workflow_ng_user.inc and workflow_ng_node.inc to use the API provided by the ACL module.

I still think it needs serious warnings and an advice to use Devel Node Access.

fago raised it so I'll leave it to him to reassign the issue, but I'm convinced we should not be touching the ACL module.

I have to agree that it should not be possible to modify existing ACLs created by other modules. This is NO good idea as it's too dangerous!
So best only work on ACLs with a defined module name for this integration - and no others.

The idea of this all is to be able to create *new* custom ACLs, and modify them. and yes, this code should ship with the acl module.

Furthermore for sake of usability, I'd suggest to not use the "load an acl" action, as this isn't easy to use. Perhaps just let users name their new ACLs and let them input their name again when modifying/deleting them.

So we're picking up at #67, right?

The idea of this all is to be able to create *new* custom ACLs, and modify them. and yes, this code should ship with the acl module.

I'm a bit on the fence here, as I said before (#57): if it's part of ACL, it's unlikely to ever get translated to any other languages. If it were in workflow-ng, its chances would be much better.

Where should the "name" go?

@name: acl stores already a name for each acl - together with the fix module name it can be used to identify the acl - right?

@translation: I wouldn't worry with the translation too much. If people are interested in it, it will be translated...
Anyway workflow-ng can't ship with integration for every possible module out there - for this there is the hook system allowing modules to integrate with it.

ACL stores the realm ("acl") and the module name ("acl-wfng") according to #66. Where would this name go?

Anyway workflow-ng can't ship with integration for every possible module out there - for this there is the hook system allowing modules to integrate with it.

The same could be said of ACL... :-)

Hi guys,
Thanks for te comments. so yes #67 should be reviewd.
As discussed previously module = acl_wfng.

@name: -> looking at acl_get_id_by_name()

to identify the acl we need the modulename == 'acl_wfng' + arbitrary name
-> the user chooses the name ->for the user of the workflow-ng integration the name is the identifier of the acl.

Anyway that's exactly what #67 does or not? I just tried to explain the solution I had in mind when starting this thread.

@foago,
No, currently all ACL are under 'acl_wfng' name, with no addition.

grml yes, that's the module entry. but there is an additional entry called 'name'....

sorry, I wrote it wrong in my previous comment. To clarify:
module = acl_wfng
name = arbitrary (user chosen).

Has anyone ported this to Drupal 6 and the Rules module (workflow_ng for D6)?