Small regression in PHP-LDAP View as Markdown

Am 03.11.15 um 15:23 schrieb Bishop Bettini:

So basically my question is, is that something that should be "fixed" to
be consistent with previous PHP version, or should it stay that way as this
is what is documented and tested?

An equivalent question: Do you want to support this syntax going forward?

It's a common enough format that users likely will try it before reading
the documentation. But supporting it implies the code, the documentation,
and now a footnote that it wasn't supported between 5.6.11 and 5.6.x.

IMO, forbid it, adding a version note to the documentation: "5.6.11 --
Support for the undocumented $host format 'hostname:port' was removed."

bishop

Hi All

+1 on that solution. Just throwing "host:port" onto something doesn't
help anyone. Try that with "www.google.com:443"; in your favourite
browser. Most will give you an error (if it's not port 80). So those
people that try it will eventually read the documentation on php.net and
then see that it's not supported.

The only extension I'd see to it would be to check whether someone is
trying that and raising an error in PHP describing that "this format is
not supported! Please use ldap(s)://hostname:port instead"

@come do you see to the version note in the documentation or shall I?

Cheers

Andreas

--
,,,
(o o)
+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| http://andreas.heigl.org http://hei.gl/wiFKy7 |
+---------------------------------------------------------------------+
| http://hei.gl/root-ca |
+---------------------------------------------------------------------+

What should be clear in the documentation :

• The LDAP URI scheme is the recommended way to call ldap_connect
• The ldap:// or ldaps:// is required
• People can use host, port if they do not need SSL

Agreed.

• Second parameter is not used if the first one is an LDAP scheme, so
  calling ldap_connect('ldap://localhost', 456) will use default port 389.

I'd prefer this raise a notice, like "Second argument ignored when using
URI syntax. Port will default to 389. Did you mean 'ldap://localhost:456'
instead?"

bishop

I got mail from someone saying that in previous version, calling
ldap_connect($host, NULL) would use default port.
While now it is considered as trying to use port 0 and trigger an error.

I believe the current behavior (interpret as zero and trigger error) is
correct.

I’m a bit troubled about it because the documentation says:
resource ldap_connect ([ string $hostname = NULL [, int $port = 389 ]] )
So $port defaults to 389 and not to NULL, and it says nothing about
accepting NULL as second parameter.

Let's compare to another PHP function that takes a numeric optional
parameter with a non-zero default:

array array_unique ( array $array [, int $sort_flags = SORT_STRING ] )

Note that SORT_STRING === 2, SORT_REGULAR === 0.

If you pass null for the second argument, you get the SORT_REGULAR
behavior, not the default SORT_STRING behavior:

print_r(array_unique([ 1, '1', '1a' ], null));

That said, it does seem handful to be able to pass NULL to ask for default

port without remembering which one it is.
The context is something like:
$port = NULL;
if (isset($options['port'])) {
$port = $options['port'];
}
$resource = ldap_connect($host, $port);

A few reasons I'd offer as arguments against this. $port is deprecated, so
why add features to deprecated arguments? Other PHP internal functions
don't behave this way (array_unique, socket_create_listen, ssh2_connect,
etc.), so why go against the grain? Why not document (in a comment) the
preferred way of doing this, which might be:

if (isset($options['port']))
$resource = ldap_connect($host, $options['port']);
else
$resource = ldap_connect($host);

Right now it either needs an if statement or hardcoding 389 instead of
NULL as the default. In the C code we use the constant LDAP_PORT for this
but there is no such thing in PHP.
Any ideas/comments about this?

A PHP user-land constant like LDAP_DEFAULT_PORT might help users out
marginally, but to my knowledge no other port numbers are exposed as such
in PHP. Like ssh2_connect, for example, uses a raw 22.

If one wanted truly robust (paranoid) code, you'd probably want to do:

$port = getservbyname('ldap', 'tcp');

if (isset($options['port']) && is_numeric($options['port']))
$port = intval($options['port']);

$resource = ldap_connect($host, $port);

And this could go in a comment and solve the whole issue without code
changes.

Am 05.11.15 um 14:14 schrieb Ferenc Kovacs:
[...]

I would keep the old behavior for 5.6, even if that was unintended nobody
complained about it(so removing it isn't a bugfix per se), so I see no
reason to break userland code working before in a micro version.
for PHP-7.0 we can remove the old undocumented behavior but drop a mention
in NEWS/upgrading.

As it's already broken in the last 3 micro-versions I'm not sure whether
it makes things more complicated to "re-enable" it or not.

Personally I'd say leave it as it is now (and try to prohibit such
things in future).

Cheers

Andreas

                                                          ,,,
                                                         (o o)

+---------------------------------------------------------ooO-(_)-Ooo-+
| Andreas Heigl |
| mailto:andreas@heigl.org N 50°22'59.5" E 08°23'58" |
| http://andreas.heigl.org http://hei.gl/wiFKy7 |
+---------------------------------------------------------------------+
| http://hei.gl/root-ca |
+---------------------------------------------------------------------+