Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:54375
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.212.42 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <1312476489.1541.28.camel@guybrush>
References: <CAF=va1VoO38yrGXpfXk+L+YZ-BiLewBe4QPkvpp7QM7MvEwn+A@mail.gmail.com>
	<1312476489.1541.28.camel@guybrush>
Date: Thu, 4 Aug 2011 17:56:51 +0100
Message-ID: <CAF=va1X2B2cSxcXtHYtVw0pSYvqCt0SDZj4BD5wV9bJB99jdxg@mail.gmail.com>
To: =?ISO-8859-1?Q?Johannes_Schl=FCter?= <johannes@schlueters.de>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PATCH] fix imap_mail actually use the rpath option
From: ava3ar@gmail.com (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 <johannes@schlueters.de> w=
rote:
> 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_path");
>> + =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 + strlen(rpat=
h) + 1);
>> + =A0 =A0 =A0 =A0 =A0 =A0strcpy(appended_sendmail_path, sendmail_path);
>> + =A0 =A0 =A0 =A0 =A0 =A0strcat(appended_sendmail_path, " -f");
>> + =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 the
> new mail.force_extra_parameters and mail.log settings, too? (Might need
> an extra bug report to be reported correctly ...)
>
> johannes
>
>