UUID Features - Menu

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.

No need to return anything from a hook_install/uninstall.
return db_add_field('menu_links', 'uuid', $schema['menu_links']['fields']['uuid']);
return db_drop_field('menu_links', 'uuid');

To avoid coder flagging exportables, might I suggest this small change? This format matches fields base and fields instance exports.

diff --git a/uuid_features_menu.features.inc b/uuid_features_menu.features.inc
index 037833f..99e8553 100755
--- a/uuid_features_menu.features.inc
+++ b/uuid_features_menu.features.inc
@@ -92,10 +92,10 @@ function uuid_features_menu_features_export_render($module, $data) {
     }
   }
   if (!empty($translatables)) {
+    $code[] = '';
     $code[] = features_translatables_export($translatables, '  ');
   }
 
-  $code[] = '';
   $code[] = '  return $menu_uuid_links;';
   $code = implode("\n", $code);
   return array('menu_default_uuid_features_menu' => $code);

First of all, thank you very much for carrying on this module, which solves a real need. I'm highly interestend in using it.

I'm doing allmost all work with features through drush. When installing your module, I got a message "Postgres doesn't currently have UUID functions build in. Please click here to generate UUIDs for your missing menu items." Hmm - no button in drush, but I supposed, drush uuid-create-missing would solve the problem.

However, when I tried drush fc '*', nothing about uuid menus showed up. The README.txt didn't help either. Only after navigating to admin/structure/features screen, I spotted the newly added tab "Generate UUIDs for Menu Items", which solved the problem.

Perhaps, you could delegate all the user interactions re. generating UUIDs to the UUID module? This would meet exspectations built by other modules, and additionally avoid a menu-specific tab (only used once) in the general features admin screen.

If this isn't feasible, it would be helpful to have a hint in the README.txt.

#3 fixed
http://drupalcode.org/sandbox/ioskevich/2163429.git/commit/1b3c154
http://drupalcode.org/sandbox/ioskevich/2163429.git/commit/1b3b4b3

Well, I tried to separate the feature request re. extended drush integration and the interface issues with the current state of the module (as described in #5). Users who will work with your module will have worked before with features and most probably with the uuid_features module. features provides a powerful drush integration, and uuid_features uses the same interface as the uuid module for generating UUIDs. So I'd suggest to consider if this interface can be reused, or otherwise to put hints about the difference into the README.txt - simply to make life easier for users coming with experiences from the mentioned modules.

I tend to agree with #2241567-8: UUID Features - Menu. Either something should be added to the project page and README about the different support for postgress and lack of support for drush. Or use uuid_generate() from the uuid module.

Sorry for not having been clear here: I did definitively not want to suggest that drush integration is mandatory for the project application. A hint would help site builders, though - and it's relatively cheap ;)

All the best - Joachim

Automated Review

n/a

Manual Review

Individual user account

(*) No: Follows the guidelines for individual user accounts. The last several commits to git have been for folks from the same company (per their email address) but not the person requesting full project access. Full project access can only be granted to a single individual.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes: Follows the guidelines for in-project documentation and the README.txt Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

• (+) No need to duplicate logic from uuid module. Use uuid_generate rather than _uuid_features_menu_generate_uuids()
• (+) This seems like unnecessary code. Each of the switch statements performs exactly the same thing. Plus this limits UUID Features to only those core entity types. No custom entities would work with UUID Features then. Remove the switch statements and the artificial entity limitation is removed. Same comments apply to uuid_features_menu_resolve_path().
  

  function uuid_features_menu_encode_path($path) {
    ...
    $router_item = $router[$router_path];
    $load_functions = unserialize($router_item['load_functions']);
    foreach ((array) $load_functions as $arg => $load_function) {
      if (is_array($load_function)) {
        $keys = array_keys($load_function);
        $load_function = $keys[0];
      }
  
      switch ($load_function) {
        case 'taxonomy_term_load':
          $term = taxonomy_term_load($args[$arg]);
          if ($term) {
            $args[$arg] = $term->uuid;
          }
          break;
  
        case 'node_load':
          $node = node_load($args[$arg]);
          if ($node) {
            $args[$arg] = $node->uuid;
          }
          break;
  
        case 'user_load':
          $user = user_load($args[$arg]);
          if ($user) {
            $args[$arg] = $user->uuid;
          }
          break;
  
      }
    }

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Hi ioskevich,

Did you try to use hook_uuid_sync() instead of manual trigerring via menu.

I tested this was, and it was fine for me.

You can replace this code:

/**
 * Implements hook_menu().
 */
function uuid_features_menu_menu() {
  $items = array();
  $items['admin/structure/features/mluuids'] = array(
    'title' => 'Generate UUIDs for Menu Items',
    'type' => MENU_LOCAL_TASK,
    'page callback' => 'uuid_features_menu_generate_page',
    'access arguments' => array('manage features'),
    'weight' => 100,
  );
  return $items;
}

With this one:

/**
 * Implements of hook_uuid_sync().
 */
function uuid_features_menu_uuid_sync() {
  function uuid_features_menu_generate_page();
}

And remove next lines then:

-batch_process('node/1');

-drupal_goto('admin/structure/features');

ioskevich: the reasoning for the individual account is that this is primarily a test of individual The Drupal Way adherence.

Does that make sense?

Can I assume then that ioskevich is the chosen individual? He must have actual commits that demonstrate his knowledge of the Drupal API.
The rest of the committers can come back later whenever they want to promote new projects. The other option is to pursue a single project promotion. In that option, a git admin promotes this project to a full project on your behalf, but no one becomes a git vetted user.

Made some code refactoring addressed to https://www.drupal.org/node/2241567#comment-8934079 and https://www.drupal.org/node/2241567#comment-8946115

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxioskevich2163429git

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

https://www.drupal.org/node/2241567#comment-8980653 should be fixed now

Based on git commit history, no single individual has demonstrated familiarity with the Drupal API. The original request was opened by @ioskevich, but then that individual hasn't made any commits for the last 2 months. The other two commiters since then have made a few changes, but nothing significant enough to demonstrate their Drupal coding competency. Assigning to another reviewer to make the final call but this application feels like it is a good candidate for a single project promotion.

Moving to RTBC as all feedback in #2241567-14: UUID Features - Menu is now addressed.

Automated Review

Review of the 7.x-1.x (124b3c5) branch:

• 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.

(+) The warning is a big one. Change the db_query in uuid_features_menu_link_set_uuid to a db_update.

Manual Review

Individual user account

No: Doesn't follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes: Follows the guidelines for in-project documentation and the README.txt Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

1. (*) In uuid_features_menu_generate_batch() it looks like $row['link_title'] is going straight to output w/o sanitization. This is user-input; see https://www.drupal.org/node/28984

Coding style & Drupal API usage

1. uuid_features_menu_generate_batch() needs a proper/better docblock.
2. Single quotes are preferred for strings per Drupal coding standards.
3. uuid_features_menu_generate_batch_finished(), would menu_rebuild() be more appropriate than a cache clear?
4. uuid_features_menu_link_load() can use one of the various fetchAll methods.
5. What is the global for in _uuid_features_menu_get_links? Comment needed
6. (*) uuid_features_menu_features_rebuild_ordered has a variable and not a placeholder in drupal_set_message.
7. (+) Change the db_query in uuid_features_menu_link_set_uuid to a db_update.
8. The limit in uuid_features_menu_generate_batch() should be a variable.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

I also agree with @heddn that we need to do single-project promotion with this.

I'd just like to chip in here and say that I've tested the functionality of this module and it works as expected. There are a couple of errors and warnings thrown at various points, although these don't seem to affect the funcitonality in any noticeable way:

When enabling the module:
PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'AS , t. AS t_ FROM t WHERE ( IN (''))' at line 1: SELECT t. AS , t. AS t_ FROM {} t WHERE ( IN (:db_condition_placeholder_0)) ; Array ( [:db_condition_placeholder_0] => ) in entity_get_id_by_uuid() (line 375 of /var/aegir/platforms/inathke-7.31-prod/sites/all/modules/uuid/uuid.entity.inc).

When creating the feature:
Warning: call_user_func() expects parameter 1 to be a valid callback, no array or string given in uuid_features_menu_encode_path() (line 139 of /var/aegir/platforms/inathke-7.31-prod/sites/all/modules/uuid_features_menu/uuid_features_menu.module).

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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