Closed (fixed)
Project:
Lightweight Directory Access Protocol
Version:
8.x-3.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
18 May 2016 at 20:43 UTC
Updated:
18 Jun 2016 at 18:39 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
grahlComment #3
larowlanconstants should be prefixed with the module namespace to avoid collisions. Even better, define them as class or interface constants and we can auto-load them in tests. Eg.
LdapSsoInterface::NO_AUTO_LOGINWe should be using the session bag on the request stack, not the superglobals
hook_boot is dead, so this code won't run
This needs a docblock
Core has a path matcher service. We should make this into a service and inject that.
trailing whitespace
Is this needed? the kernel loads these in D8
drupal_bootstrap is gone too, the kernel booting does all this
drupal_goto is gone too
Personally I vote ldap_sso is split into its own module and no longer part of the main ldap module on D8 - thoughts?
Assigning to @queenvictoria.
Comment #4
queenvictoria commentedHi @grahl.
It was our intention to remove LDAP SSO from the LDAP module. Potentially it should be reimplemented as an LDAP plugin for another SSO module (like Bakery or Connector) similar to the way LDAP Authorization works with the Authorization module.
It should probably become a sandbox module. Would you be able to create one? I'd like to git rm the module soon. So please let me know if you'd like to take it on.
Thanks!
Comment #5
grahlHi queenvictoria
Sorry for not responding earlier and thanks for the rapid feedback.
I agree with your assessment and will provide a sandbox by next week that contains the current state and will work on it from there. I will likely bring it into a usable state in a 1.x branch that aligns closely with the current solution (just to have something working) and then look into your suggestions of reimplementing it in another fashion.
Comment #6
queenvictoria commentedSuper thanks @grahl! Let's keep this ticket open until the code has moved. And thank you for stepping up!
Comment #7
grahlHi
Sandbox is ready: https://www.drupal.org/sandbox/grahl/2729937 attached is a patch that removes the ldap_sso module.
A few notes on that:
Comment #15
larowlanI think posting an issue in the ldap_sso project asking to revive the project for d8 is a good idea. It was merged into this module in D7, but I think it should live on its own given @queenvictoria and I don't want to maintain it.
Comment #16
grahlComment #18
queenvictoria commentedThanks Grahl. Removing ldap_sso now.
Comment #19
hotspoons commentedOriginal owner of ldap_sso here. I unfortunately haven't thought about this module or used Drupal in several years. Grahl contacted me today - if he will be able to steward the module and it will be maintained, please take it over and port it to D8. Thanks!
Comment #20
grahlReassignment successful, 8.x branch should be available some time tomorrow.
Ideally, I'd like to move the 7.x branch to that project, too, so that issues can be assigned in a uniform manner. I'm not going to do that for now since I expect users would easily miss that change during updates. Please let me know if you'd like me to remove it anyway.
Comment #21
grahl