Skip to content

refactor(ldap): migrate to using the pear/net_ldap2 package#981

Merged
JohnVillalovos merged 1 commit intodevelopfrom
jlvillal/update_ldap
Feb 10, 2026
Merged

refactor(ldap): migrate to using the pear/net_ldap2 package#981
JohnVillalovos merged 1 commit intodevelopfrom
jlvillal/update_ldap

Conversation

@JohnVillalovos
Copy link
Copy Markdown
Collaborator

@JohnVillalovos JohnVillalovos commented Feb 7, 2026

Previously there was an old copy of the net_ldap2 library in the plugins directory.

Switch to using a composer installed copy of pear/net_ldap2

We were able to delete the code in:

  • lib/external/pear/
  • plugins/Authentication/Ldap/LDAP2.php
  • plugins/Authentication/Ldap/LDAP2/

Also add some better error handling in
plugins/Authentication/Ldap/Ldap2Wrapper.php

Copilot AI review requested due to automatic review settings February 7, 2026 21:02
@JohnVillalovos JohnVillalovos marked this pull request as draft February 7, 2026 21:03
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates the LDAP authentication plugin away from a vendored copy of Net_LDAP2/PEAR code to the Composer package pear/net_ldap2, removing the legacy bundled sources and updating runtime/CI/documentation to match.

Changes:

  • Switch LDAP plugin bootstrapping to require Composer autoload and verify pear/net_ldap2 is installed.
  • Update LDAP wrapper error-checking to use PEAR::isError() (instead of a local wrapper).
  • Remove the vendored Net_LDAP2/PEAR library sources and adjust tests/docs/CI accordingly.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/Plugins/Authentication/Ldap/LdapTest.php Adjust test double to no longer extend Net_LDAP2_Entry and provide a dn() method.
plugins/Authentication/Ldap/namespace.php Require Composer autoload and fail fast if Net_LDAP2 is unavailable.
plugins/Authentication/Ldap/Ldap2Wrapper.php Remove include of legacy LDAP2 shim and use PEAR::isError() for connection error detection.
plugins/Authentication/Ldap/LDAP2/Util.php Remove vendored Net_LDAP2 utility implementation.
plugins/Authentication/Ldap/LDAP2/SimpleFileSchemaCache.php Remove vendored schema cache implementation.
plugins/Authentication/Ldap/LDAP2/Search.php Remove vendored search implementation.
plugins/Authentication/Ldap/LDAP2/SchemaCache.interface.php Remove vendored schema cache interface.
plugins/Authentication/Ldap/LDAP2/Schema.php Remove vendored schema implementation.
plugins/Authentication/Ldap/LDAP2/RootDSE.php Remove vendored RootDSE implementation.
plugins/Authentication/Ldap/LDAP2/LDIF.php Remove vendored LDIF implementation.
plugins/Authentication/Ldap/LDAP2/Filter.php Remove vendored filter implementation.
plugins/Authentication/Ldap/LDAP2/Entry.php Remove vendored entry implementation.
lib/external/pear/license.txt Remove vendored PEAR licensing file (part of removing bundled PEAR).
lib/external/pear/PEAR.php Remove vendored PEAR base classes (now provided by Composer dependency chain).
lib/external/pear/Config/Container/PHPArray.php Remove vendored PEAR Config component.
lib/external/pear/Config/Container.php Remove vendored PEAR Config component.
lib/external/pear/Config.php Remove vendored PEAR Config entry point.
docs/source/LDAP-Authentication.rst Document prerequisites (PHP LDAP extension + composer require pear/net_ldap2) and update configuration wording.
composer.json Add pear/net_ldap2 as a suggested dependency for the LDAP plugin.
.github/workflows/lint-and-analyse-php.yml Ensure LDAP extension is enabled and install pear/net_ldap2 for CI runs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (2)

plugins/Authentication/Ldap/Ldap2Wrapper.php:100

  • If $configFilter fails to parse, the code logs an error but still continues and passes the error object into Net_LDAP2_Filter::combine, which can lead to a fatal error later. After detecting PEAR::isError($configFilter), return false (or throw) to stop processing.
        if ($configFilter) {
            $configFilter = Net_LDAP2_Filter::parse($configFilter);
            if (PEAR::isError($configFilter)) {
                $message = 'Could not parse search filter %s: ' . $configFilter->getMessage();
                Log::Error($message, $username);
            }
            $filter = Net_LDAP2_Filter::combine('and', [$filter, $configFilter]);

plugins/Authentication/Ldap/Ldap2Wrapper.php:123

  • If $searchResult is a PEAR error, the code logs but continues and then calls $searchResult->current() / $searchResult->count(), which will crash. After PEAR::isError($searchResult), return false (or throw) to avoid dereferencing an error object.
        Log::Debug('Searching ldap for user %s', $username);
        $searchResult = $this->ldap->search(null, $filter, $options);

        if (PEAR::isError($searchResult)) {
            $message = 'Could not search ldap for user %s: ' . $searchResult->getMessage();
            Log::Error($message, $username);
        }

        $currentResult = $searchResult->current();

        if ($searchResult->count() == 1 && $currentResult !== false) {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread composer.json
Comment thread plugins/Authentication/Ldap/namespace.php Outdated
Comment thread .github/workflows/lint-and-analyse-php.yml Outdated
Comment thread .github/workflows/lint-and-analyse-php.yml Outdated
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/update_ldap branch 2 times, most recently from c11cf5c to 9b3ee87 Compare February 7, 2026 21:43
@JohnVillalovos JohnVillalovos marked this pull request as ready for review February 7, 2026 21:44
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/update_ldap branch 5 times, most recently from 1b0a3b0 to 50011c6 Compare February 7, 2026 22:19
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread plugins/Authentication/Ldap/Ldap2Wrapper.php
Comment thread plugins/Authentication/Ldap/Ldap2Wrapper.php
Comment thread composer.json
Comment thread plugins/Authentication/Ldap/Ldap2Wrapper.php
Comment thread plugins/Authentication/Ldap/Ldap2Wrapper.php
Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 7, 2026

@JohnVillalovos I've opened a new pull request, #982, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread plugins/Authentication/Ldap/Ldap2Wrapper.php
@JohnVillalovos JohnVillalovos requested review from labmecanicatec and removed request for labmecanicatec February 7, 2026 23:04
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/update_ldap branch 2 times, most recently from cf8d7c1 to 271fc25 Compare February 8, 2026 07:05
@JohnVillalovos JohnVillalovos changed the title fix(ldap): migrate to using the pear/net_ldap2 package refactor(ldap): migrate to using the pear/net_ldap2 package Feb 8, 2026
Previously there was an old copy of the `net_ldap2` library in the
plugins directory.

Switch to using a composer installed copy of `pear/net_ldap2`

We were able to delete the code in:
  - `lib/external/pear/`
  - `plugins/Authentication/Ldap/LDAP2.php`
  - `plugins/Authentication/Ldap/LDAP2/`

Also add some better error handling in
`plugins/Authentication/Ldap/Ldap2Wrapper.php`
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread docs/source/INSTALLATION.rst
@JohnVillalovos JohnVillalovos merged commit 2bbdc83 into develop Feb 10, 2026
17 checks passed
@JohnVillalovos JohnVillalovos deleted the jlvillal/update_ldap branch February 10, 2026 00:49
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.

3 participants