Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
I can't comprehend orIf/andIf. And think the interface could use a little love too in methods and a lot in documentation. Finally, there has been a bit of contention because AccessResult has these methods that are not on the interface.
Proposed resolution
- Rewrite the code and doxygen both for
andIf()
andorIf()
. - Add an
isNeutral()
method. - Make
AccessResult
immutable allowing the removal of all non-interface instance methods. - Name every factory method
X
mirroring theisX
interface methods: allowed, forbidden, neutral vs isAllowed, isForbidden, isNeutral. - Document the whole shebang implementing a Kleene's weak three-valued logic. It would have saved me quite some time knowing that -- if you ever want to know what isNeutral AND isAllowed wants to be, just open Wikipedia at the truth table knowing that isNeutral maps to F and isAllowed maps to T (and you know that because it's documented!) and there you are. We spent half an hour yesterday on the phone with Wim trying to decipher this.
Remaining tasks
Review.
User interface changes
None.
API changes
orIf()/andIf()
now return new objects rather than modifying the first operand.- Added
AccessResultInterface::isNeutral()
. - Renamed
AccessResult::create()
toAccessResult::neutral()
— this does not changeAccessResultInterface
though! - Removed the
allow()
,forbid()
,resetAccess()
instance methods onAccessResult
— this does not changeAccessResultInterface
though!
Comment | File | Size | Author |
---|---|---|---|
#36 | 2340507_36.patch | 98.39 KB | chx |
#36 | interdiff.txt | 644 bytes | chx |
#35 | 2340507-35.patch | 101.34 KB | Wim Leers |
Comments
Comment #1
chx CreditAttribution: chx commentedEdit: I was wrong.
Comment #2
chx CreditAttribution: chx commentedThis is better. Renames less and thus highlights the sechole. It also shows off the new API :)
Comment #3
chx CreditAttribution: chx commentedNow this is Wim's. I am done here :)
Comment #4
chx CreditAttribution: chx commentedComment #5
chx CreditAttribution: chx commentedComment #6
chx CreditAttribution: chx commentedComment #8
chx CreditAttribution: chx commentedComment #9
chx CreditAttribution: chx commentedComment #10
chx CreditAttribution: chx commentedComment #11
dawehnerI really like that we do have now a proper foundation and no hand-wavy implementation any longer.
10 times better!
MUCH BETTER!
Did we considered to update the docs here to talk about neutral? and $neutral?
Comment #12
chx CreditAttribution: chx commented> Did we considered to update the docs here to talk about neutral? and $neutral?
Yes I put that suggestion in the summary while you were reading the patch :D but it got cross-posted. Restored.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedI only skimmed this so far, but I like what I see. I'll leave it to Wim and dawehner to review in-depth and RTBC though.
Comment #15
chx CreditAttribution: chx commentedComment #16
chx CreditAttribution: chx commentedThis won't pass but it makes checkAll simpler and "rhyme" with checkAny.
Fun: AccessManagerTest passes just the same if you remove the $result->andIf() call from checkAll.
Comment #18
chx CreditAttribution: chx commentedCloser...
Comment #20
chx CreditAttribution: chx commentedEven closer...
Comment #22
chx CreditAttribution: chx commentedComment #23
chx CreditAttribution: chx commentedComment #25
aheimlichShouldn't that be
$permission_access->isAllowed()
?Comment #26
chx CreditAttribution: chx commentedYes, thanks. Fixed that and more. I am not happy with the change in ContentTranslationOverviewAccess cos cacheability might not propagate but I am not even clear whether it should I need to talk to Wim about this caching stuff.
Even if someone else decides to post the next patch please keep this interdiff visible for the reasons above.
Comment #27
chx CreditAttribution: chx commentedThanks aheimlich SO MUCH! I have missed that one and look, now we are green!
Comment #28
chx CreditAttribution: chx commentedComment #29
Wim LeersCode review
Overall this patch looks excellent:
YAY! I thought this might be contentious, which is why I didn't add it previously. I like this much better though :)
Good clarifications :)
I'm not a big fan of these, but it allows for a smaller (and hence simpler) API, so I think that's more than worth it :)
It also more clearly shows what's going on; it stresses the use of
orIf()
andandIf()
more, so that's great.Taking that fact into consideration, I *am* a fan of this :)
Much clearer :)
Problematic: cacheability metadata lost!
This test was not updated and is therefore now subtly broken. Fixed in this reroll.
In this reroll
From the IS:
AccessManager
test coverage is apparently weak, but that's not caused by #2287071: Add cacheability metadata to access checks.I'm glad my patch (#2287071: Add cacheability metadata to access checks) triggered your critical eye, because I merely translated what already was there and made everything use a single consistent system. I think this explicit formalness really helps understand the access system much better. It also gives peace of mind, knowing that this isn't something ad-hoc, but something solid :)
I present to you, all 72 different permutations, times two operators, for a grand total of 144 test cases.
s/DENY/NEUTRAL/
. Also dids/KILL/FORBIDDEN/
ands/ALLOW/ALLOWED/
.mergeCacheabilityMetadata()
toinheritCacheability()
, making it public, and using it here. It will only be necessary in rare cases (the only other case in core is in point 5 of my dreditor review:entity_test_entity_field_access()
, which is in a test module, and a similar example inentity.api.php
). Having the cacheability inheriting code as a public instance method also is very consistent with the cleaned upAccessResult
: all instance methods are related to cacheability now!Besides that, I also did:
isNeutral()
assertions wherever there areisAllowed()
andisForbidden()
assertions.The "very" is a bit hyperbole — I've had several people thank me for "such a nice API".
Comment #31
Wim Leers14 fails, 3 exceptions. 13 of those due to something very stupid: no
return $this;
ininheritCacheability()
— fixed and added test coverage. Off to dinner and a green patch!Comment #32
chx CreditAttribution: chx commentedComment #33
Wim Leers#32: :)
Comment #34
dawehnerGreat work! Especially the expanded test coverage!
From AccessManager.php:
It is not obvious for me, whether you can actually break in cases of OR like that. Do you maybe have to take into account all of them, in order to have all cache information stored? Some line of documentation in case you don't would be cool, why is that possible?
It would be great if we could clearly document that the access specific (non-cache) information of these access result are immutable.
Let's put an @see on both of them.
Small detail, if you would use "The $other" it would be a little bit better to understand.
Just from reading this comment it seems not clear what means used here (either it is forbidden or allowed, right?)
I don't understand why we don't have to merge cache info, if $this is neutral and $other is Allowed
Docs seems to stop in the middle of
Nitpick: One-liner needed.
#2157541: Views sets access to ANY on routes - could result in information disclosure could use a 4th state which returns AccessResultAllowed for all cases (AND/OR) but I guess we need to find a different solution for that.
You can use @return static here
Technical this is by session and URL, right? Do we have a cache context of a session already?
Did we considered to convert those to use a data provider as well? It lets you provide the available combinations in a better readable way.
Awesome!
Isn't this method executed as test, because of the prefix "test"? We use providerTestAndOr somewhere else ...
Mh, here we actually test the UncacheableTestAcessResult not AccessResult itself. I do understand why, but can we make this explicit?
Do we still need this? Just curious.
Note: There is assertInstanceOf
Comment #35
Wim LeersOOohhh!!! Excellent point! That's true. That's also why I didn't do this in my original patch.
AccessManager
should just useAccessResultInterface::(andIf|orIf)()
, which have comprehensive test coverage. If we try to do introduce such optimizations inAccessManager
, we have to repeat that entire test coverage forAccessManager
again. And as chx has shown already,AccessManager
's test coverage is weak, so I've removed this optimization for now. We can restore it once #2341501: AccessManagerTest coverage is weak lands.$other
's cacheability metadata if it actually affected the combined result. By definition, this branch means the combined result is "neutral". Therefore, we must only check if$this->isNeutral()
is FALSE, because if and only if that's the case, we've used$other
to come to the conclusion that the combined result is "neutral".Therefore: if
$this
is "neutral" and$other
is "allowed", then we did not use$other
to conclude that the combined result must be "neutral" and hence we must also not merge its cacheability metadata.Comment #36
chx CreditAttribution: chx commentedDon't try to do shortcuts in checkAll either. It's not worth the bothering, it's a micro optimization and the effects on caching is hard to understand and test.
Comment #37
dawehnerWe have some follows ups and the current progress makes things surprisingly better to understand!
Comment #38
Wim Leerschx: oops, I forgot to check if there was a similar optimization there. Thanks!
Yay for progress and simplicity!
Comment #39
xjmComment #40
xjmGreat work here; let's try to get this one in soon because BC breaks in this API are a big deal.
Comment #41
dawehnerJust a minor sechole :p
Comment #42
alexpottWhat about negation...
This means the opposite of isForbidden is isForbidden??? Seems odd to me.
Comment #43
chx CreditAttribution: chx commentedYes we talked about negation and decided there is no use case and so we won't break out brains over it. But yes, it DOES mean that, I am really sorry, but the API in HEAD already does this, I haven't implemented this, I just put the formalization label on it and made the implementation of andIf/orIf crystal clear.
Also, this behavior is the only that makes any sense. Consider that a) we want isForbidden to stick across operations b) we want X AND X to be X (same for OR) this means that the only thing we can have any discussion over is what the ops should result when fed an isNeutral and an isAllow but that's also quite easy: if you are ORing you want that to go to allow. As for isNeutral AND isAllow: 1) can't really be isForbidden that makes no sense 2) we only want the result to be isAllow if both sides are isAllow so isNeutral AND isAllow goes to isNeutral. This finishes the truth table.
Comment #44
Wim LeersBasically, these are the truth tables.
A === isAllowed === "TRUE"
N === isNeutral === "FALSE"
F === isForbidden === "SUPER EXTREME MEGA FALSE" AKA "BLACK HOLE FALSE"
As you can tell: boolean logic for the
A
and theN
, but withF
being "contagious".This indeed equals http://en.wikipedia.org/wiki/Many-valued_logic#Bochvar.27s_internal_thre.... If we use Wikipedia's notation, then:
T (TRUE) === A == isAllowed()
F (FALSE) === N == isNeutral()
I (INDETERMINATE) === F === isForbidden()
Given this,
suddenly perfectly matches what Kleene's week three-valued logic says: the negation of the indeterminate is the indeterminate, i.e. the negation ofisForbidden()
isisForbidden()
. This is once again the "contagious" aspect. Wikipedia specifically calls this out also:Comment #45
chx CreditAttribution: chx commentedHere's perhaps a way to understand this: if you are negating then the logic values stay the same but the labels turn into a negated thing. So you have isForceAllow (I) forcing allowance. But negating never happens so this is a theoretical exercise.
Comment #46
alexpottThanks for taking answering my question :)
Committed ff035fc and pushed to 8.0.x. Thanks!
We need to update https://www.drupal.org/node/2337377 and link it to this issue.
Comment #48
effulgentsia CreditAttribution: effulgentsia commentedRelated to #42, what purpose is this doc serving? Objects implementing this interface ARE NOT implementing Kleene logic. They are implementing a subset of that logic only: just conjunction and disjunction. I think we should either remove the doc entirely (since the andIf() and orIf() methods are themselves documented), or else tweak the doc to be more precise. Especially because calling isForbidden() "indeterminate" is confusing: it's not indeterminate, it's just contagious. Not opening an issue for that yet, but if someone wants to, go for it.
Comment #49
chx CreditAttribution: chx commentedthe word is intermediate (being in the middle) not indeterminate. And yes it is odd but it gives you truth tables for free. If you remove that then I want to add truth tables to orIf / andIf ( I drawn them in ASCII before, it's not hard ).
Comment #50
Wim LeersI also provided the truth tables in #44. I personally also think that would be clearer, the Wikipedia article is not so great.
Comment #51
chx CreditAttribution: chx commented#2343631: Improve AccessResultInterface andIf()/orIf() docs filed.
Comment #53
Wim LeersComment #54
Wim Leers