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

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

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.

SerkanB → closed merge request !21

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.

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!

Automatically closed - issue fixed for 2 weeks with no activity.