Daniele [daniele]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what needs to be changed.

As per Apply for contributions CVS access, the motivation must be expanded to add more details about the module features, and compare the module with similar modules already in CVS.

@Daniele, thanks for the application. The following points are just a start and do not encompass all of the changes that may be necessary for your approval. Also, a specific point may just be an example and may apply in other places.

1. As far as adding this module to the contrib space. You should add this as a new project, but ensure that the old project is mark as deprecated and points to this new one.
2. You should not include a LICENSE.txt, this is provided by the CVS packaging scripts.
3. Please follow the Drupal coding standards. This include standards for more than PHP files. this also includes having docblocks for all functions. http://drupal.org/coding-standards
4. You create variables, but do not delete them all. Your hook_uninstall() needs to be in an .install. See http://zzolo.org/thoughts/tip-managing-variables-drupal-module
5. All HTML output needs to be put through the theme system.
6. hook_requirements() needs to be in the .install file. Please see documentation: http://api.drupal.org/api/function/hook_requirements/6

7.   $items['admin/settings/phiwarevoice'] = array(
       'title' => t('Phiware Voice settings'),
       'description' => t('Settings for Phiware Voice integration'),
       'page callback' => 'drupal_get_form',
       'page arguments' => array('phiwarevoice_admin'),
       'access arguments' => $phiwarevoice_access,
       'type' => MENU_NORMAL_ITEM,
      );
   

   The menu title and description do not need to be run through t().

8. After that you can choose where to display the button by selecting the block position or simply by adding '<?php module_invoke("phiwarevoice", "button"); ?>'
   This is not good for many reasons. No one should be calling module_invoke directly. Also, your button needs to be run through the theme layer, as suggested above. Also, you should make a API function that will be wrapper to all this.
9. Instead of using arg(), please use menu_get_object() http://api.drupal.org/api/function/menu_get_object/6
10. Theres probably more. But his is a good place to start.

Overall, the module seems simple enough. I think you need to really read the coding standards and the module guidelines. There are some key Drupal coding paradigms you are missing. Good job, nonetheless and keep up the good work.
http://drupal.org/developing/modules

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article on what I look for.

There have not been replies in the past week. I am marking this application as won't fix.

Remember to change status, when you upload a new version of the code.

@Daniele, thanks for the application and patience. The following points are just a start and do not encompass all of the changes that may be necessary for your approval. Also, a specific point may just be an example and may apply in other places.

1. The Menu module is not a dependency as it only is the interface for creating custom menus, and not the menu architectural system.

2. /**
    * Detect if we've not set the customer id
    */
   

   Please use the hook reference when implementing a hook. See coding standards:

   /**
    * Implements hook_requirements().
    */
   

3. Do not include ?> at the end of PHP files.

4.     if ( arg(0) == 'node' && is_numeric(arg(1)) && ! arg(2) ) {
   

   You should be using menu_get_object().

5. Please read using the theme layer. Basically, your phiwarevoice_render() needs to be a theme function.
6. You should also not be using inline JS unless it is absolutely necessary.

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article.

1. module_invoke is used for invoking hook for a specific module. But you are not defining a new hook (another module cannot interact with this process), so you would just offer an API function. But in this specific case, you just want to provide a theme function for coders. So, they would just call theme('phiwarevoice_render', $node) to get the output, and this would allow any overrides of the theme function to happen.
2. Well, inline JS is not a good thing because of its implications on page loading, caching, organization, etc. (see this). So, you could push that into a separate JS file and use jQuery properly.
3. Theme implementation is good! You could look at using a preprocess function to clean it up, but the way it is is adequate.

4. description = Enable Phiware Voice service fo text to speech conversion.
   package = "Other"
   core = 6.x
   
   dependencies[] = 
   

   Typo and empty dependencies. Not sure how the empty dependency thing will react in Drupal, so I would say this is necessary to change.

5.     //Check were on a node
   

   Not a big deal, but should be:

       // Check were on a node
   

6. Where you create an image in your theme function, you should use theme('image', )

7.       $node->content["phiwarevoice_begin"]["#value"] = '<!-- VOICE_BEGIN --><div style="display: none">' . $node->title . '</div>';  
   

   This should be in a theme function. Also, inline CSS is bad for the same above reasons as JS, but also because they are not overridable.

Real close! Just a couple things holding this back. It should be fine next time around.

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article.

Looks fine to me!

The only thing I see is the $Id$ in the README.txt which I don't think is needed there (confusing I know), but if that's the only thing I don't think there's a need to rework the module just for that.

I'm not an expert and don't have CVS rights, so you'll have to wait for someone else to check further.

Hi @Daniele. Sorry for the delay. I assuming from your comment above, you did not change the that output to a theme function. Though I do understand what you are saying about it just being a note, but the whole idea of using the theme layer is to give complete control to a themer about what is outputted, and IMO, this includes even small bits like this. Please consider changing this.

Congratulations, you have been approved. Please read the following resources to make sure you know how to use CVS and the specifics to the Drupal CVS infrastructure, as well as how to be a good module maintainer on Drupal.org. The Drupal community is very large and dynamic; we welcome you as a module maintainer and hope that you embrace and challenge the Drupal community and continue to contribute.

• CVS reference: http://drupal.org/handbook/cvs/quickstart
• Maintaining modules: http://drupal.org/node/7765

Thanks to the following people who helped review this application, it is very appreciated:

• @kiamlaluno
• @drupalshrek

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article, or read the handbook page on how to review for reference.