I checked the latest 5.6 release and it looks fine to me. Thanks for
incorporating the patch!
Rainer
Am 13.08.2015 um 17:32 schrieb Andreas Heigl:
Hi Rainer.
Am 13.08.15 um 16:39 schrieb Rainer Jung:
Am 12.08.2015 um 13:17 schrieb Andreas Heigl:
Hi Rainer.
Am 12.08.2015 um 13:00 schrieb Rainer Jung <rainer.j...@kippdata.de>:
Hi Côme,
Am 11.08.2015 um 16:58 schrieb Côme BERNIGAUD:
On 2015-08-11 00:36, Rainer Jung wrote:
The current problems should be mostly around the above four compiler
warnings. I can test any patches you want me to test.
Can you test including the attached file in ext/ldap/ldap.c, and not
defining HAVE_3ARG_SETREBINDPROC and LDAP_CONTROL_PAGEDRESULTS ?
thanks for the patch proposal. There's a few problems with it and I
will come back to you with an alternative proposal soon. For the moment:
- the definition of ldap_control_find in terms of at the top of
ldap.c currently is protected by
#if !defined(HAVE_LDAP_CONTROL_FIND)
It should be
#if defined(LDAP_CONTROL_PAGEDRESULTS) &&
!defined(HAVE_LDAP_CONTROL_FIND)
That's not related to your patch.
- in
void ldap_memvfree( void **value )
{
ldap_value_free(value);
}
it should probably be
ldap_value_free((char **)value);
otherwise we get compiler warnings.
- instead of ldap_open() we can use ldap_init() (recommended).
- using ldap_open() or ldap_init() with a url does not work, we have
to use host and port.
- the use of ldap_sasl_bind_s() in PHP_FUNCTION(ldap_bind) does not
work, without setting "LDAP_OPT_PROTOCOL_VERSION" to LDAP_VERSION3
using ldap_set_option(). Maybe it works for OpenLDAP, but Netscape
and Solaris document it must be set and indeed return
LDAP_NOT_SUPPORTED immediately otherwise. But since in
PHP_FUNCTION(ldap_bind) no SASL functionality is actually being used,
I propose to use ldap_simple_bind_s() instead of ldap_sasl_bind_s().
It is not deprecated and using it actually simplifies the code a bit.
As I said, I'll come up with a patch proposal soon.
The patch should also help for other LDAP implementations than
OpenLDAP or Solaris.
The main question here is whether we want PHP to support other
implementations. The documentation clearly states that OpenLDAP shall
be used and even Oracle states on an official site (I don't actually
have the URL at hand but it has been in an email to this thread) that
PHPs LDAP extension has to be built against OpenLDAP.
That's you (project) call. You are doing the work, you decide. I
wouldn't rely on Oracle for a judgement ;)
We should all decide (somehow) ;) And thanks for the warning about Oracle ;)
Also note that the code and m4 macros have quite a few places where they
react on other LDAP SDKs, like Windows, Oracle (not sun) and I think
also Netware. But those are not really strong arguments for multi-SDK
support. You can purge those places and the argument is gone. I really
think it depends on your goals.
But you should consider 5.6 a stable release. Maybe I'm the only guy on
the planet haven't build the ldap extension against non-OpenLDAP, but I
doubt it. 7.0 is in front of the doors and compatibility is much less an
issue there, maybe combined with a NEWS entry strengthening, that you
now really demand OpenLDAP and throw an error, if you can't find it
during configure.
You are actually right on that one. We should keep BC in the 5.6
release. That was the reason for the call to what other libs are used
where no one stepped up for others and therefore there wasn't any BC
expected.... :(
@Côme, can we fix that in the 5.6 branch somehow?
Come has done a great job to clean up the LDAP code to start
implementing long awaited features and I'm not really sure whether we
should give that up for maintaining compatibility with some edge-case
usages. So I'm very interested in your patch to see how we can rech
both goals!
Agreed, and much of the cleanup wouldn't have to be undone to make the
result still compatible with more SDKs. For instance all of the new use
of the "_ext" variants of functions is nice and OK also for other LDAP
implementations.
That's great! I'm not that deep into the C-business (yet) therefore I'll
need a day or two to digest the patch. I'd just love to see the
LDAP-extension move towards better implementation of such things as
paginated and/or serversided sorted results.
Thanks for your input and contribution!
Cheers
Andreas
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
