Stubs in ext/ldap

On Thu, 17 Sep 2020 at 15:18, Côme Chilliet <
come.chilliet@fusiondirectory.org> wrote:

Le Thu, 17 Sep 2020 15:03:05 +0200,
"G. P. B." george.banyard@gmail.com a écrit :

A lot of the documentation is not up to date and will needs to be updated
after
there has been a check of the argument names for consistency with other
extensions in regards to named params, the stubs are the source of trust.

I think a review of ext/ldap is needed then.
https://github.com/php/php-tasks/issues/23

There is a tracker about extensions which needs to have a review still:

UNKNOWN means that the default value cannot be specified due to some
reason
mostly the case for arguments passed by references or funcky function
signature
implementation which check the number of argument passed to the function.

I would like to fix those conflicts.
Basically my questions are:

1. What is the difference between s and S in zend_parse_parameters and
   how
   do
   they behave for a NULL value?

None in regards to userland as 's' is a char* followed by a size_t ad 'S'
is a zend_string.
As for all internal functions which are not nullable they will coerce
null
to the respective type
so for a string argument null would become an empty string.

So in the example of ldap_bind:
if (zend_parse_parameters(ZEND_NUM_ARGS(), "r|ss", &link,
&ldap_bind_dn, &ldap_bind_dnlen, &ldap_bind_pw, &ldap_bind_pwlen) !=
SUCCESS)
{ RETURN_THROWS(); }

With ldap_bind($link, NULL), ldap_bind_dn is not NULL but an empty string?

Exactly as Tyson also just explained

2. What would changing = UNKNOWN to = NULL impact?

Explicit support for nullable types in the C implementation

3. When a function like ldap_bind is called, is calling
   ldap_bind($link),
   and
   ldap_bind($link, NULL) changing anything else than ZEND_NUM_ARGS?

This relates to point 1 and 2 above, if there is no support for nullable
types
passing null can change the meaning of it.

Then I think most of these needs to be changed to NULL with explicit
support,
as it was most likely what was intended. (the underlying C functions from
libldap accepts NULL).

This should make it very easy then just need to add the '!' specifier to
say the argument
is nullable and it would set the char* to NULL if I'm not mistaken

Also, there are tests like ldap_sasl_bind_basic.phpt which tests a NULL
parameter.

What is the dead line for this kind of changes in ext/ldap regarding to
PHP-8?

As we added another beta version before RC you have at least another week
and a half for sure.
However, I expect getting rid of UNKNOWN values can also be done in an RC
release whereas
naming should probably be fixed before RC1 comes out.

Côme

George P. Banyard

Hi Côme Chilliet,

So in the example of ldap_bind:
        if (zend_parse_parameters(ZEND_NUM_ARGS(), "r|ss", &link,
&ldap_bind_dn, &ldap_bind_dnlen, &ldap_bind_pw, &ldap_bind_pwlen) != SUCCESS)
{ RETURN_THROWS(); }

With ldap_bind($link, NULL), ldap_bind_dn is not NULL but an empty string?

I manually tested that to make sure - yes, it's the empty string in the current implementation, not null.

if (zend_parse_parameters(ZEND_NUM_ARGS(), "r|sssssss", &link, &binddn, &dn_len, ...) != SUCCESS) {
    RETURN_THROWS();
}
fprintf(stderr, "binddn=%llx binddn_len=%d\n", (long long)binddn, binddn ? (int) strlen(binddn) : 0);

Outputs binddn=23ddbd8 binddn_len=0 when the second argument is null.
So yes, it's currently an empty string, but only if null is passed.

Named parameters cannot be used to skip passing those UNKNOWN parameters right now.
ArgumentCountError: ldap_sasl_bind(): Argument #2 ($binddn) must be passed explicitly, because the default value is not known
is the output for ldap_sasl_bind($resource, props: $props)

• Tyson