Menu Rules

Hi,

welcome and thanks for the module.

PAReview
Came out all clean, good job.
http://ventral.org/pareview/httpgitdrupalorgsandboxordermind1524832git-7...

Readme
The ö in your name comes out wrong, guess converting the readme to UTF8 should help?
Maybe you can describe a bit more what you could do with the menu except automenu or how to get automenu to work. Do I need to enable rules UI as well? I would try to make this understandable for user have no clue about rules as you are "replacing" a module which had nothing to with rules. This is at least one usecase.

Manual review
The code looks clean and well structured. You have only 1 inline comment... I guess this lies within the nature of your module as most of it is configuration. I would consider this module a rules plugin, the actual functionality can be reduced to 4 small functions. But that's fine I guess and it does what it says is does. I couldn't get how I could make use of the automenu_replacement.txt though.

Issues:
.module
- Line 14: Not sure I have ever seen this anywhere before nl2br(utf8_encode(file_get_contents(....))), looks a bit evil to me, at least when it's just for setting the help text.

.rules.inc
- Line 72: I'm not doubting node_object_prepare() works to check if a node has a menu entry, but for me this seems the be the wrong way to do it. Have you tried e.g. menu_get_item('node/' . $nid)? This should return false if there is no menu item for the node.

automenu_replacement.txt
- how to make use of it

Someone who knows its way around rules should maybe take a look. Can't say anything about the implementation/configuration of the rules hooks. A simple functionality test worked. After all a nice helper module which I would definetly use. Way more flexible then the old autocomplete module and I guess way less code. Good idea.

> Someone who knows its way around rules should maybe take a look.
Looks good to me. You'd get even more thorough feedback by posting this as a patch in #459894: Integration with Menu API. I highly encourage this route, since its a core integration, and Rules sets out to provide those internally.

I'm +1 on the user review wrt code standards and API usage. So if it's possible to both recommend accepting the reviewed code and encouraging it not to be made into a full project, then that's my vote. Let's get this man some permisssions! :)

> Maybe you can describe a bit more what you could do with the menu except automenu or how to get automenu to work.
This makes automenu unnecessary. (I think Rules Bonus Pack is similar in that respect.)

> Do I need to enable rules UI as well?
You could enable Rules and this module, then import the provided example and have automated menu management without Rules UI enabled. It's only used for the admin configuration pages and when exposing configurable component rules to users, which this module doesn't do.

> I would try to make this understandable for user have no clue about rules as you are "replacing" a module which had nothing to with rules. This is at least one usecase.
Rules plugins developments often do this. See #417664-3: Action: switch theme for a similar case of 7 modules. (That lead to Theme Rules, which I should deprecate soon or move to a sandbox, and then an even better solution in 7.x #1540180: Add a property for user's default theme).

> this is a Rules plugin and should really be posted as a patch
Cool, RTBC then.

> I usually make separate modules first so that I can achieve the desired functionality quickly without having to wait for my patches getting accepted and committed
For sure!.. Another workflow you might consider is to fork modules in a sandbox, and use that while posting patches, but either way.. I'm glad you'll helping with Rules directly. Good luck!

Hi,

sorry, menu_get_item doesn't work, you are right. The menu system is a bit funny if you ask me. If not through the api then I would do something like:

  $query = db_select('menu_links')->condition('link_path', 'node/' . $nid);
  $num = $query->countQuery()->execute()->fetchField();

I love the Drupal API, but sometimes it's a bit heavy and if it's not used in the way it's meant to tones of db queries often get fired while one would do the trick. But nothing major of course, was just interested myself.

There is still a master branch, make sure to remove it. See also step 6 and 7 in http://drupal.org/node/1127732

manual review:

1. menu_rules_rules_condition_info(): the parameter should be called "node" and should be of type "node". Same for menu_rules_rules_action_info(). Rules will do the node loading and saving for you then.
2. I don't think this is a perfect fit for Rules core, so keeping it as separate project is ok. Of course you can open a issue in the Rules queue to discuss this further.

Thanks for your contribution, ordermind! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

just tagging

> Okay, I guess I'll keep it as a separate project for now.
I'm doing the proverbial dive in front of a speeding bullet right now..

You and ti2m are right, this is a Rules plugin and should really be posted as a patch. I'll get to work on that

This is good! This is right! This would be awesome! The people in #459894: Integration with Menu API are also waiting to review this code.

I can give you a little more instruction on how to port your existing code to a patch if you need. Just ask there, and link to your sandbox for others to test until then.

You can find out more about how significant your posting that patch would be by reading about the Rules Maintainers' Crew.