HelloBar

Very nice module and video also ! Congrats :-)

Automatic review

1. http://ventral.org/pareview/httpgitdrupalorgsandboxmrded1793538git
There quite a lot of code standard errors.

Manual review

2. Javascript error: The js file defining helloBar is missing, maybe not commited ?

ReferenceError: HelloBar is not defined
[Break On This Error]
new HelloBar(message, options);

3. Line 25 of module file. As I understand from http://drupal.org/node/28984 there is a security issue here.
The url is an user entered value and it should be checked with check_url().

4. Line 25 of module file. The correct way to display a field is using this function field_view_value().
See: http://api.drupal.org/api/drupal/modules!field!field.module/function/fie...

Hope you will repair this js missing file so I can test this !

Bye,

Sergio

hellobar.module line 25

4. Line 25 of module file. The correct way to display a field is using this function field_view_value().
See: http://api.drupal.org/api/drupal/modules!field!field.module/function/fie...

True, is not on hellobar.module but hellobar_content.module line 25.

I hope you will find a solution to avoid people coping the hellobar.js file, as it stays on client side...

Can I copy it for the purpose of this review and delete it after ?

The security issue is still there, the link_element is rendered directly without passing by check_url.
render($link_element)

There is also a reference problem:
Strict warning: Only variables should be passed by reference in hellobar_content_message_prepare() (line 32 of /var/www/d7/sites/all/modules/hellobar/modules/hellobar_content/hellobar_content.module).

To solve that don't encapsulate field_get_items() inside reset(), make intermediary variable.

I'm having a deeper look to your code concerning the .3 remark of #1.
As the unchecked content from the database is not outputted directly but given to the js file the security check is out of the scope of the drupal module. I made mistake with that on #1 .3 and there is no blocker for the module.

In any case I would advice you to remove the strict warning that is still there before releasing as full project.
Strict warning: Only variables should be passed by reference in hellobar_content_message_prepare() (line 32 of /var/www/d7/sites/all/modules/hellobar/modules/hellobar_content/hellobar_content.module).

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

I don't see any other problem here. I guess this module is ready.

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.