PHP 8.1 no longer returns a resource

[necrogami]

According to the changelog for https://www.php.net/manual/en/function.ldap-connect.php

php 8.1 no longer returns a resource for ldap_connect it now returns a class which is causing src/Ldap.php:839 to fail.

[michelsp]

[Ocramius]

Can we please have a test case for this?

[necrogami]

When updating to PHP 8.1 with PHP's LDAP extension I get is not a resource error when trying to connect to LDAP

[Ocramius]

Yes, I'm asking for a test case that runs in CI here, and that prevents this from regressions.

The fact that this BC break wasn't caught, is precisely because an automated test does not cover this.

[DerpgonCz]

So, how do we approach this? Or rather, what is the expected outcome?

First I'd decide to either split the code base to "compatible with 8.1", and "compatible with <8.1", or make a fork in code to handle both cases. I am not sure which is better, as I am not really experienced in this field.

Then, the tests. Do we cover just the connection part? Or that ldap_connect returns a resource? Or that both works (in case of the code fork)?

I'd really like to help, although I am not sure what exactly is needed.

[kevinpapst]

Are you sure that the test cases do not cover it?
The PR that integrated PHP 8.1 said in the description Note that Laminas_Ldap online tests were not available..

Looking at the test I believe that the connection test would find the problem. But looking at the action (e.g. here) I cannot see the env TESTS_LAMINAS_LDAP_ONLINE_ENABLED, instead I see 200+ skipped tests. Are the real connection tests probably "only" disabled in Github actions?

Seems there are two things to work on:

• activating LDAP online tests in CI
• moving the 8 calls to is_resource() in LDAP class to a dedicated method that can handle both result types (and probably more necessary refactoring)

[DerpgonCz]

On the topic of tests:

Sure enough, during my local testing, a lot of errors for PHPUnit in PHP 8.1, a lot less in PHP 8.0 (not allowing SSL, no seeded data, etc.).

HOWEVER 1, it seems like the tests were available, at least according to Travis.
HOWEVER 2, it tested for php:master
HOWEVER 3, php:master at the time was not 8.1, it was 8.0.3

$ php --version
PHP 8.0.3-dev (cli) (built: Feb 11 2021 11:15:07) ( ZTS )

HOWEVER 4, the current up-to-date 2.13.x branch does not go through Travis due to change in CI workflow which was part of said PHP 8.1 PR, only 2.12.0 went through Travis, and tested for LDAP, but it does not run for PHP 8.1.

I think that refactoring CI overlooked LDAP tests.

So, basically two things needs to be done:

• Run Online LDAP tests in the current CI configuration
• Fix the error

As far as fixing the error goes, I am suspecting that checking if is_resource could just be changed to check for !== false (as stated here for example), as other ldap_* functions changed their signatures from resources to LDAP\Connection, LDAP\Result and LDAP\ResultEntry. Those were not directly used, only passed along to other ldap_* functions.

[kevinpapst]

@Ocramius having LDAP is 8.1 is rather crucial. What can we do, how can we help? We already provided some feedback, but I can support with testing as I have automated tests that really use LDAP connections in combination with this package, but I don't feel confident enough to fix the issue here.

I wonder what @Koen1999 who worked on the PHP 8.1 PR and @froschdesign who asked for the support initially think about it.

[Ocramius]

@DerpgonCz Travis-CI is dead-dead-dead.

The last CI build on 2.12.x was on https://github.com/laminas/laminas-ldap/runs/4407780273?check_suite_focus=true, and all tests passed: https://github.com/laminas/laminas-ldap/runs/4407780273?check_suite_focus=true#step:3:346, so yep, we need to add a running LDAP server to CI :)

So here again: provide a failing test case (FIRST), then a fix, if possible, and it will gladly be reviewed/merged.

@kevinpapst if it's crucial, put dev time in it and help as described in this thread: fork the repo, get CI running with a real server, send a patch. I know as much as you do ;-)

[kevinpapst]

I know you are a busy person, but you are not even reading our comments. The tests were all skipped, so it is extremely likely that they already exist. This is from your link:

[froschdesign]

@kevinpapst

I cannot see the env TESTS_LAMINAS_LDAP_ONLINE_ENABLED

Check the configuration of PHPUnit:

laminas-ldap/phpunit.xml.dist

Line 46 in 5e0debb

<env name="TESTS_LAMINAS_LDAP_ONLINE_ENABLED" value="false"/>

[Ocramius]

but you are not even reading our comments.

What I said:

so yep, we need to add a running LDAP server to CI :)

[kevinpapst]

@Ocramius I do not understand the Laminas CI actions/docker images. So could you please join and help me with #25 ?
The tests now fail in every PHP version, but the higher versions have way more failing tests.

[Koen1999]

I wonder what @Koen1999 who worked on the PHP 8.1 PR and @froschdesign who asked for the support initially think about it.

Honestly I don't think much about it. When I made that PR, I was trying to add PHP 8.1 support for another Laminas package that somehow required laminas-ldap even though it was optional. Since the test suite passed for the PR, I assumed this would work. I don't know much about LDAP, but if this package is blocking you I suggest you stick with PHP 8.0 for now and try to work on a fix for the issue.

[kevinpapst]

After spending 2 hours now to get the test cases up and running, I can share:

• they run and they show that the issue is covered in 8.1
• they still have an issue though in lower versions, likely because there is an issue with importing iptable rules which I cannot fix

So if someone wants to join and help fixing 8.1, please post your ideas or fork my branch.

[bchen-us]

On the topic of tests:

Sure enough, during my local testing, a lot of errors for PHPUnit in PHP 8.1, a lot less in PHP 8.0 (not allowing SSL, no seeded data, etc.).

HOWEVER 1, it seems like the tests were available, at least according to Travis. HOWEVER 2, it tested for php:master HOWEVER 3, php:master at the time was not 8.1, it was 8.0.3

$ php --version
PHP 8.0.3-dev (cli) (built: Feb 11 2021 11:15:07) ( ZTS )

HOWEVER 4, the current up-to-date 2.13.x branch does not go through Travis due to change in CI workflow which was part of said PHP 8.1 PR, only 2.12.0 went through Travis, and tested for LDAP, but it does not run for PHP 8.1.

I think that refactoring CI overlooked LDAP tests.

So, basically two things needs to be done:

* [ ]  Run Online LDAP tests in the current CI configuration

* [ ]  Fix the error

As far as fixing the error goes, I am suspecting that checking if is_resource could just be changed to check for !== false (as stated here for example), as other ldap_* functions changed their signatures from resources to LDAP\Connection, LDAP\Result and LDAP\ResultEntry. Those were not directly used, only passed along to other ldap_* functions.

I find that adding isset($this->$resource) works as well. For most of the code I use 'isset($this->$resource)' and check for '!== false'. In some cases, I check for isset($this->$resource) after since certain functions will instantiate the LDAP objects and will want to check if it was set after calling certain functions.

[Ocramius]

Please no hacky workarounds: help us get the package up to date in #25!

[kevinpapst]

#25 closed in favor of #29 and now the code needs to be adjusted => in #27

[Ocramius]

Releasing 2.13.0 - please give it a shot and see if that fixes the problem, y'all :)

[arueckauer]

It looks like this is fixed and and can be closed.