[D7] Asset Aliases

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Runs through pareview cleanly with only complaints about the number of blank lines at the end of .info and the readme.
http://pareview.sh/pareview/httpgitdrupalorgsandboxjojo-m2236743git-master

Installs without errors.

Coder threw back no errors.

I would suggest removing the master branch and creating a 7.x-1.x (or as appropriate) branch.

See step 5 and 6.

https://drupal.org/empty-git-master

Hi Jojo-M

Thanks for your contribution, I've reviewed your module, but can't really find any errors to comment on. Everything looks clean and works fine. There are two errors on parereview.sh but by no means showstoppers.

But I do have some 'devil's advocate-type' questions about this module, about some of the code and how it works compared to the pathauto_entity module.

I realised that this module reproduces a lot of code of the original pathauto module, even the comments are the same. The fact that the comments (here and there replaced with the word 'asset') are the same is not really important here, I personally don't mind, don't know about comment copyrights...:). But it does make me think about possible code redundancy.

For example at asset_aliases.module:193 you could literally say

function asset_aliases_field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode) {
  pathauto_field_attach_form($entity_type, $entity, $form, $form_state, $langcode) ;
}

instead of

function asset_aliases_field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode) {
  list($id, , $bundle) = entity_extract_ids($entity_type, $entity);

  if (!isset($form['path'])) {
    // This entity must be supported by core's path.module first.
    return;
  }
  else {
    $form['path'] += array(
      '#type' => 'fieldset',
      '#title' => t('URL path settings'),
      '#collapsible' => TRUE,
      '#collapsed' => empty($form['path']['alias']),
      '#group' => 'additional_settings',
      '#attributes' => array(
        'class' => array('path-form'),
      ),
      '#access' => user_access('create url aliases') || user_access('administer url aliases'),
      '#weight' => 30,
      '#tree' => TRUE,
      '#element_validate' => array('path_form_element_validate'),
    );
  }

  $pattern = pathauto_pattern_load_by_entity($entity_type, $bundle, $langcode);
  if (empty($pattern)) {
    return;
  }

  if (!isset($entity->path['pathauto'])) {
    if (!empty($id)) {
      module_load_include('inc', 'pathauto');
      $uri = entity_uri($entity_type, $entity);
      $path = drupal_get_path_alias($uri['path'], $langcode);
      $pathauto_alias = pathauto_create_alias($entity_type, 'return', $uri['path'], array($entity_type => $entity), $bundle, $langcode);
      $entity->path['pathauto'] = ($path != $uri['path'] && $path == $pathauto_alias);
    }
    else {
      $entity->path['pathauto'] = TRUE;
    }
  }

  // Add JavaScript that will disable the path textfield when the automatic
  // alias checkbox is checked.
  $form['path']['alias']['#states']['!enabled']['input[name="path[pathauto]"]'] = array('checked' => TRUE);

  // Override path.module's vertical tabs summary.
  $form['path']['#attached']['js'] = array(
    'vertical-tabs' => drupal_get_path('module', 'pathauto') . '/pathauto.js',
  );

  $form['path']['pathauto'] = array(
    '#type' => 'checkbox',
    '#title' => t('Generate automatic URL alias'),
    '#default_value' => $entity->path['pathauto'],
    '#description' => t('Uncheck this to create a custom alias below.'),
    '#weight' => -1,
  );

  // Add a shortcut link to configure URL alias patterns.
  if (drupal_valid_path('admin/config/search/path/patterns')) {
    $form['path']['pathauto']['#description'] .= ' ' . l(t('Configure URL alias patterns.'), 'admin/config/search/path/patterns');
  }

  if ($entity->path['pathauto'] && !empty($entity->old_alias) && empty($entity->path['alias'])) {
    $form['path']['alias']['#default_value'] = $entity->old_alias;
    $entity->path['alias'] = $entity->old_alias;
  }

  // For Pathauto to remember the old alias and prevent the Path module from
  // deleting it when Pathauto wants to preserve it.
  if (!empty($entity->path['alias'])) {
    $form['path']['old_alias'] = array(
      '#type' => 'value',
      '#value' => $entity->path['alias'],
    );
  }
}

(I've tried it).

As for the already existing pathauto_entity module: the Pathauto Entity module does for all entities, what you do the Asset entity only. On the other side your module adds the possibility to change the path per asset, the same way the pathauto module allows you to change the path per node.

I see no crucial errors in this module, not even minor errors, but do think that submitting a patch to pathauto or pathauto_entity, or create a dependency of pathauto_entity, could work as well. No need to change the status though.

Automated Review

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives, and may be duplicate results from Coder Sniffer. 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. You have to get a review bonus to get a review from me.

Manual Review

You don't need to variable set in asset_aliases_install(). Just use the default when you variable_get().

(+) asset_aliases_uninstall should use Path API to get rid of the aliases (so hooks fire and the path cache gets cleared). Also that wildcard will likely catch things it shouldn't.

(+) You have a single variable in the module. Just variable_del() in the uninstall. Wildcard db_delete on the variables table can be problematic.

Look into hook_module_implements_alter() to change the hook execution order instead of setting the weight in the install.

You don't need to `if (module_exists('asset'))` if you have a dependency. Also elsewhere.

Your db_select() in asset_aliases_pathauto_bulk_update_batch_process can use chaining, except for the joins.

A bunch of your docblocks are wrong. For example, "Implements hook_insert()." should be "Implements hook_asset_insert()." Look at all and fix when you can.

If you have derived code, it would be best to document where it came from.

(+) Can you explain the top level if() at line 361 in asset_aliases.module? One, a module implementing a hook for another module is weird.

This is more a note to myself and other reviewers, but the user_access() mirrors the path.module.

Seems to work. No third-party code, I think duplication/collaboration has been adequately addressed, and I am not seeing any security concerns.

The items marked (+) should be addresses, but I don't see the as show stoppers.

Setting RBTC, but the unofficial policy is for admins to not mark as Fixed upon their own review, so this will another review or a waiting period for other people to chime in.

Hi Jojo-M,

Sounds like you've considered all the options regarding pathauto and pathauto entity, so like mpdonadio says, my question was adequately addressed.

OK, about two weeks have gone by without any objections. I took a look at the project again (which didn't look like it had changed), and I stand by my review. There are a few high priority items that should be addressed, but no blockers. So...

Thanks for your contribution, Jojo-M!

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.