raggax2 [raggax2]

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.

May you describe the differences between the proposed module, and http://drupal.org/project/filemakerform?

Thanks for the reply.

I apologize for the time it's taking to review this application. The users who review the CVS applications are few, and that is the reason applications take more time than they should to be reviewed.

Notes:

- I don't like that you DROP TABLE {...$table...}. That sounds dangerous. Make a required prefix?
- I don't like _filemaker_write_record and it's INSERT. Can't confirm but that seems to be an SQL Injection.

• 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. The version line needs to be removed from the .info file.

2.   if (db_table_exists('filemaker_table_cache')) {
       require_once './filemaker_table.php';
       $r = db_query("SELECT table_name FROM {filemaker_table_cache}");
       while ($row = db_fetch_array($r)) {
         $table = new FMTable($row['table_name']);
         $schema[$table->get_name()] = $table->get_schema();
       }
     }
   
   

   The require_once() statement would look for the file in the Drupal root directory, which is not the directory where the file is; in Drupal, the preferred prefix for these cases in .inc.

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

   /**
    * Implements hook_menu().
    */
   

4. Menu descriptions and titles, schema descriptions are not passed to t().

5.   include_once('filemaker_table.php');
   
   

   As reported before, that statement would not work; there is a specific Drupal function that should be used in those cases.

6.     db_query("UPDATE {filemaker} set timestamp=UNIX_TIMESTAMP() WHERE fid=%d", $row->fid);
   
   

   The query probably works only with a specific database engine; the coding standards should report which SQL functions can be used.

7. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted, and how constants should be written.

8.   $error = $false;
   

   The variable $false has not been defined. Why isn't the code simply using FALSE?

9.   if($error) {
       drupal_set_message('Could not save filemaker records in ' . $table->get_name(), 'error');
     }
   
   

   Use t()-placeholders.

10. The module depends on PHP5 but it doesn't declare its dependency from PHP5.
11. The class FMTable doesn't follow the coding standards, and should probably renamed to match the module name.

First of all - what version of Drupal is able to this module???

I like the idea of the module, because it is helpful when coming to practice handling of data in the customer level view. A FileMaker can do more, and do it faster then drupal does, and can be a "allover local system" including billing and payments in the bussiness.
Earlier I have been written a FileMaker Database who read thousands of products and picture (by in a local computer importing from factory cheats and a picture map) and in a few seconds an then create a hole home site in xhtml, with calculated prises etc and all the pictures, for each product, branding marks etc. From A .. to Z below one minutes.
What I am looking 4 is a best way to integrate the FileMaker data-sheet to the drupal database and relate each data to the pictures who will be placed (send to) the core-mapping-system (link data-info with picture in drupal) .
The purpose is to let Drupal take over and handling the e-bussines (integration with commerce) on the Internet, the rest of billing and selling will stay in FM local business level - placed at the company.

Anyone who knows?