Any Menu Path

Hello there,

1. Developement is under the master branch, you should switch the dev line in a standard named branch like 7.x-1.x and empty the master itself
2. You did not wrap text (@see .module:17 i.e.) in t(), so it's not localized
3. I'ts recommended to delete your variables in hook_uninstall, best practice is placing it in sneaky_menu_path.install file
4. I would suggest you to run ventral and fix those coding issues

Regards,
brazorf

caseyc,

• Good job cleaning up ventral issues.
• Have you reviewed the thread at http://drupal.org/node/308263? Please review that thread and point to this module for review; you might offer up this module's checkbox enforcing path validation approach as an adminable setting for 8.x core.
• Based on thread activity this seems like it will be useful to many people. You might consider changing the name to something less subversive and more descriptive like 'Menu Path Validation Enforcement'.
• Is the direct $_SESSION manipulation necessary?

casey,

Yep drush dl menu_path_validation_enforcement is long, maybe any_menu_path? all_your_menu_path_are_belong_to_us ?

The Session manipulation isn't a big deal here, you're doing a simple set and unset here with trusted users. Since you're already storing and retrieving the 'sneaky' mlid array var between validation and submit though, I might've just
appended it to that variable - move the modified mlid's into a nested array and added an 'active' array of uid indexes or something. That's misusing variable_set, yeah, but avoiding session. But again, don't think it really matters.

This looks good to me, setting to RTBC and curious what the community will have to say!

Regarding $_SESSION

Why not use a #value form item and for validation and submitting use its value and set it though $form_state.

$form['sneaky_menu_path_wanted_value'] = array(
  '#type' => 'value',
  '#value' => 'http://example.com',

Regarding module name:

the module overrides/bypasses the validation and not enforces. Thus I like any_menu_path.

Regarding $_SESSION

I think the cleanest thing to do would be to stick $form_state['values']['link_path'] into $form_state['storage']

function sneaky_menu_path_menu_edit_item_before_validate($form, &$form_state) {
  // remove
  $_SESSION['sneaky_menu_path_link'] = $form_state['values']['link_path'];
  // replace with 
  $form_state['storage']['sneaky_menu_path_link_path'] = $form_state['values']['link_path'];
}

function sneaky_menu_path_menu_edit_menu_submit($form, &$form_state) {
  // remove
  $link_path = $_SESSION['sneaky_menu_path_link'];
  // replace with
  $link_path = $form_state['storage']['sneaky_menu_path_link_path'];
}

There are also some other little things you can do to make the code "cleaner" but those are nit-picky. Nice job on the concept and figuring out how to make it work.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Great feedback here. Looks like people have really been waiting for this :)

Re module name:
IMHO, this is worth getting some positive feedback that hopefully matches the code reviews' sentiments before going live. I'm partial to making the module name prefix "menu_path" or "menu" and choosing the best concept id for its suffix. Here are a few suggestions: _nocheck, _any, _always_valid, ~_preexisting, or _placeholder. I like the latter most, ie menu_placeholder or menu_path_placeholder. Any Menu Path is descriptive, but I think following naming conventions has tons of benefits.

Re similar efforts (for collaboration sake. none are 'duplicate efforts', so this is good on that front):
I saw Menu Editor also referenced in #308263: Allow privileged users to bypass the validation of menu items. Could you please describe how your approach differs from that module's? This isn't a show stopper for your own work here, but maybe they could factor out that feature if that's what's keeping it unstable. Also, that core issue seems like it really needs some knowledgeable reviewers, so if you could weigh in there with an advisable approach and patch review, I'm sure the core maintainers would really appreciate it.

All in all, this is pretty much rtbc for me.

You need to set the status to "needs review" if you want to get a review. See http://drupal.org/node/532400

It works as designed and does what it says.

manual review:

1. This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project for you.
2. "'#title' => 'No Path Validation',": all user facing text must run through t() for translation.
3. "variable_get('any_menu_path', array());": why is that variable needed? I don't see the need to store which menu links have circumvented the path validation? It is only useful for the #default_value of the checkbox, but you could simply check on the fly if the path exists. If not, then path validation must have been skipped when the menu link was created.
4. any_menu_path_menu_edit_menu_submit(): why do you run the query with link_path and link_title, why can't you use mlid from $form or $form_state? And you should use db_select() instead of db_query().

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

At the risk of just shooting myself in the foot here, I'd like to add a vote that the module is useful. A denial based strictly on how many lines of code might not be a good trend to start towards.

If the module meets standards, has no security issues, and the applicant has shown they are a valuable member of the community, basing denial on an application based just on how many lines of code are in it should be reconsidered.

We do not deny short modules, where did you read that? (need to clarify the docs)

We just can't grant the git vetted user role. However, we can promote this single project manually to a full project for the applicant.

My apologies, I misinterpreted this:

This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project for you.

And I had derived some of it from this discussion: http://groups.drupal.org/node/195848

This is a great module and I'd really like to see it promoted to a full project. Couple of minor things I noticed:

1. What do you think about changing the label to "Skip path validation"? It might help first-time users understand more quickly.
2. On the subject of the variable holding non-existent paths, I really like having the admin config page that lists them so that I could check on it from time to time and clear out paths that have been created and so forth. It's a valuable feature.
3. Do you think we could re-adjust the core form weights to group the new checkbox with the "Enabled" and "Expanded" options?

    $form['any_menu_path_no_path_valid'] = array(
      '#type' => 'checkbox',
      '#title' => t('No Path Validation'),
      '#description' => t('If this is enabled, Drupal will allow you to enter any path (even non-existent ones).'),
      '#weight' => -1,
    );

    // Set a few weights on the fields provided by core to group this option
    // with the "Enabled" and "Expanded" checkboxes
    $form['link_title']['#weight'] = -5;
    $form['link_path']['#weight'] = -3;

Since none of the above points are show-stoppers in terms of proper module development, I'd say this is RTBC.

manual review:

1. "'data' => 'Menu Title'": all user facing text must run through t() for translation. Please check all your strings. Same for "'empty' => 'No menu links overridden with the Any Menu Path module...'," and others.
2. any_menu_path_view_paths(): this is vulnerable to XSS exploits. If I have a menu item with a title <script>alert('XSSmenu');</script> I get a nasty javascript popup on that page. You need to sanitize user provided input before printing if it has not been sanitized before. Please read http://drupal.org/node/28984 again.

Just some core XREFs I know of. Maybe you can help with those?

- #1251188: Set unique titles for user/register, user/password, and user/login menu items
- #308263: Allow privileged users to bypass the validation of menu items
- #1337546: Cannot reorder 'User Menu' items for user/register user/login as these are not accessible by admin

@caseyc

I completely empathize with what you are feeling right now. I went through this same process almost a year ago with a module that I thought would be useful, only to have it denied because another module incorporated similar functionality, only with more complexity and other things the user might not want.

I just went through this same process again, and was approved this time for a different module.

I got frustrated like you the first time, and this second time the process was a little frustrating and discourage, but looking back I am thankful for the experience.

Please stick with it and keep trying.

@caseyrc:

I did a PAReview for you for your latest updates: http://ventral.org/pareview/httpgitdrupalorgsandboxcaseyc1670304git-7x-1x
No errors! Congratulations!

I also downloaded and installed your latest update, and it works as designed.

Also, great forethought in adding this: admin/config/content/any_menu_path which adds that nice extra touch for usability.

Marking RTBC.

Now, if you do three more PAReviews, and no one else raises any issues, Your module should be approved within a week. You just have to be patient for the process.

Thanks for your contribution, caseyc!

I updated your account to let you 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 get 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.

Congratulations!

Updating with news review bonus review links.

Yay :-)