[D7] Coding standards

Line length.

Thank you for wanting to help, but coding standards changes can be tricky because phpcs reports a lot of false problems and because making coding standards changes in the code can't improve functionality, but CAN break things. Because of this, you can't simply change the code just to make the errors go away - you have to understand first what phpcs is complaining about and why, and make an informed decision whether something should be changed or not, and decide HOW it should be changed. Not every reported violation should be fixed.

As I said in the issue summary:

If you would like to submit patches here, make sure they are restricted to one and only one type of standards violation - that will make them easier to review.

But this patch tries to address too many things, and it would take to long to point out every single problem then re-review the patch when you fixed all those problems. Please try to fix only one type of standards violation - you can submit other patches after that to fix other types.

Right at the start of the patch there's a problem - you modify fivestar.api.php to remove coding standards messages similar to "Hook implementations should not duplicate @return documentation". This is a false positive. The fivestar.api.php file is where all hooks are documented. This IS the primary hook documentation, this is NOT duplicate documentation. If you remove this documentation in the api.php file, then the Fivestar hooks won't have any documentation.

Also, right at the end of the patch there's a problem - you do this:

   /**
    * A user with permission to vote.
+   *
+   * @var voterUser
+   * The voter user.
    */
   protected $voterUser;
 
@@ -33,9 +42,14 @@ class FivestarBaseTestCase extends AJAXTestCase {
   public function setUp() {
     parent::setUp(array('fivestar', 'dblog'));
 
-    $type = $this->drupalCreateContentType(array('type' => 'test_node_type', 'name' => 'test_node_type'));
-    $this->adminUser = $this->drupalCreateUser(array('create test_node_type content', 'rate content'));
-    $this->voterUser = $this->drupalCreateUser(array('rate content'));
+    // $type = $this->drupalCreateContentType(array('type' => 'test_node_type',

Three things wrong with this:

1. Yes, the properties need to be documented with @var tags, but the @var should document the datatype of the variable, it should not be the name of the variable.
2. You add an additional unneeded comment after the @var. There should be nothing after the @var.
3. You comment out code. That should never be done, and in this case you are causing the test to fail because you commented out the creation of the content type which the test tries to use in the next two lines.

There are many other problems with the patch - like adding @inheritdoc to functions that don't/can't inherit documentation, Instead you should be writing actual documentation.

Also, when you do something like

-    'titleUser' => t('Your rating') . ': ',
-    'titleAverage' => t('Average') . ': ',
+    'titleUser' => t('Your rating:'),
+    'titleAverage' => t('Average:'),

you make the coding standards message disappear, but you also BREAK all the translations for this module because you've removed the old strings from the translation table and added new strings which don't have translations. That is not something that should be changed, especially since the D7 version of this module has been around for so long and D7 is close to obsolescence.

Please submit a new patch that only tries to address one type of coding standards message.

Well I don't know what to do with this - it keeps getting worse.
The latest MR has all sorts of new problems. It looks to me like it was forked from a version of Fivestar that is over 2.5 YEARS out of date. For example:

diff --git a/widgets/basic/basic-rtl.css b/widgets/basic/basic-rtl.css
old mode 100755
new mode 100644
index 7c802055b217ac652df0662cd337670ee9054e7d..7205a5dee3537088e30c4f51b616b76ae9c526ac
--- a/widgets/basic/basic-rtl.css
+++ b/widgets/basic/basic-rtl.css
@@ -1,10 +1,11 @@
 /* Adjust the background position when using this widget with a RTL langauge. */
-div.fivestar-widget-static .star span.on {
+.fivestar-basic div.fivestar-widget-static .star span.on {
   background-position: right -32px;
 }
-div.fivestar-widget div.on a {
+.fivestar-basic div.fivestar-widget div.on a {
   background-position: right -16px;
 }
-div.fivestar-widget div.hover a, div.rating div a:hover {
+.fivestar-basic div.fivestar-widget div.hover a,
+.fivestar-basic div.rating div a:hover {
   background-position: right -32px;
-}
\ No newline at end of file
+}

The permissions on that file were fixed (by me) by commit d9b735be8d6ac4038 on 3 May 2020.

Likewise, as far as I can tell ALL the changes to the CSS files are out of scope of this issue - those changes are NOT coding standards changes, they change the actual properties and selectors. These changes have not been discussed and do not belong here.

While improvements to the advanced help are appreciated, again those go far beyond a "coding standards" change. And they should be done in the CURRENT version of the module. The current version of the module needs to be fixed first, then those changes can be backported to the amost-obsolete D7 version. Fixing it in D7 first is a good way to waste effort, because that version (along with the improvements) will disappear soon.

There's also a lot of D8 code in there. For example the new file fivestar.php which has class Fivestar extends FormElement { - this is not Drupal 7 code.

Patch does not apply and patch introduces new coding standards problems (trailing whitespace).
Please post an interdiff with any patch so we can see what you changed.

Short array syntax can't be used in D7 because it doesn't work with PHP 5.3. Drupal 7 still supports PHP 5.3.

@apaderno: As you said in #18:

list() isn't a deprecated function, and the coding standards don't require that change.

and

-    return isset($return) ? $return : NULL;
+    return $return ?? NULL;

That change isn't required from the Drupal coding standards. Drupal 7 doesn't require PHP 7 as minimum PHP version; the null coalescing operator cannot be used.

Agreed, HOWEVER the results from PHPCS run by DrupalCI report these as errors. See for example https://www.drupal.org/pift-ci-job/2502899
DrupalCI is using sniffs that apply standards that haven't been adopted by Drupal and aren't enforced in Drupal core. This is an ongoing problem for contributed modules.

I consider those false positives, which should not be "fixed". As I said in #7 , when creating coding standards patches one has to "make an informed decision whether something should be changed or not, and decide HOW it should be changed. Not every reported violation should be fixed."

@lalitkyttn: Still working on it?