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.
I present as evidence http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul... but there are many others.
core/modules/toolbar/lib/Drupal/toolbar/Access/SubtreeAccess.php:
/**
* @file
* Contains \Drupal\toolbar\Access\SubtreeAccess.
*/
namespace Drupal\toolbar\Access;
use Drupal\Core\Access\StaticAccessCheckInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\Route;
/**
* Defines a special access checker for the toolbar subtree route.
*/
class SubtreeAccess implements StaticAccessCheckInterface {
/**
* {@inheritdoc}
*/
public function appliesTo() {
return array('_access_toolbar_subtree');
}
/**
* {@inheritdoc}
*/
public function access(Route $route, Request $request) {
$hash = $request->get('hash');
return (user_access('access toolbar') && ($hash == _toolbar_get_subtrees_hash())) ? static::ALLOW : static::DENY;
}
}
core/modules/toolbar/toolbar.routing.yml:
toolbar.subtrees:
path: '/toolbar/subtrees/{hash}'
defaults:
_controller: '\Drupal\toolbar\Routing\ToolbarController::subtreesJsonp'
requirements:
_access_toolbar_subtree: 'TRUE'
Counter-Suggestion
toolbar.subtrees:
path: '/toolbar/subtrees/{hash}'
defaults:
_controller: '\Drupal\toolbar\Routing\ToolbarController::subtreesJsonp'
requirements:
_custom_access: '\Drupal\toolbar\Routing\ToolbarController::accessSubtree'
...and then stick the accessSubtree method with the rest of the stuff that builds the output and have ControllerBase automatically bring in the right stuff (which afaik it does already). No need for an extra class+directory+3 "use" statements, in the vast majority of cases.
Comment | File | Size | Author |
---|---|---|---|
#34 | access-check-2107137-34.patch | 9.61 KB | dawehner |
#34 | interdiff.txt | 682 bytes | dawehner |
#31 | interdiff.txt | 457 bytes | dawehner |
#31 | access-2107137-31.patch | 9.43 KB | dawehner |
#28 | interdiff.txt | 1.6 KB | dawehner |
Comments
Comment #1
dawehnerI am sorry but the issue talks about routing, but the codeexample shows an entity access controller.
This is a minimal implementation of a route access controller:
Comment #2
webchickYou are right. :) Entity access controllers are dealt with over at #2105557: Add an admin_permission to EntityType annotations to provide simple default access handling so I'll use your code example instead for this issue.
Updating the issue summary...
Comment #3
dawehnerLet's see whether this passes.
Comment #3.0
dawehnerx
Comment #4
damiankloip CreditAttribution: damiankloip commentedFYI, I opened an issue for pretty much the same thing a while back (in june), and it got closed (won't fix) - #2011424: Allow access methods on controllers to be used in routes
Just a few quotes from over there:
Comment #5
dawehnerWell, if we specify a callback directly this is not a problem.
Comment #6
Crell CreditAttribution: Crell commentedThis line looks wrong. currentUser() returns a user object, doesn't it? Don't you mean currentUser()->hasPermission()?
And don't you mean to inject the current user service anyway? :-)
Otherwise this seems reasonable, with the caveat (that should be loudly documented) that this *will* result in two instances of the controller object if the access check is on the same class. (I don't know if it's worth trying to collapse that or not.)
Comment #7
dawehnerWhy should toolbar have a test ...
Good catch! (Regarding the current user I was just lazy.
Comment #8
Crell CreditAttribution: Crell commentedLet's do.
Comment #9
webchickNeeds docblock, and afaik it's not kosher to have two classes in the same file with PSR-0?
Ick. Don't like all this boilerplate at all, and certainly not as setting a precedent of what other controllers are going to have to do. Is the current user service part of ContainerBase? If so, can we just extend from that and hide this complexity from user-space code?
Comment #10
tim.plunkettIt's okay for classes used by only one test class.
Fixed to use Controllerbase
Comment #11
dawehnerYou know the amount of code you save on the actual controller is basically the same as the amount of stuff you need to write more when writing a unit test.
Comment #12
damiankloip CreditAttribution: damiankloip commentedMe and Daniel spoke about this on IRC this morning, and agreed it makes sense to make the access checking a bit more robust if we are adding something like this. We don't want unexpected access behaviour if people decide to return random things from these ad-hoc access callbacks.
Adding a new AccessException, unit test coverage in AccessManagerTest, and some changes to the interface constants.
Comment #13
dawehnerThis is really great but it should be providerTestCheckException if you don't want to execute this function as actual test.
Comment #14
damiankloip CreditAttribution: damiankloip commentedHA, You are so right :)
Serves me right for rushing a unit test!
Comment #15
dawehnerGreat, thank you!
Comment #17
webchicktagging.
Comment #18
tim.plunkettThis change makes no sense to me, and is likely responsible for all of the fails.
Comment #19
damiankloip CreditAttribution: damiankloip commentedYeah, as mentioned, me and daniel talked about this earlier today but I forgot to mention that :-) . Could be out of scope of this issue though past just enforcing the access return values. Rerolling.
Comment #20
dawehnerLet's skip this change for now and do that on a follow up.
Comment #21
damiankloip CreditAttribution: damiankloip commentedAh, ok, we're going for reverting all of that, ok: #2108829: Make AccessManager stricter with values returned from access checkers
I think this one is back to RTBC in that case.
Comment #22
tim.plunkett+1 for RTBC
Comment #23
damiankloip CreditAttribution: damiankloip commentedThe title seems a little too vague for what the patch is?
Comment #25
Xano#20: access-2107137-20.patch queued for re-testing.
Comment #26
tim.plunkett#20: access-2107137-20.patch queued for re-testing.
Comment #27
webchickSorry, this fell way off my radar. :(
a) Shouldn't core/modules/toolbar/lib/Drupal/toolbar/Access/SubtreeAccess.php be removed now? That'd make it easier to see the DX impact of this patch.
b) I don't see Crell's concern at #6 addressed in documentation. That also sounds sub-optimal. I asked Tim about whether we could work around this and he said "um [...] it might involve some creativity." Given Crell subsequently RTBCed this despite that limitation, I'm guessing he thinks this is acceptable to push to a follow-up?
c) There are a couple of minor language things in the class PHPDoc:
Should be:
Also, "Edit" and "Toolbar" should be capitalized.
I can fix those on commit (unless this gets re-rolled for a/b) but moving to "needs review" for the others.
Comment #28
dawehnerYou are totally right here!
Regarding the documentation, this is a quote from the patch:
Comment #30
webchickOh, ok. I was specifically referring to:
"with the caveat (that should be loudly documented) that this *will* result in two instances of the controller object if the access check is on the same class."
I didn't see that in the docs.
Comment #31
dawehnerUps.
Comment #32
tim.plunkettThis still doesn't have the docs asked for in #30.
Also, it seems we missed the cut here.
Comment #33
Crell CreditAttribution: Crell commentedwebchick: Title callbacks already have the same issue. We should probably try to figure out a good way to address this "reused callable" problem, but I am OK with it not being part of this issue to resolve. (It probably requires a property cache or wrapper cache on the controller resolver spin-off class we wrote.)
In practice, it should only cause a problem if:
1) The class in question is especially memory-costly (which they really should never be) or
2) Someone mistakenly tries to reuse data from the access or title method in the content method, or vice versa.
There is probably a philosophical argument to be made as well that it's a good thing that they're separate objects since the resulting objects are not highly cohesive. I'll leave that for when Anthony Ferrara is back in town, though. :-)
Comment #34
dawehnerIf you do something costly in your constructors you are doing it wrong. Symfony 2.3 has the concept of lazy services (see http://symfony.com/doc/current/components/dependency_injection/lazy_serv...)
which basically are just initialized once you really use them so constructing them would also be no issue.
If we decide do keep one instance of a controller we really have to be aware of the problem that we basically have created singletons which for sure causes
issues on things like entity routes where you reuse the controller.
Comment #35
Crell CreditAttribution: Crell commented#34 is sufficiently loud for now. :-)
Comment #36
catchThis does cut down on boilerplate, I'm a bit surprised there aren't more examples in core, but on the other hand the less of these the better since generic access checks should be generic. Committed/pushed to 8.x, thanks!
Could use a change notice for the API addition.
Comment #37
dawehnerAdded a change notice: https://drupal.org/node/2117181
Comment #37.0
dawehnerRevamped based on /actual/ access checker. :P
Comment #38
Crell CreditAttribution: Crell commentedChange notice looks good to me.