[D7] Access Filter

@hayashi
Can you please delete master branch and port it on 7.x-1.x.

There are couple of issues with the module:

1. ReadMe.txt is missing
2. Does not follow drupal standards
3. Some unused variable

For the issues in detail refer to
http://pareview.sh/pareview/httpgitdrupalorgsandboxhayashi2073981git

Hi hayashi!

Nice module idea. The ability to provide access to specific pages by IP could be really useful. This module actually reminds me a lot of https://drupal.org/project/path_access . Have you checked that module out? Aside from filtering by IP they seem quite similar.

Upon installation I did get an error message on admin/config/people/access_filter/add page. See attached screenshot.

During manual code review I did see that the .install file does not include a hook_uninstall function that deletes database variables upon uninstall. Have you considered adding this? It really helps to avoid database cruft. Update: realized this is not correct as you are not storing any variables outside of the custom table as defined in the schema :)

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 @hayashi,
I was testing your module and I really appreciate that you have created this kind of module.
But I find one small bug which you can solve by just putting below code:-

function hook_uninstall(){
  variable_del('YOUR_VARIABLE_NAME');
}

It will help you in deleting variable when uninstalling module.

Hi Tetsuya

1. Under file access_filter.module, function access_filter_ip_match, you don't need nested if statements in the following code.
   

     if (isset($patterns[1])) {
       if ($ip_long >= ip2long($patterns[0]) && $ip_long <= ip2long($patterns[1])) {
         return TRUE;
       }
     }
   

   Since, there's no separate statement under the parent if - the two if statement can be combined.

Thanks
Raman

PS: Not marking as 'Needs review' since it isn't a blocker.

Hi hayashi,

I've tried your module and I get an Error in permissions page.

Regarding duplication of module "Path access" I think there is no stable module for D7 and last commit is 11 months ago so I guess its development status is not active.

\BR

This sounds like a feature that should live in the existing path_access project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the path_access issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

So please add the differences to the existing module to your project page. See also https://drupal.org/node/997024

Hi Hayashi Thanks for module,There is a flaw in your module. You are restricting access on path(url alias) if i write some regex it surely works. But if i access same regex url through node nid it will not work. So you should have to look for nid too to make it complete secure. Thanks. Removing review tag you can add it again if you have done 3 more reviews.

hi hayashi, i have tested it and its working fine. Thanks for your contribution.

manual review:

1. access_filter_load(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075 . Also elsewhere.
2. There are not automated tests included with the module, please consider writing simpletests https://drupal.org/simpletest
3. access_filter_save(): why do you need the suppress warnings with "@" here? Please add a comment.
4. access_filter_form_filter(): don't use drupal_add_css() here, you should style information with #attached to the render array. See https://api.drupal.org/api/drupal/developer--topics--forms_api_reference...

But otherwise looks RTBC to me.

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

access_filter_load()

You really don't need db_select() here. db_query('SELECT fid, af from {access_filter} WHERE fid = :fid', array(':fid' => $fid)); will be much faster and gives you the same results.

Use of t()-function

• When using t() its best practice to add a context so common strings used by multiple modules are not overriden with one translation.
• ACCESS_FILTER_DENY_ACTION_403 => t('Error') . ': 403 Access Forbidden', Why don't you include the last part in the translatable string? Currently the string is not translatable :(.

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

Thanks for your contribution, hayashi!

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.