Enable 'Drupal.Commenting.VariableComment.MissingVar' coding standard

Good patch
I don't see phpcs fails related to issue sniff after patch applying

But patch doesn't have changes for phpcs.xml.dist file
We have to enable fixed sniff for further checks. See step 1 from instruction.
Please add proper sniffs to this.

To avoid fails related to other subsniffs from Drupal.Commenting.VariableComment
you will need to exclude them.
Finally you will get smth like this:

+  <rule ref="../vendor/drupal/coder/coder_sniffer/Drupal/Sniffs/Commenting/VariableCommentSniff.php">
+    <!-- Sniff for: DuplicateVar -->
+    <exclude name="Drupal.Commenting.VariableComment.InlineVariableName"/>
+    <exclude name="Drupal.Commenting.VariableComment.EmptyVar"/>
+    <exclude name="Drupal.Commenting.VariableComment.IncorrectVarType"/>
+    <exclude name="Drupal.Commenting.VariableComment.MissingVar"/>
+    <exclude name="Drupal.Commenting.VariableComment.VarOrder"/>
+    <exclude name="Drupal.Commenting.VariableComment.WrongStyle"/>
+  </rule>

I reviewed the patch, added sniff section to phpcs.xml.dist as mentioned in #4 and fixed some minor issues I have found on the way.

Reroll + additions added since 9 October 2017 at 00:56

I went once again thoroughly through all changes introduced by last patch, and fixed places, I believe, were wrong (wrong order, incorrect data type, etc). Hope someone can find the time to make a final review.

1) I have justified all the changes to myself during a visual inspection of the patch

.. all looks good.

2) The phpcs.xml.dist was modified correctly

3) There are no remaining coding standard error in test runs output

I think this issue is ready.... everybodyWhoWorkedOnThis++

Thanks for the quick reroll.

I have rechecked ... everything I said in #10 is valid about this new patch.

I missed the "does the patch still apply" off my checklist

martin107--

I love it when our coding standards cleanups also make our code more usable. :)

This issue is a bit tricker to review than other coding standards patches since one has to look up the data types for things that don't provide defaults. The following is a mix of actual review points and notes to myself...

1. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -54,6 +54,8 @@ class DateTimePlus {
      /**
       * An array of possible date parts.
   +   *
   +   * @var array
       */
      protected static $dateParts = [
        'year',
   

   Is this a string[]?

2. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -66,41 +68,57 @@ class DateTimePlus {
      /**
       * The value of the language code passed to the constructor.
   +   *
   +   * @var string|null
       */
      protected $langcode = NULL;
   
   +++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
   @@ -24,6 +24,8 @@ class DrupalDateTime extends DateTimePlus {
      /**
   

   It's clear it can be NULL because there's a null there, and a machine name is obviously a string. So this is OK.

3. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -66,41 +68,57 @@ class DateTimePlus {
      /**
       * An array of errors encountered when creating this date.
   +   *
   +   * @var array
       */
      protected $errors = [];
   

   What kind of things are errors? Is this a render array, so array[]? Is it a list of strings, so string[]?

4. +++ b/core/lib/Drupal/Component/Gettext/PoItem.php
   @@ -13,7 +13,7 @@ class PoItem {
   -   * @car string
   +   * @var string
   

   A string car doesn't sound very useful. :)

5. +++ b/core/lib/Drupal/Component/Graph/Graph.php
   @@ -9,6 +9,8 @@ class Graph {
      /**
       * Holds the directed acyclic graph.
   +   *
   +   * @var array
       */
      protected $graph;
   
   +++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
   @@ -24,6 +24,8 @@ class DrupalDateTime extends DateTimePlus {
      /**
   

   Note to self: look these one up and confirm it's set in the constructor (i.e. never NULL). Scalar defaults for array values are often fragile, also, so we might find some followup issues for these.

6. +++ b/core/lib/Drupal/Core/Access/CheckProvider.php
   @@ -38,6 +38,8 @@ class CheckProvider implements CheckProviderInterface, ContainerAwareInterface {
      /**
       * Array of access checks which only will be run on the incoming request.
   +   *
   +   * @var array
       */
      protected $checksNeedsRequest = [];
   

   What kinds of things are these? Sounds like maybe objects of some access check type?

7. +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
   @@ -27,6 +27,8 @@ class PhpBackend implements CacheBackendInterface {
      /**
       * Array to store cache objects.
   +   *
   +   * @var array
       */
      protected $cache = [];
   

   Cache objects are presumably instances of some interface?

8. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
   @@ -14,6 +14,8 @@ class ConfigEntityType extends EntityType implements ConfigEntityTypeInterface {
      /**
       * The config prefix set in the configuration entity type annotation.
       *
   +   * @var string
   +   *
       * @see \Drupal\Core\Config\Entity\ConfigEntityTypeInterface::getConfigPrefix()
       */
      protected $config_prefix;
   
   +++ b/core/lib/Drupal/Core/Database/Transaction.php
   @@ -42,6 +42,8 @@ class Transaction {
       * This is used to label the transaction savepoint. It will be overridden to
       * 'drupal_transaction' if there is no transaction depth.
   +   *
   +   * @var string
       */
      protected $name;
   

   Note to self: Sounds stringy but check, and check for NULL.

9. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
   @@ -11,6 +11,8 @@
      /**
       * Parsed elements.
   +   *
   +   * @var array
       */
      protected $elements;
   

   It's called ArrayElement but check, and check NULL. What is it an array of?

10. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
    @@ -40,6 +40,8 @@ class Connection extends DatabaseConnection {
        * In PostgreSQL, 'LIKE' is case-sensitive. ILKE should be used for
        * case-insensitive statements.
    +   *
    +   * @var array
        */
       protected static $postgresqlConditionOperatorMap = [
         'LIKE' => ['operator' => 'ILIKE'],
    

    Looks like it's a string[][]?

11. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
    @@ -52,6 +54,8 @@ class Connection extends DatabaseConnection {
    +   *
    +   * @var array
    

    string[]?

12. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
    @@ -61,6 +63,8 @@ class Condition implements ConditionInterface, \Countable {
    +   * @var string
    
    +++ b/core/lib/Drupal/Core/Database/Query/Merge.php
    @@ -66,6 +66,8 @@ class Merge extends Query implements ConditionInterface {
       /**
        * The table or subquery to be used for the condition.
    +   *
    +   * @var string
        */
       protected $conditionTable;
    

    Note to self: Sounds like a string and having no table (NULL) for the condition sounds like invalid SQL, but double-check.

13. +++ b/core/lib/Drupal/Core/Database/Query/SelectExtender.php
    @@ -25,11 +25,15 @@ class SelectExtender implements SelectInterface {
       /**
        * A unique identifier for this query object.
    +   *
    +   * @var string
        */
       protected $uniqueIdentifier;
    
    +++ b/core/lib/Drupal/Core/Database/Schema.php
    @@ -30,11 +32,15 @@
        * A unique identifier for this query object.
    +   *
    +   * @var string
        */
       protected $uniqueIdentifier;
    

    Note to self: Sounds like a string but double-check that it's never NULL.

14. +++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
    @@ -24,6 +24,8 @@ class DrupalDateTime extends DateTimePlus {
       /**
        * Format string translation cache.
    +   *
    +   * @var array
        */
       protected $formatTranslationCache;
    

    The docs say string but the type says array. One of these is wrong.

    ...Or, wait. Is it the "Translation cache of format strings"?

15. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -224,11 +224,15 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
       /**
        * The machine name of the entity type group.
    +   *
    +   * @var string
        */
       protected $group;
     
       /**
        * The human-readable name of the entity type group.
    +   *
    +   * @var string
        */
       protected $group_label;
    
    +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -224,11 +224,15 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
       /**
        * The machine name of the entity type group.
    +   *
    +   * @var string
        */
       protected $group;
     
       /**
        * The human-readable name of the entity type group.
    +   *
    +   * @var string
        */
       protected $group_label;
    

    Note to self: Obviously strings but check for null values.

16. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -66,9 +66,9 @@ class EntityViewBuilder extends EntityHandlerBase implements EntityHandlerInterf
        * The EntityViewDisplay objects created for individual field rendering.
        *
    -   * @see \Drupal\Core\Entity\EntityViewBuilder::getSingleFieldDisplay()
    +   * @var \Drupal\Core\Entity\Display\EntityViewDisplayInterface[]
        *
    -   * @param \Drupal\Core\Entity\Display\EntityViewDisplayInterface[]
    +   * @see \Drupal\Core\Entity\EntityViewBuilder::getSingleFieldDisplay()
        */
       protected $singleFieldDisplays;
    

    Note to self: I probably need to look these up.

...That's as far as I got before got some reviewer fatigue. This is one of the coding standards issues I'd suggest splitting up. My suggestion:

1. One patch of things that already are documented but have a typo or whatever, like @car and @param.
2. One patch of scalars that provide default values.
3. A followup patch of scalars that don't provide default values.
4. One patch of arrays that provide default values, correctly typed to what the array is. Usually we can be more specific than plain-old array
5. Whatever's left, and enable the rule.

Thanks!

So what do you prefer - one issue per item from list in #14 or split patch in #12 into several patch files and upload them all here?

I think it's easier to control this issue by creating separate issues for first four cases from #14 list and leave 5th one here, postponed on first four.

List of child issues:

• #2924779: Fix existing but somehow invalid @var annotation
• #2924780: Missing @var annotation for scalars that provide default values
• #2924781: Missing @var annotation for scalars that do not provide default values
• #2924782: Fix 'Drupal.Commenting.VariableComment.MissingVar' outside of Plugins

Two already landed, another two to go.

This is reroll of #12.

This patch includes everything else after

• #2924781-7: Missing @var annotation for scalars that do not provide default values and
• #2924782-10: Fix 'Drupal.Commenting.VariableComment.MissingVar' outside of Plugins

Re-rolling without <exclude name="Drupal.Commenting.VariableComment.MissingVar"/>

I failed to find a documentation on this actually.

In mixed types, if the bool option is only one of the values, should it be defined specifically, or simply defined as its type?

For example, if the variable can return only a string or false, should it be @var string|bool or @var string|false?

Your suggestion makes sense, as we're defining types. But I would love to read more on that if you have any resource on that.

Thank you for the documentation @mfernea, really good resource!

For convenience to others: https://www.drupal.org/node/1354#types

Marking this needs review per #2924782-14: Fix 'Drupal.Commenting.VariableComment.MissingVar' outside of Plugins

#2924781: Missing @var annotation for scalars that do not provide default values is in 🎉

The child issue to enable rule had too many changes. So I split that up and this will become the final one to enable the sniff and fix any stragglers when all the children are committed.

The child issue #2924782: Fix 'Drupal.Commenting.VariableComment.MissingVar' outside of Plugins is small enough to fix the remaining violations and enable the sniff. So, I am closing this as a duplicate of that one and moving credit.

Due to recent commits of coding standard fixes, there are too many changes to fix and enable this sniff in #2924782: Fix 'Drupal.Commenting.VariableComment.MissingVar' outside of Plugins. So, opening this up again to do the final enable and fixes.

Created MR with what should be the remaining fixes.

And postponing on the last child.

Closed sibling issue #2924782: Fix 'Drupal.Commenting.VariableComment.MissingVar' outside of Plugins as a duplicate and transferring credit here.

The child issues have been fixed, so time to fix the stragglers and enable the sniff.

I think this is ready, the rule is enabled and the remaining issues are fixed.

Committed b9f3fe7 and pushed to 11.x. Thanks!