alex.klebeck [alexklebeck]

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 it needs to be changed.

As per requirements, the motivation message should be expanded to contain more features of the proposed project. For themes, it should include also a screenshot of the theme, and (when possible) a link to a working demo site; for modules, it should include also a comparison with the existing solutions.

Thank you for your reply.

What's the exact license? I see LGPL in miner.php for example. LoggerAppenderPool.php contains an ASF license. Drupal modules hosted on drupal.org are released as GPL 2.

Also, if the module contains external libraries, these should not be part of the module on CVS, but rather downloaded by the user when installing the module.

I am changing status as per previous comment.

When you apply for a CVS account, you agree to commit only code that is licensed under GPL License.
scor is right on what he reports.

should the user be really forced to download all of them separately and put them into correct folders of our plug-in?

Yes.

• This is a partial review.
• The points reported in this review are not in order of 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. ;Ontos Feeder, provides data semantically relevant to the text being edited.
   ;Copyright 2010 Ontos AG
   ;
   ;This program is free software; you can redistribute it and/or
   ;modify it under the terms of the GNU General Public License
   ;as published by the Free Software Foundation; either version 2
   ;of the License, or (at your option) any later version.
   ;
   ;This program is distributed in the hope that it will be useful,
   ;but WITHOUT ANY WARRANTY; without even the implied warranty of
   ;MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   ;GNU General Public License for more details.
   ;
   ;You should have received a copy of the GNU General Public License
   ;along with this program; if not, write to the Free Software
   ;Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
   ;
   ;mail:   Ontos AG, Mittelstr. 24, 2560 Nidau, Schweiz.
   ;e-mail: support@ontos.com
   
   
   

   Remove any reference to the license inside files.
   The copyright statement would not correct from the moment you accept a patch from any users; to make an example; Drupal is not copyrighted by Dries Buytaert, but from all the contributors.

2. License files cannot be committed in Drupal.org repository. Projects committed in Drupal.org repository have the same license used by Drupal.
3. Menu descriptions and titles, as well as schema descriptions, should not be passed to t().

4. require_once 'core/constants.php';
   

   There is a Drupal function to call in those cases.
   Such files have an extension .inc, in Drupal.

5. require_once 'core/constants.php';
   require_once 'core/navigationwidget.php';
   require_once 'core/miner.php';
   require_once 'ota_constants.php';
   require_once 'ota_database.php';
   require_once 'ota_options.php';
   require_once 'ota_blocks.php';
   require_once 'ota_gettagsbutton.php';
   require_once 'core/rdfa.php';
   
   

   It usually not a good idea to unconditionally include code contained in PHP files. If the files contain form handlers, theme functions, or menu callbacks, Drupal core code allows to third-party modules to define which files must be loaded in the implementations of hook_menu(), and hook_theme(); in these cases, Drupal loads the file only when necessary.

6. // $Id: filter_example.module,v 1.1.2.2 2010/02/10 06:20:11 rfay Exp $
   
   /**
    * @file
    * This is an example outlining how a module can be used to define a filter
    * to be run on user-submitted content before it is output to the browser.
    *
    * To show all the capabilities of the filter system, we will define two filters
    * in this module. One will substitute the string "foo" with an administratively-
    * defined replacement string. The other will find a custom XML tag, <time />, and
    * replace it by the current time.
    */
   
   

   The CVS ID tag is placed at the top of the file, after <?php, in a PHP file.
   Please update the comment for the proposed module; that comment is just the copy of the comment present in another module.

7. 	switch ($op) {
   		case 'list':
   			return array();
   
   		case 'description':
   			return t('Allows users to post code verbatim using <code> and <?php ?> tags.');
   
   		case 'prepare':
   			return $text;
   
   		case "process":
   			return $text;
   
   		default:
   			return $text;
   
   

   The description is copied from another module. The code implemented for hook_filter() doesn't actually do anything, not even what reported in the description.

8. 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; how constant names should be written.
9. How the code is split makes hard to review it.
   I don't see the reason for hook_menu() to call another function that just returns the array that hook_menu() could have directly returned.

10. function ontosFeeder_form_submit(&$form, $form_state) {
    	return ota_form_submit($form, $form_state);
    
    

    There is a form submission callback, but I don't see any form using it.

11. function ontosFeeder_block($op = 'list', $delta = 0) {
    	ota_load_user_JSCSS();
    	$blocks=new OTA_Blocks();
    	$content = $blocks->show($op, $delta);
    	$_SESSION[OTA_SESSION_TAGS]=null;
    
    
    	return $content;
    }
    

    The implementation of hook_block() is not checking the first parameter, and it returns always the same value.

12. Hook implementation comments should be like the following one:

    /**
     * Implements hook_menu().
     */
    

    As reported in Documenting hook implementations:

    If the implementation of a hook is rather standard and does not require more explanation than the hook reference provides, a shorthand documentation form may be used in place of the full function documentation block described above:

    /**
     * Implements hook_help().
     */
    function blog_help($section) {
      // ...
    }
    

    This generates a link to the hook reference, reminds the developer that this is a hook implementation, and avoids having to document parameters and return values that are the same for every implementation of the hook. Optionally, you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

    In the case of hooks that have variables in the names, such as hook_form_FORM_ID_alter(), a slightly expanded syntax should be used:

    /**
     * Implements hook_form_FORM_ID_alter() for node_type_form().
     */
    function mymodule_form_node_type_form_alter(&$form, &$form_state) {
      // ...
    }
    

    This generates a link to the hook reference, as well as to the particular form that is being altered. Again, optionally you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

Points #4, #5, and #8 are still valid.
Points #4 is still valid for the Drupal function to use in such cases; point #8 is still valid for the names to give to the functions.

@alex.klebeck, 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.

Overall, you do not seem to utilize the Drupal API correctly. Please read up on the coding standards, writing secure code, and other documentation linked below as to how to be writing efficient, secure code that will be beneficial to the community. We really appreciate your drive and contributions, but this still needs a fair amount of work, IMO.

1. 1. Installation 
   
   To use the Ontos Feeder in Drupal you will need to download the Ontos Feeder from the www.ontos.com
   Please extract the downloaded files and put it into the "core" sub-directory of the Ontos Feeder module 
   directory ("...\ontosFeeder\core").
   

   In is often very confusing for users to put specific files where you expect them. This is mostly cause by the fact that "extract" is relative to how things are packaged and what program is used. I would suggest you expand this to be much more detailed.

2. function ontos_feeder_schema() {
     module_load_include('inc', 'ontos_feeder', 'include/ontos_feeder_database');
     $schema=module_invoke('ontos_feeder', 'get_schema');
     return $schema;
   }
   

   You do not have a docblock for the function. There is really no need to put the schema in a different file and function; this is why there is an install file in the first place. Also, there is no reason to use module_invoke unless you think other modules should be messing with your schema (which they should not).

3. You should really follow the Drupal Coding Standards. It is difficult for me to review your module quickly, which also translates to making it hard for someone who wants to contribute to your module when you have actually released. You can automate the process some by using the Coder module, but will still need to actually read and follow the coding standards. Please read the whole section, including sub page: http://drupal.org/coding-standards

4.   global $user;
     $select_query = "SELECT * FROM {ontos} WHERE user='" . $user->name . "'";
     $result = db_query($select_query);
   

   You NEED to sanitize values in the DB query, and can do this easily by using the Drupal API correctly. Please see db_query()

5. define('OTA_URL', 'http://' . $_SERVER['HTTP_HOST'] . '/' . $drupal_folder . '/' . drupal_get_path('module', 'ontos_feeder') . '/core');
   

   You should not use a different prefix (OTA) here. Namespacing and prefixing is extremely important in Drupal. You are assuming http (when someone could be using https). You should be utilizing url() and the absolute option.

6. define('ONTOSMINER_BLOCK_NAME', "Ontos Feeder");
   define('ONTOSMINER_ERRORMESSAGE_NOCONTENT', 'No tags found');
   

   These need to be translatable using t(). Also, I am not sure why these are constants in the first place.

7. $_ontos_feeder_ontosminer_tags;
   

   There should be little need to use a global variable like this. You could utilize a static variable or use the global variable within functions to achieve the same thing probably.

8.   $configuration = module_invoke('ontos_feeder', 'get_configuration');
   

   You do not need to utilize module_invoke, just call your function directly. You are also not including the file that this function is in.

9.       return $this->fillBlock($blockName, "ERROR: Function $contentFunctionName not found!");
   

   This needs to be translatable. Also, this should not be displayed to most users, so I would suggest you log this with watchdog().

10. Please read up on: http://drupal.org/writing-secure-code

11.   if (($form['#id']==ONTOSMINER_OPTIONS_FORM_ID_GENERATED) && (!$form_state["post"])) {
    

    I don't understand why you are altering your own form?

12. function ontos_feeder_menu() {
      module_load_include('inc', 'ontos_feeder', 'include/ontos_feeder_options');
      $items = module_invoke('ontos_feeder', 'options_form_menu');
      return $items;
    }
    

    Don't need module_invoke(). There doesn't seem to be any apparent reason to keep

13. function ontos_feeder_load_admin_scripts() {
      module_load_include('php', 'ontos_feeder', 'core/constants');
    
      drupal_add_js(OTA_REL_URL . '/script/jquery/jquery-1.4.2.js', 'module');
      drupal_add_js(OTA_REL_URL . '/script/jquery/jquery.mousewheel-3.0.2.js', 'module');
      drupal_add_js(OTA_REL_URL . '/script/jquery/jquery.wait.js', 'module');
      ...
    

    You are not utilizing drupal_add_js correctly. Also, you are including almost 50 JS files which is horrible for performance. Your "OTA core" that is downloaded should include a compressed version of all this if it is really needed. Also, you are including a different version of jQuery than core which will probably conflict as they are both included.

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

To add a note to what reported by zzolo, code like the following is not correct too, basing on what reported in the documentation for t():

  define('ONTOSMINER_BLOCK_NAME', t("Ontos Feeder"));

Is there any updates on this?

Please read all the following and the links provided as this is very important information about your CVS Application.

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:Migrating from CVS Applications to (Git) Full Project Applications.

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• If your application has been "needs work" (or "postponed (maintainer needs more info)") for more than 4 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.