Everything

php

LDAP controls support API

php.internals

Côme Chilliet

9 years ago

Hello,

I’m looking into adding control support to all ldap operations in php-ldap.
You can see a WIP PR here: https://github.com/php/php-src/pull/2640

Next step is to add control support for ldap_modify, ldap_add, ldap_search, …

The historic patch for this added functions like ldap_modify_ext, ldap_add_ext… for this which I would like to avoid.

So this means I cannot change the return type, only add parameters.
One solution is to change:
bool ldap_modify ( resource $link_identifier , string $dn , array $entry )
Into:
bool ldap_modify ( resource $link_identifier , string $dn , array $entry, array $servercontrols, array $clientcontrols, array &$resultcontrols )

The problem is that it still does not give access to the result object and thus to errmsg and referrals which can be obtained with ldap_parse_result.

So I was thinking maybe:
bool ldap_modify ( resource $link_identifier , string $dn , array $entry, array $servercontrols, array $clientcontrols, [resource &$resultobject])
-> if the last parameter is omitted, it works as before, if it is given, then the user will have to call ldap_parse_result on it to get the information it wants.

What do you think of this solution ?
I don’t see any other giving access to all available information without BC.

This will have to be changed for almost all ldap_* operations. Even ldap_bind can return controls in the result.

Côme

Côme Chilliet

9 years ago

Hello again,

I was able to easily add controls support to search operations since they already return the result object. (my previous message concerns modification operations)

But I’m a bit stuck for EXOPs.
Since ldap_exop generic method can return the result object I thought it would be easy as well.
But the function is like this:
resource ldap_exop(resource link, string reqoid [, string reqdata [, string retdata [, string retoid]]])

And it returns the result object only if retdata and retoid are not provided (otherwise it parses the result right away and fills them).
So I cannot add control parameter at the end.

As ldap_exop was merged in 7.2, is it possible to have a BC on it for 7.3 or not?
It would be by adding options in the middle:
resource ldap_exop(resource link, string reqoid [, string reqdata [, array servercontrols [, array clientcontrols [, string retdata [, string retoid]]]]])

Côme

Sara Golemon

9 years ago

On Wed, Jul 26, 2017 at 12:11 PM, Côme Chilliet <come@opensides.be> wrote:

> As ldap_exop was merged in 7.2, is it possible to have a BC on it for 7.3 or not?
> It would be by adding options in the middle:
> resource ldap_exop(resource link, string reqoid [,
> string reqdata [, array servercontrols [, array clientcontrols [,
> string retdata [, string retoid]]]]])
>

I'll be honest, the code in this function looks bizarre to my eyes
(perhaps because I've been in C++14 land lately), but it's your code
and you have the context in mind for it.

The current return value for 4 or more args is simply `true`, so the
BC break would be trivial to make that a resource (which evaluates as
true), but I think we can make that even simpler.  Just fix it before
beta3 (or beta2 if the fix is already ready) and I won't tell on you
to Remi.

-Sara

Sara Golemon

9 years ago

On Wed, Jul 26, 2017 at 12:48 PM, Sara Golemon <pollita@php.net> wrote:

> On Wed, Jul 26, 2017 at 12:11 PM, Côme Chilliet <come@opensides.be> wrote:
>> As ldap_exop was merged in 7.2, is it possible to have a BC on it for 7.3 or not?
>> It would be by adding options in the middle:
>> resource ldap_exop(resource link, string reqoid [,
>> string reqdata [, array servercontrols [, array clientcontrols [,
>> string retdata [, string retoid]]]]])
>>
> I'll be honest, the code in this function looks bizarre to my eyes
>

Rather than vague "eww" motions, here's a (suggested) commit on a
branch in my checkout which you might choose to apply to ldap_exop()
(and perhaps apply to other functions in this file).

https://github.com/sgolemon/php-src/commit/5b3f4c2fb9e529880ec74e61f9e27aa41ec7f023

Your code isn't broken as-is, these idioms are just going to make it
easier to read later on and hopefully will help you understand PHP's
module API better.

-Sara

Note: I don't know for sure if this compiles.  If not, it should at
least be close.

Côme Chilliet

9 years ago

Hello,

The problem with your changes seems to be that when using "z/!" for retdata, if I call the method with a unset var, which will be the most common case as it’s an out parameter,
the test if (retdata) will fail and the method will behave as if I passed NULL.

Côme

Côme Chilliet

9 years ago

Le mercredi 26 juillet 2017, 12:48:04 CEST Sara Golemon a écrit :

> The current return value for 4 or more args is simply `true`, so the
> BC break would be trivial to make that a resource (which evaluates as
> true), but I think we can make that even simpler.  Just fix it before
> beta3 (or beta2 if the fix is already ready) and I won't tell on you
> to Remi.

So how about the patch attached?

I did not include your suggested changes as I did not manage to make it work for now, plus this will have to be changed for all functions so I’d prefer to work on this clean up after I get back from holidays at the end of August.
But thanks for the suggested changes, ext/ldap code does need more set of eyes looking at it and I do not master PHP internal features yet.

Côme