[D7] Hide Filepath

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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.

Erik,

The module installed, configured, and worked simple and easy. The video helps to easily understand!

1. This module has a couple minor pareview errors: (http://ventral.org/pareview/httpgitdrupalorgsandboxeriknewby1861422git)

• Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
• Remove "version" from the ./hide_filepath.info file, it will be added by drupal.org packaging automatically.
• Remove "project" from the ./hide_filepath.info file, it will be added by drupal.org packaging automatically.

2. Coding standards: A couple points:

In hide_filepath.install you initiate variables however do not implement a hook_uninstall to remove those variables.
Also, the db_query should be changed to a db_update (see: http://api.drupal.org/api/drupal/includes%21database%21database.inc/func...)

3. The module appears to support image fields and the video module - it would be good to list this on the Project page. In general, separating that first paragraph on the project page into sections "Use Case Scenario" and "Features" might help users understand what the module does.

(Also, the functionality for hiding videos did not work for me on a clean installation.)

Otherwise the module looks good!

FILE: /var/www/drupal-7-pareview/pareview_temp/hide_filepath.install
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
29 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------

manual review:

1. I'm a bit confused by the purpose of this module. I could just use private files supported by Drupal core instead? Also I think you should add a big fat note to your project page that this module does not protect the actual files in any way. If a user knows the path to a file she can access it without any protection and she can share that link with others. So your module does not provide any real security.
2. hide_filepath_install(): do not juggle with module weights, that is unreliable. Use hook_module_implements_alter() if you need to run before/after a specific hook.
3. hide_filepath_install(): No need to set variables upon installation as you can make use of default values with variable_get() anyway.
4. hide_filepath_init(): so your module only works with nodes and not with fields on other entity types? Please add that to the README.txt and the project page.
5. hide_filepath_init(): "$function = $module . '_hide_filepath_pre_validation';": that function is not invoked on the next line, your are kust assigning a variable.
6. "module_implements('hide_filepath_pre_validation')": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php
7. hide_filepath_init(): what is all this hacking around with arg()? Why can't you register a proper path with hook_menu()? Also hook_init() is called on every single page request, so that big function will slow down your site considerably.
8. "$field['und']": do not use "und", use LANGAUGE_NONE instead.
9. "$file_path = check_plain(file_create_url($file->uri));": why do you call check_plain() here? The URL is never printed to the user? And the created file URL is not untrusted user provided input? Please read http://drupal.org/node/28984
10. hide_filepath_settings_page(): that function can be removed, just use drupal_get_form as page callback in hook_menu() to directly invoke the form builder.
11. hide_filepath_settings_validate(): empty function, so remove it.
12. hide_filepath_settings_submit(): that function can be removed if you use system_settings_form() in hide_filepath_settings().

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

Some minor suggestions:

In hide_filepath.install:18 : It's better if you use the a tag in the t() function. This makes is less confusing to translators.
Ex:

drupal_set_message($t("Hide Filepath settings are available under <a href="@hide-filepath-admin">Administer > Site configuration > Hide Filepath</a>",
 array('@hide-filepath-admin' => url('admin/config/media/hide_filepath/settings'))
));

Using the a tag directly makes it much easier to translate the string, and that's the recommended way (There was a doc file describing it but couldn't find it yet).

hide_filepath.module:38: You are implementing hook_form_FORM_ID_alter.

hide_filepath.module:180: UID is an integer.

hide_filepath_settings.inc: Is there any reason why you didn't use system_settings_form() ?

I enabled your module, set the settings to "Access denied for Anonymous" users, but when I as anonymous clicked on the file link in my node it still loaded the file. Let me know if I misunderstood the purpose of the module.

Minor issue, Code_sniffer found two space issues.
FILE: .../nathalie/Sites/drupal-7.20/modules/hide_filepath/hide_filepath.install
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
20 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
21 | ERROR | Line indented incorrectly; expected 2 spaces, found 0
--------------------------------------------------------------------------------

Could you elaborate on why/how to use system_settings_form()? I'd be happy to, I just don't think I understand what I did vs. what you are suggesting.

What I mentioned was actually to make use of the system_settings_form() function from core, that adds the proper "Save configuration" button and does the variable_set part for you.

Change return $form; with return system_settigns_form($form);.
This will add a submit button and a handler that saves values using variable_set. Variable names are taken from the $form_state['values'] array (thus, the key from the $form array).

This is all you need in the hide_filepath_settings.inc:

function hide_filepath_settings($form, &$form_state) {
  $form = array();
  $form['hide_filepath_anonymous'] = array(
    '#type' => 'select',
    '#title' => t('Anonymous User'),
    '#description' => t('Choose what an Anonymous User should receive when clicking a file path.'),
    '#options' => array(
      '403' => t('Access Denied'),
      '404' => t('Page Not Found'),
      'login' => t('Login challenge'),
      'custom' => t('Use custom hook'),
      'bypass' => t('Allow Normal Access'),
    ),
    '#default_value' => variable_get('hide_filepath_anonymous', 'login'),
    '#required' => TRUE,
  );

  $form['hide_filepath_authenticated'] = array(
    '#type' => 'select',
    '#title' => t('Authenticated User'),
    '#description' => t('Choose what an Authenticated User should receive when clicking a file path.'),
    '#options' => array(
      '403' => t('Access Denied'),
      '404' => t('Page Not Found'),
      'custom' => t('Use custom hook'),
      'bypass' => t('Allow Normal Access'),
    ),
    '#default_value' => variable_get('hide_filepath_authenticated', 'bypass'),
    '#required' => TRUE,
  );

  return system_settings_form($form);
}

You don't need to add a submit button and/or a submit handler.
Also pay attention to the variable_get() call, that we have a second parameter which returns in case the value is not set. variable_get function requires parameters by default - the variable name and a value to return in case the value is not set already.

Module worked well for me, and really nice work there. Good luck!

Nice work Erik! This looks RTBC to me!

Performed a manual code walkthrough with Erik here at Drupalcon, no critical blockers left.

manual review:

1. hide_filepath_api.module: should be hide_filepath_api.php. And should contain examples.
2. Take a look at your exit calls, they might not be needed.
3. you could use module_invoke_all() to call your hooks.
4. another check_plain() call that is not necessary.
5. hide_filepath_settings(): wrong doc block, see http://drupal.org/node/1354#functions

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

Assigning to cubeinspire as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, eriknewby!

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.