Should we also allow all the download any permissions for all file types?

Adding permission to edit and download own files for editors makes sense to me.

Should we also allow all the download any permissions for all file types?

As per Backdrop's usual cautious approach to permissions, I don't think this is a good idea.

@stpaultim this looks fine, but please see my comment for a slightly more efficient approach (saves you the use of a new variable and array_merge).

Another question. When I created the original PR to add a default Editor role, I specifically listed permissions for each available content type. This means that if a new content type is added to core, the permissions would neet to be updated to include it.

'create page content',
'edit own page content',
'edit any page content',
'delete own page content',
'create post content',
'edit own post content',
'edit any post content',
'delete own post content',
'create card content',
'edit own card content',
'edit any card content',
'delete own card content',

For these file type permissions, @klonos helped me automate it to include all available file types. If a new file type were added to core, it would be automatically included.

  $file_types = file_permissions_get_configured_types();
  foreach ($file_types as $file_type) {
    $file_type_permissions[] = "edit own $file_type files";
    $file_type_permissions[] = "download own $file_type files";
  }
 $editor_permissions = array_merge($editor_permissions,  $file_type_permissions);

Are there any concerns about this approach (or similar suggestion by @argiepiano)?

If this PR is merged, I'd be inclined to create a followup issue to do the same thing for content types.

For these file type permissions, @klonos helped me automate it to include all available file types. If a new file type were added to core, it would be automatically included.

Your PR in this issue does not automatically add the permission for a new file type. It only add permissions for file types that exist when you install Backdrop for the first time. Neither does my suggested change of code. My suggestion doesn't change the functionality of your PR at all - it just optimizes it.

I don't think it's a good idea to add code to automatically add these permissions for new file types. After all, someone can easily delete the Editor role, and what you are proposing would add permissions for a role that doesn't exist. Same concern with a D7 upgrade, which, by default, doesn't have an editor role.

@argiepiano Agreed. I think I was unclear. I meant to say that it automatically adds permissions for all file types available at the time of installation. It does nothing and should not do anything for file types added after installation.

In fact, doing it this way rather than hard-coding the file types will ONLY matter if a new file type is added to core in the future.

My question is really about whether or not there is any disadvantage to automatically setting this permission at time of installation for all available file types (which will only effect file types provided by core) or if there is any reason it is better to hard code these permissions?

Your PR in this issue does not automatically add the permission for a new file type. ...

Yes, what @stpaultim meant is that if a new file type was to be introduced to the types that are shipped out of the box with core (the same way we added cards to the content types that ship with core), then we won't need to have to remember to update these permissions here - it will be automatically included with the equivalent permissions that we allow for other file types.

...I actually suggested to @stpaultim that we ask this questions here, because I assumed that there might be a reason why we didn't do the same automation with content types when we introduced the new editor role in core. He amusingly said that most likely the reason for that would have been that it was an oversight, because the PR that introduced these permissions for the editor role was also filed by him, and suggested that most likely no one from the people that reviewed the PR back then might have thought to suggest that we automate things. I on the other hand have concerns that there might be an actual very good reason why we did things that way specifically, so I thought best to ask here and double-check with others.

@argiepiano I accepted your changes. Should be ready to review again.

I changed this from "Feature Request" to "Task", because I think this should be eligible for 1.28 - if we agree that it's a good idea. If we are going to do this, I think it would be good to do it now as a follow-up issue to: #4961 which is a new change to 1.28.