Use connection string in non-ssl case when proto is not defined in host option #69
Conversation
|
Thanks for raising this issue! It brought up a much larger thing. Since the difference between URI-connect and the old one is only in there due to openldap-libraries older that version 2.2 and openldap 2.2 was tagged 11 years ago, we should probably remove this switch completely! |
There was a problem hiding this comment.
@heiglandreas, thanks, good catch)
Hoping understood you properly, i've just updated the PR. Please, take a look.
src/Ldap.php
Outdated
| /* Because ldap_connect doesn't really try to connect, any connect error | ||
| * will actually occur during the ldap_bind call. Therefore, we save the | ||
| * connect string here for reporting it in error handling in bind(). | ||
| */ | ||
| $hosts = []; | ||
| if (preg_match_all('~ldap(?:i|s)?://~', $host, $hosts, PREG_SET_ORDER) > 0) { | ||
| $this->connectString = $host; | ||
| $useUri = true; | ||
| $useSsl = false; |
There was a problem hiding this comment.
@heiglandreas
I leaved this assignment untouched for now, but should we recognize $useSsl here depending on schema? I mean ldaps:// should switch $useSsl to true, shouldn't it?
There was a problem hiding this comment.
That's a good catch! Would you add that?
|
@heiglandreas, WDYT about finishing with it?) |
Use connection string in non-ssl case when proto is not defined in host option
For now inside connection procedure we surprisingly generating connection string in non-ssl mode and in when proto is not specified inside host option but never use it during factual connection which is probably wrong since we do not pass generated string (containing proto) to ldap_connect function.
This PR fixes this behaviour.