Updated: Comment #N
Problem/Motivation
After #2107137: Fix the DX for declaring custom access checkers we will have the ability to add one off custom access checks. These do not even have to implement accessInterface etc... So it makes sense to enforce that what is returned from access checkers in general is one of the AccessInterface constants.
Proposed resolution
Throw an exception in Access Manager if the return value is not one of the allowed constants. To also help with this, we may change the constants from bool/null values to strings. This will now allow people to just return TRUE, or any other value.
Remaining tasks
Patch (from related issue below), tests, fix other tests.
User interface changes
None
API changes
You have to use interface constants (which should be the case anyway).
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff-2108829-23.txt | 1.64 KB | damiankloip |
#23 | 2108829-23.patch | 11.21 KB | damiankloip |
#8 | interdiff-2108829-8.txt | 2.18 KB | damiankloip |
#8 | 2108829-8.patch | 7.73 KB | damiankloip |
#5 | interdiff-2108829-5.txt | 1.55 KB | damiankloip |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedHere's the initial patch (yoinked from related issue).
Comment #3
damiankloip CreditAttribution: damiankloip commentedOK, rerolled for addition of AccountInterface for access checks and fix the actual test. The should pass now.
Comment #4
dawehnerIs there a reason why we haven't changed the actual values of these constants? Only that change ensures that we do use the proper values everywhere.
Comment #5
damiankloip CreditAttribution: damiankloip commentedOK, I was going to have a follow up for that, but let's do it here. So how this gets on first.
I will update the tests for TRUE, FALSE in the next patch.
Comment #6
dawehnerDo you talk about the default access checker here? Maybe we could do that in another issue.
Comment #8
damiankloip CreditAttribution: damiankloip commentedLet's see if this fixes the tests.
Comment #10
damiankloip CreditAttribution: damiankloip commentedOh.. Let's try this. That Node revision check needs to return too, so let's return FALSE instead of NULL like it was doing before. Also fixed a couple of other unit tests.
Comment #12
dawehnerThis should fix the test failures.
Comment #13
damiankloip CreditAttribution: damiankloip commentedThe last one, great. I knew there would be one more access checker not playing by the rules. Shows that this patch is worth it IMO.
Comment #14
damiankloip CreditAttribution: damiankloip commented12: access-2108829.patch queued for re-testing.
Comment #16
damiankloip CreditAttribution: damiankloip commented12: access-2108829.patch queued for re-testing.
Comment #17
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #18
dawehnerAdding some tags.
Comment #20
damiankloip CreditAttribution: damiankloip commented17: 2108829-17.patch queued for re-testing.
Comment #21
dawehnerre-rtbc
Comment #22
Crell CreditAttribution: Crell commentedWhich access checker failed? If we're going to throw an exception with the expectation that it gets displayed, it should be helpful to the developer and say which access checker/method is Doing It Wrong(tm). That way it's easier to fix.
I like the idea of this patch on "fail usefully" grounds, but we should go all the way to failing very usefully.
Comment #23
damiankloip CreditAttribution: damiankloip commentedOk, that IS a fair point I guess :)
Comment #24
dawehnerGood point!
Comment #25
catchCommitted/pushed to 8.x, thanks!