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
As #2295469: Add support for static permission definitions with *.permissions.yml removed hook_permissions from core renders
$perms = module_invoke($module, 'permission');
useless. This breaks drush https://github.com/drush-ops/drush/pull/830
Proposed resolution
Add method permissionsByModule.
Remaining tasks
Write the mock for the added test.
cd core
phpunit modules/user/tests/src/Unit/PermissionHandlerTest.php
give
1) Drupal\Tests\user\Unit\PermissionHandlerTest::testPermissionsYamlStaticAndCallback
Drupal\Core\Controller\ControllerResolverInterface::getControllerFromDefinition('Drupal\user\Tests\TestPermiss...iption') was not expected to be called more than once.
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#17 | add_permissionbymodule-2342229-14.patch | 8.86 KB | mgifford |
#14 | add_permissionbymodule-2342229-14.patch | 8.86 KB | AjitS |
#7 | interdiff.txt | 3.74 KB | dawehner |
#7 | permissions_handler-2342229-7.patch | 8.86 KB | dawehner |
#5 | interdiff.txt | 9.3 KB | dawehner |
Comments
Comment #2
clemens.tolboomComment #3
dawehnerSo there seems to be indeed a usecase out there, but yeah I am not sure whether this is semantically a bug but rather a task/feature?
Let's fix the codestyle and documentation here.
As you expand the test coverage let's put @covers at the top.
Comment #4
clemens.tolboom@dawehner #2 i'm not sure how to fix @covers :-(
I consider this a bug as it broke drush which has no alternative then copy the code from
::permissionsByModule()
Test needs work. How to fix this?
Attached patch fixes #3 - #1
Comment #5
dawehnerThere are a couple of problems:
There we go.
Comment #6
damiankloip CreditAttribution: damiankloip commentedDo you think !empty() would be more self descriptive here?
We have a mixture between module and provider here. Do you think we should convert everything to provider? That seems to be the direction we are heading in?
Provides
Should we just use an @see there?
I don't really mind, but I would rather just see the expected in a separate variable so the assertEquals is easier to read.
Sets up*
Comment #7
dawehnerWell yeah but should we really rename also the moduleProvidesPermissions? I think we should do a better naming in #2338469: Add support for a core.permissions.yml as well as core.routing.yml
I would vote to have both, because, well the inline reference helps to understand it.
Sure, let's do it.
Comment #8
damiankloip CreditAttribution: damiankloip commentedThis is more permissions FOR module? By would imply I get a list grouped by module..
Comment #9
Xano@return array
is never ever good documentation. Always specify what exactly is in the array. Is it an array of arrays? Then use@return array[]
.Comment #10
jhedstromPatch no longer applies, and the drush issue mentioned in the summary has been fixed by other means, so I'm not sure if this is still applicable.
Comment #11
piyuesh23 CreditAttribution: piyuesh23 commentedComment #12
piyuesh23 CreditAttribution: piyuesh23 commentedComment #13
AjitSWorking on it.
Comment #14
AjitSPatch was not applicable. Just rerolling for now.
Comment #15
dawehnerLooks pretty fine
Comment #17
mgiffordRe-submitting the prior patch for the bots to review.
Comment #29
smustgrave CreditAttribution: smustgrave at Mobomo commentedPart of the new review initiative this one came up.
Wonder after 7 years if its still a valid bug report.
If so can we also get an updated issue summary.
Comment #31
smustgrave CreditAttribution: smustgrave at Mobomo commentedSince there has been no activity going to close for now.
If still a valid bug please reopen updating issue summary.
Thanks all!