$nodes array modified inappropriately by comment notification logic in _notify_send()

FTR, Either patch format is fine by me.

Thanks for the fix. Committed.

Is this working for others?

It was committed on 2010-10-08 and is in the dev release but not in the 6.x-1.2 release.

When I tested this as part of an update awhile ago it caused a cron error when new comments existed. Sorry, it was a while ago and I don't have any more details.

We are using the 6.x-1.2 release without problems and it does not have this patch.

Is anyone else having problems with this commit?

I have changed the status to "Needs review", hope that is ok.

PS there is a bug in d.o. issue summary process; my id is now showing as the author of the issue summary which is not the case.

izmeez, please try apply fixed in this issues regarding your problem. i'm totally sure that it cause a cron to be stop because of this: #947264: Nodes with new comments are announced as "new posts"

at line 547,

$node = $node[$nid];

it supposed to be:

$node = $nodes[$nid];

the string should be $nodes not $node which cause the error shown. $node is reserve for drupal variable. while in this module, $nodes refer to the array. it fixed at my site.

@mradcliffe So if I understand the problem as described in comments #1 and #7 users were getting email notifications for nodes they are not subscribed to.

We have not encountered this however, we are using some added patches for default settings and user permissions. I am wondering if others can confirm this bug, the traffic on this issue seems amazingly low considering the number of sites that appear to have this module. (3387 sites report 6.x of which only 105 report 6.x-1.x-dev)

Meanwhile, I can confirm that the refinement in comment #9 solves the cron problem. But, still don't know if this patch is required.

@mradcliffe, Thanks for taking the time to review this. Was the small typo you found the one identified in comment #9 ?

I've no idea why this was re-opened in #17, and no reason was given.
I've verified that the original issue, as well as the typo identified in #9, has been fixed.

I think this should be closed. If someone still think it needs work, feel free to reopen but then also state the reason for reopening.

Several issues report some similar problem. Here is the list:

• #538306: repeat notifications
• #935458: $nodes array modified inappropriately by comment notification logic in _notify_send() (this)
• #947264: Nodes with new comments are announced as "new posts"
• #1336084: Groundhog day: Repeat notifications when notify_cron() times out during _notify_send().

@gisle, where did you confirm the typo in comment 9 was committed?

@izmeez, my bad.

It is fixed in the 7.x-1.x branch (line #997 in notify.module in the 7.x-1.x-dev version).

But you're absolutely right. As of now, it remains unfixed in 6.x-1.x-dev.

I don't know how I got the two versions mixed up, but somehow I did.

As for maintenance of the Notify project: I am only working actively on the Drupal 7 port. I am not happy about the the issue queue for the 6.x-1.x branch, but since I am not the project owner, and only signed on as co-maintainer because I desperately wanted a stable Drupal 7 port, I'll leave it to the project owner matt2000 and the community to sort out the issue queue for the 6.x-1.x branch. (Also see the section Support Status on the project page.

Since this is fixed in the 7.x-1.x branch, I'm setting the status to "patch (to be ported)" (but "fixed" was clearly wrong).

Edit: Since the bug mentioned in comment #9 was introduced in patch that was applied after the release of 6.x-1.2, it does not apply to that particular version, only to the development snapshots released after 6.x-1.2. It has been fixed in the 6.x-1.x-dev development snapshot commited today. Se also: #947264: Nodes with new comments are announced as "new posts"

@gisle, thanks for the update. I'll have to checkout your work on the 7.x branch when I get a chance.

I've finally managed to get my head around this:

A simple typo was present in the patch in comment #1 above and made it into commit 5ec7f53 in 6.x-1.x-dev (committed on Oct. 8 2010).

The typo pas pointed out by parasolx on Sep. 5 2011 in comment #9, but the correction was for some reason only applied to the 7.x-1.x branch (the buggy patch had been applied to both branches).

This typo has now also been corrected in the 6.x-1-x branch (commit 5f9be1a). There is a link to the Developement release on the Notify project page.

Please review and report back to this thread.

@gisle ok, so just to be clear,

the patch in #1 with the typo corrected is the fix and has been committed, right?

and you are asking users to report their experience on testing the 6.x-1.x-dev release, right?

or can this patch be marked as RTBC?

Thanks.

The the patch in #1 with the typo corrected is the fix and it has now been committed to 6.x-1.x-dev in a corrected version.

I am asking users running Notify on a Drupal 6 production site to report their experience on testing the 6.x-1.x-dev release. (It is OK to test on a staging server - I understand that people don't want to use a production server for test purposes.)

Only after it (I hope) passes community testing, it will be marked as RTBC. I usually change status about two weeks after the first favourable review from someone who is not the author of the patch. The delay is to encourage multiple reviews. But after some time, a single favourable review is enough to progress a proposed patch to RTBC.

Btw. if you (or anyone else) are aware of other patches in the issue queue that should be included in the dev version to improve its stability, please point them out so that they may be considered for inclusion in the next snapshot.

Likewise, if your site uses custom patches that you think improves stability compared to 6.x-1.2, please post them here for consideration.

If all this goes well, I will probably release Notify 6.x-1.3 in the not too distant future. I.e. after all applicable patches has been committed to dev and tested. This is the moment when RTBC is changed into Fixed.

Since I am not running any Drupal 6 production sites that uses Notify, I prefer to not review patches for the 6.x-1.x-dev myself (I do of course test them on a test server before committing). This is why progress on improving the 6.x-1.x branch depends so much on community effort.

Thus is fixed per comment #27 above.