Coding standards

Please review!

Thank you for the patch, Suresh!
There are still 11 coding standard errors...

I would avoid making changes to code that will probably be removed: #3122497: Class name doesn't match filename
That will save us from having to review those changes time and again.

Likewise, some of these changes are already part of other pending patches, like #2791129: Vote rollover settings are missing the "Never" option and #3136360: Add support for Drush 9+. Those should be committed first. Coding standards are a low priority task, and should not be breaking other patches that are fixing bugs or fixing broken features. It's going to be a lot of work to re-roll all those other patches.

+   * @var \Drupal\Core\Annotation\Translationoptional
    *
-   * @var \Drupal\Core\Annotation\Translation (optional)

What is this?

- *
+ * {@inheritDoc}

Should be lower case. In many places.

+ * Delete votes for deleted entity everywhere in database.
Should be "Deletes"

You missed things that should be changed like @param bool|true $update

I'm changing the status of this to "Postponed" because it's too disruptive at the moment until the pending issues in the queue get resolved.

Most of the other changes have gone in - let's see if we can finish this off ...

Here's a re-roll of #10 against the current HEAD, with additional fixes added and no-longer-applicable fixes removed.

I think we can reasonable fix a few more here.

I have reviewed the patch from #14 and most of the coding standards errors are removed.
However there are still some left so I have attempted to fix them in the appended patch.
Please review.

Hi @jernejmramor:

I deliberately did not change the array type hints in function/method calls because that represents an API change, which is generally not allowed between minor point releases because that will break and/or cause problems for other modules like Fivestar which use the Voting API.

Like I said in #12, the MongoDB section of the votingapi.api.php documentation needs to be removed/rewritten - see the linked related issue #3122497: Class name doesn't match filename. I don't think it makes sense to spend effort on "improving" it when it is wrong and outdated - instead the effort should be spent on addressing that other issue.

There are a number of other problems that will exist even after this patch - most notably the line lengths in all the .txt files. I left out those line length changes because they are very messy and make these other changes difficult to review. I intend to post a second patch in this issue to take care of those separately, and after that we can see if any of the few remaining issues need to be addressed.

Committed #16.

Here's a patch to fix the line lengths in .txt files. The easiest way to review the changes is to apply the patch on your local system then do a git diff --word-diff which will show you just the actual changes, ignoring changes to whitespace (new lines, etc.). Because most of this patch is changes to whitespace, you will see only a small number of changes to spelling and punctuation.

Committed #18 - it only affects documentation.

Hi,

I reviewed the patch and the module, for 8.x-3.x, I am getting this issues.

urvasi@urvasi-Inspiron-15-3552:/var/www/html/contribution/d8_cont/web/modules/contrib$ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig votingapi-3003096/

FILE: ...tribution/d8_cont/web/modules/contrib/votingapi-3003096/votingapi.api.php
-------------------------------------------------------------------------------
FOUND 12 ERRORS AND 3 WARNINGS AFFECTING 13 LINES
-------------------------------------------------------------------------------
  19 | ERROR   | [ ] Type hint "array" missing for $results
  83 | ERROR   | [ ] Type hint "array" missing for $data
 139 | ERROR   | [ ] Doc comment short description must be on a single line,
     |         |     further text should be a separate paragraph
 143 | ERROR   | [ ] Doc comment is empty
 146 | ERROR   | [ ] Class name doesn't match filename; expected "class
     |         |     votingapi.api"
 146 | ERROR   | [ ] Class name must use UpperCamel naming without underscores
 146 | WARNING | [ ] Class name must be prefixed with the project name
     |         |     "Votingapi"
 151 | ERROR   | [ ] Missing parameter type
 161 | ERROR   | [ ] Missing parameter type
 164 | ERROR   | [ ] Missing parameter type
 174 | ERROR   | [ ] Missing parameter type
 176 | ERROR   | [ ] Missing parameter type
 180 | ERROR   | [ ] Return type missing for @return tag in function comment
 200 | WARNING | [x] 'TODO.' should match the format '@todo Fix problem X
     |         |     here.'
 203 | WARNING | [x] 'TODO.' should match the format '@todo Fix problem X
     |         |     here.'
-------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------


FILE: .../contribution/d8_cont/web/modules/contrib/votingapi-3003096/CHANGELOG.txt
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 31 | WARNING | Line exceeds 80 characters; contains 81 characters
-------------------------------------------------------------------------------


FILE: ...dules/contrib/votingapi-3003096/tests/src/Functional/VoteDeletionTest.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-------------------------------------------------------------------------------
  93 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and
     |         | $this->t() instead
 100 | WARNING | t() calls should be avoided in classes, use
     |         | \Drupal\Core\StringTranslation\StringTranslationTrait and
     |         | $this->t() instead
-------------------------------------------------------------------------------


FILE: ...ion/d8_cont/web/modules/contrib/votingapi-3003096/src/Entity/VoteType.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 102 | WARNING | [x] 'TODO: needed?' should match the format '@todo Fix
     |         |     problem X here.'
-------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------


FILE: ...n/d8_cont/web/modules/contrib/votingapi-3003096/src/Entity/VoteResult.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 8 | WARNING | [x] Unused use statement
-------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------


FILE: ...cont/web/modules/contrib/votingapi-3003096/src/VoteResultFunctionBase.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-------------------------------------------------------------------------------
 16 | WARNING | Only string literals should be passed to t() where possible
 23 | WARNING | Only string literals should be passed to t() where possible
-------------------------------------------------------------------------------


FILE: ...eb/modules/contrib/votingapi-3003096/src/VoteTypeAccessControlHandler.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 19 | WARNING | Possible useless method overriding detected
-------------------------------------------------------------------------------


FILE: ...t/web/modules/contrib/votingapi-3003096/src/VoteResultFunctionManager.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 179 | WARNING | Unused variable $id.
-------------------------------------------------------------------------------


FILE: .../web/modules/contrib/votingapi-3003096/src/Commands/VotingApiCommands.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 11 WARNINGS AFFECTING 9 LINES
-------------------------------------------------------------------------------
  76 | WARNING | \Drupal calls should be avoided in classes, use dependency
     |         | injection instead
  91 | WARNING | \Drupal calls should be avoided in classes, use dependency
     |         | injection instead
 115 | WARNING | \Drupal calls should be avoided in classes, use dependency
     |         | injection instead
 116 | WARNING | \Drupal calls should be avoided in classes, use dependency
     |         | injection instead
 152 | WARNING | Unused variable $cache.
 152 | WARNING | \Drupal calls should be avoided in classes, use dependency
     |         | injection instead
 155 | WARNING | Unused variable $votes.
 155 | WARNING | \Drupal calls should be avoided in classes, use dependency
     |         | injection instead
 159 | WARNING | \Drupal calls should be avoided in classes, use dependency
     |         | injection instead
 162 | WARNING | \Drupal calls should be avoided in classes, use dependency
     |         | injection instead
 178 | WARNING | \Drupal calls should be avoided in classes, use dependency
     |         | injection instead
-------------------------------------------------------------------------------


FILE: ...web/modules/contrib/votingapi-3003096/src/VoteResultFunctionInterface.php
-------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------
 40 | ERROR | Type hint "array" missing for $votes
-------------------------------------------------------------------------------


FILE: ...w/html/contribution/d8_cont/web/modules/contrib/votingapi-3003096/API.txt
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
-------------------------------------------------------------------------------
 152 | WARNING | Line exceeds 80 characters; contains 83 characters
 154 | WARNING | Line exceeds 80 characters; contains 82 characters
 158 | WARNING | Line exceeds 80 characters; contains 92 characters
 168 | WARNING | Line exceeds 80 characters; contains 105 characters
-------------------------------------------------------------------------------

Time: 2.34 secs; Memory: 12MB

I will work on this and will provide a patch.

Thanks

I resolved all errors, except these.

urvasi@urvasi-Inspiron-15-3552:/var/www/html/contribution/d8_cont/web/modules/contrib$ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig votingapi-3003096/

FILE: ...tribution/d8_cont/web/modules/contrib/votingapi-3003096/votingapi.api.php
-------------------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 149 | ERROR   | Class name doesn't match filename; expected "class
     |         | votingapi.api"
 149 | ERROR   | Class name must use UpperCamel naming without underscores
 149 | WARNING | Class name must be prefixed with the project name "Votingapi"
-------------------------------------------------------------------------------


FILE: ...cont/web/modules/contrib/votingapi-3003096/src/VoteResultFunctionBase.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-------------------------------------------------------------------------------
 16 | WARNING | Only string literals should be passed to t() where possible
 23 | WARNING | Only string literals should be passed to t() where possible
-------------------------------------------------------------------------------


FILE: ...eb/modules/contrib/votingapi-3003096/src/VoteTypeAccessControlHandler.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------
 19 | WARNING | Possible useless method overriding detected
-------------------------------------------------------------------------------


FILE: .../web/modules/contrib/votingapi-3003096/src/Commands/VotingApiCommands.php
-------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
-------------------------------------------------------------------------------
 142 | WARNING | \Drupal calls should be avoided in classes, use dependency
     |         | injection instead
 203 | WARNING | Unused variable $cache.
 206 | WARNING | Unused variable $votes.
-------------------------------------------------------------------------------

Time: 2.44 secs; Memory: 12MB

Providing a patch.

Please review this patch.

Thanks

The last patch had some trailing whitespaces on some lines.
Also, a new violation was introduced since #24. Only these are left:

FILE: votingapi/src/VoteResultFunctionBase.php
-----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------
 16 | WARNING | Only string literals should be passed to t() where possible
 23 | WARNING | Only string literals should be passed to t() where possible
-----------------------------------------------------------------------------------------------


FILE: votingapi/src/VoteTypeAccessControlHandler.php
-----------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------
 19 | WARNING | Possible useless method overriding detected
-----------------------------------------------------------------------------------------------------


FILE: votingapi/votingapi.api.php
-----------------------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------
 149 | ERROR   | Class name must use UpperCamel naming without underscores
 149 | ERROR   | Class name doesn't match filename; expected "class votingapi.api"
 149 | WARNING | Class name must be prefixed with the project name "Votingapi"
-----------------------------------------------------------------------------------

I will review it.

I did the patch review and according to what I read in the description and comments #12 and #16, it is not necessary for the changes to be applied in the MongoDB section of the votesapi.api.php, if it is right, the coding standards changes in this part should be reverted.

The reason we address Coding Standards is NOT simply to satisfy the automated DrupalCI checks - those have false positives and sometimes suggest wrong changes. They are suggestions, and guidelines, and not absolute rules.

The whole point of Coding Standards is to make the code better, not just to make arbitrary DrupalCI warnings go away. It's perfectly fine to leave things unaddressed. For example "Only string literals should be passed to t() where possible", should be looked at then if it is not "possible" the code should NOT be changed. Yes, the CS warning will remain, and that's OK.

I think there are like only 3 lines in this patch that address coding standards. All the rest should NOT be done, or should be done in a separate issue. For example, dependency injection in the Drush command class is not a coding standard. It's also not needed - it's not wrong, but it's also not useful unless someone wants to write tests for the Drush commands. Since the Drush commands are just thin wrappers around Votingapi API functions, the normal tests of the API functions covers most of the operations, so I deliberately left the boilerplate dependency injection out in order to make updates easier (won't have to keep changing the constructor and drush.services.yml every time a new command is added). And BTW the dependency injection is done wrong in the above patch because drush.services.yml wasn't modified. If you want to address dependency injection in the Drush command class, that should be a separate issue where we can discuss that and make sure it is done correctly.

Likewise, t() shouldn't be used in tests, and {@inheritdoc} is only for when you actually have documentation in a parent class that you can inherit, and changing TODO to @todo doesn't actually help anything - if you want to help then you should actually write the missing documentation.

And please read everything I said above - most of what I said seems to have been ignored in this patch.

Uploading file PHPCS - Apr 22th, 2023.txt with latest PHPCS review and generated patch #29.

I read notes from #28 several times and those are some considerations while creating this patch:

1. Drush Commands vs Dependecy Injection: I added Ignore Flags on all so we don't need to go trough this discussion again.
2. Drupal t(): Also did the same: using Ignore Flags to remove it.
3. Voting API file: In the end of it there is a abstract example of Storage changing. I tried to fill all "Todo" flags there but also Ignoring that file made sense to me.
4. VoteTypeAccessControlHandler.php: DID NOTHING AT ALL. My honest opinion is that this should be deleted, so I prefer to investigate it in a new issue instead of this one.

TR said:

Yes, the CS warning will remain, and that's OK.

He did not suggest to add @codingStandardsIgnoreLine to all the lines that must not be changed. He said to ignore the warnings/errors for those lines that should not be changed, or that should be changed in a different issue.

diff --git a/entity.module b/entity.module
index b1a72ef..562e322 100644
--- a/entity.module
+++ b/entity.module

The last patch is not for this project, since there is not any entity.module file in this project.

+/**
+ * @file
+ */
+
 use Drupal\Core\Entity\ContentEntityInterface;

Leaving out the patch is not for this project, TR already explained that is the wrong type of changes to do. That documentation comment requires a short description, after the line containing @file; adding just the line containing @file to avoid a PHP_CodeSniffer warning/error is not the type of changes he will accept.

Please review patch.

Please ignore #38.

Please review patch.

No, and I really don't feel like repeating myself.

 /**
- * Voting API's vote storage can be overridden by setting the
- * 'votingapi_vote_storage' state variable to an alternative class.
+ * Voting API's.
  */
-\Drupal::state()->set('votingapi_vote_storage', 'Mongodb_VoteStorage');
+// Vote storage can be overridden by .
+// setting the 'votingapi_vote_storage'state variable to an alternative class.
+\Drupal::state()->set('votingapi_vote_storage', 'VotingApi');

Replacing a full sentence with Voting API's is not a correct change, as it is not correct to split a sentence in a phrase and a sentence.
Furthermore, why is the class name changed?

+   * @param int $vote
    *   instance of VotingApi_Vote.
    */
   public function addVote(&$vote) {

If $vote is an object, it cannot be an integer.