[D7] Plista

Automatic review of the 7.x-1.x branch:

No problems, very cool!

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. You have to get a review bonus to get a review from me.

Source: http://ventral.org/pareview - PAReview.sh online service

Code review:

1. plista.admin.inc - wrap user-visible text in https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7
2. plista.admin.inc - validate plista_api_key, plista_domain_id (assuming they're required and should be no longer than XXX)
3. plista.admin.inc - remove empty line 155
4. plista.install - delete empty lines 11, 25
5. plista.module - wrap user-visible text in t()
6. plista.module - plista_field_extra_fields - check if empty and an array first before iterating
7. plista.module - plista_perform_update - mention the operation (update) in the watchdog entries
8. theme/plista-code.tpl.php - use https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_... instead

Project application checklist - https://drupal.org/node/1587704

1. 5.1/5.2 - Provide a project overview; what does it do (many people won't know what plista is or RecommendationAds are), why would someone want to install this?
2. 5.3 - Pretty sparse on online comments in plista.module
3. 7.1 - t function especially - see https://drupal.org/node/1187664

Pretty solid overall, haven't done a functionality test yet.

Hello chr.fritsch

My manual review:

.install

lines 10ss

function plista_uninstall() {
  $variables_to_delete = array(
    'plista_javascript_url',
    'plista_widgetname',
    'plista_field_title',
    'plista_field_text',
    'plista_field_img',
    'plista_field_category',
    'plista_node_types',
    'plista_hidden_paths',
    'plista_advanced',
  );

  foreach ($variables_to_delete as $variable) {
    variable_del($variable);
  }
}

Extra code. You create an array and later a foreach to delete the variables. You can changed to

function plista_uninstall() {
  variable_del('plista_javascript_url');
  variable_del('plista_widgetname');
  variable_del('plista_field_title');
  variable_del('plista_field_text');
  variable_del('plista_field_img');
  variable_del('plista_field_category');
  variable_del('plista_node_types');
  variable_del('plista_hidden_paths');
  variable_del('plista_advanced');
}

less codelines and less cpu use.

.module lines 135ss

  if (
    empty($js_url) || !in_array($node->type, $selected_node_types) ||
    $page_match || !$node->status
  ) {
    return '';
  }

Coding standards "Line length and wrapping" (https://drupal.org/coding-standards#linelength): "Control structure conditions may exceed 80 chars, if they are simple to read and understand"

.module lines 148 149

You can use check_plain instead strip_tags

.admin.inc line

if (module_exists('token')) {

useless. The token module is a dependency so alway must exists.

Extra code. You create an array and later a foreach to delete the variables... less codelines and less cpu use.

Actually, I disagree with #3 on this, the foreach is just fine. Less CPU use? That's not a valid criticism, especially in the context of an uninstall. The original method was clean; populate an array with what you want to do, then run the same command on every value on the array.

No action needed, don't change it back or anything; I just want you to know that a foreach to delete variables is just fine.

Starting review

Hi @chr.fritsch

Looks like a cool module, nicely documented and worked as stated when I installed and tested it. Here's my notes from code review.

1. plista_help

function plista_help($path, $arg) {
  switch ($path) {
    case 'admin/help#plista':
      return check_plain(file_get_contents(dirname(__FILE__) . "/README.txt"));
  }
}

This is more of a question - is this approach for hook_help reading a READEME.txt a standard pattern in Drupal? I've not seen it before, but it may be.

2. plista_node_view

function plista_node_view($node, $view_mode, $langcode) {

  $node->content['plista_widget'] = array(
    '#type' => 'markup',
    '#markup' => plista_view_widget($node),
  );
}

2.1 Drop the first empty line (there's a couple of other examples of that too)
2.2. #type => "markup" is not required, as that is the default

3. watchdog calls

  switch ($request->code) {
    case "200":
      watchdog('plista', "Action '" . $action . "' successful", WATCHDOG_INFO);
      break;

    case "400":
      watchdog('plista', 'Unknown node: ' . $node->title, WATCHDOG_WARNING);
      break;

    case "403":
      watchdog('plista', 'Access denied', WATCHDOG_WARNING);
      break;

    default:
      watchdog('plista', 'Unknown Response-Code: ' . $request->code, WATCHDOG_WARNING);
      break;
  }

For things like " 'Unknown node: ' . $node->title" see the documentation from function watchdog()

* @param $message
* The message to store in the log. Keep $message translatable
* by not concatenating dynamic values into it! Variables in the
* message should be added by using placeholder strings alongside
* the variables argument to declare the value of the placeholders.
* See t() for documentation on how $message and $variables interact.

For an example of this approach see code from salesforce_pull.module (in salesforce module)

          watchdog('Salesforce Pull',
            'Created entity %label associated with Salesforce Object ID: %sfid',
            array(
              '%label' => $wrapper->label(),
              '%sfid' => $sf_object['Id'],
            )
          );

Hope this helps.

Regards,
Willie

1. You should use current_path() instead of $_GET['q'] and drupal_substr() or truncate_utf8() instead of substr in plista_view_widget() function.
2. Use url() to generate external URL at line 198 in plista_perform_update() function.
$url = 'http://farm.plista.com/api/item/' . $action . '/' . $node->nid . '?' . drupal_http_build_query(array(
'domainid' => $plista_advanced['plista_domain_id'],
'apikey' => $plista_advanced['plista_api_key'],
'status' => FALSE,
));

No problems found, nice integration module!

----
Top Shelf Modules - Crafted, Curated, Contributed.

Thanks for your contribution, Christian!

I updated your account so you can 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 stay 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.