D8 Views Tag Access

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxandrewbelcher2650960git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

This looks pretty cool!

I haven't done a full review, but I have looked through the code and installed it and looked through the UI.

It works, is intuitive and will probably be quite useful for stopping users without views training accidentally breaking the site :)

@andrewbelcher, I have checked through your code, it's a great module with no real code issues. Just some small nitpicks here:

function views_tag_access_module_implements_alter(&$implementations, $hook) {
  if ($hook == 'entity_type_build' && isset($implementations['views_tag_access'])) {
    $group = $implementations['views_tag_access'];
    unset($implementations['views_tag_access']);
    $implementations['views_tag_access'] = $group;
  }
}

This function does some kind of magic, a comment here would be useful (in case someone else in future is maintaining the code :))

/**
 * Configure decoupled auth settings for this site.
 */
class ViewsTagAccessSettingsForm extends ConfigFormBase {

The class doc doesn't seem to match the class functionality.

Edit: also in views_tag_access.module, for easier readability, you don't need to use fully namespaced class names (e.g. \Drupal\Core\Access\AccessResult::neutral()) in code, you can just place a use \Drupal\Core\Access\AccessResult; statement at the top of the file after the @file docblock and then use the class name only.

This module is very useful and worked as advertised in my manual tests. A permission-based module such as this would be well-suited to automated tests though, so give some consideration to adding simple tests to make sure permissions settings are being respected.

You should probably mention in the documentation that in order for the permissions to have an affect, the "Administer views" permission needs to be disabled for the role that should be restricted.

A link to the configuration page from the modules page would be helpful. Add configure: views_tag_access.settings to views_tag_access.info.yml

Automated Review

There are a handful of (generally minor) coding standards issues. Evidently, "Namespaced classes, interfaces and traits should not begin with a file doc comment" is a fairly new change to the standards (see #2304909: Relax requirement for @file when using OO Class or Interface per file).

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

The text in the README documentation for the project is thorough, except for the item about the "Administer views" permission mentioned above. It would be helpful if some, or all, of the information from the readme file were included on the project description page also.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

@rlhawk: I think you forgot to change the status. Are there any blockers left or is this now RTBC?

Yes, RTBC, though I hope my suggestions about tests, documentation, and usability improvements will be considered.

Looking at this now. Will look at this as soon as packagist is back up.

Automated Review

Review of the 8.x-1.x branch (commit 2904809):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

FILE: ...Users/matt/PAR/pareview_temp/src/Access/ViewsTagAccessAccessCheck.php
---------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
---------------------------------------------------------------------------
  3 | ERROR | [x] Namespaced classes, interfaces and traits should not
    |       |     begin with a file doc comment
 25 | ERROR | [ ] Description for the @return value is missing
 41 | ERROR | [ ] If the line declaring an array spans longer than 80
    |       |     characters, each element should be broken into its own
    |       |     line
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/src/Routing/RouteSubscriber.php
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/src/ViewListBuilder.php
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/src/ViewsTagAccessHelper.php
---------------------------------------------------------------------------
FOUND 7 ERRORS AFFECTING 7 LINES
---------------------------------------------------------------------------
   3 | ERROR | [x] Namespaced classes, interfaces and traits should not
     |       |     begin with a file doc comment
  80 | ERROR | [ ] Description for the @return value is missing
  94 | ERROR | [ ] Description for the @return value is missing
 108 | ERROR | [ ] Description for the @return value is missing
 123 | ERROR | [ ] Description for the @return value is missing
 127 | ERROR | [ ] If the line declaring an array spans longer than 80
     |       |     characters, each element should be broken into its own
     |       |     line
 153 | ERROR | [ ] If the line declaring an array spans longer than 80
     |       |     characters, each element should be broken into its own
     |       |     line
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/src/ViewsTagAccessPermissions.php
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/src/ViewsTagAccessSettingsForm.php
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 3 | ERROR | [x] Namespaced classes, interfaces and traits should not
   |       |     begin with a file doc comment
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/views_tag_access.module
---------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 3 LINES
---------------------------------------------------------------------------
 39 | ERROR | [x] Namespaced classes/interfaces/traits should be
    |       |     referenced with use statements
 39 | ERROR | [x] Namespaced classes/interfaces/traits should be
    |       |     referenced with use statements
 52 | ERROR | [x] Namespaced classes/interfaces/traits should be
    |       |     referenced with use statements
 64 | ERROR | [x] Namespaced classes/interfaces/traits should be
    |       |     referenced with use statements
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

Time: 521ms; Memory: 7Mb

Manual Review

Don't worry about wrappng code to 80 cols, just comments.

You have a buch of fully qualified classes; use statements are more standard.

You have some places in OO code where you access \Drupal. Use DI instead.

Nifty module. Assigning to @klausi for a second look from a security perspective, but this looks good to me. If he doesn't have time w/in 7 days, I'll do the vetting.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

manual review:

1. views_tag_access_entity_create_access(): why is this hook needed? Please add a comment that teh "create views" permission is provided by your module and that you need to enforce that.
2. class ViewListBuilder: why do you override the class? Please add a class comment.

Otherwise looks good to me.

Thanks for your contribution, Andrew!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.