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.

CommentFileSizeAuthor
#12 interdiff_2-12.txt1.67 KBsamitk
#12 3335994-12.patch1.55 KBsamitk
#2 3335994-2.patch1.57 KBsamitk
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:

Comments

samit.310@gmail.com created an issue. See original summary.

samitk’s picture

Assigned: samitk » Unassigned
Status: Needs work » Needs review
StatusFileSize
new1.57 KB

Above warnings are fixed.

sahilgidwani’s picture

Assigned: Unassigned » sahilgidwani

I will review patch and will update the status once done.
Also, please try to follow the MR workflow for new issues.

sahilgidwani’s picture

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

I have checked and reviewed patch and it works perfectly for me.
Moving it to RTBC.

charchil khandelwal’s picture

Assigned: Unassigned » charchil khandelwal

charchil khandelwal’s picture

Assigned: charchil khandelwal » Unassigned

Created MR for this issue.
Please review.

elimw’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -, -

Rather than calling the class "BrowsersyncConstants", please rename it to "BrowsersyncHelper" and remove the "final" keyword from the class declaration.

aditi saraf’s picture

Assigned: Unassigned » aditi saraf

i'll work on this ..

elimw’s picture

Category: Bug report » Task
aditi saraf’s picture

Assigned: aditi saraf » Unassigned
samitk’s picture

Status: Needs work » Needs review
StatusFileSize
new1.55 KB
new1.67 KB

Hi @elimw,

changes has been made. Please review.

avpaderno’s picture

Title: Drupal Coding Standards Issues | phpcs » Fix the issues reported by phpcs
Priority: Normal » Minor
Status: Needs review » Needs work
+/**
+ * Block Token Constants.
+ */

That 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.

elimw’s picture

Please make all changes to "Issue fork browsersync-3335994" rather than the old patch file method.

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

avpaderno’s picture

avpaderno’s picture

Status: Needs work » Needs review

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

pray_12’s picture

Fixed Line indented incorrectly for browsersync.module.

cleavinjosh’s picture

Hi @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,yml

➜  browsersync git:(main) ✗ curl https://git.drupalcode.org/project/browsersync/-/merge_requests/2.diff | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8165    0  8165    0     0  18933      0 --:--:-- --:--:-- --:--:-- 18944
patching file .gitlab-ci.yml
patching file browsersync.module
patching file src/BrowserSyncConstants.php
➜  browsersync git:(main) ✗ ..
➜  contrib git:(main) ✗ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml browsersync

FILE: /Users/interns/Demo-site/drupal-orgissue/web/modules/contrib/browsersync/browsersync.info.yml
-------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------
 1 | WARNING | Remove "project" from the info file, it will be added by drupal.org packaging automatically
 1 | WARNING | Remove "datestamp" from the info file, it will be added by drupal.org packaging automatically
 1 | WARNING | Remove "version" from the info file, it will be added by drupal.org packaging automatically
-------------------------------------------------------------------------------------------------------------

Time: 190ms; Memory: 10MB

➜  contrib git:(main) ✗

Thank you.

cleavinjosh’s picture

Status: Needs review » Needs work
cleavinjosh’s picture

Status: Needs work » Reviewed & tested by the community

Hi @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.

➜  browsersync git:(3.0.0-beta2) curl https://git.drupalcode.org/project/browsersync/-/merge_requests/2.diff | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8165    0  8165    0     0  27680      0 --:--:-- --:--:-- --:--:-- 27677
patching file .gitlab-ci.yml
patching file browsersync.module
patching file src/BrowserSyncConstants.php
➜  browsersync git:(3.0.0-beta2) ✗ ..
➜  contrib phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml browsersync
➜  contrib

Thank you.

avpaderno’s picture

There is no need to test a merge request locally, since GitLab CI reports PHP_CodeSniffer errors/warnings.