Fixed warning in ldap_set_rebind_proc()

[ptomulik]

No description provided.

[kocsismate]

@ptomulik Where is your question regarding the empty string case? I remember that I received an email notification about it.

Unfortunately, I hadn't seen that ldap_set_rebind_proc() also accepts an empty string before I posted my comment. What a mess :(

Now, I'm not exactly sure what to do.

• We could say in the error message that either a callable or an empty string is allowed. In this case, we can't add the callable type to the stub :(
• Ideally, we should accept null instead of "". But this is really not a nice way to introduce a BC break :(

@nikic Should we go for the first one, or do you have a better suggestion?

[ptomulik]

@ptomulik Where is your question regarding the empty string case? I remember that I received an email notification about it.

I'm sorry, I've deleted it (after writing it I found it irrevelant).

Unfortunately, I hadn't seen that ldap_set_rebind_proc() also accepts an empty string before I posted my comment. What a mess :(

Now, I'm not exactly sure what to do.

I believe, empty string is still accepted after the change in ldap.stub.php. At least as I've seen it in my local manual tests. Do any of CI run tests with ldap enabled?

* We could say in the error message that either a callable or an empty string is allowed. In this case, we can't add the `callable` type to the stub :(

I have no strict knowledge about the purpose of ldap.stub.php, but it looks like it's only a declarative data (for documentation?, for reflection?). The actual parameter parsing and checking is done within the implementation of ldap_set_rebind_proc() wrapper, which didn't changed in this scope and it still seems to accept empty string.

* Ideally, we should accept null instead of `""`. But this is really not a nice way to introduce a BC break :(

@nikic Should we go for the first one, or do you have a better suggestion?

[ptomulik]

* We could say in the error message that either a callable or an empty string is allowed. In this case, we can't add the `callable` type to the stub :(

.. or add callable|string (php-8 union types) and say in error message as above (also, documentation needs to be revised then).

[nikic]

Especially as the current behavior is undocumented (https://www.php.net/ldap_set_rebind_proc), I think it may be acceptable to change this from "" to null in PHP 8. The current behavior is really quite unusual.

However, I don't have any particular familiarity with LDAP and don't know whether removing the callback via ldap_set_rebind_proc is a common operation (I would assume no). Maybe @MCMic can chime in, who is the maintainer of this extension if I remember correctly.

[nikic]

@ptomulik To answer the other questions: The type specified in the stub still needs to be correct, and there will be failures in debug builds if it isn't (assuming the test coverage exists). LDAP is being built in CI to check it compiles, but we don't have the necessary setup to actually run the tests.

[ptomulik]

Just changed the function signature and few lines of code to make the $callback nullable with minimal effort. The related tests seem to pass. Just wonder whether parsing the parameter as "mixed" (zend_parse_parameters with "z") is the way to go. No experience in this matter.

ptomulik@barakus:$ LDAP_TEST_HOST=172.18.0.2 LDAP_TEST_BASE="dc=example,dc=org" LDAP_TEST_USER='cn=admin,dc=example,dc=org' LDAP_TEST_PASSWD='admin' sapi/cli/php ./run-tests.php ext/ldap/tests/ldap_set_rebind_proc_basic.phpt ext/ldap/tests/ldap_set_rebind_proc_error.phpt

=====================================================================
PHP         : /home/ptomulik/sources/php-src/sapi/cli/php 
PHP_SAPI    : cli
PHP_VERSION : 8.0.0-dev
ZEND_VERSION: 4.0.0-dev
PHP_OS      : Linux - Linux barakus 5.6.0-2-amd64 #1 SMP Debian 5.6.14-2 (2020-06-09) x86_64
INI actual  : /home/ptomulik/sources/php-src
More .INIs  :   
---------------------------------------------------------------------
PHP         : /home/ptomulik/sources/php-src/sapi/phpdbg/phpdbg 
PHP_SAPI    : phpdbg
PHP_VERSION : 8.0.0-dev
ZEND_VERSION: 4.0.0-dev
PHP_OS      : Linux - Linux barakus 5.6.0-2-amd64 #1 SMP Debian 5.6.14-2 (2020-06-09) x86_64
INI actual  : /home/ptomulik/sources/php-src
More .INIs  : 
---------------------------------------------------------------------
CWD         : /home/ptomulik/sources/php-src
Extra dirs  : 
VALGRIND    : Not used
=====================================================================
Running selected tests.
PASS ldap_set_rebind_proc() - Basic ldap_set_rebind_proc test [ext/ldap/tests/ldap_set_rebind_proc_basic.phpt] 
PASS ldap_set_rebind_proc() - Testing ldap_set_rebind_proc() that should fail [ext/ldap/tests/ldap_set_rebind_proc_error.phpt] 
=====================================================================
Number of tests :    2                 2
Tests skipped   :    0 (  0.0%) --------
Tests warned    :    0 (  0.0%) (  0.0%)
Tests failed    :    0 (  0.0%) (  0.0%)
Tests passed    :    2 (100.0%) (100.0%)
---------------------------------------------------------------------
Time taken      :    0 seconds
=====================================================================

[kocsismate]

@ptomulik The patch looks good for me, but let's wait for Nikita's review. However, could you please also add an entry to the UPGRADING file?

[nikic]

LGTM

[ptomulik]

@ptomulik The patch looks good for me, but let's wait for Nikita's review. However, could you please also add an entry to the UPGRADING file?

Done.

[ptomulik]

Thanks!

[MCMic]

Hello,

Sorry for the delay on looking into this, glad to see it was handled :-)

I’m fine with the change to NULL, seems consistent, as long as it’s listed in UPGRADING.

@ptomulik I may have missed it but what is the warning that this PR fixed, I did not find the information?

[ptomulik]

Hello,

Sorry for the delay on looking into this, glad to see it was handled :-)

I’m fine with the change to NULL, seems consistent, as long as it’s listed in UPGRADING.

@ptomulik I may have missed it but what is the warning that this PR fixed, I did not find the information?

In ldap.c around line 3766:

-               zend_string *callback_name = zend_get_callable_name(callback);
- 		php_error_docref(NULL, E_WARNING, "Two arguments expected for '%s' to be a valid callback", ZSTR_VAL(callback_name));
+	        zend_argument_type_error(2, "must be a valid callback or null, %s given", zend_zval_type_name(callback));

The previous version yielded something like "Two arguments expected for $callback to be a valid callback", which looked quite "non-standard".