Issue with MoodleInternal Sniff (MoodleInternalGlobalState)

[keevan]

This is easier to demonstrate with an example:

https://github.com/moodle/moodle/blob/master/cache/classes/administration_helper.php

In the current version of this checker, you will get a warning on the line with:

defined('MOODLE_INTERNAL') || die();

Unexpected MOODLE_INTERNAL check. No side effects or multiple artifacts detected.
(moodle.Files.MoodleInternal.MoodleInternalNotNeeded)

Removing this line will remove this problematic warning, however, it will cause an error to appear near its place, namely on the following line:

use cache_helper, cache_store, cache_config, cache_factory, cache_definition;

Expected MOODLE_INTERNAL check or config.php inclusion. Change in global state detected.
(moodle.Files.MoodleInternal.MoodleInternalGlobalState)

.. which I believe is incorrect. The current definitions are as follows (as per MDLSITE-5967)

Side effects include:

    requiring other files
    defining functions outside of classes
    etc.

Side effects do not include:

    defining classes, interfaces, or traits
    using the use stanza to alias a classname

I do not believe the usage of the use statement here should be treated as a side-effect according to the above definition.

[stronk7]

Thanks @keevan !

Yeah, that seems to be a bug in the calculations of global state (side-effects) changes. For sure that use statement shouldn't lead to requiring the MOODLE_INTERNAL check.

BTW, note that multiple use statements in a line is also a coding style violation (individual ones is the way).

In fact, I've verified that converting them to multiple individual statements, then the check works ok.

In any case, that's a good case, I'll investigate why the checker it detecting them as side-effects (incorrectly, I'd say).

Ciao :-)

[stronk7]

Aha, confirmed, the problem is that, both for NAMESPACE and USE tokens, we use the ->findEndOfStatement() method, that, by default, doesn't work ok for comma-separated values, stopping with the first one in the sentence, instead of going until the final semicolon.

That makes the next token to be "cache_store" (the second value in the USE statement after the first comma) and obviously that is a side effect (something @ global scope that shouldn't be there).

I think it's easy to fix, will try it along the day.

[keevan]

Thanks Eloy,

Yes I know it's a current coding style violation but appears in many places currently :-)

I had tested that workaround/fix in a couple of places already, and can confirm it works fine:

• keevan/moodle-tool_forcedcache@781ab57
• keevan/moodle-logstore_standardqueued@3e65fbb#diff-c314899a44a0d434db372102aab0739dcdf97edf25e2da648c177f546c5e318bR31-R32

Thanks for the fast response. Looking forward to the update 👍

[stronk7]

Yeah, yeah.

To fix it in Moodle is easy, just make individual use statements (or remove the check where it's not needed). But that's something to go fixing apart from this, recently I ran the check against the whole Moodle codebase and found hundreds of violations.

And, still, we don't have a proper namespaces/uses sniff in codechecker that will reveal more current issues in codebase (I'll be working on it soon, recently did some related work with unit-tests namespaces that will reuse for namespaces in general).

What I want to fix here (in the codechecker) is just the exact case that you commented, aka:

"At all effects, comma-separated use statements do not create side-effects, hence we shouldn't be reporting MOODLE_INTERNAL as needed for them".

Thanks!

[stronk7]

Have created #176 that should fix the problems with comma-separated use statements (this).