GitLab CI reports PHP_CodeSniffer errors/warnings which should be fixed.
| Comment | File | Size | Author |
|---|---|---|---|
| #29 | fixedphpcs.png | 44.55 KB | roberttabigue |
| #20 | 3349252-20.patch | 6.58 KB | nitin_lama |
| #17 | 3349252-17.patch | 7 KB | chaitanyadessai |
| #17 | patch.png | 57.92 KB | chaitanyadessai |
| #13 | 3349252-13.patch | 5.53 KB | hardik_patel_12 |
Issue fork logout_redirect-3349252
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:
- 3349252-phpcs
changes, plain diff MR !2
- 8.x-1.x
changes, plain diff MR !4
- logout_redirect-3349252
changes, plain diff MR !1
Comments
Comment #3
rassoni commentedFixed PHPCS issues and generate MR
https://git.drupalcode.org/project/logout_redirect/-/merge_requests/1
Please review.
Comment #4
avpadernoSince that line is changed, the change should also correctly spell URL and use redirect users instead of redirect user.
The usual comment is Hook implementations for [module name]. It does not need to explain the module purpose, which is already explained in its
hook_help()implementation.That comment can be removed, since it does not explain anything already understandable from the code.
Commented out code should be simply removed.
Comment #7
kunalgautam commentedComment #8
avpadernoIt is Hook implementations and the module name should be shown as shown on the top of this page.
The last comment is not necessary, since that is already clear from the code.
url is not the correct spelling of URL.
you want's to, while used in the original comment, is not grammatical.
redirect if browser back button is clicked after logout is missing an article, not using the possessive, and it would probably say after users logged out.
It should be The submission button, but that comment is not necessary, since that is evident from the code.
That comment should be placed some lines earlier, but it is not necessary, as that is understandable from the code itself.
Comment #9
kunalgautam commented@apaderno I have updated the MR accordingly please review it.
Comment #10
avpadernoThat sentence is still not grammatically correct. Those errors need to be corrected, since that comment is changed.
A correct sentence would be:
+ * Hook implementations for the logout_redirect module.The module name is not logout_redirect; that is its machine name. The module name is the one reported after Project: on the top of this page, which is the same name reported in the module's .info.yml file (the value for the
namekey).you want's to is not grammatical. Since that line is edited, that should be corrected too.
Comment #11
kunalgautam commentedComment #12
avpadernoThat could be grammatically correct, but it is not correct. It's not the user who wants to redirect, but the administrator user who is entering the URL.
The module name should be spelled using uppercase letters where the module name uses them.
It is the same mistake done for the previous change.
Comment #13
hardik_patel_12 commentedI have solved the warnings given by PHPCS and here is the patch.
Comment #14
avpadernoThe module name is Logout Redirect, not logout redirect. Capitalized letter must be written capitalized.
Furthermore, the
@filedescription for a module is just the last line.Comment #15
avpadernoFurthermore, running
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml ./, I get a slightly different report.Comment #16
yashaswi18 commentedI cloned and ran the command :
phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml logout_redirect/
I'm not seeing any errors except for this one.
Comment #17
chaitanyadessai commentedPlease review patch.
Comment #18
avpadernoIt is not necessary to remove the
coreline, if thecore_version_requirementline is not changed.That change is not correct, since to logout_redirect.settings is not a sentence on its own. It is part of the previous sentence.
The usual description for a module file is Hook implementations for the [module name] module. where [module name] is the module name reported in its .info.yml file.
Outside of functions or classes, the comment delimiter used is not
//. We use a single documentation comment per function/method.Verify the redirect field is not empty. is better.
Also, let's keep using merge requests.
Comment #19
nitin_lamaComment #20
nitin_lamaAddressed #18. Providing updated patch.
Comment #21
nitin_lamaPlease review. Thanks.
Comment #22
nitin_lamaComment #23
yashaswi18 commentedHello, applied MR!2, found one error remaining on running the command
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,ymlComment #26
cleavinjosh commentedHi @silvi.addweb,
I tried to apply MR!2 but was skipped.
Please check and advise.
Thank you.
Comment #27
avpadernoThe issue fork is eight commits ahead of the upstream repository. There is nothing that needs to be fixed in the MR.
Comment #28
avpadernoGitLab CI does not report any PHP_CodeSniffer warning/error which still needs to be fixed.
Comment #29
roberttabigue commentedHi,
I have applied the latest MR to the Logout Redirect module against 2.0.1 on Drupal 10 and confirmed all PHPCS errors have been fixed.
I ran this command on the module:
phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml logout_redirectPlease see the attached file for reference.
I'm moving this now to ‘RTBC’.
Thank you!
Comment #31
avpadernoComment #33
avpaderno