Update BoilerplateCommentSniff and add fixes.

micaherne commented 6 months ago

There are a few troublesome things I find about BoilerplateCommentSniff:

It doesn't allow you to use declare(strict_types=1) in the standard place - on the same line as the opening PHP tag. You have to put it directly after the Moodle comment, which is obviously valid but it's non-standard and harder to spot.
If you have anything at all on the line after the PHP tag, except for the Moodle boilerplate, you get 14 separate messages saying that some random token doesn't match a line of the Moodle boilerplate. I'd expect a single message saying that the boilerplate wasn't found, or at least for it only to attempt to validate comment tokens.
There are no fixes available

(I appreciate that the second point probably doesn't affect core development so much because all core files have the boilerplate, but it just swamps the output with noise if you have a plugin that doesn't yet have the boilerplate in any of the files.)

Could I propose this patch to improve things a bit? It:

allows declare() calls on the same line as the opening PHP tag
only checks the boilerplate for contiguous comment tokens, and bails out if it encounters any other kind of token
provides fixes to automate inserting / fixing the boilerplate when it's found in the wrong place or incomplete

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.08%. Comparing base (22b99c9) to head (ba88826).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #158 +/- ## ============================================ + Coverage 98.03% 98.08% +0.05% - Complexity 901 932 +31 ============================================ Files 40 40 Lines 2697 2769 +72 ============================================ + Hits 2644 2716 +72 Misses 53 53 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

stronk7 commented 6 months ago

Hi,

just sharing... also note that, regarding declare() statements, we recently adhered about that to PSR-12 rules (in #27), so it's not clear for me why should we allow it in the php opening tag (if I've understood what is being proposed here).

Edited: Note that it's only allowed in the php opening tag when the script has mixes of php and html sections, something that is not normally used by Moodle nowadays.

Ciao :-)

micaherne commented 6 months ago

Thanks for the feedback @andrewnicols and @stronk7!

I see declare strict types in line with the PHP opening tag an awful lot, so I'd assumed it was standard practice, but if it's not allowed by the standard that's good. I've excised all the code relating to declares and it's simplified the patch a lot.

I've also added a unit test for the new error condition I added (CommentEndedTooSoon). I'd like to add tests for the fixes but I'm at a bit of a loss as to how to do that, as I can't find any examples anywhere - I've looked in quite a few standards.

micaherne commented 6 months ago

Also I have a couple of questions that could help me to improve the patch if I knew the answers.

I retained the original behaviour where it will only check the boilerplate if it's found after five or fewer blank lines at the start of the file. I'm guessing this was for performance reasons, but it would make things a lot simpler and more logical if we could just find the first comment in the file with $file->findNext(T_COMMENT, ...) and check it for correctness regardless of where it is. Obviously for a big file with no comments this could mean searching through the whole file (although I'm pretty sure that would be really fast). Would that be acceptable or is only checking the first five tokens a hard requirement?
The text for checking the comment is missing the text "Moodle - https://moodle.org/" at the end of the first line. Reading between the lines this looks like it's so that the web URL can be replaced with a different one (given that any product name is supported, not just Moodle). Is it still a requirement to support other names and URLs or could this just be the canonical Moodle boilerplate text?

(I was also wondering whether the http: URLs could be changed to https: ?)

Do you know the answers to these?

micaherne commented 6 months ago

I've got a version of this that I'm happy with now, I hope you might consider it. Apologies for the confusion around the declare statements - I was mistaken about that and I've removed all traces of it.

Hopefully it's clear enough from the code but the logic of the checks is now:

Check the first line of the first comment in the file (if it exists) matches "This file is part of". Otherwise assume there is no boilerplate, show an error, and stop checking.
Check each following "//" comment line matches the correct boilerplate text, and show an error if not. If there are too few contiguous comment lines, show an error saying that the boilerplate is incomplete and stop.
Check that the boilerplate is at the opening line of the file and show an error if it is not. (This will still show first even though it is checked last.)
Check that there is exactly one newline (or the end of the file) after the boilerplate.

(The last one is a new rule, but it seems reasonable to me and helps with the fixes. I'll remove it if it's not acceptable.)

The way the checks are done simplifies the corresponding fixes:

Insert the boilerplate at the first line.
Fix incorrect lines / complete with missing lines
Move boilerplate to first line of file
Remove multiple trailing lines

As well as the unit tests I ran the fixes against Moodle core and can't see any issues: https://github.com/moodle/moodle/compare/main...micaherne:moodle:boilerplate-sniff-test (apart from where there are some existing very non-standard copyright notices that couldn't be detected!)