Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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);
Comment | File | Size | Author |
---|---|---|---|
#40 | use_constants-1246982-40.patch | 29.95 KB | sammyd56 |
#40 | interdiff.txt | 9.16 KB | sammyd56 |
#38 | use_constants-1246982-36.patch | 30.23 KB | sammyd56 |
#34 | use_constants-1246982-34.patch | 29.08 KB | sammyd56 |
#34 | use_constants-1246982-34-OMISSIONS-HIGHLIGHTED-D7.patch | 14.55 KB | sammyd56 |
Comments
Comment #0.0
xjmUpdated issue summary.
Comment #0.1
xjmUpdated issue summary.
Comment #0.2
xjmUpdated issue summary.
Comment #1
xjmSo 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.
Comment #2
sammyd56 CreditAttribution: sammyd56 commentedStarted working on this... should have a patch ready soon.
Comment #3
sammyd56 CreditAttribution: sammyd56 commentedLet's run this by testbot...
Comment #5
sammyd56 CreditAttribution: sammyd56 commented#3: use_constants-1246982-3.patch queued for re-testing.
Comment #7
sammyd56 CreditAttribution: sammyd56 commentedNot sure what's going on here, let's try again...
EDIT: Oops, empty patch!
Comment #8
sammyd56 CreditAttribution: sammyd56 commentedLet's try again...
Comment #10
sammyd56 CreditAttribution: sammyd56 commentedComment #12
sammyd56 CreditAttribution: sammyd56 commentedComment #13
sammyd56 CreditAttribution: sammyd56 commentedComment #15
sammyd56 CreditAttribution: sammyd56 commentedApparently today is Drupal fail day for me... :P
Comment #17
sammyd56 CreditAttribution: sammyd56 commentedComment #19
sammyd56 CreditAttribution: sammyd56 commentedLast attempt for today...
Comment #21
xjmThe test failures here turn out to be related to a recent core change, so we might want to wait on that being fixed.
Comment #22
xjmAlright, the branch test issue is fixed now, so requeuing.
#19: use_constants-1246982-19.patch queued for re-testing.
Comment #24
xjmAnd now it needs a reroll apparently. Silly testbot.
Comment #25
sammyd56 CreditAttribution: sammyd56 commentedHere's a re-roll with my latest changes, let's see what the testbot thinks of this.
Comment #26
sammyd56 CreditAttribution: sammyd56 commentedI live in hope that, one day, one of my patches in this issue will turn green =)
Comment #27
xjmHaha. Branch tests were stuck again... so I requeued them again...
Edit: Yay.
Comment #28
sammyd56 CreditAttribution: sammyd56 commentedHurray!
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?
Comment #29
xjmAh, that does sound useful. Thanks!
Comment #30
xjmAwesome 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:
TAXONOMY_ACCESS_GLOBAL_DEFAULT
andTAXONOMY_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.These queries don't look quite right. From left to right, the values in the first query should be:
The second query is the same but with DRUPAL_AUTHENTICATED_RID instead. So, I think we need the following placeholders:
Also, it's a little bit hard to scan as is. I think it will probably fit to do:
If it doesn't quite fit then we can wrap the field list to a second line and indent by two spaces, e.g.,
This last part isn't any particular standard, just my preference. :)
For this part, each constant should have its own docblock that explains what each is. E.g.:
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!
Comment #31
sammyd56 CreditAttribution: sammyd56 commentedWhat 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!)Comment #32
xjmPassing to the bot.
Comment #33
sammyd56 CreditAttribution: sammyd56 commentedFixed a couple of omissions. Looking much more committable now I think :)
Comment #34
sammyd56 CreditAttribution: sammyd56 commentedRight, 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 =)
Comment #35
xjmThe 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:
This zero is the vocabulary default.
Zero is the global default.
Zero is the global default.
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.
Comment #36
xjmWow, 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? :)
I think we probably should change this to:
if (($role_gd[$op] == TAXONOMY_ACCESS_NODE_IGNORE) || ($role_gd[$op] == TAXONOMY_ACCESS_NODE_ALLOW))
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
.I think we can leave this hunk unchanged, because:
$record->grant_foo
is the result of theBIT_OR
rather than an actual grant from the table record.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.
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.) :)
I think these are actually (in order) the global default, vocab default, and node allow.
The zeroes here are the vocabulary default.
Phew!
Comment #37
xjmOh! 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. :)
Comment #38
sammyd56 CreditAttribution: sammyd56 commentedPerfect, 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...
Comment #40
sammyd56 CreditAttribution: sammyd56 commentedUpdated 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).
Comment #41
xjmInteresting. 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
and1
. It appears the expected values fromhook_node_access()
andhook_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!
Comment #42
xjmFinally had a chance to do one more review. Fixed in -dev:
http://drupal.org/commitlog/commit/364/0ea1c8fdc1a0e9822aa148820a0e016bf...
Comment #43.0
(not verified) CreditAttribution: commentedUpdated issue summary.