Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:19932
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Date: Wed, 9 Nov 2005 10:03:54 +0200 (EET)
Reply-To: Jani Taskinen <sniper@iki.fi>
To: Pierangelo Masarati <ando@sys-net.it>
cc: venaas@php.net, internals@lists.php.net
In-Reply-To: <1131521067.20182.2.camel@ando>
Message-ID: <Pine.LNX.4.61.0511090957290.23759@arfg.argcubovn.sv>
References: <1131485315.3327.15.camel@ando>  <Pine.LNX.4.61.0511090035400.17812@arfg.argcubovn.sv>
	<1131492177.3327.25.camel@ando>  <Pine.LNX.4.61.0511090702110.21476@arfg.argcubovn.sv>
	<1131521067.20182.2.camel@ando>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed
Subject: Re: [PHP-DEV] LDAP controls in response
From: sniper@iki.fi (Jani Taskinen)

On Wed, 9 Nov 2005, Pierangelo Masarati wrote:

>
> On Wed, 2005-11-09 at 07:05 +0200, Jani Taskinen wrote:
>>      The "control-patch" looks okay. But the "warning-fix-patch" really doesn't.
>>      You're actually changing some functions behaviour with some of those
>>      changes. Maybe you didn't notice but there's ldap_get_values() and
>>      ldap_get_values_len() separately. And there's ldap_bind() and ldap_sasl_bind(),
>
> What I'm doing is driven by the fact that the ldap_*value*() family, and
> the ldap_bind*() are being deprecated (that is: they're in the library,
> but they're not declared in ldap.h any more, and they are implemented as
> wrappers to the replacements I'm using, at the cost of extra execution
> time and resources, plus decent compiler warnings).

     Perhaps with OpenLDAP, but what about the other implementations with
     which this extension works just fine? You didn't wrap all those changes
     inside the #ifdef's.

> ldap_*value*_len() is the safe implementation of ldap_*value*(), because
> it can also handle non-null terminated values; this doesn't inhibit the
> handling null-terminated ones.  Moreover, PHP needs the length of the
> string anyway, so...  Note that this would allow PHP to use only
> ldap_get_values() for both, making the API more simple and robust (I
> vaguely recall the issue that brought to implementing
> ldap_get_values_len() in PHP, I think it was somewhere in PHP 3 and the
> patch took a while to get there).

     I agree it's better this way, but (even as unlikely it is) changing this
     MIGHT break someone's script.

     Anyway, it might be the time to cleanup the API of this extension
     and make it simpler. It's pretty confusing as it is.
     The returned arrays are also weird with their "count" fields and such.

> Anyway, none of these is an issue to me, so I'm fine with the controls
> patch.  A final comment: apart from some fuzziness complaints, both
> patches apply also to PHP 4 without changes.  This means we can get into
> production immediately (well, after some testing... :)

     This is a new feature and new features are no longer added in PHP 4.
     We only add new stuff in PHP >= 5.1.

     --Jani