ext/ldap parameter fixes #6153

MCMic · 2020-09-17T15:15:23Z

This fixes inconsistencies between code, documentation and stub to better support NULL as an equivalent of not passing a parameter.

It also renames parameters to be consistent and concise.

For mech, realm, authcid, authzid and props NULL means do not change current server setting.

…tribute It has been unused and deprecated since PHP 5.2.4

Renamed all parameters to be consistent and concise

ext/ldap/ldap.c

Co-authored-by: Nikita Popov <[email protected]>

nikic · 2020-09-18T08:47:00Z

ext/ldap/ldap.stub.php

@@ -4,177 +4,177 @@

 #ifdef HAVE_ORALDAP
 /** @return resource|false */
-function ldap_connect(string $hostname = UNKNOWN, int $port = 389, string $wallet = UNKNOWN, string $wallet_passwd = UNKNOWN, int $authmode = GSLC_SSL_NO_AUTH) {}
+function ldap_connect(string $uri = null, int $port = 389, string $wallet = UNKNOWN, string $wallet_passwd = UNKNOWN, int $authmode = GSLC_SSL_NO_AUTH) {}


Suggested change

function ldap_connect(string $uri = null, int $port = 389, string $wallet = UNKNOWN, string $wallet_passwd = UNKNOWN, int $authmode = GSLC_SSL_NO_AUTH) {}

function ldap_connect(?string $uri = null, int $port = 389, string $wallet = UNKNOWN, string $wallet_passwd = UNKNOWN, int $authmode = GSLC_SSL_NO_AUTH) {}

and elsewhere. These need to be explicitly marked nullable. I thought the script would throw an error for that :/

I think it does throw (at least I got such an exception a few weeks ago)

Oh, ok.

Should I use ?string or string|null ? I think I’ve seen commits changing one for the other which is why I ask.

Yes, I changed a few such types. Now, we exclusively use ? for nullable types where it is possible, so please use this format.

Crell · 2020-09-21T14:25:40Z

ext/ldap/ldap.stub.php

-/** @param resource $link_identifier */
-function ldap_unbind($link_identifier): bool {}
+/** @param resource $link */
+function ldap_unbind($link): bool {}


Over in #6172 we're changing $mysql_link to $connection. It's probably wise to do the same here. I don't know anyone anywhere that calls this thing a "link".

Hum, it was called $link in some functions and $link_identifier in others which is why I changed to use $link everywhere.

I usually call this $ldap, I think I’ve seen $cid for connection id.

I’m not sure $connection is better, it’s way longer for sure (and for me prone to errors since we write "connexion" in french).

Are there other occurencies than mysql?
Can other people give their opinion (especially native english speakers)?

hello @MCMic i'am also in favour of $ldap i think it make more sense in the context of a ldap library

Cheers

The mysqli change ended up deciding on $mysql as the name, so $ldap would be the one to use here now.

nikic

A couple suggestions for the parameter naming, mostly around using _ to separate words.

ext/ldap/ldap.stub.php

nikic · 2020-09-28T07:57:12Z

ext/ldap/ldap.stub.php

-/** @param resource $link_identifier */
-function ldap_unbind($link_identifier): bool {}
+/** @param resource $link */
+function ldap_unbind($link): bool {}


The mysqli change ended up deciding on $mysql as the name, so $ldap would be the one to use here now.

ext/ldap/ldap.stub.php

Co-authored-by: Nikita Popov <[email protected]>

Also fixed ldap_set_option places where $newvalue was still used

MCMic · 2020-09-28T14:24:37Z

@nikic Should be good now

nikic

Maybe @kocsismate can take a look as well. Sorry for the naming bikeshed :)

ext/ldap/ldap.stub.php

Co-authored-by: Nikita Popov <[email protected]>

kocsismate

I added a few questions/suggestions. And I'm also sorry for the prolonged bikeshed :)

kocsismate · 2020-09-28T14:46:40Z

ext/ldap/ldap.stub.php

 * @return resource|false
 */
-function ldap_read($link_identifier, array|string $base_dn, array|string $filter, array $attributes = [], int $attrsonly = 0, int $sizelimit = -1, int $timelimit = -1, int $deref = LDAP_DEREF_NEVER, array $servercontrols = []) {}
+function ldap_read($ldap, array|string $base, array|string $filter, array $attributes = [], int $attrsonly = 0, int $sizelimit = -1, int $timelimit = -1, int $deref = LDAP_DEREF_NEVER, array $controls = []) {}


Suggested change

function ldap_read($ldap, array|string $base, array|string $filter, array $attributes = [], int $attrsonly = 0, int $sizelimit = -1, int $timelimit = -1, int $deref = LDAP_DEREF_NEVER, array $controls = []) {}

function ldap_read($ldap, array|string $base, array|string $filter, array $attributes = [], int $attributes_only = 0, int $size_limit = -1, int $time_limit = -1, int $deref = LDAP_DEREF_NEVER, array $controls = []) {}

See above, @nikic proposed the same, but both openldap and php use sizelimit and timelimit in other places, it’s always have been a whole word.

I can change $attrsonly though.

I don't see a big problem with diverging from these other names, but I also don't mind if these stay without the _. So I'm ok.

kocsismate · 2020-09-28T14:46:53Z

ext/ldap/ldap.stub.php

 * @return resource|false
 */
-function ldap_list($link_identifier, array|string $base_dn, array|string $filter, array $attributes = [], int $attrsonly = 0, int $sizelimit = -1, int $timelimit = -1, int $deref = LDAP_DEREF_NEVER, array $servercontrols = []) {}
+function ldap_list($ldap, array|string $base, array|string $filter, array $attributes = [], int $attrsonly = 0, int $sizelimit = -1, int $timelimit = -1, int $deref = LDAP_DEREF_NEVER, array $controls = []) {}


same here (and one more below)

kocsismate · 2020-09-28T14:49:03Z

ext/ldap/ldap.stub.php

 * @deprecated since 7.4
 */
-function ldap_control_paged_result($link, int $pagesize, bool $iscritical = false, string $cookie = ""): bool {}
+function ldap_control_paged_result($ldap, int $pagesize, bool $iscritical = false, string $cookie = ""): bool {}


Suggested change

function ldap_control_paged_result($ldap, int $pagesize, bool $iscritical = false, string $cookie = ""): bool {}

function ldap_control_paged_result($ldap, int $page_size, bool $critical = false, string $cookie = ""): bool {}

Could just $size work? Nevertheless, using the page_ prefix is perfectly fine for me.
$is_critical would be ok for me as well, but I think there's an agreement that we don't use the is_ prefix

Actually this function is deprecated since PHP-7.4. Can it be removed in PHP-8?

Yes, I think so.

Yes, it should be fine to drop. It looks like another deprecated function (ldap_sort) did get dropped already. I'd suggest landing that separately from this PR (just directly commit it with a note in UPGRADING).

Ok, I will do that.

As we should land this today to ensure the removal goes into PHP 8.0, I've put up #6235. Does that look reasonable?

(Feel free to land your own version though, I just want to make sure it goes in...)

kocsismate · 2020-09-28T14:50:36Z

ext/ldap/ldap.stub.php

 * @param array|string|int $retval
 */
-function ldap_get_option($link_identifier, int $option, &$retval = null): bool {}
+function ldap_get_option($ldap, int $option, &$retval = null): bool {}


Suggested change

function ldap_get_option($ldap, int $option, &$retval = null): bool {}

function ldap_get_option($ldap, int $option, &$return_value = null): bool {}

or just $value? the manual says "This will be set to the option value.", so maybe this name would be ok.

kocsismate · 2020-09-28T14:51:07Z

ext/ldap/ldap.stub.php

 */
-function ldap_parse_result($link, $result, &$errcode, &$matcheddn = null, &$errmsg = null, &$referrals = null, &$serverctrls = null): bool {}
+function ldap_parse_result($ldap, $result, &$errcode, &$matcheddn = null, &$errmsg = null, &$referrals = null, &$controls = null): bool {}


Suggested change

function ldap_parse_result($ldap, $result, &$errcode, &$matcheddn = null, &$errmsg = null, &$referrals = null, &$controls = null): bool {}

function ldap_parse_result($ldap, $result, &$error_code, &$matched_dn = null, &$error_message = null, &$referrals = null, &$controls = null): bool {}

$errno could also be used, but so far $error_code is what we preferred in another PR.

kocsismate · 2020-09-28T14:53:17Z

ext/ldap/ldap.stub.php

 * @param string $retdata
 * @param string $retoid
 * @return resource|bool
 */
-function ldap_exop($link, string $reqoid, ?string $reqdata = null, ?array $servercontrols = [], &$retdata = null, &$retoid = null) {}
+function ldap_exop($ldap, string $reqoid, ?string $reqdata = null, ?array $controls = [], &$retdata = null, &$retoid = null) {}


Would using a $request_ prefix work well? And $response_data or something similar?

kocsismate · 2020-09-28T14:53:42Z

ext/ldap/ldap.stub.php

 * @param resource $result
 * @param string $retdata
 * @param string $retoid
 */
-function ldap_parse_exop($link, $result, &$retdata = null, &$retoid = null): bool {}
+function ldap_parse_exop($ldap, $result, &$retdata = null, &$retoid = null): bool {}


same questions apply :)

MCMic · 2020-09-29T07:37:50Z

@kocsismate Should be good now, except for the deprecated functions.

kocsismate

Looks good!

MCMic · 2020-09-29T15:03:50Z

I merged this

Côme Chilliet added 7 commits September 17, 2020 16:33

Make URI parameter in ldap_connect default to NULL

0dfede7

Default bind dn and password to NULL in ldap_bind

a4b7df5

Default bind dn and password to NULL in ldap_bind_ext

8667f5c

Set all parameters to ldap_sasl_bind as NULL by default

d2645cb

For mech, realm, authcid, authzid and props NULL means do not change current server setting.

Remove dummy_ber parameter from ldap_first_attribute and ldap_next_at…

cd38890

…tribute It has been unused and deprecated since PHP 5.2.4

Fix "null" case in ldap.stub.php

d2e13e3

Fix parameter names in ext/ldap

1b828b8

Renamed all parameters to be consistent and concise

MCMic mentioned this pull request Sep 17, 2020

Improve remaining stubs with = UNKNOWN where possible php/php-tasks#24

Closed

nikic reviewed Sep 17, 2020

View reviewed changes

ext/ldap/ldap.c Outdated Show resolved Hide resolved

Fix parameter mask in ldap_next_attribute

e2f91d6

Co-authored-by: Nikita Popov <[email protected]>

nikic reviewed Sep 18, 2020

View reviewed changes

nikic mentioned this pull request Sep 21, 2020

Rename mysqli parameters to be more logical. #6172

Closed

Explicitely mark nullable strings

79f0efe

Crell reviewed Sep 21, 2020

View reviewed changes

Girgias mentioned this pull request Sep 24, 2020

Consistent named param: Extension Tracker php/php-tasks#23

Closed

65 tasks

Girgias added the Parameter names label Sep 24, 2020

nikic reviewed Sep 28, 2020

View reviewed changes

MCMic and others added 5 commits September 28, 2020 14:18

Improve parameter naming in ldap_set_option ext/ldap/ldap.stub.php

6141ba8

Co-authored-by: Nikita Popov <[email protected]>

Improve parameter naming in ldap_connect

7c4e1aa

Co-authored-by: Nikita Popov <[email protected]>

Improve parameter naming in ldap_exop_passwd

998814a

Co-authored-by: Nikita Popov <[email protected]>

Improve parameter naming in ldap_rename

104fef4

Co-authored-by: Nikita Popov <[email protected]>

Rename $link parameters to $ldap in ldap functions

2cf6e24

Also fixed ldap_set_option places where $newvalue was still used

nikic reviewed Sep 28, 2020

View reviewed changes

ext/ldap/ldap.stub.php Outdated Show resolved Hide resolved

Harmonize ldap_rename_ext with ldap_rename parameter names

36830f6

Co-authored-by: Nikita Popov <[email protected]>

kocsismate reviewed Sep 28, 2020

View reviewed changes

Improve parameter names in ext/ldap

ac12ce1

kocsismate reviewed Sep 29, 2020

View reviewed changes

MCMic closed this Sep 29, 2020

nikic mentioned this pull request Sep 30, 2020

Remove deprecated ldap_control_paged_result(_response) #6235

Closed

MCMic mentioned this pull request Nov 19, 2020

LDAP signatures errors phpstan/phpstan#4098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ext/ldap parameter fixes #6153

ext/ldap parameter fixes #6153

MCMic commented Sep 17, 2020

nikic Sep 18, 2020

kocsismate Sep 18, 2020 •

edited

Loading

MCMic Sep 18, 2020

kocsismate Sep 18, 2020 •

edited

Loading

Crell Sep 21, 2020

MCMic Sep 21, 2020

bilbo-the-hobbit Sep 24, 2020

nikic Sep 28, 2020

nikic left a comment

nikic Sep 28, 2020

MCMic commented Sep 28, 2020

nikic left a comment

kocsismate left a comment

kocsismate Sep 28, 2020

MCMic Sep 29, 2020

kocsismate Sep 29, 2020

kocsismate Sep 28, 2020

kocsismate Sep 28, 2020

MCMic Sep 29, 2020

kocsismate Sep 29, 2020

nikic Sep 29, 2020

MCMic Sep 29, 2020

nikic Sep 29, 2020 •

edited

Loading

kocsismate Sep 28, 2020 •

edited

Loading

kocsismate Sep 28, 2020

kocsismate Sep 28, 2020

kocsismate Sep 28, 2020

MCMic commented Sep 29, 2020

kocsismate left a comment

MCMic commented Sep 29, 2020

	function ldap_connect(string $uri = null, int $port = 389, string $wallet = UNKNOWN, string $wallet_passwd = UNKNOWN, int $authmode = GSLC_SSL_NO_AUTH) {}
	function ldap_connect(?string $uri = null, int $port = 389, string $wallet = UNKNOWN, string $wallet_passwd = UNKNOWN, int $authmode = GSLC_SSL_NO_AUTH) {}

	function ldap_read($ldap, array\|string $base, array\|string $filter, array $attributes = [], int $attrsonly = 0, int $sizelimit = -1, int $timelimit = -1, int $deref = LDAP_DEREF_NEVER, array $controls = []) {}
	function ldap_read($ldap, array\|string $base, array\|string $filter, array $attributes = [], int $attributes_only = 0, int $size_limit = -1, int $time_limit = -1, int $deref = LDAP_DEREF_NEVER, array $controls = []) {}

	function ldap_control_paged_result($ldap, int $pagesize, bool $iscritical = false, string $cookie = ""): bool {}
	function ldap_control_paged_result($ldap, int $page_size, bool $critical = false, string $cookie = ""): bool {}

	function ldap_get_option($ldap, int $option, &$retval = null): bool {}
	function ldap_get_option($ldap, int $option, &$return_value = null): bool {}

	function ldap_parse_result($ldap, $result, &$errcode, &$matcheddn = null, &$errmsg = null, &$referrals = null, &$controls = null): bool {}
	function ldap_parse_result($ldap, $result, &$error_code, &$matched_dn = null, &$error_message = null, &$referrals = null, &$controls = null): bool {}

ext/ldap parameter fixes #6153

ext/ldap parameter fixes #6153

Conversation

MCMic commented Sep 17, 2020

Choose a reason for hiding this comment

kocsismate Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocsismate Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MCMic commented Sep 28, 2020

nikic left a comment

Choose a reason for hiding this comment

kocsismate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

kocsismate Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MCMic commented Sep 29, 2020

kocsismate left a comment

Choose a reason for hiding this comment

MCMic commented Sep 29, 2020

kocsismate Sep 18, 2020 •

edited

Loading

kocsismate Sep 18, 2020 •

edited

Loading

nikic Sep 29, 2020 •

edited

Loading

kocsismate Sep 28, 2020 •

edited

Loading