Problem/Motivation
Getting following warnings.
FILE: /var/www/html/modules/contrib/browsersync/browsersync.module
-------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-------------------------------------------------------------------------------------
15 | WARNING | Global constants should not be used, move it to a class or interface
22 | WARNING | Global constants should not be used, move it to a class or interface
-------------------------------------------------------------------------------------
Time: 1.37 secs; Memory: 6MB
Steps to reproduce
Run following command
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml modules/contrib/browsersync/
Proposed resolution
Above warnings need to be fixed.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | interdiff_2-12.txt | 1.67 KB | samitk |
| #12 | 3335994-12.patch | 1.55 KB | samitk |
| #2 | 3335994-2.patch | 1.57 KB | samitk |
Issue fork browsersync-3335994
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:
Comments
Comment #2
samitk commentedAbove warnings are fixed.
Comment #3
sahilgidwani commentedI will review patch and will update the status once done.
Also, please try to follow the MR workflow for new issues.
Comment #4
sahilgidwani commentedI have checked and reviewed patch and it works perfectly for me.
Moving it to RTBC.
Comment #5
charchil khandelwal commentedComment #7
charchil khandelwal commentedCreated MR for this issue.
Please review.
Comment #8
elimw commentedRather than calling the class "BrowsersyncConstants", please rename it to "BrowsersyncHelper" and remove the "final" keyword from the class declaration.
Comment #9
aditi saraf commentedi'll work on this ..
Comment #10
elimw commentedComment #11
aditi saraf commentedComment #12
samitk commentedHi @elimw,
changes has been made. Please review.
Comment #13
avpadernoThat does not describe the class. Constants should not be capitalized, since it is not at the begin of a sentence.
I am not sure I would even call those constants Block Token constants, since they are used in the definition of the browsersync_snippet theme function.
Comment #14
elimw commentedPlease make all changes to "Issue fork browsersync-3335994" rather than the old patch file method.
Comment #16
avpadernoThe PHP_CodeSniffer warnings/errors still to fix are listed by GitLab CI.
Comment #17
avpadernoComment #19
pray_12 commentedFixed Line indented incorrectly for browsersync.module.
Comment #20
cleavinjosh commentedHi @pray_12,
I applied MR!2 and confirmed that the issues found on the browsersync.module file by the phpcs are fixed.
However, I am still encountering some issues when I run
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,ymlThank you.
Comment #22
cleavinjosh commentedComment #23
cleavinjosh commentedHi @pray_12,
I apologize for my previous testing. I discovered that the warnings I encountered were because I installed the module via composer.
I reapplied MR!2, it was applied smoothly and fixed the phpcs issues.
Thank you.
Comment #24
avpadernoThere is no need to test a merge request locally, since GitLab CI reports PHP_CodeSniffer errors/warnings.