As first, thank you for the 1.0 version. It is always nice to see that the module is out of -dev status, what brings a bit more confidence to the site admin.

Checking the code (with Coder) however shows that there are quite some (mainly minor, but still...) details to be revised before we could really call this version a final one. Just my thought...

CommentFileSizeAuthor
#9 cleanup.patch5.35 KBxjm
#3 taxonomy_access-code_style_cleanup-20091005.patch6.43 KBdman
#1 taxonomy_access-PHP_E_ALL_cleanup.patch889 bytesdman

Comments

dman’s picture

dman commented
StatusFileSize
new taxonomy_access-PHP_E_ALL_cleanup.patch889 bytes

I got a php E_NOTICE warning on node edit pages for nodes that are NOT using taxonomy_access tags.

notice: Undefined index: 146 in /.../taxonomy_access/taxonomy_access.module on line 380.

In the interests of cleanup, declaring the array before putting stuff into it is good practice:

@@ -367,6 +367,7 @@ function taxonomy_access_preserve_terms(
   // any other API call, the taxonomy API functions all use 
   // db_rewrite_sql() so we must query the database tables directly)
   $result = db_query('SELECT tid FROM {term_node} WHERE vid = %d', $node->vid);
+  $tids[$nid] = array();
   while ($row = db_fetch_array($result)) {
     // only include those terms the current user does not have access to
     if (!isset($user_terms[$row['tid']])) {

there may be more, but that's the one i've seen so far.

dman’s picture

dman commented
Status: Active » Needs review

FWIW, here's a full code-style cleanup while I was there. Just whitepace in 20 places, plus a SQL argument tweak.

It leaves one warning outstanding in the install file that I don't understand.

Line 70: Use update_sql() instead of db_query() in hook_update_N()

      db_query('INSERT INTO {term_access_defaults} (vid, rid, grant_view, grant_update, grant_delete, grant_create, grant_list) VALUES(0, %d, %d, %d, %d, %d, %d)', $row);

yet update_sql() does not support arg substitution, so I'm not sure if I should mess with that. It's easy enough, I'm just not sure it's right to do so.

dman’s picture

dman commented
Issue tags: +code cleanup
StatusFileSize
new taxonomy_access-code_style_cleanup-20091005.patch6.43 KB

Um, attachment :-)

luti’s picture

luti commented

Coder is still not happy...

I've found the following is needed to make a complete cleanup for at least taxonomy_access.module and taxonomy_access_admin.inc (taxonomy_access.install remains unfixed):

--- /taxonomy_access/taxonomy_access.module.OK1		2009-10-05 09:55:49.000000000 +0200
+++ /taxonomy_access/taxonomy_access.module.OK2		2009-10-05 10:04:42.000000000 +0200
@@ -178,8 +178,8 @@ function taxonomy_access_menu() {
   $items = array();
 
   $items['admin/user/taxonomy_access'] = array(
-      'title' => t('Taxonomy access permissions'),
-      'description' => t('Taxonomy-based access control for content'),
+      'title' => 'Taxonomy access permissions',
+      'description' => 'Taxonomy-based access control for content',
       'page callback' => 'taxonomy_access_admin',
       'access arguments' => array('administer permissions'),
     );
--- /taxonomy_access/taxonomy_access_admin.inc.OK1	2009-10-05 10:00:39.000000000 +0200
+++ /taxonomy_access/taxonomy_access_admin.inc.OK2	2009-10-05 10:06:51.000000000 +0200
@@ -30,7 +30,7 @@ function taxonomy_access_admin($op = NUL
       return drupal_get_form('taxonomy_access_admin_delete_role', $rid);
     }
   }
-  else if (!isset($op) AND !isset($rid)) {
+  elseif (!isset($op) AND !isset($rid)) {
     return theme_taxonomy_access_admin();
   }
   //TODO something odd happens here
@@ -80,7 +80,7 @@ function taxonomy_access_admin_delete_ro
       // issue #167977 - klance
       //node_access_rebuild();
       _taxonomy_access_node_access_update($affected_nodes);            
-      drupal_set_message("All term access rules deleted for role $rid.");
+      drupal_set_message(t("All term access rules deleted for role $rid."));
       drupal_goto('admin/user/taxonomy_access');
     }
     else {
@@ -472,7 +472,7 @@ function taxonomy_access_get_grants($rid
 **/
 function taxonomy_access_get_default_grants($rid) {
   if (!is_numeric($rid)) {
-    return false;
+    return FALSE;
   }
   $result = db_query("SELECT * FROM {term_access_defaults} WHERE rid=%d", $rid);
   $grants = array();
dman’s picture

dman commented

Hm, I was on coder DRUPAL-6-1 stable. Looks like there's some new conditions been added in dev.

keve’s picture

keve commented
Status: Needs review » Patch (to be ported)

Thanks for the cleanup.

Commited with a small change:

@@ -80,7 +80,7 @@ function taxonomy_access_admin_delete_ro
       // issue #167977 - klance
       //node_access_rebuild();
       _taxonomy_access_node_access_update($affected_nodes);            
-      drupal_set_message("All term access rules deleted for role $rid.");
+      drupal_set_message(t('All term access rules deleted for role %rid.', array('%rid' => $rid)));
       drupal_goto('admin/user/taxonomy_access');
     }
     else {
keve’s picture

keve commented

To commemt #2:
Solution would be: create your own update_sql function
http://api.drupal.org/api/function/update_sql/6#comment-412

keve’s picture

keve commented
Status: Patch (to be ported) » Active

Ported until comment #6.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Active
StatusFileSize
new cleanup.patch5.35 KB

Re #2: In this situation, it should be sufficient to typecast the grants as integers for use in the query string, since there is no other data type nor user-submitted data involved.

Patch attached using that method, plus some code cleanup for recently committed features. Patch is against CVS and passes coder on "Normal."

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Active » Needs review
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 6.x-1.0 » 6.x-1.x-dev
Status: Active » Needs review
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Fixed

Committed to 6.x-1.x-dev.

Status: Fixed » Closed (fixed)
Issue tags: -code cleanup

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.