Problem/Motivation
#2295469: Add support for static permission definitions with *.permissions.yml added support for permissions in YML files.
Proposed resolution
Let's add them everywhere we can.
Remaining tasks
Review the latest patch. RTBC and commit!
Decide whether to use structure in #24 (where 'title' key is defined separately) or structure in #28 (where shorthand permission syntax for permissions with titles only is used), and possibly update the change record to mention the shorthand syntax. (If this is a concern, let's open a follow-up issue. It is not yet documented and breaks the D7->D8 mental transition of using a separate key for title.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#74 | 2328411-74.patch | 311 bytes | herom |
#70 | permissions-2328411-70.patch | 88.58 KB | dawehner |
#70 | interdiff.txt | 962 bytes | dawehner |
#66 | interdiff.txt | 792 bytes | dawehner |
#66 | permissions-2328411-66.patch | 88.59 KB | dawehner |
Comments
Comment #1
dawehnerFirst part.
Comment #2
dawehnerHere is a patch.
Comment #3
dawehnerThe dynamic permissions are handled by damiankloip himself.
Comment #5
tstoecklerAwesome script, but it would ne nicer if the permission names (i.e. the keys in the YAML) would not be quoted. Maybe some sed-magic on top? :-)
Comment #6
dawehnerOH I wasn't aware that you don't need the quotes at all.
Note: This patch for now just fixes the failures but I do agree, we could drop the quotes.
Comment #7
geerlingguy CreditAttribution: geerlingguy commented@tstoeckler / #5: Keys with spaces need to be quoted to be valid YAML. Hopefully we can change that after this conversion is complete, over in #2309869: Use proper machine names for permissions.
Comment #9
dawehnerMeh, some code relies on hook_permission() being there.
Comment #11
dawehnerMore work. Simple patches are never simple.
Comment #13
tstoecklerRe #7: That's not correct. I just checked and the Symfony parser parser YAML with spaces in the key without quotes just fine. Now that you brought up, though, I do agree removing the qutotes would be fairly confusing due to the spaces. So let's just leave it as is.
Comment #14
dawehnerThis is indeed part of the spec: http://yaml.org/spec/1.2/spec.html, see "Flow scalar Styles".
Comment #15
dawehnerUsed no quotes now. This is much better to read.
PS: I am stupid.
Comment #17
geerlingguy CreditAttribution: geerlingguy commentedhttp://yaml.org/spec/1.2/spec.html#id2788859, specifically the plain implicit keys. Huh, learn something new every day! I still think snake case would be most appropriate (and consistent with other parts of our APIs), but at least it's not explicitly required...
(Edit: though, to avoid incomplete parser implementations, it might still be best to quote the keys; does our YAML parser work with spaces in keys?).
Comment #18
dawehner@geerlingguy
See patch
It is a small quote for me, but it is a big quote for drupalkind.
Comment #20
geerlingguy CreditAttribution: geerlingguy commentedMissed one more little quote :)
to:
Comment #22
geerlingguy CreditAttribution: geerlingguy commentedSomeone added a
link to any page
permission in system_permission(), it seems. This patch should hopefully work.Comment #24
geerlingguy CreditAttribution: geerlingguy commentedTrying that again... don't know how it missed the permission in system.permissions.yml.
Comment #25
dawehner@geerlingguy
Thank you for the work in this issue. Feel free to bring home this issue, if you want.
Comment #26
geerlingguy CreditAttribution: geerlingguy commentedWhat still needs work, besides the issue summary; we don't need a change notice for this issue, correct?
Comment #27
dawehnerI don't see why there should be another one. Updated the IS a bit.
Comment #28
herom CreditAttribution: herom commentedAlthough it wasn't mentioned in the change record, there is a shorthand way to do this (for permissions that only have a title):
Comment #29
geerlingguy CreditAttribution: geerlingguy commentedCould we get that added to the change record? If this is a supported/recommended method of defining permissions, it seems it should be mentioned. It feels too informal for my liking, but I see how it's a nice way to do it.
Comment #30
geerlingguy CreditAttribution: geerlingguy commentedComment #31
damiankloip CreditAttribution: damiankloip commentedThis seems like a bit of a cut corner here. We are now losing the link. This should stay in hook_permission if that's the case. #2329485: Allow permissions.yml files to declare 'permission_callbacks' for dynamic permissions can then implement that or something.
Also, RE #28: This was the case anyway, before this patch?
Comment #32
dawehnerYes it was. I mirrored the way how the hook was handled. I actually though dislike the change a lot as it makes it a bit inconsistent. Note: All core permissions
are actually using title only. Classical out of scope change, if you ask me.
Comment #33
dawehnerLet's wait on #2329485: Allow permissions.yml files to declare 'permission_callbacks' for dynamic permissions
Comment #34
geerlingguy CreditAttribution: geerlingguy commentedComment #35
geerlingguy CreditAttribution: geerlingguy commentedOkay, back to needs review—though we need to decide if we'll use the shorthand title syntax, or the regular title/description structured key syntax (and whether or not that should be added to the original change record)...
Comment #36
dawehnerReroll of #24 for now. So yeah we should update the patch to use permission_callbacks now.
Comment #38
dawehnerLet's convert all other instances.
Comment #40
geerlingguy CreditAttribution: geerlingguy commentedOne callback needed to be fixed (see interdiff). Failing tests passed locally on current HEAD.
Comment #41
dawehnerAh cool, thank you for the fix. Anyone wants to RTBC it?
Comment #42
herom CreditAttribution: herom commentedJust a minor nitpick:
s/TRUE/true
Comment #43
geerlingguy CreditAttribution: geerlingguy commentedStill needs review—I picked up another s/TRUE/true in views_ui permissions too, thanks!
Comment #44
herom CreditAttribution: herom commentedLet's remove their mentions in the docs too (grepped for
'_permission('
).Comment #45
geerlingguy CreditAttribution: geerlingguy commentedAlso fixed a couple other docs instances, and added mention of
$module.permissions.yml
to core API docs.Comment #46
geerlingguy CreditAttribution: geerlingguy commentedComment #47
dawehnerThis still refers to hook_permission ... but it should actually talk about the permissions_callback ...
Comment #48
geerlingguy CreditAttribution: geerlingguy commentedBut doesn't hook_permission() still work fine? The change records indicate that neither is really preferred, and make no mention of permissions_callbacks—is this just a matter of the change record being out of date?
From the 'Dynamic Permissions' section.
Comment #49
dawehner@geerlingguy
Well, the idea of this issue was actually to get rid of the module based support.
Comment #50
geerlingguy CreditAttribution: geerlingguy commentedI see. In that case, shouldn't we mark the use of hook_permission as deprecated? Would that be done in a separate/follow-up issue?
Comment #51
damiankloip CreditAttribution: damiankloip commentedPatch looks good. Only thing is whether we keep the hook_permission for now, deprecate. Then remove in a follow up? I think the maintainers like that pattern?
Comment #52
dawehnerOkay, let's adapt just the comment and mark it as deprecated for now.
Comment #53
geerlingguy CreditAttribution: geerlingguy commentedI would rtbc but I have too much skin in the game. Shall I open a follow-up to mark the old hook deprecated?
Comment #54
dawehnerThis would be great!
Comment #55
herom CreditAttribution: herom commentedSince my changes didn't make it into the patch, I guess I can RTBC.
The patch looks great.
Comment #56
dawehner@herom
It would be also great if you could open up a follow up for your changes, so we can discuss things there.
Comment #57
geerlingguy CreditAttribution: geerlingguy commentedFYI #babywatch14 has come to an end today, so I will not be able to open that follow up (or do much else) for a few days... Glad to see this issue RTBC, and if someone else doesn't get the CR updated and follow-ups created by then, I'll jump back in and do it once I get some rest!
Comment #58
dawehner@geerlingguy
So have fun with whatever new episode you start now.
Adding a possible follow up for more consistency.
Comment #59
herom CreditAttribution: herom commentedfiled the followup issues:
#2338475: Remove hook_permission() and #2338487: Document and use the shorthand syntax in .permission.yml files..
Comment #60
herom CreditAttribution: herom commentedoops! bringing back.
Comment #61
alexpott@see to a non existing file
Above here there is a corresponding assertion that is still checking hook_permission
How about something like:
Or maybe something on the PermissionHandler to do this.
Now you are injecting you can replace the \Drupal::service in this class.
user_permission_get_modules()
has no usages and therefore should be removed. It usage we as removed by #1872876: Turn role permission assignments into configuration. so where are changing code with 0 test coverage and zero usages - better to remove it.Comment #62
dawehnerThank you alex, for your great review!
Comment #64
dawehnerMeh.
Comment #65
tim.plunkettI guess since that last interdiff, this is useless? But are we sure we want to call build and sort on every call to getPermissions? That seems like a mistake.
Comment #66
dawehnerWell, those permissions never cached its result previously, so we did not added it in the original permissions handler issue, so do I want in that,
because this causes test failures, see #62
Added a follow up.
Comment #67
dawehnerOh mh, the interdiff is totally screwed up.
Comment #68
tim.plunkettWow, I have no idea how you generated that interdiff :)
But the actual changes in the patch are correct. Thanks @dawehner!
Comment #70
dawehnerMeh!
Comment #71
alexpottCan we update https://www.drupal.org/node/2311427
Committed 52a101b and pushed to 8.0.x. Thanks!
Comment #73
dawehnerUpdated.
Comment #74
herom CreditAttribution: herom commentedIn the meantime... the entity module was removed :)
Comment #75
alexpottCommitted e206190 and pushed to 8.0.x. Thanks!
Comment #78
deguif CreditAttribution: deguif commentedThe link to permissions from modules page has disappeared since this patch.
Comment #79
akalata CreditAttribution: akalata commentedRegression reported at #2513626: [Regression] Module permission links missing from module list page
Comment #80
Balu Ertl