Everything

php

Home » Project » Newsgroups » php.internals » Stubs in ext/ldap »

Stubs in ext/ldap

php.internals

Côme Chilliet

5 years ago

Hello,

After playing with phpstan and seeing their stubs for some LDAP functions were
wrong, I noticed that documentation for LDAP functions and stubs in
ext/ldap/ldap.stub.php differs.

Especially in a lot of functions, the documentation states "string $dn = NULL"
but the stub states non-nullable string parameter. Also, the ldap.stub.php
file contains a lot of "string $bind_rdn = UNKNOWN" in places where the
documentations states = NULL. What does UNKOWN mean here?

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?
2) What would changing = UNKNOWN to = NULL impact?
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?

Côme

Girgias

5 years ago

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

> Hello,
>
> After playing with phpstan and seeing their stubs for some LDAP functions
> were
>  wrong, I noticed that documentation for LDAP functions and stubs in
>  ext/ldap/ldap.stub.php differs.
>

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.

> Especially in a lot of functions, the documentation states "string $dn =
> NULL"
>  but the stub states non-nullable string parameter. Also, the ldap.stub.php
>  file contains a lot of "string $bind_rdn = UNKNOWN" in places where the
>  documentations states = NULL. What does UNKOWN mean here?
>

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.

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

Côme

>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>

Hope this answers most of the questions

Best regards

George P. Banyard

tyson andre

5 years ago

Hi Côme Chilliet and G. P. B.,

> Hello,
>
> After playing with phpstan and seeing their stubs for some LDAP functions
> were
>  wrong, I noticed that documentation for LDAP functions and stubs in
>  ext/ldap/ldap.stub.php differs.

Also see discussion in https://github.com/phan/phan/issues/4194

https://github.com/php/php-src/blob/master/ext/ldap/tests/ldap_sasl_bind_basic.phpt#L26
is a test that passes null to a string parameter
of https://www.php.net/manual/en/function.ldap-sasl-bind.php . It seems like it silently gets coerced to the empty string
when strict_types=0 and throws a TypeError when strict_types=1

Cheers,
- Tyson

Côme Chilliet

5 years ago

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.

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

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

Côme

Girgias

5 years ago

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

tyson andre

5 years ago

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