Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Declaring a static perm in a permission callback object is unnecessary.
access content overview:
title: 'Access the Content overview page'
description: 'Get an overview of all content.'
from node.permissions.yml
versus
public function contentPermissions() {
return array(
'access content overview' => array(
'title' => $this->t('Access the Content overview page'),
'description' => $this->t('Get an overview of <a href="!url">all content</a>.', array('!url' => $this->url('system.admin_content'))),
),
);
}
At the moment the static yml declaration wins so removing the unnecessary code.
Comment | File | Size | Author |
---|---|---|---|
#6 | d8.node-perms.patch | 1.19 KB | alexpott |
d8.node-perms.patch | 1.19 KB | alexpott | |
Comments
Comment #1
dawehnerYeah I think getting rid of the stupid dynamic URL is a good idea. Actually I thought there was an issue for that
Comment #2
tim.plunkettWhen node permissions were originally ported from hook_permission:
#2329485: Allow permissions.yml files to declare 'permission_callbacks' for dynamic permissions added the dynamic method, removing "access content overview" from node_permission
#2328411: Convert all permissions to yml files and permission callbacks then added "access content overview" back after comment #31, not realizing it had been converted to a dynamic one.
Removing the link is a UX regression, right? So removing the static definition (which was wrongly added anyway) seems to be the proper fix
Comment #3
tim.plunkettWe could even add a test asserting the link, if we wanted...
Comment #4
dawehnerWell, I think having the link in the first place is kinda pointless. Who goes to the permissions page and then clicks on that link. We removed instances of other more advanced cases (which checked access) already.
Comment #5
tim.plunkettFair enough. Re-RTBCing the patch from the OP, please uncheck me for commit credit.
Comment #6
alexpottRe-uploading so the rtbc patch is the latest one in the issue.
Comment #8
webchickThe link I think is somewhat useful for those who are new to Drupal and trying to make some kind of sense of that overwhelming matrix of choices. However, other permissions don't have links to their respective admin pages (e.x. "administer blocks," etc.) so I think indeed removing the link is more inline with the rest here. We could always revisit this in a more holistic way among all admin $something permissions if we wanted to add links (or Tour or whatever) later.
Committed and pushed #6 to 8.0.x. Thanks!
Comment #9
dawehnerIts a good idea to explain those kind of details as a tour.