JaapB [jaapb]

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.

The only acceptable module and code under the project name/namespace "contextual" would be a backport of the Contextual module shipped with D7.

I really like the adminstertooltip module so far. Any ideas how we might incorporate the Skinr module's "Edit Skin" task?

I thought I might be able to replace the Skinr cog with your administertooltip module, by inserting some code like the snippet below into some of the hook_administertooltip_block_links()

E.g., in hook_administertooltip_ENTITY_AREA():

    if (module_exists('skinr') && module_exists('skinr_ui') && user_access('access skinr')) {
      $admin_links[] = l(t('Edit Skin'), 'admin/build/skinr/edit/nojs/block/'. $variables['block']->module .'-'. $variables['block']->delta, array('attributes' => array('class' => 'skinr-link ctools-use-dialog ctools-use-dialog-processed')));
    } 

While this did add the "Edit Skin" link, it does not open in a modal dialog window, nor does it do all the cool AJAX live updating in the background like the Skinr cog link does.

The Skinr cog appears to be constructed entirely in Javascript (skinr.js) where it seems to be hopelessly isolated from a more general (and extensible) contextual admin menu solution.

Any thoughts?

(My kingdom for a decent contextual-admin.)

• All user-submitted values and content need to be properly sanitized before output:
  

      $items[] = l(t('Edit content-type ') .'"'. drupal_ucfirst($node_type->name) .'"', 'admin/content/node-type/'. $variables['type'], array('query' => drupal_get_destination()));
  

• All of the module's JavaScript should implement Drupal behaviors. Some JavaScript properties and functions are outside of the namespace of this module, which also applies to Drupal.xyz properties/functions.
• The module contains a copy of the jQuery BeautyTips plugin, which exists as separate module on drupal.org. This module needs to add a dependency on the existing module, instead of duplicating the code (and maintenance).
• The excanvas JavaScript library may not be committed to drupal.org CVS. (Apache license) All code must be licensed under the GPL.
• Note that _administertooltip_extend() is based on a 3 year old tutorial, which suggests an architectural design that is considered bad today.
• LICENSE.txt as well as .hg directories should not be contained nor committed.

1. As it is, the function _administertooltip_extend() is just a copy of the Drupal function module_invoke_all().

   function _administertooltip_extend($hook_name, $data = NULL) {
     $items = array();
     foreach (module_implements($hook_name) as $module) {
       if ($new = module_invoke($module, $hook_name, $data)) {
         $items = array_merge($items, $new);
       }
     }
     // Make sure we return an array.
     if (!is_array($items)) {
       return array();
     }
     return $items;
   }
   

   function module_invoke_all() {
     $args = func_get_args();
     $hook = $args[0];
     unset($args[0]);
     $return = array();
     foreach (module_implements($hook) as $module) {
       $function = $module .'_'. $hook;
       $result = call_user_func_array($function, $args);
       if (isset($result) && is_array($result)) {
         $return = array_merge_recursive($return, $result);
       }
       else if (isset($result)) {
         $return[] = $result;
       }
     }
   
     return $return;
   }
   

   Why isn't the code using module_invoke_all()?

2. /**
    * Implementation of module_preprocess().
    */
   function administertooltip_preprocess(&$variables, $hook) {
   }
   

   The hook implementations are commented as Implementation of hook_<hook-name> (or more correctly Implements hook_<hook-name>).

3. Drupal 6 used hook_preprocess_page(), hook_preprocess_user_profile(), etc; rather than having a unique function, it would be better to split the code in different functions.

Point #3 is only a suggestion, not something that really needs to be done.
To explain it better, I will add some more notes here.

1. hook_preprocess_HOOK() implementations receive only an argument; $hook is not passed them, because they already know for which hook then are invoked.
2. The reason to use different hooks instead of only one is to avoid conditional code inside them with the only purpose to alter variables basing on the hook currently being processed.

  $entities = _administertooltip_get_entities();
  foreach ($entities as $entity => $array) {
    if (is_file(dirname(__FILE__) . '/entities/' . $entity . '.administertooltip.inc')) {
      require_once( dirname(__FILE__) . '/entities/' . $entity . '.administertooltip.inc');
    }
  }

Needs to use drupal_get_path() instead. And, actually, you should use module_load_include() instead.

  if (drupal_load('module', 'jquery_ui')) {
    jquery_ui_add(array('ui.draggable', 'ui.droppable', 'ui.sortable'));
  }
  else {
    drupal_set_message(t("You need to have the jquery_ui module installed."), 'error');
  }

Requirements error handling should happen in hook_requirements(), not during runtime.

jquery_ui module is listed as dependency in the .info file. It therefore needs to be installed to install this module. Drupal loads all installed modules.

  if (function_exists('beautytips_add_beautytips')) {
    beautytips_add_beautytips();
  }
  else {
    drupal_set_message(t("You need to have the beautytips module installed."), 'error');
  }

Identical issue as with jquery_ui here.

    $current_theme = variable_get('theme_default', 'none');
    $block_regions = system_region_list($current_theme);

You have to properly initialize the theme in order to detect its regions. There are various modules (plus system.module in core) that allow to change the theme depending on request conditions, such as an administration theme.

---

Sorry, this module is quite large and hard to review and therefore takes it time. I'm running out of time now. Leaving as "needs review" so perhaps others will help reviewing.

Let's start to fix what already reported.

• The points reported in this review are not in order or importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. The module doesn't implement hook_uninstall() to remove the Drupal variables it defines.

2. /**
    * Implements hook_perm.
    *
    * @return array
    */
   function administertooltip_perm() {
     return array('access administer tool tips', 'dragndrop blocks', 'administer administer tool tips');
   }
   

   The first line should be Implements hook_perm().. For hook implementations, parameters and returned value are not described.
   I would use different permissions; administer administer tool tips is not the best.

3. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.

1. function administertooltip_perm() {
     return array('access administer tooltips', 'drag and drop blocks', 'configure administer tooltips');
   }
   

   The last permission should be administer tooltips.

2. The code is not completely formatted as reported by the coding standards.
3. The JavaScript namespaces Drupal.ATT_DND and Drupal.ATT should be named to something closer to the name of the module.

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.

@JaapB: Thank you for reporting here the changed code. Your CVS application has been approved; you can commit the code in CVS, now. :-)