It would be better practice to use constants for the grant values and default IDs:

define('TAXONOMY_ACCESS_GLOBAL_DEFAULT_VID', 0);
define('TAXONOMY_ACCESS_VOCABULARY_DEFAULT_TID', 0);
define('TAXONOMY_ACCESS_NODE_ALLOW', 1);
define('TAXONOMY_ACCESS_NODE_IGNORE', 0);
define('TAXONOMY_ACCESS_NODE_DENY', 2);
define('TAXONOMY_ACCESS_TERM_ALLOW', 1);
define('TAXONOMY_ACCESS_TERM_DENY', 0);
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue tags: +Novice

So to patch this issue, you'd have to essentially search through the whole module codebase, looking for 0, 1, and 2, and see in each case which constant is appropriate (if any). Not difficult, but not a quick fix, either.

Be sure to rebase from origin frequently if you work on this issue. The module is under active development currently, and a patch for this issue will touch a lot of the codebase.

sammyd56’s picture

Assigned: Unassigned » sammyd56

Started working on this... should have a patch ready soon.

sammyd56’s picture

Status: Active » Needs review
FileSize
13.17 KB

Let's run this by testbot...

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, use_constants-1246982-3.patch, failed testing.

sammyd56’s picture

Status: Needs work » Needs review

#3: use_constants-1246982-3.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch, use_constants-1246982-3.patch, failed testing.

sammyd56’s picture

Status: Needs work » Needs review
FileSize
0 bytes

Not sure what's going on here, let's try again...

EDIT: Oops, empty patch!

sammyd56’s picture

Let's try again...

Status: Needs review » Needs work

The last submitted patch, use_constants-1246982-8.patch, failed testing.

sammyd56’s picture

Status: Needs work » Needs review
FileSize
20.87 KB

Status: Needs review » Needs work

The last submitted patch, use_constants-1246982-10.patch, failed testing.

sammyd56’s picture

sammyd56’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, use_constants-1246982-12.patch, failed testing.

sammyd56’s picture

Status: Needs work » Needs review
FileSize
25.04 KB

Apparently today is Drupal fail day for me... :P

Status: Needs review » Needs work

The last submitted patch, use_constants-1246982-15.patch, failed testing.

sammyd56’s picture

Status: Needs work » Needs review
FileSize
25.04 KB

Status: Needs review » Needs work

The last submitted patch, use_constants-1246982-17.patch, failed testing.

sammyd56’s picture

Status: Needs work » Needs review
FileSize
25.1 KB

Last attempt for today...

Status: Needs review » Needs work

The last submitted patch, use_constants-1246982-19.patch, failed testing.

xjm’s picture

The test failures here turn out to be related to a recent core change, so we might want to wait on that being fixed.

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Novice

Alright, the branch test issue is fixed now, so requeuing.

#19: use_constants-1246982-19.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch, use_constants-1246982-19.patch, failed testing.

xjm’s picture

And now it needs a reroll apparently. Silly testbot.

sammyd56’s picture

Status: Needs work » Needs review
FileSize
28.54 KB

Here's a re-roll with my latest changes, let's see what the testbot thinks of this.

sammyd56’s picture

I live in hope that, one day, one of my patches in this issue will turn green =)

xjm’s picture

Haha. Branch tests were stuck again... so I requeued them again...

Edit: Yay.

sammyd56’s picture

Hurray!

When I get a chance I'm going to add a "dummy" patch highlighting all the remaining 0, 1, 2 that I have left unchanged, to make it easy to see if there are any omissions.

Does that sound like a good idea?

xjm’s picture

Ah, that does sound useful. Thanks!

xjm’s picture

Priority: Minor » Normal
Status: Needs review » Needs work

Awesome job on this. It's actually a pretty complicated task, but it will improve our code readability significantly to not have magic zeroes and ones and twos everywhere that mean different things. (I'm actually bumping the priority for that reason.)

I did notice that this patch also seems to include some of #1360506: Replace /*******/ comment header blocks with proper doxygen groups. It might be possible to separate that back out by applying this patch, then reversing the patch from #1360506: Replace /*******/ comment header blocks with proper doxygen groups with patch -p1 -R < patchname.patch., and finally rolling a new version of this patch.

Here's my first few observations:

  1. After thinking about it, I think we can just use TAXONOMY_ACCESS_GLOBAL_DEFAULT and TAXONOMY_ACCESS_VOCABULARY_DEFAULT. They're not really vocabulary or term IDs, plus shorter constant names will of course be easier to read. I'd love a second opinion, though.
  2. +++ b/taxonomy_access.installundefined
    @@ -21,9 +21,38 @@ function taxonomy_access_last_removed() {
    +  db_query(
    +    'INSERT INTO {taxonomy_access_default}
    +    (vid, rid, grant_view, grant_update, grant_delete, grant_create, grant_list)
    +    VALUES(
    +    :vid,
    +    1,
    +    :allow,
    +    :deny,
    +    :deny,
    +    :allow,
    +    :allow)',
    +    array(
    +      ':vid' => TAXONOMY_ACCESS_GLOBAL_DEFAULT_VID,
    +      ':allow' => TAXONOMY_ACCESS_TERM_ALLOW,
    +      ':deny' => TAXONOMY_ACCESS_TERM_DENY)
    +  );
    +  db_query(
    +    'INSERT INTO {taxonomy_access_default}
    +    (vid, rid, grant_view, grant_update, grant_delete, grant_create, grant_list)
    +    VALUES(
    +    :vid,
    +    2,
    +    :allow,
    +    :deny,
    +    :deny,
    +    :allow,
    +    :allow)',
    +    array(
    +      ':vid' => TAXONOMY_ACCESS_GLOBAL_DEFAULT_VID,
    +      ':allow' => TAXONOMY_ACCESS_TERM_ALLOW,
    +      ':deny' => TAXONOMY_ACCESS_TERM_DENY)
    +  );
    

    These queries don't look quite right. From left to right, the values in the first query should be:

    • TAXONOMY_ACCESS_GLOBAL_DEFAULT_VID
    • DRUPAL_ANONYMOUS_RID
    • TAXONOMY_ACCESS_NODE_ALLOW
    • TAXONOMY_ACCESS_NODE_IGNORE
    • TAXONOMY_ACCESS_NODE_IGNORE
    • TAXONOMY_ACCESS_TERM_ALLOW
    • TAXONOMY_ACCESS_TERM_ALLOW

    The second query is the same but with DRUPAL_AUTHENTICATED_RID instead. So, I think we need the following placeholders:

    • :vid
    • :rid
    • :node_allow
    • :ignore
    • :term_allow

    Also, it's a little bit hard to scan as is. I think it will probably fit to do:

    INSERT INTO {table}
    (field, field, field...)
    VALUES
    (:vid, :rid, :node_allow, :ignore, :ignore, :term_allow, :term_allow)
    

    If it doesn't quite fit then we can wrap the field list to a second line and indent by two spaces, e.g.,

    (foo, bar, baz, bat
       fish, frog, cow)
    

    This last part isn't any particular standard, just my preference. :)

  3. +++ b/taxonomy_access.moduleundefined
    @@ -6,6 +6,18 @@
     /**
    + * Define constants
    + */
    +
    +define('TAXONOMY_ACCESS_GLOBAL_DEFAULT_VID', 0);
    +define('TAXONOMY_ACCESS_VOCABULARY_DEFAULT_TID', 0);
    +define('TAXONOMY_ACCESS_NODE_ALLOW', 1);
    +define('TAXONOMY_ACCESS_NODE_IGNORE', 0);
    +define('TAXONOMY_ACCESS_NODE_DENY', 2);
    +define('TAXONOMY_ACCESS_TERM_ALLOW', 1);
    +define('TAXONOMY_ACCESS_TERM_DENY', 0);
    

    For this part, each constant should have its own docblock that explains what each is. E.g.:

    /**
     * 'Allow' grant value for nodes.
     */
    define('TAXONOMY_ACCESS_NODE_ALLOW', 1);
    
    /**
     * 'Ignore' grant value for nodes.
     */
    define('TAXONOMY_ACCESS_NODE_IGNORE', 0);
    

I'll do a more thorough review once the mixed-in stuff from the other issue is removed (so that it's a bit easier for me to read). Thanks!

sammyd56’s picture

What a mess that last patch was! I guess you're regretting tagging this one as 'Novice' now ;)

This patch should be much improved, and also easier to review (patch -p1 -R < patchname.patch worked a treat, thanks!)

xjm’s picture

Status: Needs work » Needs review

Passing to the bot.

sammyd56’s picture

Fixed a couple of omissions. Looking much more committable now I think :)

sammyd56’s picture

Right, here's the final patch. I've also attached a dummy patch pointing out the 37 solitary numbers that survived the cut. Phew, time for a beer =)

xjm’s picture

The patch showing the omissions is very helpful; thank you. Enjoy that beer. :)

I've read through it and found a couple additional ones that should be the constants:

  1. +++ b/taxonomy_access.installundefined
    @@ -239,7 +239,7 @@ function taxonomy_access_enable() {
    -      WHERE ta.tid <> 0 AND td.tid IS NULL"
    +      WHERE ta.tid <> 0 AND td.tid IS NULL" // ***** UNCHANGED *****
    

    This zero is the vocabulary default.

  2. +++ b/taxonomy_access.installundefined
    @@ -254,7 +254,7 @@ function taxonomy_access_enable() {
    -      WHERE tad.vid <> 0 AND tv.vid IS NULL"
    +      WHERE tad.vid <> 0 AND tv.vid IS NULL" // ***** UNCHANGED *****
    

    Zero is the global default.

  3. +++ b/taxonomy_access.moduleundefined
    @@ -1235,7 +1235,7 @@ function _taxonomy_access_grant_query(array $grants, $default = FALSE) {
    -    'tadg.vid = 0'
    +    'tadg.vid = 0' // ***** UNCHANGED *****
    

    Zero is the global default.

  4. +++ b/taxonomy_access.testundefined
    @@ -267,7 +267,7 @@ class TaxonomyAccessTestCase extends DrupalWebTestCase {
    @@ -649,7 +649,7 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
    
    @@ -649,7 +649,7 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
         // Use the admin form to give anonymous view allow for the v1 default.
         $this->drupalGet(TAXONOMY_ACCESS_CONFIG . '/role/' . DRUPAL_ANONYMOUS_RID . '/edit');
         $edit = array();
    -    $this->addFormRow($edit, $this->vocabs['v1']->vid, 0, TAXONOMY_ACCESS_NODE_ALLOW);
    +    $this->addFormRow($edit, $this->vocabs['v1']->vid, 0, TAXONOMY_ACCESS_NODE_ALLOW); // ***** UNCHANGED *****
         $this->drupalPost(NULL, $edit, 'Add');
     
         // Log out.
    @@ -682,7 +682,7 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
    
    @@ -682,7 +682,7 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
         // Use the admin form to give anonymous view deny for the v2 default.
         $this->drupalGet(TAXONOMY_ACCESS_CONFIG . '/role/' . DRUPAL_ANONYMOUS_RID . '/edit');
         $edit = array();
    -    $this->addFormRow($edit, $this->vocabs['v2']->vid, 0, TAXONOMY_ACCESS_NODE_DENY);
    +    $this->addFormRow($edit, $this->vocabs['v2']->vid, 0, TAXONOMY_ACCESS_NODE_DENY); // ***** UNCHANGED *****
         $this->drupalPost(NULL, $edit, 'Add');
     
         // Log out.
    @@ -716,8 +716,8 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
    
    @@ -716,8 +716,8 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
         // Use the form to change the configuration: Allow for v2; Deny for v1.
         $this->drupalGet(TAXONOMY_ACCESS_CONFIG . '/role/' . DRUPAL_ANONYMOUS_RID . '/edit');
         $edit = array();
    -    $this->configureFormRow($edit, $this->vocabs['v2']->vid, 0, TAXONOMY_ACCESS_NODE_ALLOW);
    -    $this->configureFormRow($edit, $this->vocabs['v1']->vid, 0, TAXONOMY_ACCESS_NODE_DENY);
    +    $this->configureFormRow($edit, $this->vocabs['v2']->vid, 0, TAXONOMY_ACCESS_NODE_ALLOW); // ***** UNCHANGED *****
    +    $this->configureFormRow($edit, $this->vocabs['v1']->vid, 0, TAXONOMY_ACCESS_NODE_DENY); // ***** UNCHANGED *****
         $this->drupalPost(NULL, $edit, 'Save all');
    

    These zeroes are all the vocabulary default.

I'll check the patch itself next to make sure there aren't any false positives in there.

xjm’s picture

Status: Needs review » Needs work

Wow, reviewing the patch makes me see just how insanely confusing this is currently. Did I mention it is awesome that you are working on this? :)

  1. +++ b/taxonomy_access.moduleundefined
    @@ -922,15 +957,15 @@ function taxonomy_access_delete_role_grants($rid, $update_nodes = TRUE) {
    +        case TAXONOMY_ACCESS_NODE_DENY:
    +          if ($role_gd[$op] < TAXONOMY_ACCESS_NODE_DENY) {
    

    I think we probably should change this to:

    if (($role_gd[$op] == TAXONOMY_ACCESS_NODE_IGNORE) || ($role_gd[$op] == TAXONOMY_ACCESS_NODE_ALLOW))

  2. +++ b/taxonomy_access.moduleundefined
    @@ -1343,17 +1378,16 @@ function taxonomy_access_global_defaults($reset = FALSE) {
    -  // Ignore => 0, Allow => 1, Deny => 2 ('10' in binary).
    -  // Only a value of 1 is considered an 'Allow';
    -  // with an 'Allow' and no 'Deny', the value from the BIT_OR will be 1.
    -  // If a 'Deny' is present, the value will then be 3 ('11' in binary).
    

    I think we do need to still leave this comment, because it explains how the bitwise logic works (which is important for understanding the query above). However, we can change it to (e.g.)

    TAXONOMY_ACCESS_NODE_IGNORE => 0.

  3. +++ b/taxonomy_access.moduleundefined
    @@ -1343,17 +1378,16 @@ function taxonomy_access_global_defaults($reset = FALSE) {
       return array(
         'realm' => 'taxonomy_access_role',
         'gid' => $record->rid,
    -    'grant_view' => ($record->grant_view == 1) ? 1 : 0,
    -    'grant_update' => ($record->grant_update == 1) ? 1 : 0,
    -    'grant_delete' => ($record->grant_delete == 1) ? 1 : 0,
    -    'priority' => 0,
    +    'grant_view' => ($record->grant_view == TAXONOMY_ACCESS_NODE_ALLOW)
    +      ? TAXONOMY_ACCESS_NODE_ALLOW : TAXONOMY_ACCESS_NODE_IGNORE,
    +    'grant_update' => ($record->grant_update == TAXONOMY_ACCESS_NODE_ALLOW)
    +      ? TAXONOMY_ACCESS_NODE_ALLOW : TAXONOMY_ACCESS_NODE_IGNORE,
    +    'grant_delete' => ($record->grant_delete == TAXONOMY_ACCESS_NODE_ALLOW)
    +      ? TAXONOMY_ACCESS_NODE_ALLOW : TAXONOMY_ACCESS_NODE_IGNORE,
    +    'priority' => TAXONOMY_ACCESS_NODE_IGNORE,
       );
     }
    

    I think we can leave this hunk unchanged, because:

    1. Each $record->grant_foo is the result of the BIT_OR rather than an actual grant from the table record.
    2. The 1 and 0 in the result parts of the ternaries aren't actually TAC values at all. They're node access grant values. I wonder if core has constants for those we could use instead?
  4. +++ b/taxonomy_access.testundefined
    @@ -292,21 +293,28 @@ class TaxonomyAccessTestCase extends DrupalWebTestCase {
        * @param array &$edit
        *   The form data to post.
        * @param int $vid
    -   *   (optional) The vocabulary ID.  Defaults to 0 (global default).
    -   * @param $int tid
    -   *   (optional) The term ID.  Defaults to 0 (vocabulary default).
    +   *   (optional) The vocabulary ID.  Defaults to global default.
    +   * @param int $tid
    +   *   (optional) The term ID.  Defaults to vocabulary default.
        * @param int $view
    -   *   (optional) The view grant value (1, 0, or 2). Defaults to 0 (ignore).
    +   *   (optional) The view grant value. Defaults to ignore.
        * @param int $update
    -   *   (optional) The update grant value (1, 0, or 2). Defaults to 0 (ignore).
    +   *   (optional) The update grant value. Defaults to ignore.
        * @param int $delete
    -   *   (optional) The delete grant value (1, 0, or 2). Defaults to 0 (ignore).
    +   *   (optional) The delete grant value. Defaults to ignore.
        * @param int $create
    -   *   (optional) The create grant value (0 or 1). Defaults to 0 (unchecked).
    +   *   (optional) The create grant value. Defaults to deny (unchecked).
        * @param int $list
    -   *   (optional) The list grant value  (0 or 1). Defaults to 0 (unchecked).
    +   *   (optional) The list grant value. Defaults to deny (unchecked).
    
    @@ -322,21 +330,28 @@ class TaxonomyAccessTestCase extends DrupalWebTestCase {
        * @param array &$edit
        *   The form data to post.
        * @param int $vid
    -   *   (optional) The vocabulary ID.  Defaults to 0 (global default).
    +   *   (optional) The vocabulary ID.  Defaults to global default.
        * @param int $tid
    -   *   (optional) The term ID.  Defaults to 0 (vocabulary default).
    +   *   (optional) The term ID.  Defaults to vocabulary default.
        * @param int $view
    -   *   (optional) The view grant value (1, 0, or 2). Defaults to 0 (ignore).
    +   *   (optional) The view grant value. Defaults to ignore.
        * @param int $update
    -   *   (optional) The update grant value (1, 0, or 2). Defaults to 0 (ignore).
    +   *   (optional) The update grant value. Defaults to ignore.
        * @param int $delete
    -   *   (optional) The delete grant value (1, 0, or 2). Defaults to 0 (ignore).
    +   *   (optional) The delete grant value. Defaults to ignore.
        * @param int $create
    -   *   (optional) The create grant value (0 or 1). Defaults to 0 (unchecked).
    +   *   (optional) The create grant value. Defaults to deny (unchecked).
        * @param int $list
    -   *   (optional) The list grant value  (0 or 1). Defaults to 0 (unchecked).
    +   *   (optional) The list grant value. Defaults to deny (unchecked).
    

    In the parameter declarations, we can write out the defaults explicitly, e.g."Defaults to TAXONOMY_ACCESS_GLOBAL_DEFAULT." It's okay to wrap if necessary, with the subsequent line indented the same amount.

  5. +++ b/taxonomy_access.testundefined
    @@ -292,21 +293,28 @@ class TaxonomyAccessTestCase extends DrupalWebTestCase {
    -  function addFormRow(&$edit,  $vid = 0, $tid = 0, $view = 0, $update = 0, $delete = 0, $create = 0, $list = 0) {
    +  function addFormRow(&$edit,
    +  $vid = TAXONOMY_ACCESS_GLOBAL_DEFAULT,
    +  $tid = TAXONOMY_ACCESS_VOCABULARY_DEFAULT,
    +  $view = TAXONOMY_ACCESS_NODE_IGNORE,
    +  $update = TAXONOMY_ACCESS_NODE_IGNORE,
    +  $delete = TAXONOMY_ACCESS_NODE_IGNORE,
    +  $create = TAXONOMY_ACCESS_TERM_DENY,
    +  $list = TAXONOMY_ACCESS_TERM_DENY) {
    
    @@ -322,21 +330,28 @@ class TaxonomyAccessTestCase extends DrupalWebTestCase {
    -  function configureFormRow(&$edit,  $vid = 0, $tid = 0, $view = 0, $update = 0, $delete = 0, $create = 0, $list = 0) {
    +  function configureFormRow(&$edit,
    +  $vid = TAXONOMY_ACCESS_GLOBAL_DEFAULT,
    +  $tid = TAXONOMY_ACCESS_VOCABULARY_DEFAULT,
    +  $view = TAXONOMY_ACCESS_NODE_IGNORE,
    +  $update = TAXONOMY_ACCESS_NODE_IGNORE,
    +  $delete = TAXONOMY_ACCESS_NODE_IGNORE,
    +  $create = TAXONOMY_ACCESS_TERM_DENY,
    +  $list = TAXONOMY_ACCESS_TERM_DENY) {
    

    For this, we should put it all on one line. (The way you have it is easier to read, but unfortunately the Drupal coding standards say function declarations should be all on one line.) :)

  6. +++ b/taxonomy_access.testundefined
    @@ -542,7 +557,8 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
    -    $this->configureFormRow($edit, 0, 0, 1);
    +    $this->configureFormRow($edit, TAXONOMY_ACCESS_NODE_IGNORE,
    +      TAXONOMY_ACCESS_NODE_IGNORE, TAXONOMY_ACCESS_NODE_ALLOW);
    
    @@ -583,7 +599,8 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
    -    $this->configureFormRow($edit, 0, 0, 2);
    +    $this->configureFormRow($edit, TAXONOMY_ACCESS_NODE_IGNORE,
    +      TAXONOMY_ACCESS_NODE_IGNORE, TAXONOMY_ACCESS_NODE_DENY);
    
    @@ -962,7 +979,8 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
    -    $this->configureFormRow($edit, 0, 0, 1);
    +    $this->configureFormRow($edit, TAXONOMY_ACCESS_NODE_IGNORE,
    +      TAXONOMY_ACCESS_NODE_IGNORE, TAXONOMY_ACCESS_NODE_ALLOW);
    

    I think these are actually (in order) the global default, vocab default, and node allow.

  7. +++ b/taxonomy_access.testundefined
    @@ -632,7 +649,7 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
    -    $this->addFormRow($edit, $this->vocabs['v1']->vid, 0, 1);
    +    $this->addFormRow($edit, $this->vocabs['v1']->vid, 0, TAXONOMY_ACCESS_NODE_ALLOW);
    
    @@ -665,7 +682,7 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
    -    $this->addFormRow($edit, $this->vocabs['v2']->vid, 0, 2);
    +    $this->addFormRow($edit, $this->vocabs['v2']->vid, 0, TAXONOMY_ACCESS_NODE_DENY);
    
    @@ -699,8 +716,8 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
    -    $this->configureFormRow($edit, $this->vocabs['v2']->vid, 0, 1);
    -    $this->configureFormRow($edit, $this->vocabs['v1']->vid, 0, 2);
    +    $this->configureFormRow($edit, $this->vocabs['v2']->vid, 0, TAXONOMY_ACCESS_NODE_ALLOW);
    +    $this->configureFormRow($edit, $this->vocabs['v1']->vid, 0, TAXONOMY_ACCESS_NODE_DENY);
    

    The zeroes here are the vocabulary default.

Phew!

xjm’s picture

Oh! Before I forget. It'd be helpful when rerolling after a review to create an interdiff against the previously reviewed patch. See:
http://xjm.drupalgardens.com/blog/interdiffs

This is useful in general, by the way, especially when you're touching up someone else's patch. I thought of this when I glanced at one of your core patch revisions the other day, but something distracted me and I forgot to follow up. :)

sammyd56’s picture

Status: Needs work » Needs review
FileSize
30.23 KB

Perfect, I wasn't sure about those lines, thanks for clearing that up. Here's the updated patch.

edit: whoops, missed a couple of posts above! Will have some more Drupal play-time tomorrow so will see if I can iron out the other issues...

The last submitted patch, use_constants-1246982-36.patch, failed testing.

sammyd56’s picture

Updated as per #35 and #36. Also attached an interdiff between this patch the last reviewed patch (34). I couldn't find a constant that we could use for the node access grants so I have left them at 0 and 1. Also, I forgot to say earlier but I agree 100% with the shorter constant names for the defaults (without TID and VID).

xjm’s picture

Interesting. You're right--there are core node access constants, but not ones we can use:
http://api.drupal.org/api/drupal/modules--node--node.module/constant/NOD...
http://api.drupal.org/api/drupal/modules--node--node.module/constant/NOD...

These have string values rather than the expected 0 and 1. It appears the expected values from hook_node_access() and hook_node_access_records() differ... perhaps I'll look for a core issue for that.

All the changes in the interdiff look correct. I'll review the patch one more time maybe this afternoon and hopefully we can get this in!

xjm’s picture

Status: Needs review » Fixed

Finally had a chance to do one more review. Fixed in -dev:
http://drupal.org/commitlog/commit/364/0ea1c8fdc1a0e9822aa148820a0e016bf...

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.