gourley [gourley]

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.

1. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted (which includes the character used to indent the code), and how the constant should be written. The coding standards also report which names to use for constants, functions, and classes defined from the module.

2.     $result = db_query("SELECT name FROM {users} ORDER BY name");
       while ($u = db_fetch_object($result)) {
           $users[$u->name] = $u->name;
       }
   

   The query doesn't filter out the user accounts that have been blocked, depending on the case, this can be a security issue.

3.     if (module_exists('taxonomy')) {
           foreach(taxonomy_get_vocabularies( ) as $v) {
               $vocabs[$v->name] = $v->name;
           }
       }
   

   The code should probably use db_rewrite_sql().

4.             drupal_set_message("WARNING: user '$nuser' not found; nodes will be owned by 'admin'.",'warning');
   

   Strings used in the user interface should be passed to t() (with the exception of when that is already done by Drupal code).

5.     try {
           $client = new FedoraClient( $server, NULL, NULL, $version );
       } catch (Exception $e) {
           $exception = '<pre>'.print_r($e, true).'</pre>';
           drupal_set_message( "Exception Object:: $exception\n", 'error');
           fedora_rest_save_messages();
           return FALSE;
       }
   

   Exceptions are only present in PHP 5, but the module doesn't declare its dependency from that version. Drupal 6 is still compatible with PHP 4.

6.     $coll = db_fetch_object( db_query("SELECT * FROM {fedora_rest_collection} WHERE uri='%s' AND server='%s' AND version='%s'", check_plain($collection),check_plain($server),check_plain($version)) );
   

   The query can return more than a row. If you want to get only the first row, you should call db_query_range().

7. version = "6.x-1.0-beta1"
   

   The line is added by the packaging script, and it should not be manually added.

8.     db_query("DELETE FROM {variable} WHERE name like 'fedora_rest_%'");
   

   The query could delete variables defined from a module with a name starting with fedora_rest_ (someone could create a module called fedora_rest_extension, and the module would delete also those variables). The preferred way to delete variables is to delete them singularly with variable_del().

9. function fedora_rest_update_1() {}
   

   The module has never been hosted on Drupal.org. The update functions should be removed.

10. define('FEDORA_REST_CREATE', 'create Fedora Object node');
    define('FEDORA_REST_UPDATE_OWN', 'update own FO node');
    define('FEDORA_REST_UPDATE_ANY', 'update any FO node');
    define('FEDORA_REST_DELETE_OWN', 'delete own FO node');
    define('FEDORA_REST_DELETE_ANY', 'delete any FO node');
    

    Using constants for the permissions would probably cause problems for the translation script, which is not able to extract the permission strings to put them in the translation template file (this means the permission strings will not be translated).
    It is also better to keep the permission strings consistent (if the first permission makes reference to Fedora object node, also the other permission strings should use the same term).

11.     fedora_rest_trace("fedora_rest_node_info()");
    

    Remove any calls to debugging functions.

12.         $form['show_pid'] = array(
                '#value' => t('<strong>Identifier:</strong> ')
                            . "<pre>$node->persistent_id</pre>\n",
                '#weight'=> -20
            );
    

    Use the t()-placeholders, instead of concatenating strings. This gives more context to who translates strings.

13.         $form['persistent_id'] = array(
                '#type' => 'hidden',
                '#default_value' => $node->persistent_id,
            );
    

    Why isn't the code using '#type' => 'value'?

14.             '#title' => t('Repository Collection'),
    

    Strings used in the user interface should be in sentence case.

15.     if (isset($node->format)) {
            $format = $node->format;
        } else {
            $format = variable_get('fedora_rest_format', 0);
        }
    

    Why isn't the code using the default format set by Drupal?

16. /**
     * Implements hook_alter()
     *
     */
    function fedora_rest_form_alter(&$form, $form_state, $form_id,
                                    $my_form='fedora_object_node_form')
    {
    

    The hook is an implementation of hook_form_alter(), not hook_alter(). The last parameter is not passed to the function from Drupal. The code should use a local variable, or simply a literal string.

There is already an application for a similar module (#771670: alxp [alxp]).

Hi Don and Alberto,

I've just had a presentation on Fedora Commons and Drupal at DrupalCamp Timisoara 2010 (http://drupalcamp.ro) this weekend. I included Don's module in a comparison vs Islandora and not Alexander's O'Neill work (http://drupal.org/node/771670) since Don's implementation of Fedora REST Api is complete: it includes both REST API's of FC, API-A for access, and API-M for management. As far as I've seen in the other project (alxp), only API-A is implemented there.

I followed Gabor Hojsty's suggestion to give some feedback on this issue, after presenting Fedora Commons and Drupal at our camp, so here it is. I suggest we have a merge between projects, maybe Don and Alex can agree. I think Don's module is more complete now. More, Don already has live sites running based on his contribution, so we have a broader base for starting and expanding the module. I've already contacted Don and I am volunteering to help solve the code standards issues. I already have a CVS account, so we can make a team of three working on this integration.

All the best,
Gabriel Dragomir
Drupal Romania Association

Let's see what gourley thinks.

Alexander O'Neill is one of the developers of Islandora, at UPEI. I am not sure where he intends to head with development on his API module and what's next on Islandora's plate, but as long as we get along having a Fedora REST API project released, we can think about what should be done next. For sure, a complete rewrite will be needed for Drupal 7 to accomodate the fields-instance-bundle paradigm and new database abstraction layer in D7.

When thinking about merging, I was more thinking about deciding which of the projects should get promoted into Drupal.org, since they really implement the same functionality, maybe with slightly different programming approaches. I will try to compare what we have in both projects. So far, I've seen Don's project as being more complete.

Please grant CVS access as a co-maintainer for Fedora Commons API #862262: Grant CVS access to gourley and alxp as co-maintainers

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

Hi EveryBody,

First of all, thanks for your module. Im trying to use your module but I dont know how to configure Fedora API settings. What should I have to put in the field "Collection URI"??

I have a fedora in http://localhost:8080/fedora with this information:

Base URL: http://localhost:8080/fedora
Version: 3.4.2
PID Namespace: changeme
PID Delimiter: :
Sample PID: changeme:100
Retain PID Namespace: *
OAI Namespace: example.org
OAI Delimiter: :
Sample OAI Identifier: oai:example.org:changeme:100
Sample Search URL: http://localhost:8080/fedora/objects
Sample Access URL: http://localhost:8080/fedora/objects/demo:5
Sample OAI URL: http://localhost:8080/fedora/oai?verb=Identify
Admin Email: bob@example.org
Admin Email: sally@example.org

The problem, is that when I try to create a fedora object, the combo "Repository Collection" is not filled.

Thanks in advance

I am giving credits to the users who participated in this issue.