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
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
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
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.
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
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