Hi !

When reviewing this module I ran into a couple of coding standards violations against 8.x-1.x:

1. Non static method should not be called statically

$permissions = $this::getPerMenuPermissions($account);

Should be:

$permissions = $this->getPerMenuPermissions($account);

2. Violation of SlevomatCodingStandard.PHP.ShortList (included in Drupal ruleset here)

list($menu) = explode(':', $key, 2);

Should be:

[$menu] = explode(':', $key, 2);

Attached is a proposed patch.

CommentFileSizeAuthor
#5 menu_admin_per_menu-coding_standards-3268189-5.patch3.53 KBfengtan
menu_admin_per_menu-coding_standards.patch3.51 KBfengtan

Issue fork menu_admin_per_menu-3268189

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

fengtan created an issue. See original summary.

kimberlly_amaral’s picture

kimberlly_amaral commented
Assigned: Unassigned » kimberlly_amaral

I'll review that.

kimberlly_amaral’s picture

kimberlly_amaral commented
Assigned: kimberlly_amaral » Unassigned
Status: Needs review » Reviewed & tested by the community

I applied the patch and tested the changes. I found no errors.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Reviewed & tested by the community » Needs work

Patch no longer applies.

fengtan’s picture

fengtan
Location Montreal, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new menu_admin_per_menu-coding_standards-3268189-5.patch3.53 KB

Thanks -- rerolled.

damiaosj made their first commit to this issue’s fork.

damiaosj’s picture

damiaosj commented
Assigned: Unassigned » damiaosj

Hi! Reviewing it!

damiaosj opened merge request !12

damiaosj’s picture

damiaosj commented
Assigned: damiaosj » Unassigned
Status: Needs review » Reviewed & tested by the community

Patch reviewed and it's all fine. There's other PHPCS warnings, but I think that should be worked on other issue.

I've also created another branch with the patch applied and opened a MR.

Changing the issue to RTBC :) .

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented

About the list($menu) vs [$menu], this was reverted in #3159649: version 8.x-1.3 get unexpected error because this gives errors in PHP 7.0.
But since we dropped support for Drupal 8 and only support 9 and 10, the minimum PHP version is 7.3 So I guess it's ok to replace this now.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented

  • JeroenT committed 80bc975 on 8.x-1.x authored by damiaosj 
    Issue #3268189 by fengtan, damiaosj, JeroenT: PHP...
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Reviewed & tested by the community » Fixed

Merged to 8.x-1.x. Thanks!

Status: Fixed » Closed (fixed)

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