Use proper machine names for permissions

Just a note that the first thing we'd probably want to change would be:
'restrict access': TRUE

I think it should not be out of scope ... given that we have to touch nearly all permissions.

Or rather, it could be a separate issue before this one. But it doesn't make sense to change all the machine names to get rid of quoting/spaces for permissions if we're still using it for something that's rather more machine-y than the permission names. :)

Given that you can have keys without any quotes in YML I wonder whether we still want to proceed with this issue.

I agree with #9. I think we should only consider to remove the quotes around the keys, but that's a separate issue/discussion.

That said, given that this issue seemingly suggested to change all permission names:

We once had a major proposal to properly namespace all permission IDs by owner/module and introduce a hierarchical notation; i.e.:

entity.user.register
entity.user.create
entity.user.edit.own
entity.user.edit.any
entity.user.delete
entity.user.cancel
entity.node.create
entity.node.edit.own
...

#381584: Hierarchical Permissions System
#1200572: Concept of a hierarchical permission system
https://groups.drupal.org/node/205928, https://groups.drupal.org/node/154054

So for some reason I failed to write out my reasoning for why it would be okay to break all the permission names right up until RC1, which is what my tagging would imply. Talk about a noisy change between betas! At least, it would be best to have this by beta 1 if we are going to do it, and it might actually be a beta deadline. But I'm not sure from #1 and #3 whether I discussed it with catch or not. xjm--

Dot namespaces are scope creep IMO. While I see the appeal and logic of the suggested patterns, we've already decided not to make the effort to do those conversions in other subsystems. Let's just replace the spaces with underscores and be done with it, if we want to do this at all.

Dot namespaces are scope creep IMO. While I see the appeal and logic of the suggested patterns, we've already decided not to make the effort to do those conversions in other subsystems. Let's just replace the spaces with underscores and be done with it, if we want to do this at all.

100% agreed.

Well, one reason why this issue was started, was that we didn't knew that keys with spaces don't require quotes ... so writing those yml files is actually less hard, as well as the readability doesn't sucks
completely.

I had originally proposed hierarchical permissions years ago, but it never caught on. I agree that it's too late to do that for Drupal 8. Let's just s/ /_/ in the permission "machine name" for consistency and call it a day for now.

Per comment #8, #2328411: Convert all permissions to yml files and permission callbacks is complete and this is also tagged RC deadline, so marking active.

updating remaining tasks in issue summary.

Of the module permissions, I got through the first four:

• action.permissions.yml
• aggregator.permissions.yml
• ban.permissions.yml
• block.permissions.yml

I think it makes sense to throw an exception in PermissionHandler if a non-machine name permission key is set.

This patch actually enforces the machine name. I don't know the generally-accepted regex or if there is a core function already. It throws a simple exception. We should create a custom exception.

I changed permissions for the book module. Note that it is normal for install to fail at this stage because the machine-name detection check introduced in this issue throws an exception.

I added the custom exception.

changing permissions for comment.permissions.yml file.

working on permissions in config.permissions.yml

comment.permissions.yml permissions changed.

Here are the correct patch, Please ignore previous one.

config.permissions.yml changed

changing config_translation.permissions.yml permissions

config_translation.permissions.yml changed.

+++ b/core/modules/user/src/PermissionHandler.php
@@ -149,7 +150,8 @@ protected function buildPermissionsYaml() {
+        $this->checkValidPermissionName($permission_name);

If anyone wants the tests to pass, comment out this line. Just remember to put it back later.

converting contact.permissions.yml

wrt https://www.drupal.org/node/2309869#comment-9293257 I have commented the line, so that this patch can be tested.

@cilefen : I think we should keep this line commented till all the permissions are changed, in this way we will be able to see the test result and once all the permissions are changed successfully, we will uncomment it again. Makes sense ?

Changed permissions for contact.permissions.yml

Changes in this patch :

1. content_translation.permissions.yml converted.
2. contextual.permissions.yml converted.
3. Test failed in the previous patch due to wrong conversion of permission is now fixed.

+1 for postponing until Drupal 9.

I'd actually go one higher and call this "won't fix." :\ In addition to the huge disruption caused to existing modules trying to port to D8 (which means this is not post-beta material according to https://www.drupal.org/contribute/core/beta-changes), there's nothing inherent in YAML that prevents keys with spaces in them (http://www.yamllint.com/), and adding underscores makes the resulting code less natural to read.

For now just moving to 9.x though.

Hm? We already use spaces with keys in them elsewhere ('base theme' in themes, for example). Keys with spaces in them are used as examples in the YAML spec: http://yaml.org/spec/1.2/spec.html (see "not a number" and "Sammy Sosa" and others) as well as the Symfony YAML examples http://symfony.com/doc/current/components/yaml/yaml_format.html ("symfony 1.0" as a key). It's not unprecedented at all, though granted that "not-space" is more common.

And sorry, but this really clearly is not 8.x material anymore. See https://www.drupal.org/contribute/core/beta-changes. There's no way to justify the impact (addressing an inconsistency) being greater than the disruption (breaking every single module that touches permissions) here.

Although I don't have nearly enough experience as many of you all, I just wanted to point out that during a recent training in D7 module development, a co-worker pointed out this exact same issue, where some naming conventions had underscores while others didn't, i.e., permissions, etc.

This was slightly confusing to some of us as we were navigating through different functions and module files. @webchick and @geerlingguy, your explanations make it very clear as to why it is the way it is, but as @geerlingguy pointed out, it is definitely worthwhile to keep in mind in a general sense. From a newbie perspective, consistency is key when learning this type of stuff, as I'm sure you all can remember those days. ;)

Clearly this isn't exactly a big issue that needs to be addressed, but I would agree it is at least worth noting for 9.x.

Sorry for my lengthy explanation and hope I made some sense. As a newb it's a little intimidating to jump in these issue queues sometimes.

@Daniel_Rose So that is a +1 from you. I like this issue too. But, it will just have to wait because as you read it is too disruptive now.

The issue queues are "intimidating"? Stick with us. That will wear off. Visit us at core mentoring hours to level up. We are nice—I promise.

@cilefen definitely a +1 from me and I appreciate the words of encouragement.

This looks like the right status.

Under our continuous upgrade path and deprecation policy, feature and API additions should be added with backwards compatibility in minor releases, so moving to 8.8.x. Thanks!

Should there be some kind of naming convention for permissions to avoid collisions?

Curious if this is still valid?

Define valid. It certainly didn't happen elsewhere in the meantime, or we'd have all noticed :)

Not sure if the status is right, needs work/active would probably make more sense. It's basically set to this status to define a plan on how this could be a approached in a BC-compatible way, which is very much not straight forward which is why no one even tried.

The only way for this to become invalid would be if we decide to won't fix it. I'm not sure about that. Conceptually, I like this, but not convinced at all that the BC impact and amount would be worth it. And apparently nobody in the last 7 or so years thought otherwise and dared to pick this up.

that was more my question is if it's in the right status.

I agree this will break virtually everything most likely. Should this maybe be a target for D11?

Setting proper issues status per above conversation.

Can we establish a new policy for the permission names and apply it only to new permissions? Contributed modules and custom modules would benefit from it right now because they will not have to follow this weird naming convention.

Added patch

#67 should be answered.

Removing novice tag for now as it looks like a question of policy change was asked.

Re #67, nobody is forced to do anything in contrib/custom code. permissions are just a string as far as core is concerned. It just happens to commonly use spaces instead of underscores.

The payment module for example uses a hierarchical permission names, separated by .: https://git.drupalcode.org/project/payment/-/blob/8.x-2.x/payment.permis...

IMHO, switching spaces to underscores without any other change like the patch in #68 is pretty pointless. It doesn't solve conflicts, it doesn't enable us to do anything we can't do now. The issue summary is also wrong in regards to YAML, spaces in keys do not need to be quoted as visible in those patches. If we were to introduce a change, it should be in context of #2919636: Design a better model for managing permissions IMHO and for example move us toward a hierarchical permission system.

So I'm changing my opinion of #62 and am voting in favor of closing this as a duplicate of that ideas issue.