Skip to content

Conversation

kocsismate
Copy link
Member

No description provided.

@kocsismate kocsismate requested a review from Girgias October 3, 2021 23:10
@kamil-tekiela
Copy link
Member

I'm not convinced by this. The parameter name is still $uri, but for backwards compatibility, you can provide only the hostname as a value. This should be documented differently. The signature doesn't change. In fact, it looks like there are 2 signatures, but there is no signature that accepts just one parameter.

I am not very familiar with LDAP, but it seems to me like we would want to document the actual signature and then explain the weird behaviour with passing the hostname as the $uri parameter. Specifying pseudo-parameters in the manual to explain strange behaviour doesn't sit well with me.

@cmb69
Copy link
Member

cmb69 commented Oct 4, 2021

This PR looks good to me. This function is basically overloaded: either pass the URI, or pass hostname and port. The latter won't work with named arguments, but is soft deprecated anyway. And as it is now, we have two descriptions of the $uri parameter.

@kocsismate
Copy link
Member Author

Yeah, I agree with Christoph: while I admit that this PR is very far from perfect, but at least it retains the previous state of the page by fixing an issue with the duplicated param name. As I have no intention for further improving the documentation of ldap_connect, I think it's a sensible hotfix for now. Hopefully someone in the future will feel the urge to review the content of this page, and will the document all the missing parameters.

@cmb69
Copy link
Member

cmb69 commented Oct 4, 2021

plus actually deprecate the old signature...

@kocsismate
Copy link
Member Author

kocsismate commented Oct 4, 2021

plus actually deprecate the old signature...

At some point of the development cycle of PHP 8.2, I plan to start an RFC which would globally tackle all overloaded signatures, where possible (first example: php/php-src#6754).

@afilina
Copy link
Contributor

afilina commented Dec 23, 2022

Pinging reviewer @cmb69: it seems like you gave an informal approval in the comments. Can we formalize the approval or merge?

@cmb69 cmb69 merged commit 48eeb30 into php:master Dec 24, 2022
@cmb69
Copy link
Member

cmb69 commented Dec 24, 2022

Oh, indeed, this could be merged. Thank you!

@kocsismate kocsismate deleted the fix-lpda-connect branch December 24, 2022 10:42
claudepache pushed a commit to claudepache/php-doc-en that referenced this pull request Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants