# Small regression in PHP-LDAP

- Thread: https://externals.io/message/89047
- Original archive: https://news-web.php.net/php.internals/89047
- Started by come@opensides.be on 2015-11-03T05:03:38+00:00
- Messages: 25
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #89047 — come@opensides.be — 2015-11-03T05:03:38+00:00

URL: https://externals.io/message/89047#89047

Hello,

It has been brought to my attention that some PHP code using LDAP connect was broken by the update to PHP 5.6.11.
This is because the use on ldap_connect(host:port) is not allowed anymore.
You have to use either ldap_connect(host, port) or ldap_connect(ldap://host:port).

The use of ldap_connect(host:port) without ldap:// is not something that is in the tests, and is not explicitely allowed by our documentation: http://php.net/manual/en/function.ldap-connect.php
(Maybe it could be made clearer than when not using ldaps://, ldap:// is still needed for an LDAP URI to be valid)

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?

Here are the problems people encountered because of this change (most assume they have to use host and port separately and do not think to add the ldap:// in there):
https://www.netways.org/issues/2931
https://dev.icinga.org/issues/9298
https://github.com/owncloud/core/issues/20020

MCMic

---

## #89051 — Bishop Bettini — 2015-11-03T14:23:22+00:00

URL: https://externals.io/message/89047#89051 · In reply to: #89047

> 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

---

## #89052 — Andreas Heigl — 2015-11-03T14:39:33+00:00

URL: https://externals.io/message/89047#89052 · In reply to: #89051

Am 03.11.15 um 15:23 schrieb Bishop Bettini:
> 
> 
> > […]
> 
> 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                                               |
+---------------------------------------------------------------------+

---

## #89056 — Andrea Faulds — 2015-11-03T19:07:33+00:00

URL: https://externals.io/message/89047#89056 · In reply to: #89052

Hi Andreas,

Andreas Heigl wrote:
> +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.

If you give the port number for a non-HTTP TCP application then yes, 
your browser will spit an error back at you, because HTTP is the 
implicit URI scheme. That doesn't mean hostname:port is wrong, it's 
perfectly valid for HTTP. Similarly, I could give my LDAP URI as 
google.com:80 and get an error, but I don't think that's a fault with 
hostname:port.

The PHP interpreter itself uses it for HTTP:

$ php -S localhost:8000

I'm unconvinced.

Thanks.
-- 
Andrea Faulds
http://ajf.me/

---

## #89058 — Andreas Heigl — 2015-11-03T19:41:49+00:00

URL: https://externals.io/message/89047#89058 · In reply to: #89056

Am 03.11.15 um 20:07 schrieb Andrea Faulds:
> Hi Andreas,
> 
> Andreas Heigl wrote:
> > […]
> 
> If you give the port number for a non-HTTP TCP application then yes,
> your browser will spit an error back at you, because HTTP is the
> implicit URI scheme. That doesn't mean hostname:port is wrong, it's
> perfectly valid for HTTP. Similarly, I could give my LDAP URI as
> google.com:80 and get an error, but I don't think that's a fault with
> hostname:port.
> 
> The PHP interpreter itself uses it for HTTP:
> 
> $ php -S localhost:8000
> 
> I'm unconvinced.

Hi Andrea.

The documentation mentions two different ways of using ldap_connect:
either with 2 parameters being host and port (from which the port is
optional) or with a single parameter using an LDAP-URI. There is a third
possibility mentioned in one of the comments of passing in a string of
multiple LDAP-URIs separated by a space. But I couldn't find a mention
of "host:port".

As there is an explicit mention of an URI the scheme is mandatory. And
as the scheme is used to differentiate between ldap and ldaps it's a
somewhat different story than the paramteter to phps internal server as
that one ONLY serves http (AFAIK). And it is explicitly stated that the
first version of ldap_connect with two parameters *only* allows
connections via ldap (not ldaps). If you want to use ldaps as scheme you
are required to use the second form with the LDAP-URI. So if someone
passes a single parameter 'host:port' to ldap_connect I'm sure they want
to use ldaps as protocol?

In my eyes the main question is whether we want to support something
that was never intended and nowhere documented just because someone
tried something and by luck didn't break something. I'd consider the
possibility to pass 'host:port' as single parameter to ldap_connect as a
bug and not as a feature. Therefore I'd rather see that bug fixed sooner
than later. And have the documentation point out explicitly that using
'host:port' without a scheme is NOT supported even though it might have
once worked.

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

---

## #89064 — come@opensides.be — 2015-11-04T03:28:20+00:00

URL: https://externals.io/message/89047#89064

Le mardi 3 novembre 2015, 18:59:42 Andrea Faulds a écrit :
> I'd argue this is a regression, documented feature or not. Supporting 
> 'host:port' is not a bug, and this has broken existing code, which micro 
> (x.y.Z) versions are not allowed to do. It should be fixed ASAP.

So I got two votes against and one for putting host:port back.

I feel bad about pissing off users that used host:port, but it does seems safer to require the scheme.

Detecting the host:port syntax only to show a warning seems tedious. It’s like saying to the user «I get what you want me to do, but I won’t do it».

And the point that it’s already been gone since a few version anyway is also a good point, people would have a code that works on some versions and not others, let’s save them the nightmare.

After reading the reactions on the bug tickets related to this problem, it seems users are understanding the change and adapting without problem, so I’m for fixing the documentation on this one. Discussing the details on a separate email.

MCMic

---

## #89065 — come@opensides.be — 2015-11-04T04:00:38+00:00

URL: https://externals.io/message/89047#89065 · In reply to: #89047

Hello,

About ldap_connect documentation, we got an interesting proposition there: https://github.com/owncloud/core/issues/20020#issuecomment-153387886
The idea is to documment the LDAP URI version as a default and only list the host, port syntax as a backward compatibilty thing.
Which it is IMO.
The fact that when using (host, port) SSL is not supported goes strongly in that direction.
http://php.net/manual/en/ldap.requirements.php already says OpenLDAP>=2.4 is needed so «If you are using OpenLDAP 2.x.x» can be removed.
 
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
- 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.

Does that seem ok?

MCMic

PS: just saw that Andreas Heigl already edited the doc, to some of it is already fixed. It could still be improved to recommend LDAP URIS more clearly.

---

## #89066 — come@opensides.be — 2015-11-04T04:13:35+00:00

URL: https://externals.io/message/89047#89066 · In reply to: #89047

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

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);

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?

MCMic

---

## #89070 — Bishop Bettini — 2015-11-05T02:10:09+00:00

URL: https://externals.io/message/89047#89070 · In reply to: #89065

> 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

---

## #89071 — Bishop Bettini — 2015-11-05T02:41:20+00:00

URL: https://externals.io/message/89047#89071 · In reply to: #89066

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

---

## #89072 — come@opensides.be — 2015-11-05T03:20:37+00:00

URL: https://externals.io/message/89047#89072 · In reply to: #89070

Le mercredi 4 novembre 2015, 21:10:09 Bishop Bettini a écrit :
> > […]
> 
> 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?"

Good idea, this will avoid confusion. I will add this.

MCMic

---

## #89073 — come@opensides.be — 2015-11-05T03:26:34+00:00

URL: https://externals.io/message/89047#89073 · In reply to: #89071

Le mercredi 4 novembre 2015, 21:41:20 Bishop Bettini a écrit :
> 
> 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.

Ok, so we could just put in the documentation the tip about getservbyname if one does not want to use 389 directly.

---

## #89074 — come@opensides.be — 2015-11-05T04:30:57+00:00

URL: https://externals.io/message/89047#89074 · In reply to: #89072

Does this seems correct?

https://github.com/php/php-src/pull/1618

---

## #89076 — Ferenc Kovacs — 2015-11-05T13:14:54+00:00

URL: https://externals.io/message/89047#89076 · In reply to: #89064

> Le mardi 3 novembre 2015, 18:59:42 Andrea Faulds a écrit :
> > […]
>
> So I got two votes against and one for putting host:port back.
>
> I feel bad about pissing off users that used host:port, but it does seems
> safer to require the scheme.
>
> Detecting the host:port syntax only to show a warning seems tedious. It’s
> like saying to the user «I get what you want me to do, but I won’t do it».
>
> And the point that it’s already been gone since a few version anyway is
> also a good point, people would have a code that works on some versions and
> not others, let’s save them the nightmare.
>
> After reading the reactions on the bug tickets related to this problem, it
> seems users are understanding the change and adapting without problem, so
> I’m for fixing the documentation on this one. Discussing the details on a
> separate email.
>
> MCMic
>
> --
> 
> 
>
>
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.

-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu

---

## #89077 — Andreas Heigl — 2015-11-05T13:26:27+00:00

URL: https://externals.io/message/89047#89077 · In reply to: #89076

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

---

## #89078 — Ferenc Kovacs — 2015-11-05T13:56:45+00:00

URL: https://externals.io/message/89047#89078 · In reply to: #89077

> Am 05.11.15 um 14:14 schrieb Ferenc Kovacs:
> [...]
> > […]
> mention
> > […]
> 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                                               |
> +---------------------------------------------------------------------+
>
>
5.6 is not even halfway until EOL, so I think that argument of keeping the
BC break because there are already 3 micro versions affected it is a bit
weak:
http://php.net/supported-versions.php

-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu

---

## #89085 — Bishop Bettini — 2015-11-05T16:38:44+00:00

URL: https://externals.io/message/89047#89085 · In reply to: #89078

> 
>
> > […]
> nobody
> > […]
> 5.6 is not even halfway until EOL, so I think that argument of keeping the
> BC break because there are already 3 micro versions affected it is a bit
> weak:
> http://php.net/supported-versions.php


Some are vendor-pinned and can't get the upgrade, so they have to fix their
code anyway.  Those who can upgrade would have to fix their code by 7.0,
and IMO it seems better to fix it now while its on their mind.

We're talking about a very small surface area of affected code, one that is
easily changed with a sed. The damage of "breaking the behavior" is already
done. Fixing user code or upgrading the engine is the only resolution. To
me, fixing user code is the best solution: it's long term necessary, it's
short term easy.  If this were breaking documented code (as happened with
array_unique in 5.2.9), then I'd say fix the engine. But it's not, it's
breaking undocumented side-effected user code.  That to me sounds like a
user code fix.

---

## #89087 — Ferenc Kovacs — 2015-11-05T19:37:25+00:00

URL: https://externals.io/message/89047#89087 · In reply to: #89085

> 
>
> > […]
>
>
> Some are vendor-pinned and can't get the upgrade, so they have to fix
> their code anyway.  Those who can upgrade would have to fix their code by
> 7.0, and IMO it seems better to fix it now while its on their mind.
>
> We're talking about a very small surface area of affected code, one that
> is easily changed with a sed. The damage of "breaking the behavior" is
> already done. Fixing user code or upgrading the engine is the only
> resolution. To me, fixing user code is the best solution: it's long term
> necessary, it's short term easy.  If this were breaking documented code (as
> happened with array_unique in 5.2.9), then I'd say fix the engine. But it's
> not, it's breaking undocumented side-effected user code.  That to me sounds
> like a user code fix.
>

and some will be pinned to the version before the BC break, some after the
possible fix, some will backport this fix anyways when their users complain.

-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu

---

## #89088 — Adam Howard — 2015-11-05T20:53:44+00:00

URL: https://externals.io/message/89047#89088 · In reply to: #89087

I don't think it is possible to make everyone happy all the time.  I think
this should be kept for a user code fix.



> 
>
> > […]
> wrote:
> > […]
> no
> > […]
> whether
> > […]
> the
> > […]
> (as
> > […]
> it's
> > […]
> sounds
> > […]
>
> and some will be pinned to the version before the BC break, some after the
> possible fix, some will backport this fix anyways when their users
> complain.
>
> --
> Ferenc Kovács
> @Tyr43l - http://tyrael.hu

---

## #89090 — Ferenc Kovacs — 2015-11-06T00:07:08+00:00

URL: https://externals.io/message/89047#89090 · In reply to: #89088

> I don't think it is possible to make everyone happy all the time.  I think
> this should be kept for a user code fix.
>
>
please don't top post on the php.net mailing lists.
on one hand this change doesn't really make any person happy (apart maybe
the ext/ldap maintainers) but already caused headache for people upgrading
to a patch release and expecting their code to still work.
I don't think that it is tradeoff worth taking.

now I've just took the time to look the actual commit which break this
feature:
https://github.com/php/php-src/commit/e7af0fe1eb89e40671e86a588aa1b78607b85461
previously we were just passing host and port to ldap_init:
https://github.com/php/php-src/commit/e7af0fe1eb89e40671e86a588aa1b78607b85461#diff-baba61601f82518312197f5420a76ae7L346
and the documentation for ldap_init states that
*ldap_open()* takes *host*, the hostname on which the LDAP server is
running, and *port*, the port number to which to connect. If the default
IANA-assigned port of 389 is desired, LDAP_PORT should be specified for
*port*. The *host* parameter may contain a blank-separated list of hosts to
try to connect to, and each host may optionally by of the form *host:port*.
If present, the *:port* overrides the *port* parameter to *ldap_open()*.
http://linux.die.net/man/3/ldap_init

so this wasn't just a bogus feature but php being the glue and the
underlying library providing this feature, then when we removed the else
block to remove the usage of the now deprecated ldap_open we lost this
feature.
it would be fairly trivial to transform host:port formatted hosts to be
transformed to the new schema://host:port format required by
ldap_initialize (and ldap_is_ldap_url()) instead of just assuming that
receiving an url not validated by ldap_is_ldap_url() means that we received
a naked hostname

-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu

---

## #89094 — Ferenc Kovacs — 2015-11-06T10:43:58+00:00

URL: https://externals.io/message/89047#89094 · In reply to: #89071

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

we have precedence for both behavior.


>
>
>
> > […]
>
> 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));
>

on the other hand functions like ftp_connect assumes the default port when
passed NULL:

[tyrael@Ferencs-MBP]$ php -r '$c=ftp_connect("ftp.de.debian.org",
NULL);ftp_login($c, "anonymous", "");print_r(ftp_nlist($c, "-la ."));'
Array
(
    [0] => debian-security
    [1] => backports.org
    [2] => debian-archive
    [3] => .
    [4] => HEADER.html
    [5] => .welcome.msg
    [6] => debian-backports
    [7] => debian-ports
    [8] => debian-cd
    [9] => debian
    [10] => ..
    [11] => pub
)


>
>
> That said, it does seem handful to be able to pass `NULL` to ask for default
> > […]
>
> 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);
>

I'm fine with changing this for future versions but my understanding is
that this BC break (along with some others) was introduced in a micro
release (5.6.11 afair), so at least for 5.6 I would prefer restoring the
old behavior and for 7.0 I'm fine with the change but please document it in
NEWS and UPGRADING so we can incorporate it in the docs.


-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu

---

## #89194 — come@opensides.be — 2015-11-12T06:09:52+00:00

URL: https://externals.io/message/89047#89194 · In reply to: #89094

Le vendredi 6 novembre 2015, 11:43:58 Ferenc Kovacs a écrit :
> 
> on the other hand functions like ftp_connect assumes the default port when
> passed NULL:
> 
> I'm fine with changing this for future versions but my understanding is
> that this BC break (along with some others) was introduced in a micro
> release (5.6.11 afair), so at least for 5.6 I would prefer restoring the
> old behavior and for 7.0 I'm fine with the change but please document it in
> NEWS and UPGRADING so we can incorporate it in the docs.
> 

Ok, so I pushed a fix to treat `NULL` (and 0 for that matter, same as in ftp_connect) into the default ldap port.
I pushed only to 5.6 as I think for 7 we should keep the error, it does not makes much sense to accept `NULL` as port and the documentation does not mention it.

MCMic

---

## #89195 — Ferenc Kovacs — 2015-11-12T08:39:52+00:00

URL: https://externals.io/message/89047#89195 · In reply to: #89194

> Le vendredi 6 novembre 2015, 11:43:58 Ferenc Kovacs a écrit :
> > […]
> when
> > […]
> in
> > […]
>
> Ok, so I pushed a fix to treat `NULL` (and 0 for that matter, same as in
> ftp_connect) into the default ldap port.
> I pushed only to 5.6 as I think for 7 we should keep the error, it does
> not makes much sense to accept `NULL` as port and the documentation does not
> mention it.
>
> MCMic
>
> --
> 
> 
>
>
thanks!

-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu

---

## #89215 — Ferenc Kovacs — 2015-11-14T15:06:14+00:00

URL: https://externals.io/message/89047#89215 · In reply to: #89090

2015. nov. 6. 1:07 ezt írta ("Ferenc Kovacs" <tyra3l@gmail.com>):
>
>
>
> On Thu, Nov 5, 2015 at 9:53 PM, Adam Howard <oldschooldsl@gmail.com>
wrote:
> > […]
think this should be kept for a user code fix.
> > […]
>
> please don't top post on the php.net mailing lists.
> on one hand this change doesn't really make any person happy (apart maybe
the ext/ldap maintainers) but already caused headache for people upgrading
to a patch release and expecting their code to still work.
> I don't think that it is tradeoff worth taking.
>
> now I've just took the time to look the actual commit which break this
feature:
>
https://github.com/php/php-src/commit/e7af0fe1eb89e40671e86a588aa1b78607b85461
> previously we were just passing host and port to ldap_init:
>
https://github.com/php/php-src/commit/e7af0fe1eb89e40671e86a588aa1b78607b85461#diff-baba61601f82518312197f5420a76ae7L346
> and the documentation for ldap_init states that
> ldap_open() takes host, the hostname on which the LDAP server is running,
and port, the port number to which to connect. If the default IANA-assigned
port of 389 is desired, LDAP_PORT should be specified for port. The host
parameter may contain a blank-separated list of hosts to try to connect to,
and each host may optionally by of the form host:port. If present, the
:port overrides the port parameter to ldap_open().
> http://linux.die.net/man/3/ldap_init
>
> so this wasn't just a bogus feature but php being the glue and the
underlying library providing this feature, then when we removed the else
block to remove the usage of the now deprecated ldap_open we lost this
feature.
> it would be fairly trivial to transform host:port formatted hosts to be
transformed to the new schema://host:port format required by
ldap_initialize (and ldap_is_ldap_url()) instead of just assuming that
receiving an url not validated by ldap_is_ldap_url() means that we received
a naked hostname

Côme, what do you think about my suggestion for 5.6?

---

## #89220 — come@opensides.be — 2015-11-16T02:22:18+00:00

URL: https://externals.io/message/89047#89220 · In reply to: #89215

Le samedi 14 novembre 2015, 16:06:14 Ferenc Kovacs a écrit :
> Côme, what do you think about my suggestion for 5.6?

I’m gonna do the same thing as for the port=NULL support, only commit this to 5.6.
I’ll push to 7 the notice when using URI+PORT on the other hand.