Better access permissions D8

Awesome, thanks for working on this, feedback below:

1. +++ b/src/Controller/EntityPrintController.php
   @@ -124,10 +123,24 @@ class EntityPrintController extends ControllerBase {
   +    if (AccessResult::allowedIfHasPermission($account, 'bypass entity print access')->isAllowed() && $entity = $this->entityTypeManager->getStorage($entity_type)->load($entity_id)) {
   

   We've already loaded the $entity right above and then called $bundle on it, I think we should simply do:

   if (!$entity = $this->entityTypeManager->getStorage($entity_type)->load(....)) {
     return FALSE;
   }
   

   Then we can not check $entity at all below and we'll know that $entity->bundle() isn't going to fail.

   All this proves we need more tests though, what happens if you add a test now to view an entity that doesn't exist?

2. +++ b/src/Controller/EntityPrintController.php
   @@ -124,10 +123,24 @@ class EntityPrintController extends ControllerBase {
   +    if (AccessResult::allowedIfHasPermission($account, 'entity print access type '.$entity_type)->isAllowed() && $entity = $this->entityTypeManager->getStorage($entity_type)->load($entity_id)) {
   

   Missing space between the "."

3. +++ b/src/Controller/EntityPrintController.php
   @@ -124,10 +123,24 @@ class EntityPrintController extends ControllerBase {
   +    if (AccessResult::allowedIfHasPermission($account, 'entity print access bundle '.$entity_bundle)->isAllowed() && $entity = $this->entityTypeManager->getStorage($entity_type)->load($entity_id)) {
   

   Missing space here too, plus remove the && $entity

4. +++ b/src/EntityPrintPermissions.php
   @@ -0,0 +1,74 @@
   +    $EntityTypes = \Drupal::entityTypeManager()->getDefinitions();
   +    $ContentEntityTypes = array();
   +    foreach($EntityTypes as $key => $EntityType) {
   +      $group = $EntityType->getGroup();
   

   Drupal 8 still uses lowercase underscore separated names for local variables. E.g. $entity_types, $content_entity_types.

5. +++ b/src/EntityPrintPermissions.php
   @@ -0,0 +1,74 @@
   +      $group = $EntityType->getGroup();
   +      if($group == 'content') {
   +        $ContentEntityTypes[$key] = $EntityType;
   +      }
   +    }
   

   You could probably use array_filter here, might simplify things.

6. +++ b/src/EntityPrintPermissions.php
   @@ -0,0 +1,74 @@
   +      $EntityTypeLabel = $ContentEntityType->get('label')->__toString();
   

   There should be a method called ->label() here. If you add a comment in PHPStorm in the format /** @var \Path\To\ContentEntityInterface $var_name */ it will allow you to autocomplete them.

7. +++ b/src/EntityPrintPermissions.php
   @@ -0,0 +1,74 @@
   +        'title' => t('Use entity print for all bundles of type "@entity_label"', array(
   ...
   +        'description' => t('Allow a user to use entity print to view the generated PDF.'),
   

   Can we use the wording we used in the D7 version? That was one of the things I changed on commit.

8. +++ b/src/EntityPrintPermissions.php
   @@ -0,0 +1,74 @@
   +        if (is_string($EntityTypeBundle['label'])) {
   +          $EntityTypeBundleLabel = $EntityTypeBundle['label'];
   +        }
   +        if (is_object($EntityTypeBundle['label'])) {
   +          $EntityTypeBundleLabel = $EntityTypeBundle['label']->__toString();
   +        }
   

   Again, i'd expect there to be a ->label() method, if not and this is some kind of translation object? You can just cast it to a string with (string) and handle this is one line.

9. +++ b/src/EntityPrintPermissions.php
   @@ -0,0 +1,74 @@
   +          'title' => t('Use entity print for bundle "@entity_label - @entity_bundle_label"', array(
   

   If you add "use StringTranslationTrait;" inside your class, you'll be able to use $this->t()

10. +++ b/src/Tests/EntityPrintTest.php
    +++ b/src/Tests/EntityPrintTest.php
    @@ -62,7 +62,7 @@ class EntityPrintTest extends WebTestBase {
    

    Lets add a test to a non-existent entity to catch the problem I mentioned above.

Any luck with this? Thanks for the work so far.

Un-assigning, going to try roll this into the next release in the next couple of weeks, feel free to pick it back up again if you have time otherwise i'll finish it off.

I tried to update the patch in #2 using @benjy's suggestions. I'm not sure what's going on with the tests locally. It seems like it might just be an error on my end, maybe something with the permissions. I'll look into it later. Lets see what testbot thinks.

trying to fix some of the test errors

Thanks for re-rolling this, a few comments to start with, i'll have to give it a proper test.

1. +++ b/entity_print.install
   @@ -54,3 +54,19 @@ function entity_print_requirements($phase) {
   +/*
   + * Upgrade path for the permission system.
   + */
   +function entity_print_update_7001() {
   

   This needs to be converted to the 8.x version, also need to test it.

2. +++ b/src/Controller/EntityPrintController.php
   @@ -2,7 +2,7 @@
    /**
     * @file
   - * Contains \Drupal\entity_print\Controller\EntityPrintController
   + * Contains \Drupal\entity_print\Controller\EntityPrintController.
     */
   

   Just delete this comment now, no longer part of the coding standards.

3. +++ b/src/Controller/EntityPrintController.php
   @@ -135,9 +135,26 @@ class EntityPrintController extends ControllerBase {
   +    }
   +    else {
   +      // Check if the user has the permission "bypass entity print access".
   

   Just remove this else, the statement above does a return anyway so no need for the nesting.

Okay, I made those changes except for testing hook_update_N(). I've never done that before. I'm a newer contributor. I checked the 7.x version of this module and didn't see a comparable test there to try to convert to Drupal 8. Am I missing something or is the test missing in both versions?

When I said test it I actually just meant a manual test, that's what i've done previously.
However, upgrade tests are possible in Drupal 8, if you want to have a go the instructions are here: https://www.drupal.org/node/2536494 might take you awhile though so completely fine with me if you'd rather leave it.

Thanks for working on this, final review below and then I think we're nearly there.

1. +++ b/src/EntityPrintPermissions.php
   @@ -0,0 +1,68 @@
   +    $entity_types = \Drupal::entityTypeManager()->getDefinitions();
   

   If we use ContainerInjectionInterface we can inject this.

2. +++ b/src/EntityPrintPermissions.php
   @@ -0,0 +1,68 @@
   +    foreach ($entity_types as $key => $entity_type) {
   +      $group = $entity_type->getGroup();
   +      if ($group == 'content') {
   +        $content_entity_types[$key] = $entity_type;
   +      }
   +    }
   

   I think array filter reads better here:

   $content_entity_types = array_filter(function($entity_type) { return $entity_type->getGroup() === 'content';}, $entity_types);
   

3. +++ b/src/EntityPrintPermissions.php
   @@ -0,0 +1,68 @@
   +      $entity_type_label = $content_entity_type->getLabel();
   ...
   +            '@entity_label' => $entity_type_label,
   

   No need to store this in a local variable, just call it inline.

4. +++ b/src/EntityPrintPermissions.php
   @@ -0,0 +1,68 @@
   +        'description' => $this->t('Allow a user to use entity print to view the generated PDF.'),
   ...
   +          'description' => $this->t('Allow a user to use entity print to view the generated PDF.'),
   

   Can we update the descriptions to accurately describe each permission?

5. +++ b/src/EntityPrintPermissions.php
   @@ -0,0 +1,68 @@
   +      $entity_type_bundles = \Drupal::service('entity_type.bundle.info')->getBundleInfo($entity_type_id);
   

   Can also inject this.

6. +++ b/src/EntityPrintPermissions.php
   @@ -0,0 +1,68 @@
   +        $entity_type_bundle_label = $entity_type_bundle['label'];
   

   Again, just inline this.

7. +++ b/src/Tests/EntityPrintTest.php
   @@ -101,14 +101,38 @@ class EntityPrintTest extends WebTestBase {
   +    // User with different bundle permission and entity view.
   +    $account = $this->drupalCreateUser(array('entity print access bundle user', 'access content'));
   

   Maybe we could add an extra test to ensure this user can print the user entity?

@benjy Ah, I understand now. I ran updb locally. It applied cleanly.

I, somehow, missed until this moment your comment #13. You must have added it as I was adding my comment #14. I will review your comments and make another patch sometime today. Thanks!

I'm pretty sure that I've incorporated all of your suggestions into this patch. If not, let me know. I wrote a test to check that that a User can print themself and it's passing. I injected what I could. It looks pretty spiffy now. Oh, also, I removed the permissions descriptions for now. I didn't think that the current ones were adding any value that weren't already present in the titles, and I couldn't think of a better set of descriptions. If you have any thoughts, I'd be happy to incorporate them. Thanks as always.

Great this looks good, i'll try do some manual testing in the next couple of days.

One point:

+++ b/entity_print.install
@@ -54,3 +54,19 @@ function entity_print_requirements($phase) {
+    if($role->hasPermission('entity print access')) {
+
+      // Set permission.
+      $role->grantPermission('bypass entity print access');
+      $role->save();

Do we need to revoke the old permission?

OK, I tested this and we do need to revoke the permission as well, although the config object had numeric keys after the upgrade, need to investigate that.

I fixed up some coding standards and also made a few changes in the access callback to ensure cachability data is maintained. It doesn't actually matter for this module because with PDF's the responses are uncacheable anyway but I think it still makes sense to do it right.

Sounds good. Do you prefer interdiffs on this project? I can include interdiffs in the future. I usually don't include interdiffs on my contrib work because it seems like very few maintainers care for them, as opposed to core maintainers.

OK, it was an issue in core, revokePermission() incorrectly preserves the array keys, not sure if it has any possible side effects but there is already an issue #2409129: Enforce order of permissions in config export

@jordanpagewhite it's just a habit i've gotten into working on core so much, interdiffs are handy when you have a big patch and you only want to review the difference. Doesn't matter so much here because most patches are small :)

If you're happy with my recent changes, i'll commit this tomorrow.

@benjy I only had a very brief look at it because I'm at work, but from that brief scan, it looks good to me. If you happen to have a second to explain what you did with the dependency injection of \Drupal\Core\StringTranslation\TranslationManager, I would be really interested in that. Thanks again for taking the time to work through this one with me. The permissions will be great, and the file looks nice now.

If I have a moment this evening, I will download your latest patch and make sure that everything applies cleanly.

Looks good! It applied cleanly. I fixed a typo in a comment.

Committed and pushed, thanks.