It would be nice if we refactored the module code in accordance with the Drupal 8 coding standards.
This mainly includes the following:

  1. Add a description of files, functions, classes and methods.
  2. The correct naming of classes and variables.
  3. Indents and line breaks.

What do you think about it?

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

WalkingDexter created an issue. See original summary.

gbyte’s picture

Assigned: Unassigned » WalkingDexter

I am not religious about coding standards but I was under the impression that the 3 examples you mentioned are already in place. Well not all methods have their phpdoc comment, but all API methods should by now.

So not sure were you see the issue, maybe you can create a patch for one of the classes so I can see where my coding deviates from the standards.

Also it would be great if you could justify changes by pointing me to the relevant Drupal coding standard.

WalkingDexter’s picture

gbyte’s picture

Category: Plan » Task
Status: Active » Needs work

This patch is totally fine and these changes make sense. If you feel like it, go ahead and just push changes to 3.x-dev.

You also mentioned indents and line breaks, where do you see the problem? I believe you do not make code blocks within a method, whereas I like to separate logical chunks with line breaks - I believe both is acceptable; if you agree, we can leave it as is.

Feel free to push any non-breaking improvements to 3.x - it's your call whether to create an issue or just push. I don't mind reverting a change if I disagree so it's alright.

thalles’s picture

thalles’s picture

Status: Needs work » Needs review

  • WalkingDexter authored bd32bd4 on 8.x-3.x
    Issue #3018715 by WalkingDexter, thalles: Coding standards on...
WalkingDexter’s picture

Status: Needs review » Active

@thalles thank you for contributing! I made a few changes and committed.

WalkingDexter’s picture

Suresh Prabhu Parkala’s picture

Status: Active » Needs review
FileSize
111.62 KB

Please review!

rokzabukovec’s picture

The patch from #11 needed to be rerolled and I fixed all of the remaining coding standards issues.

Please review.

gbyte’s picture

Thanks @rokzabukovec; would you mind creating a merge request? That will make the review process much smoother.

rokzabukovec’s picture

No problem @gbyte. I opened a merge request so you can review the patch. I'm using the code sniffer tool with the Drupal standard set as a guideline. In this standard, every method should have a docblock and every parameter and return statement should have a description. It's a lot of change so you are going to probably need to change some descriptions, but these guidelines should be followed by every module or at least it's a good practice. That way the IDE can recognise the methods and offer some help when working with a module.

This should be at least a good starting point to make the module better.

gbyte’s picture

Status: Needs review » Needs work

We've made plenty of changes in the past week, the MR will have to be rerolled.

WalkingDexter’s picture

WalkingDexter’s picture

Status: Needs work » Needs review
FileSize
209.19 KB

Patch with a complete code refactoring in accordance with the current Drupal coding standards. I didn’t fix some points, as they relate to description of update hooks and interface translation. @gbyte please review after tests succeeded.

P.S. I was unable to create a new branch (for 4.x) in the current issue fork. Therefore, I am sending a patch.

  • gbyte committed 2d32f81 on 4.x authored by WalkingDexter
    Issue #3018715 by gbyte, rokzabukovec, WalkingDexter, Suresh Prabhu...
gbyte’s picture

Status: Needs review » Fixed

Great work! I changed a bunch of the method descriptions and caught a missing import that led to exceptions.

@WalkingDexter

I didn’t fix some points, as they relate to description of update hooks and interface translation.

What about interface translation? There is #3238405: Make new configuration entities translatable, any chance you could check the codebase for problems in regards to it? I am e.g not sure if calls to $entity->label() are supposed to be run through t(). Thanks in advance.

WalkingDexter’s picture

Status: Fixed » Needs work

@gbyte

To check standards, I use the PHP_CodeSniffer tool and the ruleset from the Coder project. There is also an online version, but it doesn't work yet. Also note that the PHP_CodeSniffer integrates with PhpStorm.

To avoid reopening this issue, please use the above tools.

I checked out the code again locally in the 4.x branch. The result is as follows:

FILE: tests/src/Functional/SimplesitemapTest.php
------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------
 614 | ERROR | [ ] @throws comment must be on the next line
 615 | ERROR | [x] Separate the @throws and @dataProvider sections by a blank line.
------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------


FILE: src/Logger.php
-----------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------
 124 | WARNING | Only string literals should be passed to t() where possible
-----------------------------------------------------------------------------


FILE: src/Queue/BatchTrait.php
----------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------
 43 | WARNING | Only string literals should be passed to t() where possible
----------------------------------------------------------------------------


FILE: src/SimpleSitemapListBuilder.php
----------------------------------------------------------------------------
FOUND 0 ERRORS AND 7 WARNINGS AFFECTING 5 LINES
----------------------------------------------------------------------------
 41 | WARNING | Only string literals should be passed to t() where possible
 41 | WARNING | Only string literals should be passed to t() where possible
 42 | WARNING | Only string literals should be passed to t() where possible
 42 | WARNING | Only string literals should be passed to t() where possible
 56 | WARNING | Only string literals should be passed to t() where possible
 58 | WARNING | Only string literals should be passed to t() where possible
 59 | WARNING | Only string literals should be passed to t() where possible
----------------------------------------------------------------------------


FILE: src/Form/CustomLinksForm.php
-----------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------
 101 | WARNING | Only string literals should be passed to t() where possible
-----------------------------------------------------------------------------


FILE: src/Form/EntitiesForm.php
---------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
---------------------------------------------------------------------------------------------------------------------------------------------
 154 | WARNING | Do not concatenate strings to translatable strings, they should be part of the t() argument and you should use placeholders
 171 | ERROR   | Concatenating translatable strings is not allowed, use placeholders instead and only one string literal
---------------------------------------------------------------------------------------------------------------------------------------------


FILE: src/Form/FormHelper.php
--------------------------------------------------------------------------------------
FOUND 16 ERRORS AND 6 WARNINGS AFFECTING 18 LINES
--------------------------------------------------------------------------------------
 390 | WARNING | [ ] Only string literals should be passed to t() where possible
 403 | ERROR   | [x] Array indentation error, expected 10 spaces but found 12
 404 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 405 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 405 | WARNING | [ ] Only string literals should be passed to t() where possible
 406 | ERROR   | [x] Array closing indentation error, expected 12 spaces but found 10
 407 | ERROR   | [x] Array indentation error, expected 10 spaces but found 12
 408 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 409 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 409 | WARNING | [ ] Only string literals should be passed to t() where possible
 410 | ERROR   | [x] Array closing indentation error, expected 12 spaces but found 10
 412 | ERROR   | [x] Array indentation error, expected 10 spaces but found 12
 413 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 414 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 414 | WARNING | [ ] Only string literals should be passed to t() where possible
 415 | ERROR   | [x] Array closing indentation error, expected 12 spaces but found 10
 416 | ERROR   | [x] Array indentation error, expected 10 spaces but found 12
 417 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 418 | ERROR   | [x] Array indentation error, expected 14 spaces but found 12
 418 | WARNING | [ ] Only string literals should be passed to t() where possible
 419 | ERROR   | [x] Array closing indentation error, expected 12 spaces but found 10
 634 | WARNING | [ ] Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 16 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------


FILE: src/Form/SettingsForm.php
-----------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------
 165 | WARNING | Only string literals should be passed to t() where possible
-----------------------------------------------------------------------------


FILE: src/SimpleSitemapTypeListBuilder.php
----------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------------
 31 | WARNING | Only string literals should be passed to t() where possible
 32 | WARNING | Only string literals should be passed to t() where possible
 33 | WARNING | Only string literals should be passed to t() where possible
 36 | WARNING | Only string literals should be passed to t() where possible
----------------------------------------------------------------------------


FILE: simple_sitemap.install
---------------------------------------------------------------------------------------------------------------------
FOUND 4 ERRORS AND 7 WARNINGS AFFECTING 11 LINES
---------------------------------------------------------------------------------------------------------------------
 297 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 314 | WARNING | Line exceeds 80 characters; contains 81 characters
 315 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 340 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 382 | WARNING | Line exceeds 80 characters; contains 84 characters
 398 | WARNING | Line exceeds 80 characters; contains 81 characters
 434 | WARNING | Line exceeds 80 characters; contains 86 characters
 536 | WARNING | Line exceeds 80 characters; contains 86 characters
 604 | WARNING | Line exceeds 80 characters; contains 89 characters
 722 | ERROR   | Doc comment short description must be on a single line, further text should be a separate paragraph
 846 | WARNING | Line exceeds 80 characters; contains 92 characters
---------------------------------------------------------------------------------------------------------------------


FILE: modules/simple_sitemap_engines/src/Form/SimplesitemapEnginesForm.php
-----------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------
 124 | WARNING | Only string literals should be passed to t() where possible
-----------------------------------------------------------------------------

Interface translation problems are points related to the t() function. This can be partially solved by #3238405: Make new configuration entities translatable. I think that other points also need to be fixed.

gbyte’s picture

@WalkingDexter Feel free to look into this issue and fix the remaining warnings.

I'll try my best, but cannot promise to adhere to some of these standards - I find commit-level checking that forces you to break up lines kills programming flow big time.

WalkingDexter’s picture

Status: Needs work » Fixed

The main points have been fixed for now.

Status: Fixed » Closed (fixed)

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