This patch moves the changes in comment_post between 4.5 and 4.6 to mailhandler.

It also makes the user swicth more robust to work with external modules (listhandler)

It fixes a bug where base64 and other encoded text would not be converted to utf-8.

Should be applied to 4.6 and cvs. I'll roll new patches if neccessary.

CommentFileSizeAuthor
#14 mh-mysql.patch709 byteskilles@www.drop.org
#13 mh-46_0.patch14.16 KBkilles@www.drop.org
#12 mh-cvs_0.patch13.86 KBkilles@www.drop.org
#8 mh-46.patch15.08 KBkilles@www.drop.org
#7 mh-cvs.patch14.81 KBkilles@www.drop.org
#5 mh_0.patch13.31 KBkilles@www.drop.org
mh.patch13.77 KBkilles@www.drop.org

Comments

moshe weitzman’s picture

moshe weitzman commented
Title: Several chenges to mailhandler » Several changes to mailhandler
Version: 4.6.x-1.x-dev »

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.

killes@www.drop.org’s picture

killes@www.drop.org commented

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.

moshe weitzman’s picture

moshe weitzman commented

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

killes@www.drop.org’s picture

killes@www.drop.org commented

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).

killes@www.drop.org’s picture

killes@www.drop.org commented
StatusFileSize
new mh_0.patch13.31 KB

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.

killes@www.drop.org’s picture

killes@www.drop.org commented

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

killes@www.drop.org’s picture

killes@www.drop.org commented
StatusFileSize
new mh-cvs.patch14.81 KB

here is the cvs version.

killes@www.drop.org’s picture

killes@www.drop.org commented
StatusFileSize
new mh-46.patch15.08 KB

the 4.6 patch...

moshe weitzman’s picture

moshe weitzman commented

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.

killes@www.drop.org’s picture

killes@www.drop.org commented

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.

moshe weitzman’s picture

moshe weitzman commented

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

killes@www.drop.org’s picture

killes@www.drop.org commented
StatusFileSize
new mh-cvs_0.patch13.86 KB

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.

killes@www.drop.org’s picture

killes@www.drop.org commented
StatusFileSize
new mh-46_0.patch14.16 KB

here's the patch for 4.6.

killes@www.drop.org’s picture

killes@www.drop.org commented
StatusFileSize
new mh-mysql.patch709 bytes

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

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Fixed

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.

Anonymous’s picture

(not verified) commented
Anonymous’s picture

(not verified) commented
Anonymous’s picture

(not verified) commented
osherl’s picture

osherl commented
Anonymous’s picture

(not verified) commented
Anonymous’s picture

(not verified) commented
Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)