[D7] Permissions by Term

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2325361

Project 2: https://www.drupal.org/node/2312097

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxjepSter2325357git

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 jepSter,

Your git clone path has been edited by me as the path provided was your private path not the public path which can be used for cloning of the project by any other user.

Manual review

Individual user account
OK. Applicant geekygnr is an individual user account.
Multiple applications
OK. No additional applications.
Licensing
OK. Follows the Drupal.org licensing requirements.
3rd party (non-GPL) code
OK. No 3rd party code.
Module duplication
OK. No duplication.
Project page
OK. Adequate presentation of project.
README.txt
Not OK. Please upload README.txt as per the guidelines of drupal as this is uploaded as blank file.
Code length/complecity
OK. About 400 lines of code .
Coding style
Not OK. The code is not formatted as per drupal coding standards.

@jepSter, thankyou for your contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Yes, http://pareview.sh/pareview/httpgitdrupalorgsandboxjepster2325357git reported number of issues that need to be fix.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

(*) May Be: Does not cause module duplication and fragmentation. I tested your module functionality, I feel this feature can be incorporated into Taxonomy Term Permissions module. Could you open an issue to Taxonomy Term Permissions and confirm that they don't have plans to incorporate similar functionality any time soon?

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

(*) No: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*) IMHO I would suggest you, explain in Readme.txt and on your project page how this module works because lack of documentation even I was unable to test this module functionality. As I tried to assign a role and user specific permission to one term but dint work as intended. As I gone through your module code, came to know what exactly you are trying to do but still not 100% clear. for example, if I have a vocabulary Shop that contains two terms shop1 and shop2. If one shop have permission to one user and another shop have access permission to other user then it behaves incorrectly. Also this taxonomy term reference field don't get visible in assigned content type, even for user 1(also try assigning permission to user 1 (:). Also at every stage it producing number of warnings and notices listed below:
   
   
   
   
   

   Also I am wonder with following code, you simply unset the field for unauthorized user but if any authorized user have updated this value then what happen. Still I am unable to complete the flow of this module so unsure how it is going to be work. Also there should be consistency for unauthorized user, at one place you are using drupal_access_denied(); and at other place 401-unauthorized.

   Not sure about following code as I am unable to produce this message for unauthorized user.

   
   if (count($options) <= 1) {
             if ($vocabulary['#required']) {
   ...
         drupal_access_denied();
               exit();
      }
   unset($form[$field_name]);
   }
   

2. (+) permissions_by_term_menu(): Given menu path like is visible to every user in left navigation that looks wired, it shouldn't be link anywhere as this used internally. Also still I feel it would be better to use drupal_access_denied() instead.
   

   $items['401-unauthorized'] = array(
       'title' => "401: Unauthorized",
       'page callback' => 'page_user_not_allowed',
       'access arguments' => array('access content'),
     );

   
   
   

3. (+) Why are you using array_shift() in this code, rather use simply user_load_by_name() and check the return value that will evaluate either user name exist or not.
   if (!(array_shift(user_load_multiple(array(), array('name' => $name))))) {

   Same at this place. $u = array_shift(user_load_multiple(array(), array('name' => $name)));

4. Project page: So development happened and the result is the "Taxonomy Term Permissions"-module. It needs correction and also go through https://www.drupal.org/node/997024 again.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!

Hi jepSter,
The module seems to work fine although there are some minor warnings in the code, see the attachment.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder.

http://pareview.sh/pareview/httpgitdrupalorgsandboxjepster2325357git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Got similar module Taxonomy Term Permissions but you already provided explaination for this.
Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the

href="https://www.drupal.org/node/2181737">README Template

.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes/No. If "no", list security issues identified.

Coding style & Drupal API usage

Enabaled this module on a vanila Drupal 7.3 installation. Tried providing permission to multiple terms to different user roles. Seems to be working fine.

Just want to understand use of hook_node_view in your module

/**
 * Implements hook_node_view().
 */
function permissions_by_term_node_view($node, $view_mode, $langcode) {
  global $user;
  $node_wrapper = entity_metadata_wrapper('node', $node);

  $user_is_allowed_to_view = FALSE;

  foreach ($node_wrapper->field_secured_areas->getIterator() as $term_wrapper) {

    $term_secured_areas = $term_wrapper->value();

    if (isset($term_secured_areas->tid) && permissions_by_term_allowed($term_secured_areas->tid, $user) === TRUE) {
      $user_is_allowed_to_view = TRUE;
    }

  }

  if ($user_is_allowed_to_view === FALSE) {
    drupal_access_denied();
    exit();
  }
}

Somehow I generated content using devel generate module and then start getting this error:

Looks like all issues was fixed by jepSter.

Automated Review

Review of the 7.x-1.x branch (commit ac5ebaf):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Causes module duplication and/or fragmentation. Have you contacted the Taxonomy Term Permissions maintainers via the contact form?

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party code

Yes: Follows the guidelines for 3rd party code. README mentions fork.

README.txt/README.md

Yes: Follows / No: Does not follow] the guidelines for in-project documentation and/or the README Template. Could be better.

Code long/complex enough for review

(*) Possibly follows the guidelines for project length and complexity. I did a diff of this and term_permissions, and it is hard to determine how much of this is original code versus renamed code.

Secure code

Coding style & Drupal API usage

(+) Don't split translated strings across lines just to satisy PAReview. Split strings are harder to translate.

I would split permissions_by_term_form_alter() into specifc alters for the different forms instead of having them all in one form.

(*) permissions_by_term_form_alter() for 'taxonomy_form_term' needs some sore of permission check on it. You should implement hook_permission, and define your own.

The db_select() in permissions_by_term_form_alter can utilize one of the ->fetchAll() methods for building up the results.

permissions_by_term_submit() isn't a hook_submit. It's the submit handler for permissions_by_term_form_alter().

permissions_by_term_validate isn't a hook_validate. It's the validation handler for permissions_by_term_form_alter().

In permissions_by_term_process_node(), isn't the $variables['node'] aready there?

(*) hook_process_node() is the wrong place to that logic for hiding results in a view. Look at https://api.drupal.org/api/views/views.api.php/group/views_hooks/7 for a better hook to use. I think hook_views_post_execute would be the best fit.

(*) hook_node_view should not be used to deny access to a node. There are many ways this can go wrong. You need to use the node access system. See https://api.drupal.org/api/drupal/modules%21node%21node.module/group/nod... for how to do this properly. You really need to use grants for this.

In permissions_by_term_node_view, where is field_secured_areas coming from?

You aren't using a lot of the Entity API. You can likely just use the Field API, eg field_get_items, to eliminiate a module dependency.

(*) permissions_by_term_autocomplete_multiple, the `exit(drupal_json_output($matches));` wrong. See https://api.drupal.org/api/drupal/modules%21profile%21profile.admin.inc/... and https://api.drupal.org/api/drupal/modules%21profile%21profile.module/fun... for an example in core for autocomplete w/ JSON.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Review of the 7.x-1.x branch (commit dd7ad77):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/permissions_by_term.module
  ---------------------------------------------------------------------------
  FOUND 9 ERRORS AFFECTING 9 LINES
  ---------------------------------------------------------------------------
   194 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
   195 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
   204 | ERROR | [x] Line indented incorrectly; expected 8 spaces, found 6
   208 | ERROR | [x] Line indented incorrectly; expected 8 spaces, found 6
   214 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
   215 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
   223 | ERROR | [x] Line indented incorrectly; expected 8 spaces, found 6
   325 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 5
   326 | ERROR | [x] Line indented incorrectly; expected 3 spaces, found 2
  ---------------------------------------------------------------------------
  PHPCBF CAN FIX THE 9 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ---------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

manual review:

1. permissions_by_term_schema(): descriptions for the schema fields are missing, foreign keys are missing. See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
2. permissions_by_term_help(): do not split up translatable strings like that, they will make no sense for translators. Code lines such as t() calls are allowed to exceed 80 characters. Same in permissions_by_term_permission() and in many other places.
3. permissions_by_term_form_taxonomy_form_term_alter(): why do you call user_load() here, just join to the user table to get the name? Otherwise this could get very slow if there are many users for this tid.

Will continue later.

I'm using the latest dev version of Coder sniffer, which might not be updated on the online pareview.sh site yet.

manual review continued:

1. permissions_by_term_schema(): no, I did not mean adding comments in the code but using the 'description' key of the schema API. And "Specifications for tabe 'permissions_by_term_user'" is not a useful comment, you shuld document why you are doing something. Possible example: 'description' => 'Stores suer accounts that are allowed to view/edit a particular taxonomy term.'
2. permissions_by_term_form_taxonomy_form_term_alter(): now instead of user_load() you use one db_select() query per user, which is not really an improvement. I meant doing something like db_query("SELECT u.name FROM {permissions_by_term_user} p JOIN {user} u ON p.uid = u.uid WHERE p.tid = :tid", ...);. That way you have only one query and get all the information you need, instead of 21 SQL queries when you have 20 user permissions for one term ID.
3. permissions_by_term_form_taxonomy_form_term_alter(): the foreach() to collect $allowed_roles is not necessary, just use ->fetchCol() instead. See https://www.drupal.org/node/1251174
4. permissions_by_term_validate(): the form element 'search_user' does not exist, did you mean 'access][user'? See https://api.drupal.org/api/drupal/includes!form.inc/function/form_set_er...
5. permissions_by_term_form_taxonomy_form_term_alter(): Do not use db_select() for simple static queries, use db_query() instead. See https://www.drupal.org/node/310075
6. permissions_by_term_validate(): the user_load_multiple() looks like overkill here. Just use a simple db_query() to check if they exist?
7. permissions_by_term_submit(): same here, you just need to query the uid from the user names, no need for loading all fields and all entities with user_load_multiple()? Same problem with 20 DB queries for 20 users instead of one here.
8. permissions_by_term_submit(): insert queries can take multiple value calls and then execute them all at once, see multi-insert on https://www.drupal.org/node/310079
9. permissions_by_term_views_post_execute(): hm, where do you check that this is only executed for the taxonomy term views you want to change? Looks like this will alter every single view in the system? And instead of using this post_execute hack why don't you replace your taxonomy views with your own view where you use a join to the permissions_by_term_user table. That would just require a small Views integration hook to expose your table to Views. This looks like a serious performance penalty here since you are invoking node_load() for every single result row.
10. permissions_by_term_node_access(): why don't you check $op here? Shouldn't this only be invoked when $op == 'view'?
11. permissions_by_term_node_access(): do not break node access checking here with drupal_access_denied(). You should return the appropriate value as documented on https://api.drupal.org/api/drupal/modules!node!node.api.php/function/hoo...
12. permissions_by_term_autocomplete_multiple(): doc block is wrong with the "Implements ...", this is not a hook. See https://www.drupal.org/coding-standards/docs#menu-callback
13. permissions_by_term_form_taxonomy_form_term_alter(): instead of your own DB table I would probably use an entityreference field https://www.drupal.org/project/entityreference on terms.

So those are quite a few issues and the permissions_by_term_views_post_execute() is a blocker right now, since this will blow up any site that uses a View which is completely unrelated to nodes and terms. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Hi,
After a look I would like to suggest that you must work on the below mentioned points

Install file
1. Need to set one column as primary key for permissions_by_term_user and permissions_by_term_role tables in the schema.

Info file
1. File format must be changed. See how to write info files in drupal 7 in URL: https://www.drupal.org/node/542202

Module file
1. Line lenght must not exceed 80 characters.
2. Remove unwanted new lines and spaces.
3. Remove unused variables.
4. Move forms to inc files so as to make module file clean.
5. Use global keywords at start of functions using it.
6. Use db_select, db_insert instead of db_query all the times.
7. For fetching rows in associative array use fetchAssoc() instead of fetchAll(PDO::FETCH_ASSOC).
8. use join function instead of writing plain join sql queries.
9. Remove too many return statements in your functions.

Thanks

Some corrections:

1. db_query() is fine and preferred for simple static queries. See https://www.drupal.org/node/310075
2. You can use as many return statements as you like.
3. Line length for code (not comments) can exceed 80 characters, there are many exceptions to this general rule.

@polaki_viswanath: that points don't look like application blockers, anything else that you found or should this now be RTBC?

manual review:

1. "updates after code review" is not a good git commit message, you should describe what you actually changed. See https://www.drupal.org/node/52287
2. permissions_by_term_form_taxonomy_form_term_alter(): the result of db_select('permissions_by_term_user') is never used, should it be removed?
3. permissions_by_term_form_taxonomy_form_term_alter(): the foreach() to collect $allowed_roles is not necessary, just use ->fetchCol() instead. See https://www.drupal.org/node/1251174
4. permissions_by_term_views_post_execute(): I still think it is risky to execute this for every single view on the site that has nodes. This can mess up a lot of admin views for example. Please change the comment from "Hides nodes in a view." to "Hides nodes in every view on the site.".
5. permissions_by_term_node_access(): I don't understand the define() here, why do you need that constant? You should return the appropriate value: "NODE_ACCESS_ALLOW: if the operation is to be allowed. NODE_ACCESS_DENY: if the operation is to be denied. NODE_ACCESS_IGNORE: to not affect this operation at all." from https://api.drupal.org/api/drupal/modules!node!node.api.php/function/hoo...

So the last point is a blocker right now, you need to know how Drupal security API functions work.

manual review:

1. permissions_by_term_node_access(): why do you need to call node_load() here? $node is already the node object? Please add a comment.
2. permissions_by_term_allowed(): You should use "//" style comments in function bodies instead fo "/* ... */. See https://www.drupal.org/coding-standards/docs#inline

But otherwise looks RTBC to me now.

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

Just did a quick look into this:

* I really (really!) would prefer creating a patch for Taxonomy Term Permissions to add the missing features (it seems you copied large parts of the module and simply renamed some stuff?). Patches have a stronger impact than a simple question about a feature ...
* User A creates a node associated to Term 1. User B has no access to Term 1 but edits the node. What will happen?

Thank you for addressing the concerns about Taxonomy Term Permissions, but how does this module compare to Taxonomy Access Control or TAxonomy Access Control Lite?

"Restricts users from accessing the nodes related to specific taxonomy terms per roles and users." vs "Access control for user roles based on taxonomy categories"

I guess since your module is a fork from an existing one there's probably a good reason, but this info will also help users choose the right module. Can you describe the reason for the fork or why a user should use your module instead?

So back to RTBC I guess? Could you add the differences to the modules kscheirer mentioned to the project page?

@jepSter, you're right, the reason for forking is given on the project page, I just missed it!

I'll do a final review of this tomorrow.

Automated Review

Git errors:

• The last commit message is just one word, you should provide a meaningful short summary what you changed. See https://www.drupal.org/node/52287

Review of the 7.x-1.x branch (commit cedd148):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

Manual Review

@klausi #35-1,2 are not addressed, but those are not blocking issues.

Avoid translated breaking strings (eg, hook_help) across lines. Code can be longer that 80 cols, comments shouldn't. See https://www.drupal.org/node/322729

Can permissions_by_term_form_alter() use one of the more specific forms, eg hook_form_FORM_ID_alter()? If so, it should. If not, you should comment why.

(+) Agree that permissions_by_term_views_post_execute() needs some rethinking. I can see this messing up a lot of admin views.

Personally, I prefer to return NODE_ACCESS_IGNORE at the bottom of hook_node_access to make it more obvious to someone reading the code what is going on.

The comment on permissions_by_term_node_access() doesn't match what the code is doing, since no redirection is happening.

Not seeing any blocking issues.

Thanks for your contribution, jepSter!

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.