in tracking down a non-existent database field error, I noticed that the LDAP_Groups and LDAP_Data modules are installed and upgraded by the ldapauth.install file, and since they don't have their own.

BTW: the error seems to be caused by the fact that I'm playing around with the HEAD version, and so the hook_update() didn't automatically fire when I ran update.php, not a big deal, since that can be run manually, but I still think that each module should be able to be installed, uninstalled, and updated individually rather than clumping it all together in the ldapauth.install file.

This patch isn't against CVS since that gets a little weird with files that don't exist yet, but it's against a clean HEAD version. If you cd to your contributed modules directory and apply it with patch -p0 < path/file.patch it should apply cleanly. If people want, I can also upload the two new .install files.

Basically, I just moved the code specific to the two modules out of ldapauth.install, and into their own .install files, then created a hook_uninstall() for each. The uninstall removes the columns individually since I was running into errors removing all of the ldap_groups_ or ldapdata_ columns in the same query.

Any thoughts? suggestions?

CommentFileSizeAuthor
#5 ldap_integration_seperate-installs_1.patch10.38 KBashtonium
ldap_integration_seperate-installs_0.patch8.02 KBashtonium

Comments

ashtonium’s picture

ashtonium commented

Can I get a comment on this proposal?

LDAP Integration is a pack of separate modules. The .conf.php files and settings pages have been separated, so it seems to make much more sense to have separate install, upgrade and uninstall abilities for each one as well. (Take a look at CCK for an example, it is a similar package of dependent yet separate modules and they each have their own .install files)

Clumping it all together into one .install file just seems like it's going to cause problems down the road.

The patch above longer applies cleanly against head, but I can re-patch if this is something that we want to work towards. If this is something others think is important, please reply and I'll re-roll the patch and/or upload a .tar of the new .install files.

kreaper’s picture

kreaper commented

ashtonium

Sorry for the late response. I took a long break from drupal and just getting back into it. Scafmac was busy with the 5.1.2 codebase.
In principle, I completely agree with your proposal. They are three different modules and should be treated as such. That is one of those minor things that we never had the time to take care of.

Are you able to provide patch files that have been tested ? I can review them and commit them.

kreaper

ashtonium’s picture

ashtonium commented

No problem, I know how it is. Thanks for responding. I should have some time over the next couple of days to re-roll the patch and test it out.

kreaper’s picture

kreaper commented

I tried your patch but I am getting errors

patching file ldap_integration/ldapauth.install
Hunk #1 FAILED at 8.
Hunk #2 FAILED at 38.
Hunk #3 FAILED at 53.
3 out of 3 hunks FAILED -- saving rejects to file ldap_integration/ldapauth.install.rej
patching file ldap_integration/ldapdata.install
patching file ldap_integration/ldapgroups.install

if you can attach the three install files, that'll be better.

ashtonium’s picture

ashtonium commented
StatusFileSize
new ldap_integration_seperate-installs_1.patch10.38 KB

yeah, that patch was made against an older version of HEAD.

I've attached a new version, rolled against the latest version of HEAD. I've tested upgrading, installing, uninstalling, etc. but let me know if you encounter any problems.

I've removed the innards from ldapauth_update_1() in ldapauth.install since that is now part of ldapdata.install but left ldapauth_update_2() as it was.

I also noticed that some of the install lines no longer have the NOT NULL default '' distinction as they did in previous versions, I left these as they were since I'm not sure if they should be that way or not:

function ldapdata_install() {
  switch ($GLOBALS['db_type']) {
    case 'mysql':
    case 'mysqli':
      db_query("ALTER TABLE {ldapauth} ADD (
        ldapdata_binddn VARCHAR(255) NOT NULL default '',
        ldapdata_bindpw VARCHAR(255) NOT NULL default '',
        ldapdata_bindpw_clear VARCHAR(2) NOT NULL default '',
        ldapdata_rwattrs LONGTEXT, 
      	ldapdata_roattrs LONGTEXT, 
        ldapdata_mappings LONGTEXT
      )");
      break;
    case 'pgsql':
      db_query("ALTER TABLE {ldapauth} ADD (
        ldapdata_binddn VARCHAR(255) NOT NULL default '',
        ldapdata_bindpw VARCHAR(255) NOT NULL default '',
        ldapdata_bindpw_clear VARCHAR(2) NOT NULL default '',
        ldapdata_rwattrs TEXT, 
        ldapdata_roattrs TEXT, 
        ldapdata_mappings TEXT
      )"); 
      break;
  }
}
ashtonium’s picture

ashtonium commented

I can attach the .install files themselves if needed.

kreaper’s picture

kreaper commented
Assigned: Unassigned » kreaper
Status: Needs review » Fixed

Thank you.

Patch committed to HEAD.

kreaper’s picture

kreaper commented
Status: Fixed » Closed (fixed)