Fix 'Drupal.Commenting.VariableComment.IncorrectVarType' coding standard

optional should go to the first line, @var short description.

1. Patch needs re-roll (due to changes to phpcs.xml.dist).

2. These are issues picked up by phpcs after I've applied #2:

FILE: .../docker-core/drupal/core/lib/Drupal/Component/Gettext/PoItem.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 53 | ERROR | [x] Expected "stringorarray" but found "string or array"
    |       |     for @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...es/docker-core/drupal/core/lib/Drupal/Core/Annotation/Action.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 44 | ERROR | [x] Expected "stringoptional" but found "string (optional)"
    |       |     for @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...cker-core/drupal/core/lib/Drupal/Core/Annotation/QueueWorker.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 59 | ERROR | [x] Expected "arrayoptional" but found "array (optional)"
    |       |     for @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...er-core/drupal/core/lib/Drupal/Core/Layout/Annotation/Layout.php
----------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
----------------------------------------------------------------------
  74 | ERROR | [x] Expected "stringoptional" but found "string
     |       |     (optional)" for @var tag in member variable comment
     |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
  87 | ERROR | [x] Expected "stringoptional" but found "string
     |       |     (optional)" for @var tag in member variable comment
     |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
  96 | ERROR | [x] Expected "stringoptional" but found "string
     |       |     (optional)" for @var tag in member variable comment
     |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
 103 | ERROR | [x] Expected "stringoptional" but found "string
     |       |     (optional)" for @var tag in member variable comment
     |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
 110 | ERROR | [x] Expected "stringoptional" but found "string
     |       |     (optional)" for @var tag in member variable comment
     |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
 117 | ERROR | [x] Expected "string[][]optional" but found "string[][]
     |       |     optional" for @var tag in member variable comment
     |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ore/drupal/core/lib/Drupal/Core/Field/Annotation/FieldWidget.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 76 | ERROR | [x] Expected "intoptional" but found "int (optional)" for
    |       |     @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: .../drupal/core/lib/Drupal/Core/Field/Annotation/FieldFormatter.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 71 | ERROR | [x] Expected "intoptional" but found "int optional" for
    |       |     @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...e/lib/Drupal/Core/Entity/Annotation/EntityReferenceSelection.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 61 | ERROR | [x] Expected "arrayoptional" but found "array (optional)"
    |       |     for @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...er-core/drupal/core/modules/rest/src/Annotation/RestResource.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 46 | ERROR | [x] Expected "stringoptional" but found "string (optional)"
    |       |     for @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...pal/core/modules/migrate/src/Annotation/MigrateProcessPlugin.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 44 | ERROR | [x] Expected "booloptional" but found "bool (optional)" for
    |       |     @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...docker-core/drupal/core/modules/filter/src/Annotation/Filter.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------
 53 | ERROR | [x] Expected "\Drupal\Core\Annotation\Translationoptional"
    |       |     but found "\Drupal\Core\Annotation\Translation (optional)"
    |       |     for @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
 60 | ERROR | [x] Expected "intoptional" but found "int (optional)" for
    |       |     @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
 67 | ERROR | [x] Expected "booloptional" but found "bool (optional)" for
    |       |     @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
 74 | ERROR | [x] Expected "arrayoptional" but found "array (optional)"
    |       |     for @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...er-core/drupal/core/modules/image/src/Annotation/ImageEffect.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 51 | ERROR | [x] Expected "\Drupal\Core\Annotation\Translationoptional"
    |       |     but found "\Drupal\Core\Annotation\Translation (optional)"
    |       |     for @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: .../core/modules/views/src/Annotation/ViewsPluginAnnotationBase.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 16 | ERROR | [x] Expected "booloptional" but found "bool (optional)" for
    |       |     @var tag in member variable comment
    |       |     (Drupal.Commenting.VariableComment.IncorrectVarType)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

3. We must move (optional) from @var line to comment.

Do you think you could upload an interdiff as well?

That's more than enough :)

After manual review I think this is good to go and since composer phpcs raises no errors, I am changing the status to RTBC. Thanks.

I looked this over; the scope makes total sense:

• It fixes our incorrect (optional) use on @var.
• It adds paragraphs where needed where the docblock currently complies with our one-line summary rule and the added (optional)
• It does not change docs that already don't meet the one-line summary.

+++ b/core/modules/migrate/tests/src/Unit/MigrateTestCase.php
@@ -29,7 +29,7 @@
   /**
    * Local store for mocking setStatus()/getStatus().
    *
-   * @var \Drupal\migrate\Plugin\MigrationInterface::STATUS_*
+   * @var int
    */
   protected $migrationStatus = MigrationInterface::STATUS_IDLE;

For this one, I think we should add some @see to the constants alluded to with the asterisk. Otherwise we're losing some information from the docblock.

Thanks

Given that last time we touched at least one of STATUS_* constants was in 2013 I think it is safe to just list them all.

Back to RTBC.

#2909366: Fix 'Drupal.Commenting.VariableComment.EmptyVar' coding standard got in so this one need reroll.

Yep, back to RTBC!

Just a note that the fixes here conflict heavily with #2903027: Fix 'Drupal.Commenting.DocComment.ShortNotCapital' coding standard for callbacks. I think this one is on a better track, but it's possible we should have both issues wait on sorting it out.

https://www.drupal.org/node/1354#var does not yet specify a standard for indicating a @var is optional. I think the pattern in this patch makes sense because it matches what we do for @param and @return docs, but we should probably:

• At least, update the handbook for the pattern.
• Maybe file a coding standards project issue. It's such a small thing though...

The first thing would delay this issue by at least two weeks I think and I don't know whether we need a whole process for it. Maybe this one should also be an issue where we split out the obvious typos from dealing with the optional stuff, so we have two issues, one which is blocked on coding standards feedback and one which is not.

Thoughts?

My thoughts:

1. I tried to reach out to Coder maintainers before regarding #2909371: Fix 'Drupal.Commenting.VariableComment.VarOrder' coding standard, which is postponed on Coder issue #2909391: Drupal.Commenting.VariableComment.VarOrder - @var before @code, but no echo from their side. That makes me assume it will take longer than 2 weeks to finish issue that depends on coding standard change/coder project.
2. Number of changes in latest patch is significant, thus not easy to review.
3. Frankly, I don't think it makes any sense to call @var (optional) in the first place. IMO all member variables are optional if not required by constructor.

That said I am ok with splitting this issue into two separate ones, and fix "obvious" errors first, such as typos, trailing characters, s/or/|, etc.

Friendly post #2902381: Revert phpcs.xml.dist changes from "Ensure that we're using the right ruleset for coding standards checking" reroll.
No feedback addressed. No credit needed.

I wouldn't split the issue. From my pov as soon as we merge new sniffs the better.

Then we should scope issues in chunks that are reasonably reviewable and separate them from stuff that coflicts with something else and needs further discussion. @zaporylie split off the easier parts of the related issue and they've landed already, so we don't need to think of them again.

It's also a good idea to follow a committer's scoping recommendation. ;)

See https://www.drupal.org/core/scope#context and https://www.drupal.org/core/scope#size.

Ok, since we * are * gonna split this issue into smaller chunks, I reviewed #23 I think following ones could be good material for child issues:

1. @var tag must not end with a full stop
2. Multiple types must be separated by a vertical bar
3. Use lower-case, PEAR style, primitive data types

We may want to create one more for `(optional)`, but tbh I agree with #24 on this one, and believe we should just move it to the same place we keep it already in many other places in Drupal's codebase (in front of docblock comment) and deal with it in #2903027: Fix 'Drupal.Commenting.DocComment.ShortNotCapital' coding standard for callbacks.

#2926120: @var tag must not end with a full stop
#2926121: Multiple data types must be separated by a vertical bar
#2926122: Use lower-case, PEAR style, primitive data types

#2926121: Multiple data types must be separated by a vertical bar was fixed

The sniff cannot be added without deciding on the code style for optional variables first: #2909370: Fix 'Drupal.Commenting.VariableComment.IncorrectVarType' coding standard

I agree that having any optional standard for @var doesn't really make any sense. It's not a param being passed in. I don't think it adds anything to the documentation. In fact for me it makes think "huh? I wonder what they mean by optional".

#2926120: @var tag must not end with a full stop is in

By comment of @alexpott in #32 I'm unpostponing this issue by #2909370: Fix 'Drupal.Commenting.VariableComment.IncorrectVarType' coding standard. In the same time I add one more split to remove all occurances of (optional) placed inline with @var type - #2999672: @var cannot be marked as optional

Exciting - #2926122: Use lower-case, PEAR style, primitive data types is in. Only one left!

Unpostponing (see #2999672-19: @var cannot be marked as optional)

@committer/people-with-The-Power: Please transfer the credits from #2999672: @var cannot be marked as optional to here. (I certainly don't have The Power...)

• osman
• BartoszUrbaniak at Linkfactory A/S

Adding credits as per #42

Thanks @alexpott

I reviewed the patch and I have only one thing I am not sure about and it is the sentence, "This property is optional in the annotation and can be left out.' It is great that this is there. It just think is is a bit long. Ah, I see just remove 'in the annotation' because this is in an annotation class. It would then be "This property is optional and can be left out." But how challenging is 'left out' for people whose first language is not English? Would "This property is optional and it does not need to be declared." or "This property is optional and can be omitted." be better?

@quiteone

That sentence originates from this comment: [#2999672:5] and was used in patches from that issue (here is the latest one).

I do agree it's not the most clear for non-native English speakers (like I am), personally I like the "This property is optional and it does not need to be declared." far better. (The one with "omitted" also might be a hurdle for us non-natives).

Going to run with that in a quick update on this MR.

@quietone:

I've just noticed we went for:

-   * (optional) The short title used in the views UI.
+   * An optional short title used in the views UI.

(https://git.drupalcode.org/project/drupal/-/merge_requests/540/diffs#note_21056)
in #2903027: Fix 'Drupal.Commenting.DocComment.ShortNotCapital' coding standard for callbacks.

I think that's even better? We have to make a decision either way, I guess.

Thanks @quietone!

- Merged latest 9.3.x into MR.
- Resolved all threads.

@Spokje, thanks.

I applied the patch locally and ran phpcs on the codebase. The sniff being added here did not detect anything. However, it did report an error in TourViewBuilder.php.

FILE: /var/www/html/core/modules/tour/src/TourViewBuilder.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 74 | ERROR | Comment indentation error, expected only 1 spaces
 78 | ERROR | Comment indentation error, expected only 1 spaces
----------------------------------------------------------------------

Those lines were recently committed in #3051766: Deprecate and replace jQuery Joyride (for tours). I guess that needs an issue.

But this MR is good to go.

I found the same problem with TourViewBuilder in #3224583: The testbot does not run PHPCS on all files when core/phpcs.xml.dist is changed.

Issue for the identation problem created here

Committed 53e5e33 and pushed to 9.3.x. Thanks!