LDAP controls support API

  100046
July 26, 2017 12:55 come@opensides.be (=?ISO-8859-1?Q?C=F4me?= Chilliet)
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
  100050
July 26, 2017 16:11 come@opensides.be (=?ISO-8859-1?Q?C=F4me?= Chilliet)
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
  100051
July 26, 2017 16:48 pollita@php.net (Sara Golemon)
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
  100052
July 26, 2017 17:06 pollita@php.net (Sara Golemon)
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.
  100057
July 27, 2017 10:13 come@opensides.be (=?ISO-8859-1?Q?C=F4me?= Chilliet)
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
  100056
July 27, 2017 08:21 come@opensides.be (=?ISO-8859-1?Q?C=F4me?= Chilliet)
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