Problem/Motivation

Running PHPCS in the module I figured out that there are several sniff violations that needs to be fixed in order to achieve Drupal's best practices in code standards.

Steps to reproduce

1. Run phpcs in the file src/Controller/WeMegaMenuAdminController.php:

phpcs --standard=Drupal,DrupalPractice --extensions=php [name of the file]

Proposed resolution

Fix the indicated errors in the command output.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#24 we_megamenu_phpcs.png14.25 KBxaviemirmon
#19 AfterapplyinglatestPatch.png99.15 KBroberttabigue
#18 3247955-phpcs.patch10.84 KBsakthi_dev
#16 AfterapplyingtheMR.png501.9 KBroberttabigue
#16 Beforeapplyingdiff.png584.62 KBroberttabigue
#6 recent-cs.png16.2 KBlucienchalom
#4 post-2.png171.16 KBgabrielda
#4 post-1.png169.07 KBgabrielda
#4 pre-3.png85.13 KBgabrielda
#4 pre-2.png175.44 KBgabrielda
#4 pre-1.png163.47 KBgabrielda

Issue fork we_megamenu-3247955

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

gabrielda created an issue. See original summary.

gabrielda’s picture

gabrielda
He/Him
commented

I will work on this issue today

gabrielda opened merge request !5

gabrielda’s picture

gabrielda
He/Him
commented
Assigned: gabrielda » Unassigned
Status: Active » Needs review
StatusFileSize
new pre-1.png163.47 KB
new pre-2.png175.44 KB
new pre-3.png85.13 KB
new post-1.png169.07 KB
new post-2.png171.16 KB

When I started working in this issue, the errors indicated were the ones indicated in images pre-1, pre-2 and pre-3. Now, the status is the one indicated in the images post-1 and post -2
Please review it.

Cheers,
Gabriel

lucienchalom’s picture

lucienchalom commented
Assigned: Unassigned » lucienchalom
Status: Needs review » Needs work

I looked at the code, it is okay but I will continue with it, on the $POST super global and t() injections.

lucienchalom’s picture

lucienchalom commented
StatusFileSize
new recent-cs.png16.2 KB

all the Post superglobal variables have been cleaned, and the 3 t().
the phpcs only show an unused variable and one dependency injection.
will keep working on them.

lucienchalom’s picture

lucienchalom commented
Assigned: lucienchalom » Unassigned
Status: Needs work » Needs review

I did the last dependency injection and the
phpcs --standard=Drupal,DrupalPractice --extensions=php
returns clean.

Thank you

Matheus Macario’s picture

Matheus Macario commented
Assigned: Unassigned » Matheus Macario
Matheus Macario’s picture

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

Review done on last update made by @lucienchalom about the dependency injections and no errors popped out.

rjjakes’s picture

rjjakes commented
Status: Reviewed & tested by the community » Needs work

The latest MR now has conflicts that need resolving.

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

shivam_tiwari opened merge request !24

shivam_tiwari’s picture

shivam_tiwari commented
Status: Needs work » Needs review
rjjakes’s picture

rjjakes commented
Status: Needs review » Needs work

@shivam_tiwari - you've just committed the patch file to the repo. That's not a valid way of working.

shivam_tiwari’s picture

shivam_tiwari commented
Status: Needs work » Needs review
roberttabigue’s picture

roberttabigue commented
Status: Needs review » Needs work
StatusFileSize
new Beforeapplyingdiff.png584.62 KB
new AfterapplyingtheMR.png501.9 KB

Hi,

I can still reproduce the issue after applying the latest fix to the module Drupal Mega Menu against 8.x-1.x-dev and with Drupal core version 9.5.6.

Please see the attached screenshot for reference.

sakthi_dev’s picture

sakthi_dev commented
Assigned: Unassigned » sakthi_dev
sakthi_dev’s picture

sakthi_dev commented
Assigned: sakthi_dev » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3247955-phpcs.patch10.84 KB

Created patch. Please review.

roberttabigue’s picture

roberttabigue commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new AfterapplyinglatestPatch.png99.15 KB

Hi @sakthi_dev

Confirmed fixed the issue after applying Patch #17.

See the screenshot for reference.

Thanks.

rjjakes’s picture

rjjakes commented
Status: Reviewed & tested by the community » Needs work

The MR needs rebasing.

sourabhjain’s picture

sourabhjain
Primary language English
commented
Assigned: Unassigned » sourabhjain

Let me rebase the MR.

sourabhjain’s picture

sourabhjain
Primary language English
commented
Assigned: sourabhjain » Unassigned
lucienchalom’s picture

lucienchalom commented
Status: Needs work » Needs review

I did a rebase on the branch, but I recomend a full review since some changes are ages old.

xaviemirmon’s picture

xaviemirmon
Primary language English
Location Brighton
commented
StatusFileSize
new we_megamenu_phpcs.png14.25 KB

@lucienchalom LGTM I've just rerun PHPCS and it's not reporting any problems.

xaviemirmon’s picture

xaviemirmon
Primary language English
Location Brighton
commented
Status: Needs review » Reviewed & tested by the community

  • rjjakes committed 6f04cf9e on 8.x-1.x authored by gabrielda 
    Issue #3247955 by lucienchalom, gabrielda, shivam_tiwari, sakthi_dev,...
rjjakes’s picture

rjjakes commented
Status: Reviewed & tested by the community » Fixed

Merged to 1.x

Status: Fixed » Closed (fixed)

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