node author notification was not ported properly

I've just run into this same problem but with authenticated users.

I get this error:

Undefined property: stdClass::$node_notify_mailalert in path/sites/all/modules/comment_notify/comment_notify.module on line 351.

But not a fatal error.

Does the error happen every time? Does it depend on which option they choose in the dropdown?

I did a bit more testing to produce a test case that will hopefully help reproduce the error. This was done in in the standard forums and both users had "Receive node follups" checked and "Just for replies to my comments" selected in their profiles.

1. User1 creates a new forum topic - No Error.
2. User1 adds a comment to the topic - No Error.
3. User1 replies to own comment - No Error.
4. User2 adds a comment to the topic - ERROR.
5. User2 replies to User1's comment - ERROR.

Both errors were the same "unsupported operand" reported in the original issue. It also doesn't seem to matter what I select in the drop down box on the comment/reply. Even "No notifications" gives me the error.

@Karlkedrovsky - that is _very_ helpful. It seems that the error happens when sending the mail. I don't have much time to debug this right now, unfortunately. Will try to do so next week.

It looks like the bug is related to the changes in the url() function. Some of the calls to url() look like they've already been updated but a few were not. I've done a small amount of testing with the attached patch and it seems to have fixed my problem.

Great detective work, KarlKedrovsky.

Most of this looks good to me.

The first hunk about '#description' seems wrong. Did you test that? I think the current version of that one is fine.

+ '!comment_url' => url('node/'. $nid, array('absolute' => TRUE)) .'#comment-'. $cid,

Is also wrong. I think it should be more like:

+ '!comment_url' => url('node/'. $nid, array('absolute' => TRUE, 'fragment' => '#comment-'. $cid)),

Sorry about that greggles, I had only meant the patch to be illustrative of what I think the fix is. To be honest, I was in too big of a hurry and should have taken time to explain myself. I took another look and you're right the #description hunk was wrong. I backed out that change in the attached patch. I looked through the code again and I'm pretty sure the two lines that are changed in the patch file are the only ones that needed to be changed for the module to conform to the new url() api. For my cases that seems to have fixed the problem.

I've only tested the cases where I was able to see the problem. I should have some time tomorrow to do some more testing and I'll let you know how that turns out.

Here's a patch which I think should fix it and also get better style for the fragments as well.

@undoIT and @KarlKedrovsky - did you get a chance to test the patch from #8?

I've applied the patch, deployed it to our development server and run through the few test cases I described in this thread. Unfortunately there's an issue on our server that's keeping email from being sent so I haven't been able to fully test the patch. I can say that for the cases where I was getting an error I am no longer seeing it so the patch looks like it fixes the issue, although I can't say for sure that it didn't break anything else yet. I plan to do some more testing tomorrow after our sys admin has time to fix the mail server problem.

Also, since this is the first time I've even tried to install comment notify I may not be the best person to rely on for a good regression test. I'd be more than happy to run through a list of test cases if such a thing exists but the only things I've been testing up until now are the use cases that we're going to be using on my current project.

Thanks for your help testing, this is now committed.

If we find more tweaks to make to improve it...we can always patch again, but I got another report about this today so I wanted to really clean it up.

Thanks again! You are obviously very detail oriented so I hope you will have more time and to help with issues related to comment_notify.

thanks - saw this error reffing line 1265 on common.inc - but isolated by disabling this module

is your commit for all such instances? read about this elsewhere as some api change from d5 to d6...

@zilla - I believe that all initial examples of this problem were identified when the module was originally ported to 6.x. This particular set of bugs crept in as part of a new feature (#221196: adding notification for the node owner) which I relied on somewhat blindly. Can you confirm that this patch fixes the problem for you? If so I will create a 6.x-1.2 release.

After aplying the patch at #8 the crashing issue was gone.

But i have problems with mail notification, because mail had fatal errors.
As you can see at the mail that follows, code is confusing the Default mail text for sending out the notifications to node authors, with the recipients.

----- The following addresses had permanent fatal errors -----
post.
(reason: 550 5.1.1 User unknown)
your
(reason: 550 5.1.1 User unknown)
for
(reason: 550 5.1.1 User unknown)
comment
(reason: 550 5.1.1 User unknown)
new
(reason: 550 5.1.1 User unknown)
::
Final-Recipient: RFC822; your@mydomain.com
Action: failed
Status: 5.1.1
Diagnostic-Code: X-Unix; 550 5.1.1 User unknown
Last-Attempt-Date: Sun, 26 Oct 2008 14:37:19 -0600

Final-Recipient: RFC822; for@mydomain.com
Action: failed
Status: 5.1.1
Diagnostic-Code: X-Unix; 550 5.1.1 User unknown
Last-Attempt-Date: Sun, 26 Oct 2008 14:37:19 -0600

Final-Recipient: RFC822; comment@mydomain.com
Action: failed
Status: 5.1.1
Diagnostic-Code: X-Unix; 550 5.1.1 User unknown
Last-Attempt-Date: Sun, 26 Oct 2008 14:37:19 -0600

Final-Recipient: RFC822; new@mydomain.com
Action: failed
Status: 5.1.1
Diagnostic-Code: X-Unix; 550 5.1.1 User unknown
Last-Attempt-Date: Sun, 26 Oct 2008 14:37:19 -0600

@greggles - i unfortunately do not know how to apply patches! if you can roll up into a full module i can test it out, though i know that this is not the preferred way of doing things...or perhaps an updated dev release on the project page?

@sotiris - thanks for the report (though it's somewhat out of place...). I just committed a fix which should address that http://drupal.org/cvs?commit=149062

If you check out the code from CVS or just wait until tomorrow you can download the latest tarball.

Ok i made the changes from the CVS an now everything is working well for me!
I'll be testing Comment_notify, and if something goes wrong i'll let you know.

Thanks a lot,

Sotiris

Note that there are string changes in the permissions in these latest releases, so after upgrading:

1) You will need to visit the admin/build/modules page to rebuild the menu cache
2) You will need to visit admin/user/permissions and set the permissions for roles again

Already done.

;-)

Just giving verification that the code committed in 6.x-1.x-dev (2008-Oct-28) did fix this issue. Thanks.

Thanks for the report BryanSD. I guess it's time to roll a new release ;)

Same error now on Drupal 6.8 using comment notify 6.x-1.1

Fatal error: Unsupported operand types in /home/islandcricket/islandc../includes/common.inc on line 1369

Correct, that's why you can use 6.x-1.x-dev which has this fixed.