vgarvardt [vgarvardt]

See the Drupal coding standards to understand how a module code should be written.

very interesting project.
go go go!

subscribing

*привет с Хабра

1.   if (!isset($form[$key])) return;
   

   That is not how the coding standards say to write the code.

2.     $options = array();
       foreach ($form['provider']['#options'] as $pname => $title) {
         $attrs = array();
         if ($pname == $def_prv) $attrs['selected'] = 'selected';
         if (isset($providers[$pname]['icon']) && is_file($providers[$pname]['icon'])) {
           $attrs['class'] = 'with-icon ' . $pname;
           $styles['.with-icon'] = 'background-repeat:no-repeat;padding-left:17px;';
           $styles['.' . $pname] = 'background-image:url(' . $base_url . '/' . $providers[$pname]['icon'] . ');';
           $settings[$pname]['icon'] = $providers[$pname]['icon'];
         }
         if (isset($providers[$pname]['input'])) {
           $settings[$pname]['title'] = $providers[$pname]['input'] . ': ';
         }
         $options[] = '<option value="' . check_plain($pname) . '"' . drupal_attributes($attrs) . '>' . check_plain($title) . '</option>';
       }
   
       $select = array(
         '#prefix' => '<select name="provider" class="form-select" id="edit-provider" >',
         '#value' => implode('', $options),
         '#suffix' => '</select>',
       );
       $form['provider'] = array(
         '#type' => 'item',
         '#value' => drupal_render($select),
       );
   

   I think that the FAPI allows to create a select form field.

3. Referring to the previously reported code, why isn't the code using some theme functions?

4. version = "6.x-0.6.2"
   

   That string should be removed.

5. function openid_ext_install() {
     // module should have greater weight than OpenID does to remove some form additions
     $weight = db_result(db_query("SELECT weight FROM {system} WHERE name = 'openid' AND type = 'module'"));
     db_query("UPDATE {system} SET weight = %d + 1 WHERE name = 'openid_ext' AND type = 'module'", $weight);
   }
   

   Considering that by default, all modules have a weight set to 0, the code could be simpler.

6.   // remove all variables for this module
     db_query("DELETE FROM {variable} WHERE name LIKE 'openid_ext_%'");
   

   It is preferable to use variable_del() to remove Drupal variables.

Not using the form API to built the form field would probably render it differently from the form field as generated from the FAPI.

To make an exemple, the form field Component shown for below Post new comment is rendered by the following HTML code:

<div class="form-item" id="edit-project-info-component-wrapper">
  <label for="edit-project-info-component">Component:
   <span class="form-required" title="This field is required.">*
   </span>
  </label>
  <select name="project_info[component]" class="form-select required" id="edit-project-info-component" >
    <option value="Code">Code</option>
    <option value="Documentation">Documentation</option>
    <option value="Miscellaneous" selected="selected">Miscellaneous</option>
    <option value="User interface">User interface</option>
  </select>
</div>

I am not sure if being able to set different HTML attributes for each select option is worth the fact the form field could not be rendered differently from the other form fields of the same type rendered through the form API.

This element should be simple select form element and takes all styles that current Drupal theme applies to select elements.

That depends from the theme being used; the form field select created from the form API has the CSS class form-class, and it is contained in a tag DIV with CSS class form-item.

function openid_ext_provider_livejournal($input_value) {
  return 'http://' . check_plain($input_value) . '.livejournal.com';
}

check_plain() is not thought to be used with a domain name; if you want to be sure the input passes is a true domain name, then you need to use a different function.

There is already a module for the same purpose: http://drupal.org/project/comfortid.

Those are still no reasons to create a duplicate of a module that already exists in Drupal.org.

As per previous comment, I am marking the report as won't fix.

It's pity to see Your work is not on drupal.org still. Anyway, could You please host it on Google Code or elsewhere?

It Supports vkontakte (the biggest russian social network) and "Comfortable OpenID Login Box" doesnt.

"Comfortable OpenID" is abandoned its clear that nobody will use it.

There is nothing to review; see the comments from #15 to #18.

http://drupal.org/project/comfortid

Maintenance status: Abandoned

Opposite vgarvardt have begun develop in Oct 2009. Latest release have date July 05, 2010

Don't understand why you not allow this module. marasmus

See comment #18, and previous ones.