Better access permissions D7

Why don't we generate the permissions in hook_permission() by looping over the entity types and adding a "view entity print $type" permission?

Yeah I also thought about doing it in this way. But for me it feels like the community is more used to do it in the way I implemented it,

Sorry, I don't agree. Drupal provides permissions as the system to control access, I don't want to try introduce our own concept in a non-standard place.

There are already limitations in your approach, for example it cannot be controlled per role.

I think we should generate one permission per entity type and we'll also need an update hook to add the permissions to all users who have the "entity print access" permission currently.

Yeah, I think bundles would be good.

Thanks!, the patch doesn't apply for some reason, are you branching of 7.x-1.x?

Also, do you fancy trying to add some tests for this?

+++ docroot/sites/all/modules/modified/entity_print/entity_print.module	(revision )
@@ -245,20 +260,37 @@
+      'title' => t('Use entity print for all bundles of type "'.$entity['label'].'"'),
...
+        'title' => t('Use entity print for bundle "'.$entity['label'].' - '.$entity_bundle['label'].'"'),

t() supports placeholders, we should use them here.

This time created with command line git, variables for t() and "fancy" tests ;-)

"Do you fancy" is an expression that means, "if you want to do it" :) I wasn't referring to the tests :)

Patch still doesn't apply, once that's done and the testbot runs i'll review again.

Thanks for all your work on this, below is some feedback :)

1. +++ b/entity_print.module
   @@ -54,16 +54,31 @@ function entity_print_menu()  {
   +  // Check if the user is allowed to use entity print for all entity types and bundles.
   ...
   +  // Check if the user is allowed to use entity print for the currently viewed entity type.
   ...
   +  // Check if the user is allowed to use entity print for the currently viewed bundle.
   

   Coding standards say that comments need to be <80chars in length.

2. +++ b/entity_print.module
   @@ -54,16 +54,31 @@ function entity_print_menu()  {
   +  if (user_access('entity print access type '.$entity_type)) {
   

   Please add spaces around . to comply with coding standards.

3. +++ b/entity_print.module
   @@ -54,16 +54,31 @@ function entity_print_menu()  {
   +  if (user_access('entity print access bundle '.$entity->type)) {
   

   ->type doesn't always related to the bundle, use
   list($id, $vid, $bundle) entity_extract_ids()

4. +++ b/entity_print.module
   @@ -245,20 +260,44 @@ function entity_print_theme($existing, $type, $theme, $path) {
   +  $permissions['entity print access'] = array(
   

   I wonder if this should be called bypass entity print access now? We'd need an upgrade path then so i'm not too worried about that.

5. +++ b/tests/entity_print.test
   @@ -20,7 +20,7 @@ class EntityPrintTest extends DrupalWebTestCase {
   +      'description' => t('Entity print css and permission tests.'),
   

   The tests are looking great, nice work. Just one suggestion, we should add some negative tests as well, e.g. Assert that a user that can view the page bundle cannot view the article bundle, and a user that can view the user entity type cannot view the node entity type.

Great, looking good, just one more thing.

1. +++ b/entity_print.module
   @@ -54,16 +54,34 @@ function entity_print_menu()  {
   -
   -  return FALSE;
   

   Why do we remove this, don't we want to deny access in this case?

I think this is good now, I need to apply the patch and give it a test but other than that, i'll try commit later in the week.

Thanks for all your hard work on this, appreciated.

The Drupal 8 module was a straight port from D7 so it is likely pretty similar. I'd be more than happy to accept the same changes for the 8.x-1.x branch if you're want to give it a go.

Thanks for all your work here, i've just committed the patch. I made a few changes on commit (interdiff.txt attached), mainly just some wording to be more consistent with how the node module formats its permission strings as well.

Thanks again!

OK, well feel free to ask any questions.

by default Drupal doesn't provide permission like "view own content " for authenticated user. This will grant authenticated user with permission "Use Entity Print" to generate pdf of any content and thus leads to all or none scenario. So it would be nice to have "entityprint own content" permission.

Could you please open a new issue for your described permission. Thanks!