Node Access Keys

* git release branch missing, see http://drupal.org/node/1015226
* remove LICENSE.txt, it is added automatically by drupal.org packaging
* nodeaccesskeys_edit(): doc block @param description indentation is only one space, should be 2. See http://drupal.org/node/1354#functions
* nodeaccesskeys_install(): not need to initialize variables here, variable_get() makes use of default values anyway. Remove the whole function.

Otherwise the code looks very good, seems nearly ready.

Please fix all code formatting issues, we'll get back to you soon.

Review of the 7.x-1.x branch:

• nodeaccesskeys.views.inc in nodeaccesskeys.info: It's only necessary to declare files[] if they declare a class or interface.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See http://ventral.org/pareview/httpgitdrupalorgsandboxdanielkorte1264754git.

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. Go and review some other project applications, so we can get back to yours sooner.

If you got questions on this, please ask!

There was no manual in-depth review yet.

Please read the documentation about the application workflow: http://drupal.org/node/539608

Did a manual review of the code, and everything looks okay (as what I know...).
Installed the module, without problems. The setup was easy and the functionality no problem. Thumbs up from here. And a really useful module.

manual review:

• "'access arguments' => array('administer site configuration')": do not use this generic permission, create your own.
• nodeaccesskeys_menu_local_tasks_alter(): why do you need that function? why can't you use MENU_LOCAL_ACTION on the menu item in hook_menu()?
• "'#type' => 'hidden',": you should use "value" instead of "hidden" if you do not plan that the user has access to this item. you can also simply store arbitrary variables in $form['#foo'] instead, as long as '#foo' does not conflict with any other internal property of the Form API.
• "unserialize($record->nodetypes);": instead of serializing yourself you can define your DB field as 'serialize' => TRUE, the the schema API will handle serialization for you.
• You should mention on the project page that your module works with user sessions, so people running reverse proxies like Varnish for caching might get problems if they cache pages for anonymous users.
• _nodeaccesskeys_protected_nodes(): you can use array_intersect_keys() then you don't need _nodeaccesskeys_get_role_names().
• "$types = array_map('filter_xss_admin', $types);": content types and roles are always expected to be plain text, so you should use check_plain() here.
• "filter_xss_admin($key->accesskey),": same here, shouldn't that be plain text?
• Why do you have to serialize $_SESSION['nodeaccesskeys_aids']?
• "$output = '<ul>';": use theme('item_list', ...) instead.

Get a review bonus and we will come back to your application sooner.

please visit http://ventral.org/pareview/httpgitdrupalorgsandboxdanielkorte1264754git for the seeing and correcting the list of errors being generated.

don't set needs work if there are only so few and minor issues found.

Hi Daniel,

I have just completed a review of your module and can't find anything major. I can confirm that points raised by klausi have been picked up.

There are no licensing issues as there is no third party code involved.

One suggestion would be to add the configure parameter to your .info file (I know you have a message that appears once the user has enabled the module, but adding the configure link would mean that there is a persisent link to the module's admin page from the admin/modules/list page too).

The module installed and uninstalled cleanly.

Setting to RTBC.

Oops - didn't change the status.

got to go, will continue later

Clarified 'registered'

Please add installation and usage instructions to your project page, also see tips for a great project page.

1. .install, 66: filter_xss_admin($message): filter_xss() not necessary your just printing out static text, no user values contained - (but better too many filters then too few)
2. .install: you should set and unset the site_403 variable when enabling/disabling your module as the path you set already won't be reachable after disabling it
3. views_plugin...inc: return t($this->options['node_type']); don't put variables into t() only strings (t('translate me')) are translatable, also content types are "already translated" as the user enters them
4. You check_plain() the access key in the node access key list twice, doing it in admin.inc:124 again is not necessary
5. in drupal7 it's not necessary to check_plain() in #selections, you don't have to check_plain() the content types twice there
6. $form['accesskey']['#default_value'] = check_plain($record->accesskey) also here's check_plain not necessary - #default_values are check_plain()ed automatically

It seems to me that your often not sure whether you should filter a value or not.
Think about..
1. Does this value come from outside? (User input, Browser data, etc)
-> Yes, save dirty - output with checkplain()
-> No, it's static text in my own source - just print it out
On the other site the are often functions / API's in drupal that already filter for you - therefore you got to pass the raw value to it and eg. the forms api will check_plain() itself.
After some time you will know what to filter or not automatically, until then - find it out by testing:
just replace the variable with eg. xyx"> and search the page dom for xyx
if it was escaped automatically it should be xyx"> and you don't have to care about filtering
otherwise you have to filter it your self

beside that your module looks pretty solid to me!

Thanks for your contribution and 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.