Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concat hostName and port for ldap_connect #1386

Closed
wants to merge 1 commit into from

Conversation

2 participants
@arkraft
Copy link

commented Apr 11, 2019

Instead of using two parameters for ldap_connect (hostName and port), this PR combines them to a single parameter. The docs of ldap_connect describe the host parameter as either a hostName or a LDAP URI. In case the latter is used, the second parameter (port) is ignored. When using a protocol, the parameters are combined, otherwise it stays the same.

It works for me with ldaps now, did not test it without the protocol.

resolves #1220

concat hostName and port for ldap_connect
Instead of using two parameters for ldap_connect (hostName and port), this PR combines them to a single parameter. The docs of [ldap_connect](https://www.php.net/manual/en/function.ldap-connect.php) describe the host parameter as either a hostName or a LDAP URI. In case the latter is used, the second parameter (port) is ignored. When using a protocol, the parameters are combined, otherwise it stays the same.

@ssddanbrown ssddanbrown added this to the v0.26.0 milestone Apr 16, 2019

ssddanbrown added a commit that referenced this pull request Apr 16, 2019

Updated ldap server option parsing to work with protocol and port
- Aligns with PHP behaviour where ports is ignore for full LDAP URI.
- Added tests to check format being passed to LDAP is as expected.
- May be related to #1220
- Related to #1386 and #1278
@ssddanbrown

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Thanks @arkraft for offering this pull request.

We did already have a similar pull request open in #1278.
I've just commited c247640 to update & simplify the logic. Therefore will close this pull request.

This update will be in the next release (v0.26.0)

@arkraft arkraft deleted the arkraft:fix/ldap_over_tls branch May 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.