Hi! Nice to see this module is doing well :)

Just downloaded the latest version of this and noticed that there is now one module for each provider.

Each of these modules appears to be a single hook_form_alter. This doesn't seem to me to be a very good way to do things at all, for several reasons. The module admin page is not a substitute for an admin UI. Every module you add is an extra load on Drupal, as it's an extra file to load and extra data in the system table. Lastly, building the form up entirely from hook_form_alters seems to be a really messy way to do it.

I strongly recommend you move back to a single module and supply an admin UI for selecting which remote sites should be shown. If you really want to support a modular approach, for example to allow third party modules to add OpenID suppliers, then the better pattern would be to create your own hook for these modules to provide the relevant data, and for your module to collect all this and do the form altering just once.

Comments

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Agreed. This is really, really bad to cludge up the modules page when we could easily have a simple checkboxes element on the admin page with 'Enable the following services for the OpenID selector'.

andriy_gerasika’s picture

andriy_gerasika CreditAttribution: andriy_gerasika commented

@joachim: thanks. Please feel free to revisit this module every couple of months

Yes, I will merge all the submodules in v1.6. The plan is:
1. implement #966258: OpenID Selector for anonymous commenting
2. release v1.5
3. merge all submodules
4. release v1.6

barraponto’s picture

barraponto CreditAttribution: barraponto commented

good work!

andriy_gerasika’s picture

andriy_gerasika CreditAttribution: andriy_gerasika commented
Assigned: Unassigned » andriy_gerasika

started fixing in http://drupalcode.org/project/openid_selector.git/commit/d3c7589

Cyberwolf’s picture

Cyberwolf CreditAttribution: Cyberwolf commented

Subscribing.

anarcat’s picture

anarcat CreditAttribution: anarcat commented
Status: Active » Needs work

I'd love to see this happening too... it seems like unnecessary overhead and the "weight" hacks in the .install files seem to be just a bad idea. :)

I wonder though - the modules were a great way of disabling authentication modes individually... Any plans to keep that feature in the future?

For example, the "number of providers" setting could simply be a series of checkboxes to enable the providers we want to support on the site...

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs work » Active

Please don't change status... review the meaning of 'needs work' :)

anarcat’s picture

anarcat CreditAttribution: anarcat commented

Well, there *is* a patch (committed), and it does need work before it is reviewed again, isn't that correct?

Sorry for messing up the status here. :)

andriy_gerasika’s picture

andriy_gerasika CreditAttribution: andriy_gerasika commented

I cannot find a way how to migrate "to be deleted" sub-module "enable" status to variable. The following code does not work:

function openid_selector_update_6102() {
  $modules = array(
    'openid_selector_fb',
    'openid_selector_fbconnect',
    'openid_selector_twitter',
    'openid_selector_linkedin',
    'openid_selector_vkontakte',
    'openid_selector_mailru',
    'openid_selector_winliveid',
  );
  $result = db_query('SELECT name, status FROM {system} WHERE name IN ("' . implode('", "', $modules) . '")');
  while ($module = db_fetch_array($result)) {
    $variable = $module['name'];
    drupal_set_message($variable . ' ' . $module['status']); // DEBUG
    variable_set($variable, $module['status']);
  }
  $ret = array();
  return $ret;
}

It will display all zero, despite it was 1 in the database before running hook_update_N.

Can you please help how to fix this problem?