[D7] Moderate mmenu

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

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.

Hi samuel.mortenson,

I have had a look through you code and came across a few issues. I am assuming you did not see them die to the setup you have been using so a quick bit of info on what I used.

I tested your module with a standard install of drupal with contrib modules ctools, libraries, scald, views and your mdoule mederate_mmenu. The theme used was bartik as default and Garland was enabled.

Automated Review

You are getting a few errors. http://pareview.sh/pareview/httpgitdrupalorgsandboxsamuelmortenson249244...

Manual Review

README.txt/README.md

Your README.txt could follow the template.

You do not explicitly say how the external library should be installed or the location. Pointing to the Library docs could be a place to start.

Coding style & Drupal API usage

1. moderate_mmenu.module, function moderate_mmenu_get_menu_subtitle() line 107 is showing an error if workbench is not installed.
   Notice: Undefined property: stdClass::$workbench_moderation in moderate_mmenu_get_menu_subtitle()

   $state = $node->workbench_moderation;

2. moderate_mmenu.module, function moderate_mmenu_get_menu_subtitle() line 109 is showing an error if workbench is not installed.
   Notice: Trying to get property of non-object in moderate_mmenu_get_menu_subtitle()
   Notice: Trying to get property of non-object in moderate_mmenu_get_menu_subtitle()

   $state['published']->vid != $state['current']->vid

3. moderate_mmenu.module, function moderate_mmenu_preprocess_page() line 34 is showing an error.
   Notice: Undefined index: moderate_mmenu in moderate_mmenu_preprocess_page()

   Seems like it is having issues figuring out the region you are attempting to create in moderate_mmenu_system_info_alter(). I am not getting any new blocks created.

4. I am also getting an error in the js but that could be down to the other issues so I have not looked into it.

I have had a second look now and have managed to get it working and it is looking good.

I can still generate the error for moderate_mmenu.module, function moderate_mmenu_preprocess_page() line 42 (was 34). Looking into it further it is depends on how you clear the cache. If I install the module Administration menu like a lot of people use then I can reset the cache using the menu itself and all is working.

However if you flush the cache from /admin/config/development/performance then this will cause the error (It will also disable any blocks added to the region from a pervious addition). The problem seems to be in the function moderate_mmenu_system_info_alter() and what it decides the theme is at the current point in time, i.e. it is changing from seven and bartik depending on how the cache is flushed.

Their is another minor issue I noticed. If people are using the administrative overlay then the javascript from the expanded menu means you need to click twice before you can edit the node. I think you are just note down the issue on the project page as suggested in the project page template.

Looks good now. I am getting no errors and you have mentioned potential issues on you project page so I cannot see any blockers.

I was looking at your js file and ran it through ESLint with the coding standards taken from Drupal 8. You could if you are interested have a look (I attached an output) but I do not think it would be enforced. The main item is indentation. You have tab setup as 4 characters I guess, where drupal prefers 2 characters.

My only concern would be to be clear that cache reseting from /admin/config/development/performance would break the feature until another reset from the admin_menu module. In the known issue I think an explicit mention would be good so people known the quick fix for it. Also you could mention a solution to this is that they add the region themselves in the theme .info file. This would stop the issue completely.

Thanks for your contribution!

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.