Add proper documentation for entity type's admin_permission and collection_permission

So I was wondering how and where to document this and I found this in entity.api.php after the documentation of how to create a custom entity type and a bunch of the different handlers you can add (including the snippet about access handlers in the issue summary):

Additional annotation properties can be seen on entity class examples such as \Drupal\node\Entity\Node (content) and \Drupal\user\Entity\Role (configuration). These annotation properties are documented on \Drupal\Core\Entity\EntityType.

And while I don't think it's necessarily ideal to point people to \Drupal\Core\Entity\EntityType as that's not a particular well documented class in general, my vote would be to simply add some more elaborate documentation to the $adminPermission property. Maybe something like the following:

The name of the administrative permission.

This permission must be declared separately by the module providing the entity type. Users with the administrative permission by default are allowed to perform any operation (for example create, view, update or delete) on any entity of the type and are granted access to the overview (or collection) of the entities of that type, if one exists. If a custom access control handler is declared, access can be restricted even for users with this permission.

Since this came up in ✨ Allow entities to specify a "collection permission" Fixed and the lack of documentation was noted there, here's something similar I would add for the collection permission either there or here, whichever one lands later:

The name of the collection permission.

This permission must be declared separately by the module providing the entity type. Users with the collection permission by default are allowed to access the overview (or collection) of the entities of that type, if one exists. It does not grant access to any particular entities, but in combination with other permissions may allow to grant users, for example, access to view entities of this type in the overview, but not edit or delete them or only grant users access to manage certain entities from the overview, but not all of them. If a custom access control handler is declared, access can be restricted even further for users with this permission or further access can be granted.

Updating issue summary now that ✨ Allow entities to specify a "collection permission" Fixed landed.

Also going ahead and adding my proposal as the proposed resolution and marking as Novice so someone can turn my suggestion into an actual patch. (Also minimally reworded my suggestion.)

Hi, it seems like the \Drupal\Core\Entity\EntityType::$collection_permission property isn't available in 10.1.x-dev. I've made the required changes in 11.x instead.

Please review this patch.

Should check a patch applies before uploading.

As noted, it should be run against 11.x

Good catch.

Change looks simple and matches IS

Always like to see doc improvments!

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -72,6 +72,10 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
   +   * This permission must be declared separately by the module providing the entity type.
   
   @@ -79,6 +83,11 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
   +   * This permission must be declared separately by the module providing the entity type.
   

   @sidharth_soman, all change to Drupal core are to meet the coding standards. Even though the line length is currently not test it should be followed. See Line length and wrapping → .

2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -72,6 +72,10 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
   +   * This permission must be declared separately by the module providing the entity type.
   
   @@ -79,6 +83,11 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
   +   * This permission must be declared separately by the module providing the entity type.
   

   The word 'separtely' seems wrong to be. I think it can be removed without losing any meaning.

I read the two new doc blocks and find both hard to follow. The contain long sentences with many phrases and 'ifs'. Surely this can be simpler for people with English as a first language (me) and people where English is not their first language.

I have resolved the issue mentioned in #8. Please review.

Seems feedback has been addressed.

Thanks for the updates! Also, thanks for interpreting my poor typing. Reading the lastest patch with dreditor I can still see lines that are not wrapped to 80 columns. :-(

Now that I can read the text in the patch I also think it can be simplified a bit.

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -72,6 +72,14 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
   +   * If a custom access control handler is declared,
   

   This will be easier to read with a blank line before this paragraph.

2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -72,6 +72,14 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
   +   * If a custom access control handler is declared,
   +   * access can be restricted even for users with this permission.
   

   This is not wrapped correctly.

3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -79,6 +87,18 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
   +   * If a custom access control handler is declared, access can be restricted
   +   * even further for users with this permission or, alternatively,
   +   * further access can be granted.
   

   Same here, add an empty line.

4. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -79,6 +87,18 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
   +   * even further for users with this permission or, alternatively,
   +   * further access can be granted.
   

   This is not wrapped correctly

5. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -72,6 +72,14 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
   +   * This permission must be declared by the module providing the entity type.
   +   * Users with the administrative permission by default are allowed to perform
   +   * any operation (for example create, view, update or delete) on any entity
   +   * of the type and are granted access to the overview (or collection) of the
   +   * entities of that type, if one exists.
   

   I suggest that we can say the same thing but a bit clearer. Correct this if I have missed something.

   "This permission must be declared by the module providing the entity type. Users with the administrative permission can perform create, view, update or delete operations on any entity of that type. They also are granted access to the overview (or collection) of the entities of that type, if one exists."

6. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -79,6 +87,18 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
   +   * This permission must be declared by the module providing the entity type.
   +   * Users with the collection permission by default are allowed to access the
   +   * overview (or collection) of the entities of that type, if one exists.
   +   * It does not grant access to any particular entities, but in combination
   +   * with other permissions makes it possible to grant users, for example,
   +   * access to view entities of this type in the overview, but not edit or
   +   * delete them or only grant users access to manage certain entities from the
   +   * overview, but not all of them.
   

   Similarly, I suggest that we can say the same thing but a bit clearer. Correct this if I have missed something.

   "This permission must be declared by the module providing the entity type. Users with the collection permission are allowed to access the overview (or collection) of the entities of that type, if one exists. It does not grant access to any particular entities, but is used in combination with other permissions. For example. this makes it possible to grant users access to view entities of a particular type in the overview, but not edit or delete them. Another example is to grant users access to manage certain entities from the overview, but not all entities."

I added the #11's suggestions.
Please review.

Thanks @quietone, agreed that your suggestion is clearer 👍

Since no one picked this up for a month don't see an issue with rpayanm working on it.

Just FYI with the novice tag we try and leave for more newer users @rpayanm
.
Appears feedback has been addressed so will RTBC.

Thanks for making the changes for #11. However, 5 and 6 in there changed the text as well. This also needs someone familiar with this system to read those changes and confirm they are correct. It is possible that my adjustments inadvertently changed the meaning.

Setting back to NR.

@tstoeckler believe you implemented the admin_permission ticket, mind confirming the text.

Looks good to me. Since I didn't suggest this specific text and also didn't roll the patch, I think I can RTBC.

This is a nice start, so thank you for that, but I think the wording for the collection_permission doesn't address the feedback from #2953566-53: Allow entities to specify a "collection permission" → . I agree with @gabesullice's comment there that:

1. The collection permission should not control access to JSON:API collections.
2. The documentation for the collection permission should make that clear.

So, I'm tagging this for review from a jsonapi maintainer.

effulgentsia → credited gabesullice → .

Adding @gabesullice to this issue to notify him since he wrote that comment in the other issue.

This is unfortunately complicated by the fact that it's a common request that site owners want to deny access to the /jsonapi/user/user URL to anonymous users, so one could easily think that once there's a collection_permission on the User entity type, that taking that permission away from the Anonymous role would accomplish that.

Elevating to framework manager after 1 month.

@effulgentsia (a framework manager) explicitly asked for subsystem maintainer review on this issue and moved it down from RTBC in order to do so, so substituting back to a framework manager doesn't help move this issue forwards.

Posted in slack #contribute today just fyi.

Didn't see there were new threads opened.