Hi,

I just mentioned that the hook_reviews() is used in a quite strange way :-) in the implementation it says the following:

/**
 * Implements hook_reviews().
 */
function coder_review_reviews() {
  global $_coder_reviews;
  if (!isset($_coder_reviews)) {
    $_coder_reviews = array();
    $path = drupal_get_path('module', 'coder_review') . '/includes';
    $files = drupal_system_listing('/coder_review_.*\.inc$/', $path, 'filepath', 0);
    foreach ($files as $file) {
      require_once DRUPAL_ROOT . '/' . $file->uri;
      $function = $file->name . '_reviews';
      if (function_exists($function)) {
        if ($review = call_user_func($function)) {
          $_coder_reviews = array_merge($_coder_reviews, $review);
        }
      }
    }
  }
  return $_coder_reviews;
}

Why is there a global variable used? Is this to avoid creating the review array more than once on a single request? If so... this should be done in the _coder_review_reviews() function, not the hook implementation itself as all other modules defining reviews would have to do it also.

So my proposal for this would be the following:

/**
 * Implements hook_reviews().
 */
function coder_review_reviews() {
  $reviews = array();
  $path = drupal_get_path('module', 'coder_review') . '/includes';
  $files = drupal_system_listing('/coder_review_.*\.inc$/', $path, 'filepath', 0);
  foreach ($files as $file) {
    require_once DRUPAL_ROOT . '/' . $file->uri;
    $function = $file->name . '_reviews';
    if (function_exists($function)) {
      if ($review = call_user_func($function)) {
        $reviews = array_merge($reviews, $review);
      }
    }
  }
  return $reviews;
}

And in _coder_review_reviews() it should say:

/**
 * Get all of the code review modules, including contributions.
 */
function _coder_review_reviews() {
  $reviews =& drupal_static(__FUNCTION__, NULL);
  if (is_null($reviews)) {
    $reviews = module_invoke_all('reviews');
  }
  return $reviews;
}

Using this approach, every module only get invoked once and no global variable is needed...

I'd appreciate your feedback.

Thanx in advance & cheers

hctom

Comments

hctom’s picture

hctom
Primary language German
commented

HI @all,

is there any feedback for this issue?

To extend this, i'd even like to post another feature request for the _coder_review_reviews() function :)

/**
* Get all of the code review modules, including contributions.
*/
function _coder_review_reviews() {
  $reviews =& drupal_static(__FUNCTION__, NULL);
  if (is_null($reviews)) {
    $reviews = module_invoke_all('reviews');
    // Allow other modules to alter code reviews
    drupal_alter('reviews', $reviews); 
  }
  return $reviews;
}

With this approach, other module's may alter code reviews... but for that every review rule needs a unique key in its review, so you can do something like this:

function mymodule_reviews_alter(&$reviews) {
  $reviews['review_name']['rule_name']['#severity'] = SEVERITY_NORMAL;
}

... and to speed things up, a database cache table should be utilized to cache all reviews. The image_styles() function of the image module uses a similar approach:

function image_styles() {
  $styles = &drupal_static(__FUNCTION__);

  // Grab from cache or build the array.
  if (!isset($styles)) {
    if ($cache = cache_get('image_styles', 'cache')) {
      $styles = $cache->data;
    }
    else {
      $styles = array();
      // ... more code to gather all styles
      // Allow other modules to alter styles
      drupal_alter('image_styles', $styles);
      // Cache styles
      cache_set('image_styles', $styles);
    }
  }

  return $styles;
}

I'd appreciate if these things would make it into the module's source.

Thanx in advance & cheers

hctom

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Active » Closed (won't fix)

Coder 7.x-1.x is frozen now and will not receive any updates. Coder 8.x-2.x can be used to check code for any Drupal version, Coder 8.x-2.x also supports the phpcbf command to automatically fix conding standard errors. Please check if this issue is still relevant and reopen against that version if necessary.