When a property is modified on an Organic Groups post node via comment_driven the group association is mangled, saving the group as node #1.

Normally when a node is edited, the og_groups property before hook_nodeapi('presave') is called is this:

    [og_groups] => Array
        (
            [20] => 20
            [21] => 0
            [40] => 0
        )

However, when edited via comment_driven, it is this:

    [og_groups] => Array
        (
            [20] => 1
            [21] => 0
            [40] => 0
        )

When the group association is saved in og_save_ancestry(), the array value is used and not the key, resulting in node #1 being saved as the node's group (even if it is not a group node).

CommentFileSizeAuthor
#9 comment_driven-1030506-09-1.patch11.43 KBgapple
#9 comment_driven-1030506-09-2.patch1.15 KBgapple
#2 og-1030506.patch708 bytesgapple

Comments

gapple’s picture

gapple
he/they
Primary language English
commented

I made a small mistake for the values when edited by comment_driven; array values for unselected groups are empty, not 0.

    [og_groups] => Array
        (
            [20] => 1
            [21] => 
            [40] => 
        )
gapple’s picture

gapple
he/they
Primary language English
commented
StatusFileSize
new og-1030506.patch708 bytes

Here is a quick hack to og.module to restore the og_groups array values to what they are expected to be in og_presave_group().

arhak’s picture

arhak commented
Status: Active » Closed (duplicate)

ere is a quick hack [...]

thanks, it is always nice to know a hack, since it serves to understand the underlying problem

nevertheless, the root problem should be addressed #962554: Support for Organic Group

gapple’s picture

gapple
he/they
Primary language English
commented
Status: Closed (duplicate) » Active

I thought the intention of the other issue was to add support for changing the OG settings via Comment Driven (e.g. modifying the audience with a comment) which is clearly a feature request. I opened this issue to address the bug that presents when using Comment Driven in conjunction with OG, and think that it is a separate issue.
This bug may also be better addressed within Driven API (I don't have enough understanding of Driven and Comment Driven yet to determine the root), whereas I understand support for controlling OG properties through comments is solely within Comment Driven's domain.

Just let me know if you would still like to track and address these issues in combination.

arhak’s picture

arhak commented

[...] and think that it is a separate issue.

you're right

whereas I understand support for controlling OG properties through comments is solely within Comment Driven's domain.

off-topic, but for your info it might involve both

donquixote’s picture

donquixote commented

I was able to fix this with hook_nodeapi('presave').
What I do is basically the same as in the above patch, just that I don't have to hack an existing module.

Or, just noticed there is a slight difference (and probably something wrong in the patch).

<?php
function mymodule_nodeapi($node, $op) {
  if ($op === 'presave') {
    if (!empty($node->og_groups)) {
      foreach($node->og_groups as $gid => $v) {
        $node->og_groups[$gid] = empty($v) ? 0 : $gid;
      }
    }
  }
}
?>
donquixote’s picture

donquixote commented

Btw, this mostly happened if the person submitting the form had no permission to set groups for a post.

EDIT:
For a while it seemed as if admin would not suffer, but I can no longer reproduce this.
Anyway, the proposed fix does the trick for me.

gapple’s picture

gapple
he/they
Primary language English
commented

@donquixote
I think the only difference is that your code has to deal with the empty/zero values in the GID array, so the ternary operator is required. My patch deals with the GIDs after those empty/zero values have been filtered out, so I can blindly set the value of array elements.

I think your method is definitely a better option if it can be added to comment_driven; best if the values can be loaded properly but a start if we can hammer them back in to shape when needed.
I'll check it out on my site to see how it works. In my case the values were always mangled, even for the admin account.

gapple’s picture

gapple
he/they
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new comment_driven-1030506-09-2.patch1.15 KB
new comment_driven-1030506-09-1.patch11.43 KB

After a quick test, it seems like @donquixote's approach works for my site as well. Here's a patch to add in hook_nodeapi to comment_driven module. Not sure if this may be more appropriate being pushed to driven module instead though...

The first patch just corrects trailing whitespace in the file, while the second adds in the function.

arhak’s picture

arhak commented
Status: Needs review » Closed (works as designed)

it might be valid as a workaround,
but it doesn't seem to be a bug of this module

stepping with a debugger inside function _form_builder_handle_input_element (form.inc)
at line 990 the checkbox with #access=FALSE skips its form_type_checkbox_value
to later pick up the #default_value near line 1010, which will be 0/1 (or true/false for that matter), but never the #return_value of the checkbox, due to:

// Call #type_value without a second argument to request default_value handling.
[...] On not submitted forms, and for
checkboxes with #access=FALSE [...], the #value is
#default_value [...].

note that this excerpt was taken from D7 documentation, but its D6 behavior is expected to honor #default_value vs #return_value according to #access the same as described here
http://api.drupal.org/api/drupal/includes--form.inc/function/form_proces...

PS: I suggest you to open a new issue on OG to properly support FAPI Access Control (i.e. #access=FALSE)
also if any of you thinks more debate might help, feel free to reopen this issue

arhak’s picture

arhak commented

further references:
http://api.drupal.org/api/drupal/includes--form.inc/function/_form_build...
or perhaps better its more documented/commented counterpart
http://api.drupal.org/api/drupal/includes--form.inc/function/_form_build...

arhak’s picture

arhak commented

BTW, I haven't tested this, but the fix for OG could be something like changing
function og_presave_group in og.module, line 1183

   // Keep only the selected groups.
   if (isset($node->og_groups)) {
-    $node->og_groups = array_filter($node->og_groups);
+    $node->og_groups = drupal_map_assoc(array_keys(array_filter($node->og_groups)));
   }
gapple’s picture

gapple
he/they
Primary language English
commented

Thank you for further debugging this issue, Arhak.
It looks like your proposed solution would accomplish the same thing as my original 'hack'.

I've added a new issue in the OG queue, and posted my original patch as a fix.
#1114402: Support for FAPI #access=FALSE

socialnicheguru’s picture

socialnicheguru commented

subscribe