Several changes to mailhandler

why are we moving the mailhandler_switch_user() call. that was already moved once for the benefit of listhandler. see 1.72

i had to clean up someone else's mess when they re-implememted comment_post(). now it seems you are breaking it again. what are we trying to improve? the breakage i noticed is communicating error messages back to the email author in case the node doesn't allow comments.

I've moved the user switch because it makes more sense to me to be before the hooks that implement the mailhandler hook are called. I do not know why the switch was moved for 1.72. I'd be surprised if this was listhandler related as I didn't work on it at the time.

As for comment_post: I was simply moving the changes that were made in core from 4.5 to 4.6 to mailhandler in order to stay in sync. I did not notice any breakage but didn't test for closed nodes. I can make a new patch without the comment_post change.

so what are the problems with the current 4.6 and HEAD versions? ... the commit message for 1.72 specifically mentions listhandler

I've no idea why the 1.72 commit mentions listhandler. IMHO, listhandler was very broken at the time (not due to mailhandler). The comment as such does not make sense because listhandler was using a custom user_save function at the time. Even the standard user_save function works.

The only problem this patch fixes is that the user switch if invoked twice would have destroyed the original cron user. This is now fixed.

The comment_post changes were just made to keep compatible with 4.6/HEAD (no idea how significant the changes were).

I've had a second look.

- I first created a patch that implements my changes without updating mh_comment_post.
This breaks listhandler, it prints previews of the comments instead of adding them.

- I then created a new patch including the updated comment_post and your return value for non-allowed comments.

I also remodelled mh_switch_user (again). It is now initialized in the corn hook and reset there as well in the end. You can switch to any user during processing the mails. You can also revert to the cron user earlier, should you desire to do this.

This patch works with listhandler.

Ok, here is what I think is a final version of this patch for 4.6.

here is the cvs version.

the 4.6 patch...

why are the drupal_goto() calls back? i can't think of a case where thats good for mailhandler ... you also change the comment style from our documented standard of /* to // style. i don't think thats a good idea.

The drupal_gotos aren't back they are commented out in /* */ style which is hard to see in a patch.
I can change that if desired.
The standard for in-function comments is actually // not /* nowadays.

almost there. here are my nits:

did remove this part delibrately?:
return array(t('Duplicate subject'), t('Duplicate subject'));

is this change valid? i haven't seen that syntax.
- $edit["cid"] = db_next_id("{comments}_cid");
+ $edit['cid'] = db_next_id('{comments}_cid');

the following line is not true when manually retrieving messages. i suggest keeping original language
- // switch back to original user
+ // switch back to cron user

Ok, I had missed the array, I changed the comment back. ' instead of " is perfectly ok. Not sure what you meant.

Hers's the patch for cvs.

here's the patch for 4.6.

And here's a mysql patch for both versions. It fixes the problem that you cannot put "-- " as a sigseparator.

committed to CVS. i don't think it was desirable to change that mysql field from varchar to text since text is less efficient. but i used the patch as is.