This one took a while to track down.

The way total_subscription actually queues a newly added node is by using this code:

function total_subscription_node_insert($node) {
  if ($node->status == 1) {
    if (node_access('view', $node, drupal_anonymous_user())) {
      total_subscription_node_queue_mail($node);
    }
  }
}

That node_access() call is what we're looking at specifically. If we look inside the node_access() function code we can see that if module_implements('node_grants') returns TRUE (meaning that at least one module implements hook_node_grants()) then it will query the node_access DB table for access information about this node.

That's fine normally. The problem is that we're doing this from inside hook_node_insert(), which actually runs before the node_access table is updated for a node that's in the process of being inserted. Check out this code from inside node_save():

module_invoke_all('node_' . $op, $node);
module_invoke_all('entity_' . $op, $node, 'node');

// Update the node access table for this node.
node_access_acquire_grants($node);

See how it invokes hook_node_insert() before running node_access_acquire_grants()? That means that, if we're using custom node grants (for example, any site using the nodeaccess module), then our access check will fail consistently because it looks for access in the DB table before it has been put there.

Summary: if you're using any module that implements hook_node_grants() such as the nodeaccess module, then this module fails to send any subscriptions at all.

Related core issue: https://www.drupal.org/node/1425974

CommentFileSizeAuthor
#8 access_check_fails_in-2612234-8.patch8.06 KBxangy
#3 total_subscription-access-check-fails-2612234-3.patch1.21 KBmcrittenden

Comments

mcrittenden created an issue. See original summary.

mcrittenden’s picture

mcrittenden commented
Issue summary: View changes
mcrittenden’s picture

mcrittenden commented
StatusFileSize
new total_subscription-access-check-fails-2612234-3.patch1.21 KB

This just removes that access check altogether. Not the ideal solution, but fine for my purposes and likely most sites.

mcrittenden’s picture

mcrittenden commented
Status: Active » Needs review

Moving this to Needs Review, since technically there is a patch for review, even though I'm sure this patch isn't going to be accepted as is.

deepakaryan1988’s picture

deepakaryan1988
Primary language Hindi
commented

I waiting for community review for this patch.
After that I will merge it to dev branch.

ajits’s picture

ajits
he/him
Primary language Marathi
Location India
commented
Status: Needs review » Needs work

Thank you for reporting the issue and for your analysis!

IMO, removing the access check altogether is does not solve the problem.

I think the patch still could be committed with some addition. Here is what I am a solution that I would propose:

  1. Remove the checks from the implementation of hook_node_presave and hook_node_insert. This will update the cron queue.
  2. Make the check while executing the cron queue callback. In total_subscription_mailing_queue_callback() function.
deepakaryan1988’s picture

deepakaryan1988
Primary language Hindi
commented

Yes @AjitS , I agree with you

xangy’s picture

xangy
he/him
Primary language Hindi
Location India
commented
Assigned: Unassigned » xangy
StatusFileSize
new access_check_fails_in-2612234-8.patch8.06 KB

Added node_access to total_subscription_mailing_queue_callback(). Removed from total_subscription_node_presave() and total_subscription_node_insert(). Also made changes based on drupal standards.

xangy’s picture

xangy
he/him
Primary language Hindi
Location India
commented
Assigned: xangy » Unassigned
xangy’s picture

xangy
he/him
Primary language Hindi
Location India
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: access_check_fails_in-2612234-8.patch, failed testing.

deepakaryan1988’s picture

deepakaryan1988
Primary language Hindi
commented

@xangwish get in touch with @AjitS to resolve this one.
Hope you would find better patch which wouldn't be failed. :)

xangy’s picture

xangy
he/him
Primary language Hindi
Location India
commented

Thanks @deepakaryan1988. Yes, I would talk to @AjitS regarding this.

xangy’s picture

xangy
he/him
Primary language Hindi
Location India
commented

Getting this error on the tests:

12:09:01 PHP Notice:  Undefined variable: classes in /opt/drupalci_testbot/src/DrupalCI/Plugin/BuildSteps/publish/JunitXMLFormat.php on line 144
12:09:01 PHP Warning:  Invalid argument supplied for foreach() in /opt/drupalci_testbot/src/DrupalCI/Plugin/BuildSteps/publish/JunitXMLFormat.php on line 169
12:09:01 Reformatted test results written to /var/lib/drupalci/web/jenkins-default-109542/artifacts/xml/testresults.xml
12:09:01 Completed publish:junit_xmlformat
12:09:01 Completed publish
12:09:01 Checking console output
12:09:01 Recording test results
12:09:01 ERROR: Step ?Publish JUnit test result report? failed: None of the test reports contained any result
12:09:01 Finished: FAILURE

This could be a jenkins' issue.

The last submitted patch, 8: access_check_fails_in-2612234-8.patch, failed testing.

flux423’s picture

flux423
Primary language English
Location Portland, Maine
commented

After banging our heads against the wall for hours, we found this patch and solved our issue.
If its ok, I'm going to change the the issue title to help other users find this solution.

We reviewed the Pending Patches for this module but since the automated test failed, this issues did not show up in the que.

For future users.. This patch successfully applies to the 7.x-1.x-dev branch cleanly and solved the issue of not sending the email to the subscriber.

Thank you @xangy for the patch.

flux423’s picture

flux423
Primary language English
Location Portland, Maine
commented
Title: Access check fails in hook_node_insert if a module implements hook_node_grants() » Subscriptions are not sent (access denied) when using the function hook_node_grants()
roadlittledawn’s picture

roadlittledawn commented

Also banging my head against the wall here to no avail. I used this patch on the 7.x-1.x-dev version and made similar changes manually in the 7.x-1.0 version and still can't get it to work. Specifically, when a node is published with a taxonomy term set that an anonymous user is subscribed to, the email is not sent. Far as I can tell it's not added to the cron queue either.

Any additional insights anyone may have would be appreciated. I so badly want this module to work!

roadlittledawn’s picture

roadlittledawn commented

Update:
Looking at watchdog logs, looks like it's throwing this error: Undefined variable: node total_subscription.module:1118.

Those lines of code:

$taxonomy_fields = array();
  $fields_detail = field_info_fields();
  foreach ($fields_detail as $key => $value) {
    if ($value['type'] == 'taxonomy_term_reference' && isset($value['bundles']['node']) && in_array($node->type, $value['bundles']['node'])) {
      $taxonomy_fields[] = $key;
    }
  }

Still trying to understand how to correct this error...

andrew answer’s picture

andrew answer commented

Please recheck this bug with last TS version.

andrew answer’s picture

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