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

cleaned up abit more and put in the use of force params, although im not sure using them is the correct thing cant see a way of writing a test for this though unfortunatlly 2011/8/4 Keloran <ava3ar@gmail.com> > 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 > > > > >