HylkeVDS [hylkevds]

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

We review just a module per applicant; please let us know which one you want reviewed.

I reviewed Closed Question and following are the things needed to be corrected

1. Use an indent of 2 spaces, with no tabs through out the module

2. Line 279: else statements should begin on a new line
} else {

Please read coding standards for details.

@HylkeVDS, 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. Follow Drupal Coding Standards. Note that Drupal coding standards are more than just PHP code. Also, look at using Coder to help automate SOME of this.
2. You CHANGELOG has version numbers. These will have to be consolidated once put into Drupal CVS.
3. Ensure $Id$ is all files (aka TODO.txt)

4. version = 0.31
   

   Do not specify version.

5. You do not use primary keys in your database tables. This greatly helps indexing and querying.
6. You need to have docblocks for all functions, classes, and files.

7. /**
    * Install and update functions
    */
   function closedquestion_schema() {
   

   Please use the format "Implements hook_schema()." as stated in coding standards for ALL hook implementations.

8. You do not need an update function as this module has not been released yet. Overall, what we review needs to be ready for CVS and d.o.

9. include_once 'question/CQ_Question.class.php';
   include_once 'question/CQ_QuestionAbstract.class.php';
   include_once 'question/CQ_FactoryQuestion.inc.php';
   include_once 'question/CQ_UserAnswer.class.php';
   

   Do not put includes in the global scope. They should be included when necessary (ie menu items or hooks) or if absolutely necessary, in hook_init(). Also, utilize module_load_include().

10. define('CQ_ACCESS', 'access closedquestions');
    define('CQ_CREATE', 'create closedquestion');
    define('CQ_EDITOWN', 'edit own closedquestion');
    define('CQ_EDITALL', 'edit all closedquestions');
    define('CQ_ADMIN', 'administer closedquestion');
    

    Constants need documentation blocks as well.

11.   $items['node/%node/cq_text'] = array(
              'title' => 'Text',
              'page callback' => 'closedquestion_text',
              'page arguments' => array(1),
              'access callback' => '_closedquestion_text_access',
              'access arguments' => array(1),
              'type' => MENU_LOCAL_TASK,
              'weight' => 2,
      );
    

    Please space correctly as outlined in the coding standards.

12. 
    /**
     * Menu callback; show view with all the users that answered a single question.
     */
    function closedquestion_users($node) {
      // add view html to page contents
      $retval = '<h2>' . t('Users that answered question %nodename', array('%nodename' => $node->title)) . '</h2>';
      $retval .= views_embed_view('ClosedQuestionUsers', 'default', $node->nid);
      return $retval;
    }
    

    All output needs to be in theme functions.

13. /*************
    * Node hooks *
    *************/
    

    Unnecessary

14. /**
     * Wrapper to quickly create a fieldset.
     * @param string $title Title of the fieldset.
     * @param string $body Body of the fieldset.
     * @param boolean $collapsible can this fieldset be collapsed?
     * @param boolean $collapsed is this fieldset collapsed by default?
     * @return string a themed fieldset.
     */
    

    Good start put please follow standards.

Overall, good start. Please follow coding standards.

--
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.

To understand which changes are strictly necessary to get a CVS account, see http://drupal.org/node/539608.

• The points reported in this review are not in order or importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. Menu descriptions and titles, schema descriptions are not passed to t().
2. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted; how Drupal variables, global variables, constants, and functions defined from the module should be named.
3. The module requires PHP5, but it doesn't declare its dependency from it.
4. Some of the points reported by zzolo are still valid, like the one about including PHP files only when necessary.

@hylkevds, thanks for the application and patience. The following points are just a start and do not necessarily 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 mentioned by @kiamlaluno, theme function names are camel case, please follow Drupal Coding Standards. Also, variable names, class names, etc. I know this is a big change, but this is a very widely used/accepted standard for Drupal.

2.     t('Σ Transfer'),
   

   Please use HTML special entities or some other better method of encoding non-standard characters.

Very close!

--
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.

Hi @HylkeVDS. Thanks for all the work on this. I am going to push this through. Please note the suggestions below, specifically the first one, which you should really do before releasing, as naming convention and namespaces are very important in the way that Drupal does things.

1. Theme function names are not good. They should be converted: 'theme_cq_option' => 'theme_closedquestion_option' . This is really important, because it could easily cause conflicts if someone makes a "cq" module.
2. closedquestion_autoload_info() could easily happen with some short logic and better consistency with file names.
3. Please update the CHANGELOG.txt to reflect moving to development to drupal.org infrastructure

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 again for all your hard work and patience. Happy hunting.