Train moodle.Commenting.MissingDocblock.Missing to understand #[\Override]

[timhunt]

Moodle coding style rules say:

1. A method MUST have a PHP doc comment ...
2. Unless it is an overridden method, in which case it SHOULD NOT have a PHPdoc comment (because that is bad duplication).

Our code-checker rules have never understood this, and always given false-positives.

But, PHP 8.3+ now provides the #[\Override] annotation (See https://tracker.moodle.org/browse/MDL-79901 for discussion of the use of that in Moodle.) And that can safely be used in older PHP without errors.

So, we should teaching moodle.Commenting.MissingDocblock.Missing to use this annotation, and for such methods, give a warning if the comment is present (rather than if it is absent).

[timhunt]

@andrewnicols it occurs to me that when we are giving the warning about missing PHPdoc in a class that extends something else, can we make the fail message include "If this is an overridden method, please add the #[\Override] attribute." or something like that.

[andrewnicols]

That's exactly what I had in mind. A hard error if neither are provided.

[stronk7]

Was looking to the related PR #160 ... I see that only thing that we are doing is to stop warning when the attribute is added.

We aren't going to warn about "duplicate" phpdoc blocks (@ children) neither we are going to warn when both the phpdoc block and the attribute exists.

I imagine that's ok, just sharing about it. And, also, sharing about the need to update the devdocs, because:

1. IMO the example is wrong, it's missing the Summary (1st line) / Description split (note this is discutible, don't remember if we have that written down anywhere.
2. We need to add the info/note about the #[\Override] attribute.
3. We need to be clear about inherited phpdoc blocks, because it's not clear for me if they deserve any warning if there are "duplicates" or they should be allowed. Imagine an inherited function, 100% same signature, but some params or return types are going to be subclasses or arrays of different elements, or whatever. In those cases the "duplicate" phpdoc block is needed to be able to specify the differences from the parent function. Omitting (as devdocs say) would be, IMO, an error.

Ciao :-)

[andrewnicols]

We aren't going to warn about "duplicate" phpdoc blocks (@ children)

That's correct and, in my opinion, preferable.

There is no (easy) way to determine parentage in phpcs. If we want that kind of thing we need tooling like phpstan or rector.

neither we are going to warn when both the phpdoc block and the attribute exists.

Again I think that is correct. It is perfectly legitimate for a child class to have its own docs and want to declare that it is an override.

IMO the example is wrong, it's missing the Summary (1st line) / Description split (note this is discutible, don't remember if we have that written down anywhere.

Which example?

[stronk7]

Which example?

Whops sorry, I forgot to share the link, I was talking about this example, that always hurt my eyes (although, as said, I don't think that we have anything written about the summary & long description for functions):

https://moodledev.io/general/development/policies/codingstyle#functions

[timhunt]

For waht it is worth, I think it is a smell to have a comment on an overridden method. (And override should not change the contract for a method that is defined in the base class.)

But anyway, that is a dicussion for another day. Awesome that this has now landed. Thank you very much :-)