Right now in module there are 100 errors and 25 warnings in 31 files.
In order to simplify review process this issue splited to several issues to fix specific phpcs rules.
Child issue with list of rules to fix:
#3411792: Fix phpcs Drupal.WhiteSpace, Squiz.WhiteSpace issues
- Drupal.WhiteSpace.ScopeIndent
- Drupal.WhiteSpace.OpenBracketSpacing
- Drupal.WhiteSpace.CloseBracketSpacing
- Squiz.WhiteSpace.FunctionSpacing
- Squiz.WhiteSpace.SuperfluousWhitespace
#3411794: Fix phpcs Drupal.Arrays.Array issues
- Drupal.Arrays.Array
- Drupal.Classes.UnusedUseStatement
- DrupalPractice.FunctionCalls.InsecureUnserialize
- SlevomatCodingStandard.PHP.ShortList
- Drupal.Semantics.FunctionT
#3411807: Fix phpcs Drupal.Files.LineLength issues
- Drupal.Files.LineLength
#3411808: Fix phpcs Drupal.Commenting issues
- Drupal.Commenting.DocComment
- Drupal.Commenting.FunctionComment
- Drupal.Commenting.InlineVariableComment
- Drupal.Commenting.ClassComment
- Drupal.Commenting.DataTypeNamespace
- Drupal.Commenting.Deprecated
- Drupal.Commenting.FileComment
- Drupal.Commenting.InlineComment
Command to get phpcs report:
phpcs --standard=Drupal,DrupalPractice --extensions=php,inc,module,install,info,test,profile,theme modules/custom/pathauto/
PHPCS report:
| Comment | File | Size | Author |
|---|---|---|---|
| #45 | Screenshot from 2024-07-09 11-31-45.png | 136.93 KB | riddhi.addweb |
| #44 | pathauto-2833680-43.patch | 29.07 KB | silvi.addweb |
| #42 | pathauto-2833680-42.patch | 28.98 KB | silvi.addweb |
| #41 | report.txt | 32.59 KB | nikolay shapovalov |
Issue fork pathauto-2833680
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:
- 2833680-coding-standard-fixes
changes, plain diff MR !26
- 2833680-coding-standards-for
compare
Comments
Comment #2
renatog commentedPatch submitted with the fixes.
Thanks.
Regards.
Comment #3
cebasqueira commentedSmall change on patch!
Comment #4
renatog commentedThank you very much @cebasqueira.
Fixed variable unused.
Regards.
Comment #5
cebasqueira commentedThanks @renatog, works fine... RTBC
Comment #6
berdirThanks, but this adds a lot of strange comments just to technically comply to the standards, can't accept that, see below.
this isn't very useful, an actual description would be nice.
that isn't really proper english anymore, we oculd leave out automatically instead, as generating kind of implies automatically.
those descriptions are pretty bogus, if we do write something, we need to have an actual description that makes sense.
this is not valid.
this neither.
same.
this change is also not correct, force is a lower-case key.
same for those things, it only makes sense to have a description if it's at least valid english
this doesn't make sesne.
this change doesn't make sense, this is commented out code.
this and more below are also bad, if we have a docblock it needs to be an actual sentence.
Comment #7
hgunicamp commentedContinuing the work of the patch 'pathauto-codingstandards-2833680-4.patch'.
Comment #8
hgunicamp commentedComment #10
hgunicamp commentedSorry. A typo when saving the file.
Comment #12
aldairsoares commentedI'm working on it.
Comment #13
aldairsoares commentedI worked on Coding Standards against 8.x-1.x-dev.
I ran all the tests and everything seemed to work.
Needs review :D
Comment #15
aldairsoares commentedThe error occured in the patch is from Javascript Functional test (PathautoLocaleTest::testLanguagePatterns).
The code on 8.x-1.x contained a return statement in line #161 which prevented to run the rest of the assertions. The error occurs in line #182.
I was not able to fix that therefore I am leaving this one for someone with better understanding of the module.
Comment #16
aldairsoares commentedComment #17
matheusmaciel commentedComment #19
urvashi_vora commentedI am working on this.
Comment #21
urvashi_vora commentedComment #22
vitorbs commentedHi, i founded some code standards errors, i will work on that.
Comment #23
vitorbs commentedComment #24
vitorbs commentedphpcs fixed
Comment #25
vitorbs commentedComment #26
alexanderj commentedi will review it
Comment #27
alexanderj commentedI did the review on the related code standards errors and they were fixed, but the 2 tests are still failing, so I believe it is still necessary to resolve them.
Comment #28
dmariano commentedI will on that!
Comment #29
dmariano commentedMerge conflicts resolved, but the test are failing.
Comment #30
dmariano commentedComment #31
jsricardo commentedI Will work on it
Comment #32
jsricardo commentedSorry, I couldn't identify the error.
as said in comment #28, tests keep failing
Comment #33
zkhan.aamir commentedComment #35
nikolay shapovalov commentedAs requested by @Berdir I am going to split this MR to small one, and create separate issues.
I create child issue: #3411792: Fix phpcs Drupal.WhiteSpace, Squiz.WhiteSpace issues.
Comment #36
nikolay shapovalov commentedFixed issue in the MR made during MR merge conflict fix.
And create second child issue #3411794: Fix phpcs Drupal.Arrays.Array issues
Comment #37
nikolay shapovalov commentedNow there are 5 issues, this should fix all phpcs errors:
#3411792: Fix phpcs Drupal.WhiteSpace, Squiz.WhiteSpace issues
#3411794: Fix phpcs Drupal.Arrays.Array issues
#3411804: Fix phpcs Drupal.Classes, DrupalPractice.FunctionCalls, SlevomatCodingStandard.PHP, Drupal.Semantics
#3411807: Fix phpcs Drupal.Files.LineLength issues
#3411808: Fix phpcs Drupal.Commenting issues
Comment #38
nikolay shapovalov commentedConvert this issue to Meta.
Update IS.
Comment #41
nikolay shapovalov commentedAdd fresh phpcs report to IS.
Comment #42
silvi.addweb commentedRe-roll the patch according to the latest code changes.
Comment #43
silvi.addweb commentedComment #44
silvi.addweb commentedI fixed another issue which is reported by phpcs.
Comment #45
riddhi.addweb commentedThe mentioned phpcs issue is resolved, & I have also checked and it is working as expected. I am attaching the Screenshots & doing RTBC for the same.
Comment #46
riddhi.addweb commentedComment #47
berdirThis is a meta issue with referenced issues.
Comment #49
andres alvarez commentedComment #50
andres alvarez commentedComment #51
mably commentedComment #53
mably commented