Re: [PATCH] fix imap_mail actually use the rpath option

Yeah it does need cleaning up, didn't know about the new mail options, main reason I did it was because the function has had the option since 2004, but it's never actually used it On Thursday, August 4, 2011, Johannes Schlüter <johannes@schlueters.de> wrote: > Hi, > > few comments just from reading the patch, not the full context, not > testing it: > > On Thu, 2011-08-04 at 16:30 +0100, Keloran wrote: >> Index: ext/imap/php_imap.c >> =================================================================== >> --- ext/imap/php_imap.c (revision 314217) >> +++ ext/imap/php_imap.c (working copy) >> @@ -4016,7 +4016,27 @@ >>         if (!INI_STR("sendmail_path")) { >>                 return 0; >>         } >> -       sendmail = popen(INI_STR("sendmail_path"), "w"); >> + >> +       /** Used to make return-path work **/ >> +        char *sendmail_path = INI_STR("sendmail_path"); >> +        char *appended_sendmail_path = NULL; > > This won't work on all compiles, declarations have to go first in the > block. > >> +        /** Return Path or not **/ >> +        if (rpath && rpath[0]) { >> +            appended_sendmail_path = emalloc( >> +                strlen(sendmail_path) + 3 + strlen(rpath) + 1); >> +            strcpy(appended_sendmail_path, sendmail_path); >> +            strcat(appended_sendmail_path, " -f"); >> +            strcat(appended_sendmail_path, rpath); >> +            sendmail_path = appended_sendmail_path; >> +        } >> + >> +        /** open the sendmail pointer **/ >> +        sendmail = popen(sendmail_path, "w"); /* New Code */ > > How do you open a pointer ;-) > I think everybody reading the code should know popen() so the first > comment isn't really useful; I assume the second comment there was for > your purpose. > >> +       if (appended_sendmail_path) >> +            efree(appended_sendmail_path); >> + > > Always use { } even for a single line (see CODING_STANDARDS) > > While editing this functions: Should this function not also respect the > new mail.force_extra_parameters and mail.log settings, too? (Might need > an extra bug report to be reported correctly ...) > > johannes > >