Iñaki Arenaza added a comment - 07/Jan/13 9:18 AM

Sorry for being so late at reviwing this
While the fix is OK, I'd say we should fix the cause rather than the symptoms. We shouldn't be passing $ldapconnection around. We already have the active ldap connection in $this->ldapconnection, and we should use it everywhere. The $ldapconnection passing is a leftover from the old days when the auth and enrol LDAP plugins were not classes.
The only places where we should pass $this->ldapconnection are to the lib/ldaplib.php functions calls.
I guess this should go into a new bug.
Saludos.
Iñaki.
—

This issue refactors the use of ldap connection inside enrol_ldap plugin. New unit tests should guarantee that there are no regressions. During the testing I have discovered one issue with role assignments and fixed it, I decided it is safer to not backport it because it affects unsupported setups with multiple roles in one course.