Using the latest devs of depending modules, i get this error:

Notice: Undefined variable: gid in Og_Notifications_Subscription->set_group() (line 32 of /home/gezond/public_html/sites/all/modules/og_notifications/og_notifications.inc).

Comments

Yuri’s picture

Title: Error after enabling this module. Undefined variable: gid in Og_Notifications_Subscription->set_group() » Error after enabling this module 7.x-2.x-dev. Undefined variable: gid
Yuri’s picture

Priority: Normal » Major

Setting to major because the module does not work at all (for me), possibly related to this error.

Kirpaul’s picture

I have this issue as well

knsheely’s picture

I also get this error in the 7.x-1.x-dev branch.

kari.nies’s picture

Same error after enabling 7.x-2.x-dev. Running latest Organic Groups 7.x-2.0-beta1.

Any fix or workaround?

hanskuiters’s picture

Status: Active » Needs review
StatusFileSize
new500 bytes

I made a patch which works for me. I only use "notifications for entire group".

deanflory’s picture

I too get this error and the whole gid thing was messing with another module as well as it was replacing something like a db file type or something (can't remember off-hand now) which made that other module not work:

Notice: Undefined variable: gid in Og_Notifications_Subscription->set_group() (line 32 of ../sites/all/modules/og_notifications/og_notifications.inc).

I haven't yet tested what this affects, but it seems pretty major and it would be great if it were fixed very soon.

I have not tested the patch as I can't tell from the information what it actually does. Does it actually fix the issue? Does it block the error from being displayed and thus not actually fix anything? Capono, can you provide more information on whether this patch is the fix or just an error alert blocker?

The original report was from July and it's now December............and the last release of this module was in May, so I'm guessing this module is dead and if there's no release in the next 2 weeks I'll just disable it, sadly.

hanskuiters’s picture

@deanflory: The error you have is the same as in the opening post, so nothing new there. My patch doens't just block the error alert, but checks for $gid to be set. Should be sufficient, it was for me. I don't use the module anymore, switched to flag and rules to have more fine grained notifications. Maybe also a way to go for you?

deanflory’s picture

Thanks capono for the tips. I was able to get the patch to work after altering the file name to "og_notifications_error_Undefined_variable_gid.patch" (no spaces). Will see now if the notices disappear.

pembeci’s picture

StatusFileSize
new714 bytes

Extending the patch in #6 to account for the warning reported in #1782440: Warning with og_notification.

mxr576’s picture

Here it is an another patch, that may solve the gid and subscription problems. Please take a look at it.

mxr576’s picture

I've updated my prev patch to solve other related errors as well. Please test it!

Greetings, mxr576

edvanleeuwen’s picture

Tested and verified.

nicrodgers’s picture

None of the patches work for me with 7.x-2.x-dev :(

nicrodgers’s picture

Issue summary: View changes
StatusFileSize
new506 bytes

This patch removes the gid notice error for me, using the latest dev version.

MatthijsG’s picture

#15 worked for me. Is it in the latest dev?

edvanleeuwen’s picture

I think #12 is a solution. I think #15 is only a work-around to prevent the error, not solving the issue.

edvanleeuwen’s picture

Status: Needs review » Needs work
Related issues: +#2276599: Notifications sent only to the last group specfied

In recent tests I discovered that this has led to a flaw which sends out messages only to the last group subscribed. This has something to do with the double array_pop. See https://drupal.org/node/2276599.

lokapujya’s picture

Status: Needs work » Postponed (maintainer needs more info)

Does this problem still exist? I can't reproduce.

edvanleeuwen’s picture

I have attached a patch which is the patch of #12 run against the latest dev. This fixes the problem for me.

lokapujya’s picture

What's needed here is the steps to reproduce. I do not get the error mentioned. Please make sure that you are on the latest code.

edvanleeuwen’s picture

I am not able to reproduce this anymore either. In the mean time, core, OG, entity, OG Extras have been updated. Perhaps this has been solved in the combination of these modules.

edvanleeuwen’s picture

Should we close this?

lokapujya’s picture

Status: Postponed (maintainer needs more info) » Closed (cannot reproduce)

Thanks for looking into this.

NewZeal’s picture

StatusFileSize
new1.02 KB

I'm getting the same error with latest version of og_notifications. There is definitely a problem with the function og_notifications_notifications_object_node() which doesn't filter out non group nodes. The error is caused by non group nodes.

Attached is a patch. I see that none of the previous patches have been implemented.

lokapujya’s picture

Can you please list the steps to reproduce the issue?

NewZeal’s picture

The issue presents itself to users with the create subscriptions perms

Error is caused by set_group on non group affiliated content type in og_notifications.inc:

public function set_group($node) { 
    if (og_get_group_type('node', $node->type)) { 
      $gid = $node->nid; 
    } 
    elseif (og_get_group_type('node', $node->type, 'group content')) { 
      $gid = array_pop(array_pop(og_get_entity_groups('node', $node))); 
    } 
    $this->get_field('node:gid')->set_value($gid); 
    return $this; 
  } 

It originates from module notifications_content:

/**
 * Implements hook_node_view().
 */
function notifications_content_node_view($node, $view_mode, $langcode) {
  // user has no permission to create subscriptions. exit early.
  if (!user_access('create subscriptions')) {
    return;
  }

  // View mode full and teaser is supported.
  if ($view_mode == 'full') {
    $display_option = 'node_links';
  } elseif ($view_mode == 'teaser') {
    $display_option = 'teaser_links';
  } else {
    return;
  }

  notifications_content_node_links($node, $display_option);
}

/**
 * Attach subscription links to the node object.
 */
function notifications_content_node_links($node, $display_option, $account = NULL) {
  $account = ($account == NULL) ? $GLOBALS['user'] : $account;
  $notifications_node = array(notifications_object('node', $node));
  $subscription_list = Notifications_Subscription::object_subscriptions($notifications_node, $account)
    ->set_user($account)
    ->filter_option($display_option)
    ->get_instances();

  foreach($subscription_list as $key => $subscription) {
    $link = $subscription->element_link('subscription');
    $item = array(
      'title' => $link['#title'],
      'href' => $link['#href'],
    ) + $link['#options'];
    $node->content['links']['notifications_content']['#links']['notifications-' . $key] = $item;
  }
}

which fires a script in notifications.subscription.inc

static function object_subscriptions($objects, $account) {
    $subscriptions = new Notifications_Subscription_List();
    foreach ($objects as $object) {
      if ($more = $object->subscribe_options($account)) {
        $subscriptions->add($more);
      }
    }
    return $subscriptions;
  }

Which fires a script in notifications.object.inc:

/**
   * Get subscription options for object, account. Only enabled subscription types
   *
   * We pass on the user account so we cah check permissions on the fly and save lots of objects
   *
   * @return Notifications_Subscription_List
   */
  function subscribe_options($account) {
    $subscriptions = new Notifications_Subscription_List();
    if ($options = $this->invoke_all('subscriptions', $account)) {
      $subscriptions->add($options);
    }
    return $subscriptions;
  }

which invokes a hook in og_notifications:

/** 
 * Implements of hook_notifications_object_node() 
 */ 
function og_notifications_notifications_object_node($op, $node, $account = NULL) { 
  switch ($op) { 
    case 'subscription types': 
      return array('group_content', 'group_content_type'); 
    case 'subscriptions': 
      // Return available subscription options for this node 
      $options = array(); 
      if (array_keys(og_get_all_group_content_entity())) { 
        $options[] = notifications_subscription('group_content') 
          ->add_field('node:gid', $node) 
          ->set_group($node) 
          ->set_name(t('All posts in this group.')); 
      } 
      if (notifications_content_type_enabled($node->type, 'group_content_type')) { 
        $options[] = Notifications_Subscription::build_instance('group_content_type') 
          ->add_field('node:gid', $node) 
          ->add_field('node:type', $node->type) 
          ->set_group($node) 
          ->set_name(t('Posts of type @type in the current group.', array('@type' => node_type_get_name($node)))); 
      } 
      return $options; 
      break; 
  } 
} 

The hook_notifications_object_node comes from a non og related module so it is the responsibility of og_notifications to check for whether or not a node belongs to a group or is a group node.

lokapujya’s picture

  1. Error is caused by set_group on non group affiliated content type

    So, another module is invoking the subscriptions hook (with a node that is not a group node) which is causing an error ? Wouldn't that be a bug in the other module?

  2. +++ sites/all/modules/contrib/og_notifications/og_notifications.module	(revision )
    @@ -1,7 +1,7 @@
         case 'subscriptions':
    +      if (!og_is_group_type('node', $node->type)) return;
    +      if (!og_is_group_content_type('node', $node->type)) return;
           // Return available subscription options for this node
    
  3. This means that $node-type must be both a group type and a group content type; Is that the intended filter?

NewZeal’s picture

1. No, as pointed out, the other module is a non og module so is not og cognisant. The function og_notifications_notifications_object_node invokes a hook from the notifications module which is a non-og module, so the responsibility lies with og_notifications and not the other module.
2. The filter should remove anything that fails both of these:

if (og_get_group_type('node', $node->type)) { 
      $gid = $node->nid; 
    } 
    elseif (og_get_group_type('node', $node->type, 'group content')) { 
      $gid = array_pop(array_pop(og_get_entity_groups('node', $node))); 
    } 

Which I believe the patch handles.

lokapujya’s picture

1.) It seems like og_notifications implements hook_notifications_object_node(), I do not see where it invokes a hook from the notifications module. It appears that it is the notification module that passes the node in here:

<?php
  /**
   * Run module_invoke_all('notifications_object_[type]') with this (Drupal) object
   */
  protected function invoke_all($op, $param = NULL) {
    return module_invoke_all('notifications_object_' . $this->type, $op, $this->get_object(), $param);
  }
?>

I don't understand the problem enough to know whether that's notifications fault for sending a bad node. But maybe og_notifications should handle a bad node more robustly?

2.) The patch is saying (if its not a group type then return) and (if it's not group content type then return). I don't know the problem as well as you probably do, but it seems like it should OR not AND? in other words, if its a group or group content type then continue.
3.) Ideally, we should create a test for this. Not sure if you would want to do that. The module doesn't have any tests currently.

lokapujya’s picture

Issue tags: +Needs manual testing

#27 is a stack trace, but it would be helpful to have actual steps to reproduce. I know that it's kind of hard since this is integrated with many other modules.

NewZeal’s picture

1. Yes, the function og_notifications_notifications_object_node() is fired from

if ($options = $this->invoke_all('subscriptions', $account)) {
      $subscriptions->add($options);
    }

So this is where it happens

protected function invoke_all($op, $param = NULL) {
    return module_invoke_all('notifications_object_' . $this->type, $op, $this->get_object(), $param);
  }

We are not talking about "bad nodes" here, just nodes with no relation to og. The notifications module does not specifically handle og nodes but og_notifications_notifications_object_node() does.
2. It doesn't matter what the actual code in the patch is. You can use an Or if you like.
3. To replicate this, enable notifications_content, og_notifications and view a non og node in full view mode with a user that has create subscriptions perms.