Regression in @ tag sniff TagGroupSpacing

I will investigate what the changes were between 8.3.6 and 8.3.9, then we can decide how to proceed.

The commit which has the changes is https://git.drupalcode.org/project/coder/commit/c8664df from issue #2947589: TagGroupSpacing coding space fix doesn't work as it should (committed between 8.3.6 and 8.3.7)

The change in question is in coder_sniffer/Drupal/Sniffs/Commenting/DocCommentSniff.php where there is switch from a 'negative' ignored set of tags

-        $ignoreTags   = [
-            '@code',
-            '@endcode',
-            '@see',

to a 'positive' list of checked tags

+        $checkTags    = [
+            '@param',
+            '@return',
+            '@throws',
+            '@ingroup',

However, the subsequent code change is subtly different. Before we had

-                && (in_array($currentTag, ['@param', '@return', '@throws']) === true
-                || in_array($previousTag, ['@param', '@return', '@throws']) === true)

which has been replaced with

+                && in_array($currentTag, $checkTags) === true
+                && in_array($previousTag, $checkTags) === true

The key difference is that previously the check was done if either $currentTag or $previousTag were in the list, but now it is only done if both $currentTag and $previousTag are in the list. Hence, as one of the tags in my example is @todo this is why the check was allowed to pass in the later version but failed in the earlier version. Is this the intended behaviour of the change?

If so, then we could add @todo into the list of $checkTags (1)

Or (2) if the intention is to make sure that the $checkTags list are always separated from other tags, then the logic can be changed back to what we had before:

                && (in_array($currentTag, $checkTags) === true
               || in_array($previousTag, $checkTags) === true)

I think that the list of $checkTags are the ones that specifically need to be in a separate group with spaces either side, regardless of what the 'other' adjacent tag is. So the correct solution would be (2)

The test input file DocCommentUnitTest.inc has

 * @param string $param
 *   Something incredibly useful.
 * @return bool
 *   Returns FALSE.
 * @throws Exception
 *   Thrown when $param is TRUE.
 * @ingroup sniffer
 * @deprecated

and the fixed file DocCommentUnitTest.inc.fixed has

 * @param string $param
 *   Something incredibly useful.
 *
 * @return bool
 *   Returns FALSE.
 *
 * @throws Exception
 *   Thrown when $param is TRUE.
 *
 * @ingroup sniffer
 * @deprecated

Note that @ingroup is present in the new $checkTags array, but it is allowed to be adjacent to the @deprecated tag. This shows that we have an inconsistency between the intention of the check and the result. If @ingroup should be separated by spaces either side then we need to allow for any tag to be the adjacent one, hence we should use the OR test I propose restoring #3 option 2, and also adjust the fixed results file.

Or if @ingroup does not need to be separated by blank lines either side, it should be removed from the $checkTags array, and then the test would fail, because we would get a fixed version looking like:

 * @param string $param
 *   Something incredibly useful.
 *
 * @return bool
 *   Returns FALSE.
 *
 * @throws Exception
 *   Thrown when $param is TRUE.
 * @ingroup sniffer
 * @deprecated

and the @throws would not be reported when it should be. So either way, I think the OR logic should be restored. It was probably an accident that the || got replaced by && and it was not noticed that the fixed file does not agree with the intention of the sniff.

I will raise a PR with the proposed logic fix, for review and discussion on GitHub

Thanks @Arkener.

We should probably determine if the tag is part of a comment of another tag and if this is the case, ignore it. This would allow us to revert the && change

That is exactly what I had just deduced from looking at my local Coder PHPunit test failures when running the tests with the change to use ||.

We could ensure that the tag column number matches the column of the $currentTag being tested. So this would ignore any @ tag within the comment. I will work on this fix if you think this is a reasonable solution.

Usefully, we already have $tokens[$firstTag] defined from earlier. So it is nice and easy to add:

             if ($pos > 0) {
+                if ($tokens[$tag]['column'] !== $tokens[$firstTag]['column']) {
+                    continue;
+                }

This fixes the problem and allows the desired change to use || instead of &&.

However, later in the sniff, where SpacingAfterTagGroup is being checked, we have the same problem. Now that there is accurate tag data in the $tagGroups array (previously it erroneously included all the inline code tags too) the $lastTag is correct, but we now detect a later inline @tag and get a complaint that the group does not have a blank line after it. Therefore this section also requires the 'column' check as above, so that only proper doc @tags are found and checked.

Created PR 128 https://github.com/pfrenssen/coder/pull/128

The tests pass now. All ready for review.

To fix this in core I have raised #3183656: Fix 'Drupal.Commenting.DocComment.TagGroupSpacing' coding standard [part 2]

Would you like me to add new test cases for Coder to highlight the existing behavior (in Coder 8.3.7 to 8.3.11) and show that this is now fixed (in what will become 8.3.12)?

I did not find a way to use the Coder dev version in drupal.org testing - see my comments #21 to #29 here and #3 here. So this patch is added solely for use in the core issue.

Unassigning