andriy_gerasika [agerasika]

Thanks for your application.

Hm. Why can't you contribute to the existing http://drupal.org/project/comfortid instead and perhaps join forces with that project's maintainer, especially in the long run?

I agree with the applicant's point in #4. ComfortID does its own thing; the proposed module here integrates the external tool from Google code. I don't see they can be usefully merged.

However, some problems with the module code, some minor and some not so minor:

- is there no way this module can work without having to hack Drupal core? I don't know enough about jQuery and the Drupal behaviours system, but can't a behaviour be overridden by a later module?

- merge INSTALL and README into just README, since they're both pretty slim.

- add a file header documentation block

- newline needed at the end of your module file.

- the convention on D6 is a space either side of the . operator.

- openid_selector_form_alter() adds HTML to the output. This should use a theme function.

> a) form alter code produces a warning under strict mode

What's the warning?

> b) JS file is licensed under BSD

In which case you can't include it. And anyway, it's better not to include external libraries.
Have you looked at http://drupal.org/project/libraries?

Subscribing.. looking forward to this module :)

Wysiwyg will use and depend on Libraries API in D7. Libraries API is actually used a lot throughout contrib already. It is not what hook_library() in D7 does. For full details, you may want to read #315100: Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries)

I tried this module (#13) on Drupal 6.19. While it seems to be working right on Firefox, it does not on Chrome. I am trying to login with google/yahoo account, and upon clicking the Google/yahoo icon, it flashes the error message:

Username field is required.
Password field is required.

Chrome did not redirect me to the respective login pages before throwing that.

edit: Chrome v6.0.408.1 dev
the module version in #6 worked well though

I've just tested this and it works great! Very useful addition to Drupal.

A few more points, just small stuff and enhancements:

- copyright notice in several of your files. I can't find anything about this in the CVS application guidelines, but I've a feeling we avoid these (IIRC it causes a legal problem as soon as you get a patch from another person). Could a maintainer comment please?
- I'm not that clued up on JS in Drupal, but it seems to me at least some of the code in your JS file should be using Drupal.behaviors. See http://drupal.org/node/205296 for more.
- ditto the variables such as openid.img_path which you set in the hook_form_alter() -- could they be set on the Drupal object?
- " * Implementation of hook_form_alter : adds OpenID selector to the OpenID form" -- break this into two lines, with a blank line in between. The format for a hook implementation header is:

Implementation of hook_foo().

Does my awesome stuff.

- For clarity, I would lay out the definition of your $forms array on several lines.
- I'd also add a comment above that to explain that the keys are formIDs and the values the HTML ids. Though actually, you could probably just strreplace('_', '-') for the same effect, but I guess whether that makes simpler code is a matter of opinion ;)

Nearly there! :D

- copyright notice in several of your files. I can't find anything about this in the CVS application guidelines, but I've a feeling we avoid these (IIRC it causes a legal problem as soon as you get a patch from another person). Could a maintainer comment please?

That is what Crell would say. To make a comparison, Drupal core code is copyrighted by every contributor; you will not find a single file that is reported as copyrighted by Dries Buytaert.

> this will not work for 'user_login_block' => 'user-login-form', array line

Heh, I didn't notice that one ;)

All looks good to me.

I look forward to using this one :)

Yes it seems to be working well on google chrome (also) now... waiting to see its own project page :)

Approved based on joachim's RTBC.

Michelle