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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah I think getting rid of the stupid dynamic URL is a good idea. Actually I thought there was an issue for that

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
534 bytes

When 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

tim.plunkett’s picture

We could even add a test asserting the link, if we wanted...

dawehner’s picture

Removing the link is a UX regression, right? So removing the static definition (which was wrongly added anyway) seems to be the proper fix

Well, 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.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Fair enough. Re-RTBCing the patch from the OP, please uncheck me for commit credit.

alexpott’s picture

FileSize
1.19 KB

Re-uploading so the rtbc patch is the latest one in the issue.

  • webchick committed 12faec8 on 8.0.x
    Issue #2495833 by alexpott, tim.plunkett, dawehner: Drupal\node\...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

The 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!

dawehner’s picture

Its a good idea to explain those kind of details as a tour.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.