bulkpub

Please take a moment to make your project page follow tips for a great project page. Given that there are other tools to do this it would be great to add a section to your page that compares your tool to other similar tools.

Please remove the LICENSE.txt file. Drupal will add the appropriate version automatically during packaging so your repository should not include it.

Feedback from coder: "152 normal warnings, 83 minor warnings"
I noticed some very small code style issues. Please run the coder module on "minor" setting to help catch these. The coding standards have even more information in this area. I noticed things like using tabs instead of spaces and putting { on their own line. Coding standards help make your module easier to review.

Remove the version = "7.x-1.0" line from your .info - that will be added by the drupal.org packager automatically. Remove the files[] lines from your .info - those are only necessary if the files contain classes. See http://drupal.org/node/542202#files

The bulkpub_log feature is pretty non-standard. I suggest using watchdog for that. In particular watchdog should be used in bulkpub_validate_user to prevent brute force attacks on user/password combinations.

I believe bulkpub_get_page should check node_access('view' prior to returning the content. The same goes for bulkpub_delete_page for node_access('delete', $node). If your stance is that any user with the BULKPUB_PERMISSION_STR can do those things then that should be stated really clearly in the README.txt.

On a related note ;) Please take a moment to make your README.txt follow the guidelines for in-project documentation.

Hi rjohnson42,

coder results look already good. Some manual results:

• You are working on the git master branch but should move to a major release branch (see http://drupal.org/node/1127732 for details).
• You widely use Implement hook_foo_bar(). while the common standard is Implements. You should make sure all hook function comment headers use the "official" syntax.
• bulkpub.module, line 43: Function documentation is usually not needed for hook implementations (very different from the general comment requirements, see nest ;))
• While you have already added some informational comments to many functions, you should describe any non-hook function as explained here: http://drupal.org/node/1354 (I would really recommend to spend some time on this document). Following these rec's eases code readability a lot for e.g. future co-maintainers or add-on module authors.

Cheers, d.

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• "'access arguments' => array('administer site configuration'),": do not use this generic permission, create your own.
• "/* the term is defined in the vocabulary */": you should use "//" style comments for inline comments.
• ""@username" is the login username to be used to authenticate.": no need to repeat the variable name here, just describe it. See http://drupal.org/node/1354#functions
• the if/else authentication + error is repeated ine very function, I think you should move it to bulkpub_validate_user().
• "define('BULKPUB_LANG', LANGUAGE_NONE);": why do you need to define that constant?
• bulkpub.module line 391: indentation error.
• "watchdog('content', '@type: added %title using bulkpub.'...": You should use your module's name for the watchdog entry, not content.

I suppose your GIT-URL should be http://git.drupal.org/sandbox/rjohnson42/1278058.git

You should remove empty functions

/**
 * Implements hook_install().
 */
function bulkpub_install() {
}

You should use the database extraction layor correctly, please read http://drupal.org/writing-secure-code

  $result = db_query("
    SELECT mlid
    FROM {menu_links}
    WHERE link_path = 'node/" . $nid . "'
    AND menu_name = '" . $title . "'
	");

see: "never concatenate data directly into SQL queries"

you should use correct identations of only two spaces per identation-level (also check coder-module):

  if ($user->uid) {
        menu_delete_links($menu_name);
        return TRUE;
  }
  if (($error = _bulkpub_validate_content_type($blogid)) !== TRUE) {
    // Return an error if not configured type.
        return $error;
  }

Tagging.

Also, you have not listed any reviews of other project applications in your issue summary as strongly recommended here: http://drupal.org/node/1011698

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

Manual Review:

1)"module_invoke_all('node_bulkpub_edit'": if your module invokes its own hooks your should document them in an *.api.php file. See http://drupal.org/node/161085#api_php

Minor coding standard problems should not result in "needs work", the problems mentioned in comment #13 are surely no application blockers.

You can easily exit this infinite loop by getting a review bonus.

1) bulkpub.module, line 954:
In the doc block the function the first param ($node) is expected to be an object.
So why typecast the variable if you are sure it is an object?
If you are uncertain of that the function receives an object you should check the variable with is_object or similar.

/**
 * Check that the user has permission to save the node with the chosen status.
 *
 * @param object $node
 *   An object containing a node.
 * @param string $original_status
 *   A string containing the chosen status.
 *
 * @return bool
 *   A boolean TRUE if no error, or an XML-RPC error.
 */
function bulkpub_status_error_check($node, $original_status) {

  $node = (object) $node;

2) bulkpub.module, line 1198:
the function: bulkpub_validate_user returns either an object or a string and therefore the @return tag type in the docblock should be 'mixed' instead of 'array'. It seems like the function never would return an array by the way, correct me if I am wrong. :) When you write mixed you can start the description with object/string to specify what 'mixed' covers.
So instead of:

/**
 * @return array
 *   An array containing users, or an XML-RPC error.
*/

You could write:

/**
 * @return mixed
 *   object/string. A Drupal user object or an XML-RPC error.
*/

3) bulkpub.module, line 1036:
the line exceeds 80 characters and gets difficult to read.
instead of:

  $result = db_query('SELECT n.nid FROM {node} n WHERE n.type = :type', array(':type' => $content_type));

try:

  $result = db_query(
    'SELECT n.nid FROM {node} n WHERE n.type = :type',
    array(':type' => $content_type)
  );

Hi :-)

1. I think that you have a licence issue here. You state at www.drupalinfo.info documentation page that this module is released as CC v3, but contributed modules are under GPL v2. You should update this information on your website copyright information to avoid confusion.

2. There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

3. bulkpub_new_book() should check if $node is false before calling book_node_insert and return false/error message. Actually it returns an empty xml structure instead when the node id doesn't exists.

4. bulkpub_delete_image() is deleting the files with a call to file_unmanaged_delete($file);
Is this working correctly if the file is registered inside the files table ? It won't be better if this function checks first if the file has an fid and if it does then delete it using file_delete() instead ?

5. On bulkpub_validate_user() the function user_authenticate() is directly called without any anti flood filter, that could be a security problem. This could be improved checking the configuration with flood_is_allowed().
There is an interesting example on: http://api.drupal.org/api/drupal/modules!user!user.module/function/user_login_authenticate_validate/7

6. On line 613 bulkpub_new_page() the function set language as LANGUAGE_NONE. This is correct on www.ditainfo.info but other websites with internationalization would have a big problem with that. It could be interesting to add the language as an aditional parameter with a default value to LANGUAGE_NONE.

Drupal as a Service ! That's really cool !
Hope to see this released soon.

Hey rjohnson42,

Interesting Module!

I ran your code through the automated reviewer and it returned these results:

FILE: /var/www/drupal-7-pareview/pareview_temp/bulkpub.module
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
90 | ERROR | Functions must not contain multiple empty lines in a row; found
| | 2 empty lines
147 | ERROR | Functions must not contain multiple empty lines in a row; found
| | 2 empty lines
1022 | ERROR | Invalid @return data type, expected int but found integer
1591 | ERROR | Invalid @return data type, expected bool but found boolean
1628 | ERROR | Invalid @return data type, expected bool but found boolean
1651 | ERROR | Invalid @return data type, expected bool but found boolean
--------------------------------------------------------------------------------

http://ventral.org/pareview/httpgitdrupalorgsandboxrjohnson421278058git

If you can fix these few errors, it looks pretty good to me.

Also, I noticed that my code editor was breaking somewhere and I found the issue:

In blukpub.module on Line 1414 you have this:

function bulkpub_rsd() {
  global $base_url;

  $xmlrpc = $base_url . '/xmlrpc.php';
  $base = url('', array('absolute' => TRUE));
  $blogid = 1;

  drupal_add_http_header('Content-Type', 'application/rsd+xml; charset=utf-8');
  print <<<__RSD__
<?xml version="1.0"
<rsd version="1.0" xmlns="http://archipelago.phrasewise.com/rsd">
  <service>
    <engineName>Drupal</engineName>
    <engineLink>http://drupal.org/</engineLink>
    <homePageLink>$base</homePageLink>
    <apis>
      <api name="bulkpub" preferred="false" apiLink="$xmlrpc" blogID="$blogid" />
    </apis>
  </service>
</rsd>
__RSD__;
}

What if you wrapped the code in a single quote ( ' ) character?

function bulkpub_rsd() {
  global $base_url;

  $xmlrpc = $base_url . '/xmlrpc.php';
  $base = url('', array('absolute' => TRUE));
  $blogid = 1;

  drupal_add_http_header('Content-Type', 'application/rsd+xml; charset=utf-8');
  print '<<<__RSD__
<?xml version="1.0"
<rsd version="1.0" xmlns="http://archipelago.phrasewise.com/rsd">
  <service>
    <engineName>Drupal</engineName>
    <engineLink>http://drupal.org/</engineLink>
    <homePageLink>$base</homePageLink>
    <apis>
      <api name="bulkpub" preferred="false" apiLink="$xmlrpc" blogID="$blogid" />
    </apis>
  </service>
</rsd>
__RSD__;';
}

That seems to fix the code editor and is a better PHP and Drupal best practice. Also, you should always end print statements with a semicolon ";". I know the semicolon is optional in this instance, but it's in accordence with Drupal's coding standards: http://drupal.org/coding-standards#semicolon

----------------

I know a lot of these coding standards can be a real pain in the butt, but they help people test and contribute to your code immensity when they can expect the code to be consistent with all other Drupal projects.

I hope the review process goes well for you!

thanks!

Looks good to me!

thanks!

Glancing through the code, it certainly takes some understanding of how Drupal works to write such a module. :)

A quick scan didn't reveal any obvious showstoppers. I have some minor concerns around sanitization of input being returned to the xmlrpc calls; but given that we preach 'never trust the other end of the connection', the onus falls on the consumer to validate before outputting.

That said, and based on #22 ...

Thanks for your contribution, rjohnson42!

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.

I just wanted to comment that this module along with the Python script from www.ditainfo.info (converting DITA) worked with ease.
This module should be published there is a LOT of sites that could use this.