? ? Pending

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
6 Apr 2018

Summary of Changes

I'm going to be blunt here. Our LDAP client class, lightweight and functional as it may be, is a piece of Joomla software that falls under the "abandonware but lucky the damn thing still works" category and its only actual use in core is for its bind and search capabilities (as in the write portion of the API is unused in core). This is one class that I'd personally argue in favor of throwing away and replacing with something that is getting more scrutiny and maintenance because the use of LDAP is a more exotic use case than normal and we just don't have the resources to keep up with it (ask the folks on JSST how difficult it was to ensure the escaping fixes we did actually worked). So, I'm basically throwing our LdapClient class away and pulling in Symfony's LDAP component as a replacement.

Why? Symfony's component does a lot of things better than we do; options validation, error reporting/handling, and abstracting data arrays into a developer friendly structure being three focus points.

Accepting this PR actually introduces two Symfony components into our application; the LDAP component, and the OptionsResolver; this second one is actually the one I find more interesting for general use because it gives you a way to pretty sanely validate arrays and enforce things like key presence, default key values, and throwing errors on missing required keys (joomla-framework/database#101 is an example of validating the database API's options array against the resolver).

Testing Instructions

LDAP Authentication continues to work as before

Documentation Changes Required

Nothing at this time

avatar mbabker mbabker - open - 6 Apr 2018
avatar mbabker mbabker - change - 6 Apr 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 6 Apr 2018
Category External Library Composer Change Libraries
avatar brianteeman
brianteeman - comment - 6 Apr 2018

Having tried to use our LDAP recently with SSO I had to resort to using an extension so I am definitely in favour of something else.

avatar mbabker
mbabker - comment - 12 Apr 2018

After #20059 PR is no longer valid in current state. Will re-open when I've had time to rebase.

avatar mbabker mbabker - close - 12 Apr 2018
avatar mbabker mbabker - change - 12 Apr 2018
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2018-04-12 13:22:32
Closed_By mbabker
Labels Added: ? ?
avatar mbabker mbabker - change - 13 Apr 2018
Status Closed New
Closed_Date 2018-04-12 13:22:32
Closed_By mbabker
avatar mbabker mbabker - change - 13 Apr 2018
Status New Pending
avatar mbabker mbabker - reopen - 13 Apr 2018
avatar mbabker
mbabker - comment - 22 May 2018

So based on https://developer.joomla.org/news/726-state-of-the-framework.html this actually does need action otherwise we end up shipping an intended-to-be-deprecated (retired) class as part of 4.0. I'll resolve merge conflicts this week but even conflicted that shouldn't stop folks from being able to download the branch in the state it's in (which is fine, it's just behind current 4.0-dev and the merge conflicts only block merging the PR cleanly) and testing if they have access to a LDAP server.

avatar carlitorweb
carlitorweb - comment - 22 May 2018

I can test the LDAP layer with the network at work. Waiting then for merge conflict

avatar wilsonge wilsonge - change - 19 Jun 2018
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-06-19 23:46:01
Closed_By wilsonge
avatar wilsonge wilsonge - close - 19 Jun 2018
avatar wilsonge wilsonge - merge - 19 Jun 2018

Add a Comment

Login with GitHub to post a comment