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

ldap_connect() host with port signature deprecated for PHP 8.3 #1009

Closed
1 task
andrewnicols opened this issue Nov 7, 2023 · 10 comments · May be fixed by #1014
Closed
1 task

ldap_connect() host with port signature deprecated for PHP 8.3 #1009

andrewnicols opened this issue Nov 7, 2023 · 10 comments · May be fixed by #1014
Assignees
Milestone

Comments

@andrewnicols
Copy link
Contributor

Description

The secondary signautre for ldap_connect() which explicitly provides a host and port has been deprecated from PHP 8.3 onwards.
A Uri should be used instad.

https://wiki.php.net/rfc/deprecations_php_8_3#deprecate_calling_ldap_connect_with_2_parameters

Environment

  • ADOdb version: master (478c9cf)
  • Driver or Module: ldap
  • Database type and version: any
  • PHP version: 8.3
  • Platform: any
  • I have tested that the problem is reproducible in the latest release / master branch / hotfix branch
    ⚠️ Keep only the applicable option(s), i.e. where you actually tested and remove the others)

Steps to reproduce

See use of @ldap_connect() with multiple arguments here:

$this->_connectionID = @ldap_connect( $conn_info[0], $conn_info[1] );

			$this->_connectionID = @ldap_connect( $conn_info[0], $conn_info[1] );
@andrewnicols andrewnicols added the triage New issues not yet reviewed by ADOdb developers label Nov 7, 2023
@dregad dregad added bug PHP8.3 ldap and removed triage New issues not yet reviewed by ADOdb developers labels Nov 10, 2023
@dregad dregad added this to the v5.23.0 milestone Nov 10, 2023
@dregad
Copy link
Member

dregad commented Nov 16, 2023

@andrewnicols out of curiosity, are you actually using the LDAP driver, or is this coming out of some static analysis ?

@andrewnicols
Copy link
Contributor Author

No, we're not using it outselves, but I went through all of the RFC changes for the PHP 8.3 release and hunted for potential issues across our codebase. We do let our users configure a wide range of ADOdb database types for their use, but it turns out that ldap isn't one of them.

I should add that I misread the RFC and that one isn't targetted until PHP 8.4. The RFC was identified as PHP 8.3 but many of the changes are only intended for 8.4 and I missed that in my initial assessment.

@dregad
Copy link
Member

dregad commented Nov 16, 2023

Thanks for the feedback.

I was just checking because since I took over ADOdb maintenance back in 2013 I think this is the first time anyone ever opened an issue about the LDAP driver so I was curious to find out whether someone was actually using it.

I misread the RFC and that one isn't targetted until PHP 8.4. The RFC was identified as PHP 8.3 but many of the changes are only intended for 8.4 and I missed that in my initial assessment.

You're confusing me here... I see no mention of 8.4 in the RFC, and the commit implementing the change php/php-src@69a8b63 is in PHP >= 8.3.0beta1.

@andrewnicols
Copy link
Contributor Author

Then I'm confused too!

The vote for the proposal to deprecate it lists this as being targetted at 8.4 -- see the header of the vote table here:
https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures#ldap_connect

But as you say it landed in PHP 8.3.

@mnewnham mnewnham self-assigned this Nov 16, 2023
@dregad
Copy link
Member

dregad commented Nov 17, 2023

@andrewnicols actually there are 2 distinct RFCs around ldap_connect()

@dregad
Copy link
Member

dregad commented Nov 17, 2023

@mnewnham I see you assigned this issue to yourself and created a branch, but it does not contain any commits... did you forget to push your changes ?

@mnewnham
Copy link
Contributor

@dregad, I'm building an Active Directory server to test with, haven't yet had time to complete changes......

@dregad
Copy link
Member

dregad commented Nov 17, 2023

I could use my work's AD to test if you want to spare yourself the trouble of setting that up. The fix should be quite straightforward, but I don't want to step on your toes, so let me know if you want me to fix this otherwise I'll just wait for your PR.

@mnewnham
Copy link
Contributor

Thats OK It's done now.

@dregad
Copy link
Member

dregad commented Nov 18, 2023

Great, let me know when it's ready to review, I'll give it a test run.

I see that you've updated the documentation page - that's great, I was actually considering doing it. Thanks.

The following usage method is deprecated in PHP8.2 and will not work with PHP 8.3

I fully agree that this crappy array format for options initialized via a global variable needs to go away, but I'm a bit confused by the reference to PHP 8.2/8.3, what's the relationship ?

From ADOdb 5.22.7, default values set are as follows

Good idea to preset LDAP_OPT_PROTOCOL_VERSION and LDAP_OPT_REFERRALS to sensible 2023 values.

That would be 5.22.8 or 5.23.0 though.

@mnewnham mnewnham linked a pull request Nov 18, 2023 that will close this issue
@dregad dregad modified the milestones: v5.23.0, 5.22.8 Mar 22, 2024
@dregad dregad closed this as completed in 453b4f3 Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants