Code cleanup
LUTi - June 11, 2009 - 08:06
| Project: | Taxonomy Access Control |
| Version: | 6.x-1.0 |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | code cleanup |
Description
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...
#1
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.
#2
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.
#3
Um, attachment :-)
#4
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();
#5
Hm, I was on coder DRUPAL-6-1 stable. Looks like there's some new conditions been added in dev.