@inheritdoc without { } is treated as a tag and gives missleading message

This patch seems to do the job.

Updating Version to 8.3.x. I'm on that version right now, the original post is from 2017... so I assume 8.3.x is what we should use nowadays. But the patch is probably compatible with anything, adding 3 lines.

Thanks, can you file a pull request against https://github.com/pfrenssen/coder and link it here? Please also add a test case.

Can I ask a dumb question - what exactly is the problem or enhancement that this patch fixes? Is it that without the early return added here, Coder continues and tries to check other sniffs against the comment block and gives false results? If so, would we not be seeing lots of those already in current code? Or is it just an efficiency measure to exit early? When we know this, the tests will become easy to write, and we can also update the issue summary and title which don't give much clue.

If I remember correctly, the issue just was: if I add "@inheritdoc", I do that because there is already a doc-comment somewhere, I don't want to replicate that.

Without the return there, it did his job and told you "please write a comment!", which isn't the point of "@inheritdoc" :D (maybe even @inheritDoc ... should've added that in there as well.)

if I add "@inheritdoc", I do that because there is already a doc-comment somewhere, I don't want to replicate that.

If that is what this issue is about, then it is already covered. But we are expecting {@inheritdoc} with curly braces. If you have this, then there is no problem. But if you just have @inheritdoc then it does produce

----------------------------------------------------------
 32 | ERROR | Missing short description in doc comment
    |       | (Drupal.Commenting.DocComment.MissingShort)
----------------------------------------------------------

So there may be something to fix, but it is not exactly as in the patch provided.

In Drupal/Sniffs/Commenting/DocCommentSniff.php#L129 the block is only executed if the $short code is not T_DOC_COMMENT_STRING. For {@inheritdoc} the short code is T_DOC_COMMENT_STRING which is right. But if you have missed the curly braces, then @inheritdoc is treated as a tag, and the short code is T_DOC_COMMENT_TAG so that block is executed, and we get the 'missing short description'

We can help developers to get it right, maybe by testing for @inheritdoc and giving an extra separate message "Did you mean {@inheritdoc}", or some other fix. But the patch as it stands allows @inheritdoc without the { } to be accepted. Which I don't think follows the coding standards?

@jonathan1055 you sir are correct!

Removing the patch and adding those {} works just fine.

Well then, forget what I said. Reading the docs properly would've saved me from all this.

But! it seems like I'm not the only one forgetting about {}. I see it in the ckeditor5 module in core, and some contrib modules as well :D

Maybe ... add a notice to the error message, that those things are important.

Thanks @SerkanB for confirming that we now agree on the problem. I have updated the issue summary with a suggested solution and we can now discuss the best way forward.

That's good, it rmeoves it from the top of the issue. It also shows as closed on https://git.drupalcode.org/project/coder/-/merge_requests/21#note_150497 and only 5 are open. All of those should be closed too, but I think only the person who opened it or a project maintainer can do that. The project front page does have a small section on how to contribute but it is not very obvious, and users will always be openeing MRs here. It would be nice if there was a way to disable that feature. Anyway, that's for a different issue.

I have added a small bit to the DocCommentSniff to detect this situation and give a better non-misleading message. Also included a fixer to add the missing curly braces { }. You now get

FILE: /coder_test_files/inheritdoc.inc
-----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------
 9 | ERROR | [x] @inheritdoc found. Did you mean {@inheritdoc}?
   |       |     (Drupal.Commenting.DocComment.InheritDocWithoutBraces)
-----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------

https://github.com/pfrenssen/coder/pull/180
My repo https://github.com/jonathan1055/coder/tree/inheritdoc-braces

I think this is helpful, and will avoid the kind of problems seen above. If you consider that you would commit this I will write test coverage for the new bit of code.

Yep, makes a lot of sense to me, I would merge this!

Would you prefer a new pair of test files
tests/Drupal/Commenting/DocCommentUnitTest.4.inc and
tests/Drupal/Commenting/DocCommentUnitTest.4.inc.fixed ?

Or is it OK to add on to the end of the base file DocCommentUnitTest.inc or one of the existing DocCommentUnitTest.n.inc?

I think you can add to the end of DocCommentUnitTest.inc, the others are mostly testing file comments.

Added tests. Intentionally incorrect 'fixed' file and no update to expected lines, to demonstrate that the tests are run and fail. We get

1) Drupal\Test\Commenting\DocCommentUnitTest::testSniff
[LINE 130] Expected 0 error(s) in DocCommentUnitTest.inc but found 1 error(s). The error(s) found were:
 -> @inheritdoc found. Did you mean {@inheritdoc}? (Drupal.Commenting.DocComment.InheritDocWithoutBraces)
[LINE 135] Expected 0 error(s) in DocCommentUnitTest.inc but found 1 error(s). The error(s) found were:
 -> @inheritDoc found. Did you mean {@inheritDoc}? (Drupal.Commenting.DocComment.InheritDocWithoutBraces)
Fixed version of DocCommentUnitTest.inc does not match expected version in DocCommentUnitTest.inc.fixed; the diff is
--- tests/Drupal/../../tests/Drupal/Commenting/DocCommentUnitTest.inc.fixed
+++ PHP_CodeSniffer
@@ -137,11 +137,11 @@
 function test15_this_inheritdoc_is_correct();
 
 /**
- * @inheritdoc
+ * {@inheritdoc}
  */
 function test16_lower_case_fail_needs_braces();
 
 /**
- * {@inheritdoc}
+ * {@inheritDoc}
  */
 function test17_camel_case_fail_needs_braces();

Test files corrected, to demonstrate passing tests. Ready for review.
https://github.com/pfrenssen/coder/pull/180

Thanks, left 2 minor comments there!

Thanks for the review, I have answered one question and changed the code in the other. Ready for review again.

Merged, thanks!

Thanks for commiting. I have added a minor follow-up pr to add a fourth inheritdoc example https://github.com/pfrenssen/coder/pull/185

This is in prep for a future sniff which may enforce either lowercase or camelcase inheritdoc. Currently we allow both versions. Having the four examples close in the file will make it simpler to adjust the fixed file and expected lines, rather than add the fourth case later, after other test examples may have been added at the end.

Makes sense, thanks a lot!