Update BoilerplateCommentSniff and add fixes.

[micaherne]

There are a few troublesome things I find about BoilerplateCommentSniff:

1. 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.
2. 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.
3. 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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stronk7]

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]

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]

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

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

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

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:

1. 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.
2. 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.
3. 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.)
4. 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:

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

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

[stronk7]

Thanks for all the effort put on this, @micaherne.

I'm going to take a look to it now, sorry for the delay! Will come with some comments or suggestions if I find something.

Ciao :-)

[stronk7]

FYI, I've taken the liberty of rebasing your commit on top of "main", to have it updated.

[stronk7]

And another small edition, simply changing a few var names, no change in logic, so far.

[stronk7]

And, now, I've added (in a new commit, to see the changes easier) a good number of "fixture.php.fixed" files to ensure that the fixer is doing its job with all them.

Also, have added one new test (trailing_whitespace_missing) to complete the coverage. And updated the changelog file.

[stronk7]

Ok, I think this can be considered ready, with tests passing and all the previous discussions already resolved, hence, accepting.

[micaherne]

@stronk7 thanks for fixing this up and getting it into production!

[stronk7]

@stronk7 thanks for fixing this up and getting it into production!

ty!

[stronk7]

For the records, #168 has been created, regression of this, about we needing, surely, to allow "extra lines" in the boilerplate to exist, and only apply the SingleTrailingNewLine detection to the end of the whole thing.