Hi ,

Please fix Coding Standarad issue.

GOTO https://pareview.sh : Enter 8.x-1.x branch url i.e. https://git.drupal.org/project/block_permissions 8.x-1.x

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

The block_permissions.module does not implement hook_help(). See https://www.drupal.org/docs/develop/documenting-your-project/module-docu... .
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 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.

FILE: .../drupal/pareviewsh/pareview_temp/src/BlockPermissionsPermissions.php
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
32 | ERROR | [ ] Parameter $block_manager is not described in comment
76 | ERROR | [x] Short array syntax must be used to define arrays
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...ndor/drupal/pareviewsh/pareview_temp/src/Routing/RouteSubscriber.php
--------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
--------------------------------------------------------------------------
20 | ERROR | [x] Short array syntax must be used to define arrays
27 | ERROR | [x] Short array syntax must be used to define arrays
34 | ERROR | [x] Short array syntax must be used to define arrays
41 | ERROR | [x] Short array syntax must be used to define arrays
48 | ERROR | [x] Short array syntax must be used to define arrays
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: ...areviewsh/pareview_temp/src/BlockPermissionsAccessControlHandler.php
--------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
--------------------------------------------------------------------------
98 | ERROR | [x] Short array syntax must be used to define arrays
111 | ERROR | [x] Return comment indentation must be 3 spaces, found 4
| | spaces
129 | ERROR | [x] Return comment indentation must be 3 spaces, found 4
| | spaces
148 | ERROR | [x] Return comment indentation must be 3 spaces, found 4
| | spaces
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: .../web/vendor/drupal/pareviewsh/pareview_temp/block_permissions.module
--------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------
8 | ERROR | [x] When importing a class with "use", do not include a
| | leading \
9 | ERROR | [x] When importing a class with "use", do not include a
| | leading \
10 | ERROR | [x] When importing a class with "use", do not include a
| | leading \
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Time: 949ms; Memory: 4Mb

CommentFileSizeAuthor
#13 block_permissions-cs_issues-3104768-13.patch9.8 KBsinn
#11 3104768-11.patch6.8 KBnishantghetiya
#8 using_dependency_injection-3104768-8.patch6.75 KBankush_03
#5 3104768-5.patch5.41 KBnishantghetiya

Comments

ankushgautam76@gmail.com created an issue. See original summary.

ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented
Category: Task » Bug report
ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented
Title: Coding Standarad issue » Coding Standarad issues
nishantghetiya’s picture

nishantghetiya commented
Assigned: Unassigned » nishantghetiya
nishantghetiya’s picture

nishantghetiya commented
Assigned: nishantghetiya » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3104768-5.patch5.41 KB

@ankushgautam76@gmail.com

Please check below patch for fix coding standard issue.

Thanks

abhisekmazumdar’s picture

abhisekmazumdar
he/him
Primary language English
Location India
commented
Status: Needs review » Needs work

Hi, @nishantghetiya thanks for fixing the coding standard issues. I have some suggestions to make.

+++ b/src/BlockPermissionsAccessControlHandler.php
@@ -95,7 +95,7 @@ class BlockPermissionsAccessControlHandler implements ContainerInjectionInterfac
+        \Drupal::logger('block_permissions')->error('User @user has the administer block settings permission but no access to the default theme @theme, this is required.', ['@user' => $account->getAccountName(), '@theme' => $theme]);

Using dependency injection will be a better option here, which will get covered under coding standards.
Ref: https://www.drupal.org/docs/8/api/logging-api/overview

ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented

@nishantghetiya

Add changes as suggested by @abhisekmazumdar and update the new patch.

ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented
Status: Needs work » Needs review
StatusFileSize
new using_dependency_injection-3104768-8.patch6.75 KB
ankush_03’s picture

ankush_03
Location Gurgaon, India 🇮🇳
commented

@nishantghetiya

I have applied your patch first, then add logger dependency and create a new patch, Kindly check, and review.

abhisekmazumdar’s picture

abhisekmazumdar
he/him
Primary language English
Location India
commented
Status: Needs review » Needs work

Hi, @ankushgautam76@gmail.com thanks for the updated patch.
But there still a few issues left to cover kindly run phpcs for the files.

Some example:
File: /src/BlockPermissionsAccessControlHandler.php
30 | ERROR | [x] Whitespace found at end of line
49 | ERROR | [ ] Parameter $logger_factory is not described in comment
72 | WARNING | [ ] \Drupal calls should be avoided in classes, use dependency injection instead
97 | WARNING | [ ] \Drupal calls should be avoided in classes, use dependency injection instead

nishantghetiya’s picture

nishantghetiya commented
StatusFileSize
new 3104768-11.patch6.8 KB

@abhisekmazumdar @ankushgautam76@gmail.com

I have updated above patch with a solution of phpcs warning. Please review.

abhisekmazumdar’s picture

abhisekmazumdar
he/him
Primary language English
Location India
commented
Status: Needs work » Reviewed & tested by the community

Hi @nishantghetiya
The patch looks good.

sinn’s picture

sinn commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new block_permissions-cs_issues-3104768-13.patch9.8 KB

Let's test patch.

  • sinn committed 48dacf0 on 8.x-1.x 
    Issue #3104768 by nishantghetiya, ankushgautam76@gmail.com, sinn: Coding...
sinn’s picture

sinn commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.