Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:54636 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 7890 invoked from network); 16 Aug 2011 15:09:13 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 16 Aug 2011 15:09:13 -0000 Authentication-Results: pb1.pair.com smtp.mail=ava3ar@gmail.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=ava3ar@gmail.com; sender-id=pass Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.212.42 as permitted sender) X-PHP-List-Original-Sender: ava3ar@gmail.com X-Host-Fingerprint: 209.85.212.42 mail-vw0-f42.google.com Received: from [209.85.212.42] ([209.85.212.42:35481] helo=mail-vw0-f42.google.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id E3/D2-28315-8188A4E4 for ; Tue, 16 Aug 2011 11:09:13 -0400 Received: by vwl1 with SMTP id 1so5370558vwl.29 for ; Tue, 16 Aug 2011 08:09:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=6IhfP/9ro4LDOorZHystWwSk4yXVfuitJ7fe7NIm1Tw=; b=fRjWiV/GyILglgHFd6f9b65kQ/SZ5+p0bOJkQzswRSnDb76arhNczihFFy7+8j4dGM gzeBq9sSsXHDk4tn/2P0tmiFEUNzIeOdZl/Yffkb1cGXyTPnTU2XFSb0u7JLEfE+IOCO wLcbl6j4fI/MYjSH1+qKcyvhdK+ADCwuYvmUE= Received: by 10.52.99.99 with SMTP id ep3mr4667372vdb.514.1313507350077; Tue, 16 Aug 2011 08:09:10 -0700 (PDT) MIME-Version: 1.0 Received: by 10.220.162.16 with HTTP; Tue, 16 Aug 2011 08:08:50 -0700 (PDT) In-Reply-To: References: <1312476489.1541.28.camel@guybrush> Date: Tue, 16 Aug 2011 16:08:50 +0100 Message-ID: To: =?ISO-8859-1?Q?Johannes_Schl=FCter?= Cc: PHP Internals Content-Type: multipart/mixed; boundary=20cf307ca0de02e09804aaa0c32e Subject: Re: [PATCH] fix imap_mail actually use the rpath option From: ava3ar@gmail.com (Keloran) --20cf307ca0de02e09804aaa0c32e Content-Type: multipart/alternative; boundary=20cf307ca0de02e09404aaa0c32c --20cf307ca0de02e09404aaa0c32c Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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 > 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=FCter > 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 > >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > >> --- 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 =3D popen(INI_STR("sendmail_path"), "w"); > >> + > >> + /** Used to make return-path work **/ > >> + char *sendmail_path =3D INI_STR("sendmail_path"); > >> + char *appended_sendmail_path =3D 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 =3D 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 =3D appended_sendmail_path; > >> + } > >> + > >> + /** open the sendmail pointer **/ > >> + sendmail =3D 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 > > > > > --20cf307ca0de02e09404aaa0c32c Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable cleaned up abit more and put in the use of force params, although im not su= re using them is the correct thing

cant see a way of wri= ting 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=FCter <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
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> --- ext/imap/php_imap.c (revision 314217)
>> +++ ext/imap/php_imap.c (working copy)
>> @@ -4016,7 +4016,27 @@
>> =A0 =A0 =A0 =A0 if (!INI_STR("sendmail_path")) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
>> =A0 =A0 =A0 =A0 }
>> - =A0 =A0 =A0 sendmail =3D popen(INI_STR("sendmail_path"= ), "w");
>> +
>> + =A0 =A0 =A0 /** Used to make return-path work **/
>> + =A0 =A0 =A0 =A0char *sendmail_path =3D INI_STR("sendmail_pa= th");
>> + =A0 =A0 =A0 =A0char *appended_sendmail_path =3D NULL;
>
> This won't work on all compiles, declarations have to go first in = the
> block.
>
>> + =A0 =A0 =A0 =A0/** Return Path or not **/
>> + =A0 =A0 =A0 =A0if (rpath && rpath[0]) {
>> + =A0 =A0 =A0 =A0 =A0 =A0appended_sendmail_path =3D emalloc(
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0strlen(sendmail_path) + 3 + strle= n(rpath) + 1);
>> + =A0 =A0 =A0 =A0 =A0 =A0strcpy(appended_sendmail_path, sendmail_p= ath);
>> + =A0 =A0 =A0 =A0 =A0 =A0strcat(appended_sendmail_path, " -f&= quot;);
>> + =A0 =A0 =A0 =A0 =A0 =A0strcat(appended_sendmail_path, rpath); >> + =A0 =A0 =A0 =A0 =A0 =A0sendmail_path =3D appended_sendmail_path;=
>> + =A0 =A0 =A0 =A0}
>> +
>> + =A0 =A0 =A0 =A0/** open the sendmail pointer **/
>> + =A0 =A0 =A0 =A0sendmail =3D 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.
>
>> + =A0 =A0 =A0 if (appended_sendmail_path)
>> + =A0 =A0 =A0 =A0 =A0 =A0efree(appended_sendmail_path);
>> +
>
> Always use { } even for a single line (see CODING_STANDARDS)
>
> While editing this functions: Should this function not also respect th= e
> new mail.force_extra_parameters and mail.log settings, too? (Might nee= d
> an extra bug report to be reported correctly ...)
>
> johannes
>
>

--20cf307ca0de02e09404aaa0c32c-- --20cf307ca0de02e09804aaa0c32e--