Fix remaining VariableComment.MissingVar annotation

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.

quietone → closed merge request !535

quietone → changed the visibility of the branch 2924782-missing-var-annotation to hidden.

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.

quietone → credited andriyun → .

quietone → credited martin107 → .

quietone → credited shkiper → .

quietone → credited xjm → .

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 → made their first commit to this issue’s fork.

@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.

@quieton 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 →