Services Content Lock

Hi jared_sprague.

1. Pareview

pareview gives almost no errors. WARNING | Line exceeds 80 characters; contains 81 characters is pretty minor issue. So there are no errors from here.

2. Documentation

Move the documentation from the pdf file into Drupal Documentation pages. Write the documentation on the proper category for example at "Connecting to other Sites, System and Data" with a book node structure. This will make the documentation more useful and accessible for users as also as will leave space for comments, tips and proposals. See more at 5. of "Best practices for creating and maintaining projects".

3. Project page

Please move the "Dependencies" section after "Description", then show "Install" and finally show "Use Case Example".

You may, also, do the same on the README.txt

Generally, it is better if you can follow the D.O. "Tips for a great project page". According to this guide "Description" should be "Overview", ""Dependancies" should be "Requirements" etc.

4. services_content_lock.module

Please add translation support for strings. Lines 15, 25, 32, 42, 49, 59 of services_content_lock.module. You already did that for other functions of the module.

Example WITHOUT translation support:
'help' => 'Retrieves info about a lock',

Example WITH translation support:
'help' => t('Retrieves info about a lock'),

5. Coding Standards

Your code seems clean and well written.

Furthermore, try using a common coding style for quotes all over the module files. For example try using single quotes (') instead of double quotes (") except if necessary. See more at Coding Standards.

6. Drupal API

Function on line 35 of services_content_lock.inc can be written with a shorthand according to Drupal API db_select () documentation. Is there any reason you chose to split the functions?

function _services_content_lock_node_exists($nid) {
  $query = db_select('node', 'n')
          ->condition('n.nid', $nid, '=')
          ->fields('n', array('nid'))
          ->execute()
          ->fetchField();
  return !empty($query);
}

The only major problem with this module is that it is not easy to test it while working... and if you can supply a solution it would be great.

I am back. I studied a little the Services API just to check the codes. Here are some minor issues and a propose.

1) Project page

- Remove the "Documentation Services Content Lock Documentation" section or make the link open on the same window (Drupal.org rules)
- Try to make all the links relative (Content style guide)

2) services_content_lock.module

- line 5 should probably go out. You only need @file and a description for the starting comment (the same for line 4 of .inc)
- lines 26, 60. Since path => 0 refers to an index its value needs no quotes. Althought it works right now the correct code should be:
'source' => array('path' => 0),
There is an issue for that actually, #1917432: Relationships' source arg (path) only accepts numeric values.

3) services_content_lock.inc

- There are some inline comments inside function (lines 43, 65, 106). This is probably a not good commenting method. You can move these comments out of the functions.
- You can shortcode the function on line 114 without problem. Drupal core blog for example does it already.

4) Local testing

- I tried to test it but had no luck. I do not know much about xml-rpc and REST and I always got errors like "Parse error. Request not well formed..." (with XML-RPC server) as also as Access denied for Anonymous user (with REST)...
Cookie is the sid on the {session} table of drupal installation? I think I am making the mistake with Cookie value on the example script.

By the way, you can export the endpoint you created for the documentation examples and attach the codes on these pages so the user can try with the same settings.

added testing section

Everything works great now! The video is great!

My only concern is any security issues the examples folder may have but I do not know much about .sh files and REST. Probably there will be no issues but this can be tested by a Review admin.

changed link

manual review:

1. "t('Lock successfully created on node') . ': ' . $nid;": do not concatenate translatable strings with dynamic variables like that, use placeholders with t() instead. Also elsewhere.
2. _services_content_lock_node_exists(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075

But that are not application blockers, so ...

Thanks for your contribution, jared_sprague!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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

Thanks to the dedicated reviewer(s) as well.