search

moodlehq / moodle-local_moodlecheck

Checks style of phpdocs in moodle source file
26 stars 31 forks source link

Errors displayed when MoodleCheck encounters a plugin that includes some empty files #86

Closed michael-milette closed 2 years ago

michael-milette commented 2 years ago

When checking plugins that contain some empty files, the following notices and warnings are displayed:

These appear to be caused by the fact that there are no tokens if the file is empty.

An example of this might be a plugin under development that includes an empty settings.php file.

Let me know if you have any questions.

Michael Milette

stronk7 commented 2 years ago

Thanks for the report @michael-milette . Will try to reproduce it locally and find a fix.

stronk7 commented 2 years ago

Curious, are you files 100% empty (0 bytes)?

I'm trying here with an empty (0 bytes) file and I'm being able to only get these 2:

[27-May-2022 18:47:49 Europe/Madrid] PHP Warning:  Undefined array key 0 in local/moodlecheck/rules/phpdocs_basic.php on line 60
[27-May-2022 18:47:49 Europe/Madrid] PHP Warning:  Trying to access array offset on value of type null in local/moodlecheck/rules/phpdocs_basic.php on line 60
stronk7 commented 2 years ago

Ok, I've a patch here that, in early stages, if the file is empty or it doesn't have tokens (a.k.a, PHP code), then skips it with message: Line 1: The file is empy or doesn't contain PHP code. Skipped.

As commented above, I've been unable to reproduce some of the reported notices... but I think that with the changes we stop any further processing of empty files before they happen, so it should be enough.

empty_skipped

Going to create a pull request with the changes...

michael-milette commented 2 years ago

Thanks @stronk7 ,

If the file is skipped, it will likely take care of the other errors. Let me know when the patch is available and I will be happy to test it out.

michael-milette commented 2 years ago

Yes, 0 byte files.

stronk7 commented 2 years ago

You can find the candidate code @ #87

Ciao :-)

michael-milette commented 2 years ago

Hi @stronk7 ,

Thank you! Tested it and your patch got rid of the following errors:

A reminder that you will also need to bump the version number because you modified the language file.

However, I am still getting the following error:

I am testing about 180 plugins at a time and will try to figure out which one is causing the error.

Best regards,

Michael

michael-milette commented 2 years ago

Hi @stronk7 ,

I have determined that the plugin which is causing nine (9) Warning: Unterminated comment starting line 2 in /local/moodlecheck/file.php on line 162 messages (one of them is "...starting line 252") is local/codechecker.

I narrowed down the issue to something under local/codechecker/phpcs/src/Standards. If I exclude that path, the messages no longer appear.

Hope this helps.

Best regards,

Michael

stronk7 commented 2 years ago

Thanks @michael-milette ,

I've traced down the remaining problem and it's caused by unterminated phpdoc blocks. See, for example, this line (252):

https://github.com/moodlehq/moodle-local_codechecker/blob/master/phpcs/src/Standards/Generic/Tests/Commenting/DocCommentUnitTest.inc#L252

Note it's a test (fixture) file, plenty of wrong cases to see if codechecker is able to detect them. So I'd expect that and other errors to happen when token_get_all() is used to extract all the PHP tokens. It just emits the warning (correctly).

It's, basically, this case: https://3v4l.org/G3AHX (curiously it's 7.x warning only(

So, all I can imagine... is to silence (@) the call in order to get rid of the warnings.

With the example above, once silenced, we get this: https://3v4l.org/DDWSh (the very same 8 tokens are returned, but the warnings are gone).

In the other side, those "fixtures" shouldn't be checker ever. They are fixtures and can include all sort of errors.

So, really I'm not 100% convinced about what to do: 1) silence or 2) nothing.

Opinions?

michael-milette commented 2 years ago

Hi @stronk7,

Thanks for looking into this more.

Developers should be linting their code and fixing problems before running MoodleCheck on it. However, QA testers just test and and report, not fix the issues. So those folks really appreciate your efforts.

Could you somehow catch the invalid condition and output a generic "File contains one or more invalid comment block" message? As long as it appears in the report within context of the file being tested, it would still be helpful despite its vagueness.

My second choice would be to suppress the warning.

As it is right now, you cannot tell which plugin is generating the warnings if you are are testing in bulk because these messages appear at the top the results, out of context.

If you do nothing, the appearance of the warnings cause confusion and could leave the user to question the completeness of the report. To me, the appearance of an error or warning means that something did not work as designed and is therefore is unreliable. For these reasons, I would rather doing nothing not be considered as a solution.

Does that help at all?

Michael

stronk7 commented 2 years ago

Hi!

For sure it helps!

I think I'm going to go the silence route, because it's not clear to me how to proceed trying to try/catch those Warnings... there maybe others when the "code" being checked is wrong in purpose (like in this case, code specially prepared wrongly for codechecker own tests).

So, I'm going to amend the patch, adding the silence operator and, also, one test covering the case... and done. So far, looking in the documentation, the only warnings I've seen are the "unterminated comment" and another "unterminated code" one. I think both are rare, rare in practice and not worth doing any special handling of them in moodlecheck.

Ciao :-)

stronk7 commented 2 years ago

Done, have added a new commit @ #87, to verify that now there aren't PHP Warnings anymore.

Ciao :-)

michael-milette commented 2 years ago

Hi Eloy,

Your latest patch works for me. Thank you!

Michael

michael-milette commented 2 years ago

Thank you!

michael-milette commented 2 years ago

Hi @stronk7 ,

Reminder: Because you added a string, please update the version number so that it will purge the cache.

Best regards,

Michael

stronk7 commented 2 years ago

Whops, thanks for the reminder, I've straight-pushed a commit with the version bump to today (20220602).

Ciao :-)

michael-milette commented 2 years ago

Thank you Eloy!