Comments

• 

  Plugins bot

  Mon, 26 May 2014, 5:35 AM

  Thanks for sharing this plugin with the Moodle community. Some formal issues were detected and reported by our automated plugin checker tool. Please review all the plugin checker results and fix the issues mentioned, if possible. These changes are typically pretty easy to make but if you have any questions just let us know. For now, I am going to mark this plugin as needing more work. Once you get these formal issues resolved, please use the link “Schedule this plugin for re-approval” so that the plugin code can be fully reviewed and approved by my human colleagues.
• 

  Alexander Bias

  Mon, 26 May 2014, 2:22 PM

  Dear plugins bot, as this is only a authentication plugin, there is no screenshot which would make sense
• 

  David Mudrák

  Mon, 26 May 2014, 3:06 PM

  Hi Alexander. Nice to hear from you again. Please excuse our new staff member for being rather blunt. It is still learning how to do its job properly I fully understand there is probably not much to show in this case. Looking forward to review your plugin - asap. Thanks for understanding and being patient.
• 

  Alexander Bias

  Mon, 26 May 2014, 3:37 PM

  Thanks, David
• 

  Anthony Borrow

  Mon, 26 May 2014, 10:46 PM

  Alexander - You could show the settings page - especially the section that highlights what is distinctive about ldap_plus - for example, the Cron synchronization script section. Peace - Anthony
• 

  Anthony Borrow

  Tue, 27 May 2014, 3:31 AM

  Alexander - Ideally it is encouraged to avoid underscores in the names of the plugins. I emphasize that this is an ideal and not a requirement. I try to get things as close to ideal at the beginning because once approved to change it involves a great deal more work. Would it be worth it to consider renaming the folder ldapsyncplus or would you prefer to leave it as it is? Either way is fine but without the underscore would theoretically be better (but practically the same). If you have any questions just let us know. Peace - Anthony
• 

  David Mudrák

  Tue, 27 May 2014, 6:56 AM

  For the record, underscores are prohibited in activity modules and tolerated (still discouraged) in other plugin types. Having them has some impractical consequences (such as higher risk of a function/class name conflict with auth_ldap plugin in this case or extra characters in DB table names that have pretty limited length etc). Therefore we encourage plugin authors to avoid them if possible. Once the plugin is deployed to production sites, the change is very difficult, as Anthony says.
• 

  Alexander Bias

  Tue, 27 May 2014, 2:58 PM

  Dear David, dear Anthony,
  
  I have added a screenshot with the special section in the settings page.
  
  Concerning the underscore: Normally, I try to follow Moodle coding guidelines as best as possible. I was not aware that there are problems with underscores in plugin names - quite the contrary, I thought that it would be better to use underscores to separate words than dashes. Perhaps I misunderstood the Frankenstyle guidelines in this particular part. But is there a wiki page where it is mentioned that underscores are prohibited in plugin names?
  
  Originally, I added the _syncplus suffix to make clear that this is a subplugin for auth_ldap and it does not work without it. And I already have other plugins with underscores in the plugins repository (https://moodle.org/plugins/view.php?plugin=local_resort_courses and https://moodle.org/plugins/view.php?plugin=block_login_userinfo)...
  
  To sum it up, I would be willing to change the name to a version without underscores. Unfortunately, we already are at the point that the plugin is in production at some Moodle instances in germany. So, my question is: Is the absence of underscores a must-have or would you be willing to wave the plugin through this time?
  
  PS: The additions in auth_ldap_syncplus to auth_ldap are minimal. And I think these additional features could simply be included to auth_ldap. For the plugin's "grace period" functionality, there is no tracker item yet, but for the other two of the plugin's functionalities, I already created tracker items on https://tracker.moodle.org/browse/MDL-29249 and https://tracker.moodle.org/browse/MDL-30589.
  
  Thanks,
  Alex
• 

  David Mudrák

  Tue, 27 May 2014, 3:26 PM

  Thanks for adding the screenshot. As I was trying to explain above - underscore are not prohibited. The only plugin type they can't be used is the Activity module (mod). And that has deep historical reasons dating back to early years of Moodle development. As you know, activity modules are frankenstyle exception in that sense that that do not use the mod_ prefix in many places where other plugins use it (function names, classes names, DB table names, string files etc). If underscore was allowed there and there was an activity module like foo_bar, the Moodle core would have no idea if that means the 'boo' plugin of the 'foo' type or if it is mod_foo_bar. All other modules use prefix so the Moodle core can detect the plugintype correctly (by using the part of the name before the first underscore).
  
  If your auth plugin is already in production, there is really no need to worry with the renaming. There is no strict policy on this subject and even some core plugins use the underscore in the name. I am going to finish the review of this plugin now. Thanks Alexander.
• 

  David Mudrák

  Wed, 28 May 2014, 6:55 PM

  Thanks for submitting this Alexander. I am going to approve the plugin now. I appreciate you designed your plugin so that it inherits as much from the auth_ldap as possible, without copying the code. It is a nice example of customizing standard Moodle behaviour without the need to patch it.
  
  I found the extended behaviour of your plugin pretty reasonable. Let me encourage you to try and prepare a patch for the core's auth_ldap that would include both your plugin features.
  
  Said that, runway free and cleared to land. Welcome to Plugins directory!
• 

  David Andrew

  Wed, 3 Dec 2014, 12:25 PM

  Firstly This works great.
  I work for a government agency that has recently merged and hence we run 2 AD's at the moment so I am using ldap_syncplus and the default.
  Each can then search against different directories. The 2 domains trust each other and I am using the 1 AD account to search both domains. Is there a problem witht he way I am doing this, and is there a better way?