Fix 'Drupal.Commenting.VariableComment.MissingVar' outside of Plugins

I will work on this.:)

I checked the files for this error violation Drupal.Commenting.VariableComment.MissingVar and seems to be fine.

I only see a missing change in core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageTest.php for:

  /**
   * {inheritdoc}
   */
  protected $defaultCacheContexts = ['languages:language_interface', 'session', 'theme', 'url.path', 'url.query_args', 'user.permissions', 'user.roles:authenticated'];

Are you by any chance referring to core/modules/media/tests/src/Functional/MediaTranslationUITest.php actually?

+++ b/core/modules/media/tests/src/Functional/MediaTranslationUITest.php
@@ -16,6 +16,8 @@ class MediaTranslationUITest extends ContentTranslationUITestBase {
 
   /**
    * {inheritdoc}
+   *
+   * @var array
    */
   protected $defaultCacheContexts = [
     'languages:language_interface',
@@ -29,6 +31,8 @@ class MediaTranslationUITest extends ContentTranslationUITestBase {

Yes, indeed.

The best solution here is {@inheritdoc}, but that means we only need to add the "@". So, it's not part of this issue.

Adding the correct status.

I disagree with @mfernea - if the inheritdoc is broken we should be fixing it here because then we don't need to add the @var at all.

Also we should be more specific if we can be i.e. use @var string[] etc...

I made a start of reviewing for this... but all the fixes here need checking...

1. --- a/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   
   @@ -54,6 +54,8 @@ class DateTimePlus {
   +   *
   +   * @var array
   
   @@ -113,6 +115,8 @@ class DateTimePlus {
   +   *
   +   * @var array
   

   @var string[]

2. +++ b/core/lib/Drupal/Core/Access/CheckProvider.php
   @@ -38,6 +38,8 @@ class CheckProvider implements CheckProviderInterface, ContainerAwareInterface {
   +   *
   +   * @var array
   

   @var string[]

3. +++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
   @@ -23,6 +23,8 @@ class MemoryBackend implements CacheBackendInterface, CacheTagsInvalidatorInterf
   +   *
   +   * @var array
   
   +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
   @@ -28,6 +28,8 @@ class PhpBackend implements CacheBackendInterface {
   +   *
   +   * @var array
   

   @var object[]

4. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -68,6 +68,8 @@ class Connection extends DatabaseConnection {
   +   *
   +   * @var array
   

   string[]

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

   string[]

6. +++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
   @@ -37,6 +37,8 @@ class HtmlTag extends RenderElement {
   +   *
   +   * @var array
   

   string[]

7. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
   @@ -60,6 +60,8 @@ class UrlGenerator implements UrlGeneratorInterface {
   +   *
   +   * @var array
   

   string[]

8. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
   @@ -43,6 +43,8 @@ trait FunctionalTestSetupTrait {
   +   *
   +   * @var array
   

   string[]

9. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -30,6 +30,8 @@ class CommentTranslationUITest extends ContentTranslationUITestBase {
   +   *
   +   * @var array
   

   string[]

10. +++ b/core/modules/file/tests/src/Functional/Update/FileUpdateTest.php
    @@ -13,6 +13,8 @@ class FileUpdateTest extends UpdatePathTestBase {
       /**
        * Modules to enable after the database is loaded.
    +   *
    +   * @var array
        */
       protected static $modules = ['file'];
    

    Change to {@inheritdoc}

11. +++ b/core/modules/media/tests/src/Functional/MediaTranslationUITest.php
    @@ -16,6 +16,8 @@ class MediaTranslationUITest extends ContentTranslationUITestBase {
       /**
        * {inheritdoc}
    +   *
    +   * @var array
        */
       protected $defaultCacheContexts = [
    
    @@ -29,6 +31,8 @@ class MediaTranslationUITest extends ContentTranslationUITestBase {
       /**
        * {inheritdoc}
    +   *
    +   * @var array
        */
       public static $modules = [
    

    This change looks unnecessary the @var is on the parent.

12. +++ b/core/modules/migrate/tests/src/Unit/process/UrlEncodeTest.php
    @@ -15,6 +15,8 @@ class UrlEncodeTest extends MigrateTestCase {
       /**
        * @inheritdoc
    +   *
    +   * @var array
        */
       protected $migrationConfiguration = [
    

    Should be inheriting.

13. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
    @@ -13,6 +13,8 @@ class DrupalSqlBaseTest extends MigrateTestCase {
       /**
        * Define bare minimum migration configuration.
    +   *
    +   * @var array
        */
       protected $migrationConfiguration = [
    

    Use inheritdoc

14. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/d6/Drupal6SqlBaseTest.php
    @@ -18,6 +18,8 @@ class Drupal6SqlBaseTest extends MigrateTestCase {
       /**
        * Define bare minimum migration configuration.
    +   *
    +   * @var array
        */
       protected $migrationConfiguration = [
    

    inheritdoc

15. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/d6/VariableTranslationTest.php
    @@ -18,6 +18,8 @@ class VariableTranslationTest extends MigrateSqlSourceTestCase {
       /**
        * Define bare minimum migration configuration.
    +   *
    +   * @var array
        */
       protected $migrationConfiguration = [
    
    @@ -33,6 +35,8 @@ class VariableTranslationTest extends MigrateSqlSourceTestCase {
       /**
        * Expected results from the source.
    +   *
    +   * @var array
        */
       protected $expectedResults = [
    
    @@ -49,6 +53,8 @@ class VariableTranslationTest extends MigrateSqlSourceTestCase {
       /**
        * Database contents for tests.
    +   *
    +   * @var array
        */
       protected $databaseContents = [
    

    {@inheritdoc}

Indeed, I was wrong. I thought it should be fixed in a sibling issue, but it's not. Let's fix it here.

Rebased MR on 9.3.x.

Thanks @quietone for the review.

- Merged the latest 9.3.x into the MR.
- Found, added and fixed new offender.
- Resolved all but one thread.

• Rebased MR on 9.4.x-dev
• Merged latest commits

I was still concerned that the scope of the patch is greater than the Issue Summary so did some research.

I have gone back to look at the parent and the siblings of this issue. I understand that this has been split into child issues on the recommendation xjm made 4 years ago. However, if the missing @var work is being split into several issues then the sniff can't be added by each sibling. And that is true, the siblings are not enabling the sniff. In this case, it is the parent that will enable the sniff and pick up what was not covered by the other siblings or crept into the code base.

So, I think the sniff needs to be removed and changes to @var that are not for 'arrays that provide default values' need to be removed. That will result in two smaller patches and thus easier to review.

However, we are already here and this is the last sibling, maybe it is more efficient to continue on here? I can't decide.

I'd like another opinion.

Thanks @quietone.

Seeing this is indeed the last child, and the fact that the MR "only" (I use only lightly here, but most Coding Standards issues I've worked on included much more changes) contains ~50 changes, and the fact that splitting this issue up into 2 sub-issues would take a some time, I personally am a fan of:

- Not splitting this issue up: Mostly to save time, in my tiny mind, the review time will stay the same in either 1 or 2 issues. The main issue I see is that a bigger issue will take more time before it's reviewed and committed by a Core Committer/
- Enabling the sniff here: Don't do it here but do it in the parent issue opens the chances of new offenders creeping in. Seeing it's the last child issue, I can't see an advantage in enabling it in the parent, besides it being fully "according to the rules".

So, solely based on efficiency I'm a non-split advocate for this one. Willing to be convinced otherwise.

drops EUR 0.02

I too am not keen on someone spending time splitting this up, so let's move forward with this patch, unless a committer disagrees.

I'll work on reviewing the MR today.

Update IS

Thanks @quietone.

Needing a second opinion about the two remaining @var string[] or @var string[][] threads.

Since I got confused about string[] and string[][] I asked in #bugsmash. Everyone agreed with the answer posted by kim.pepper (abbreviated)

from what i've seen, string[] can be either a list or associative:

    ['a', 'b', 'c']  OR
    [ 'a' => 'a, 'b' => 'b', 'c' => 'c']

string[][] would imply:

    ['a' => ['a', 'b'], b => ['c']]

Some responses also pointed out that the properties should be documented as well. I think that is the goal but we need to get there incrementally. This is just a step and prevents MissingVar from happening in the future.

I am going to review the MR now using the above definition.

This time I went through the entire MR. I noted some things that are really out of scope. I should go back and resolve that but I need a break from this. Maybe tomorrow.

Setting to NW for the review of the MR.

Implementing sniffs are not bugs, they are tasks.

The failing test is not related to these changes, I am retesting.

Changing the scope to make the change set smaller and easier to review.

Reviewed the updates and changes appear correct.

MR has merge conflicts.

Rebased

Rebase seems good.

Rebased and changed the instance in \Drupal\FunctionalTests\Update\UpdatePathTestBase to an {@inheritdoc}.

MR has a merge conflict and a question about the change to phpcs.xml.dist.

Also, the release note snippet in the IS seems wrong, as we are enabling that in the parent issue - not here.

Rebased and had to fix mistakes so setting to NR.

Rebase seems good.

I think we should be changing phpcs.xml to enable the rule no? Also I think removing the @var when there is a default value is really odd.

@alexpott, thanks for the review.

Unfortunately, the scoping on this is unusual and the sniff can't be modified to cover this set of changes. However, I did enable the sniff for some file patterns. In the parent issue the remaining 50 errors in 32 files can be fixed and the sniff enable for all files.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

A straightforward rebase so restoring RTBC

This change confuses me because it's removing @var from stuff... also if I only apply the changes to the phpcs.xml.dist and run PHPCS the other error that's found is:

> phpcs --standard=core/phpcs.xml.dist --parallel="$( (nproc || sysctl -n hw.logicalcpu || echo 4) 2>/dev/null)" -- 'core'

FILE: /Volumes/dev/drupal/core/modules/block/tests/src/Kernel/NewDefaultThemeBlocksTest.php
-------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------
 37 | ERROR | Missing @var tag in member variable comment
-------------------------------------------------------------------------------------------

Time: 26.72 secs; Memory: 12MB

So the rule changes are not covering the changes being made here.

The 'Overview' in the MR does not reflect the actual changes and it quite confusing. There were also truly unexpected errors on my part that I can't quite figure out how they happened. So, I started over and it turns out that this can now enable the sniff as well.

So, I am closing the parent as a duplicate, changing the title and moving credit.

All the documentation changes look good to me.
The remark of @alexpott has been addressed.
For me it is RTBC.

Couple of minor issues in the MR, also needs a reroll

Thanks for working on this.

@kul.pratap, Welcome to Drupal! Thanks for working on this. I made a few comments in the MR. Also, This is failing the pre commit checks. It is good practice to Run core development checks locally before submitting a change.

There have been recent commits that are causing more errors. I don't think the sniff can be enabled here. This will have to go back to doing a subset of files. I'll look into that now.

@quietone Thank you for the feedback. I will review the comments in the MR and address them.

@kul.pratap, thanks. I just added another change to exclude files in 'Plugin' directories. That should help here.

Updating title for the new scope.

I'll work on an issue for the changes in the 'Plugin' directories.

@quietone Should I resolve the unresolved issues in the MR as mentioned by you?

@kul.pratap, Thanks for asking! The suggested changes to phpcs.xml.dist should exclude the files that have just started to cause violations from being checked by phpcs. Without those being checked that will bet this back on track to where only the feedback from larowlan and myself need to be resolved.

The files that have just started to cause the violations are going to be fixed in the parent issue, along with enabling the sniff on all of core. I've already set that up and postponed that on this issue.

I hope that all make sense.

@quietone Please review.

Pipeline has errors. Would recommend verifying that everything passes before putting into review please.

@kul.pratap, thanks for reverting those changes.

I was concerned that the number of files to change would make the MR here larger that our recommended size. So, I've updated phpcs.xml.dist to ignore Plugin files, which agrees with the title here. There are still about 25 files to edit.

More changes for recent commits.

Tried to resolve some threads but things got messy at some point. Left most open.

This is at least the second time I have had the gitlab MR UI disagree with the changes that are actually in the branch. It is very confusing.

We need to agree to the change for core/tests/Drupal/FunctionalTests/Installer/InstallerExistingConfigTestBase.php.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I meant to re-scope this but I accidentally created a new issue to divide up the work for this sniff.

I am closing as a duplicate an transferring credit to #2909372: Enable 'Drupal.Commenting.VariableComment.MissingVar' coding standard