CONTRIB-8920 moodle-cs: Deprecation alerts for capabilities

[laurentdavid]

This is a follow up of MDL-55580. The idea is to add the tooling for codechecker and alert developers of possible
capabilities deprecation.

• Add a simple deprecation warning for capabilities we might want to get rid of.
• For now the declaration of deprecated capabilities will be done in the ruleset.xml (either warning or error) and will duplicate the information in the access.php files. This will need to be dealt with in a second time.

[jrchamp]

I didn't dig into the rest of the code, but wanted to highlight the benefit of safer and more efficient strict checks for in_array.

[jrchamp]

Suggested change

	if (in_array($capability, $this->capabilitiesWarningList)) {
	if (in_array($capability, $this->capabilitiesWarningList, true)) {

[jrchamp]

Suggested change

	return in_array($token['content'], $this->accessfunctions);
	return in_array($token['content'], $this->accessfunctions, true);

[laurentdavid]

Hi @jrchamp

Thanks for the quick feedback. I went back to the doc of in_array I have not used this parameter before, so good to know. I could probably have used isset also. I am not sure which one is quicker.
I will modify this once you will have a chance to look at the rest. The global approach needs to be validated and the big picture is: do we really get this the capabilitiesWarningList through rulseset.xml or by reading each access.php.

Thanks,

Laurent

[jrchamp]

Glad it was useful! I'm just a community member, so HQ will review the rest of the code.

isset() is quicker, but would require array keys instead of array values. ['a' => true, 'b' => true] Overall, in_array() is probably fine for this use case because O(log(n)) vs O(n) isn't a huge difference when n is small.

Side note: As long as you aren't allowing meaningful null values, isset($a[$b]) is the faster replacement for array_key_exists($b, $a)

[laurentdavid]

Hi @jrchamp,

Cheers, I will take that into account once the full review is done. As we say "Make it work, Make it better"...

To give you the context will have couple of capabilities in the array so (n) will be small and as per the second in_array, the number of parameters is also very small (2 or 3 max). I knew about the isset better performances, but then again if performance is not the main issue (and here we have only a couple of elements), I prefer in_array as it is more understandable as you read the code.
I have never noticed with the third parameter of is_array thanks for the input and the detailed explanations !

Laurent

[stronk7]

Hi @laurentdavid !

Nice one, I imagine we'll be able to introduce this once MDL-55580 is done.

Said that, I've a couple of objections/ideas to comment. Haven't looked to the implementation detail but about how the deprecated capabilities are declared (both in core, in the issue, and also here, in ruleset.xml).

There is some paralelism with this requirement (be aware of the deprecated caps) and what we do in various places to know the list of components available in Moodle.

To get the avalable components in Moodle, we use this: https://github.com/moodlehq/moodle-cs/blob/main/moodle/Util/MoodleUtil.php#L123. Basically it:

1. Looks if we are passing a list of components via runtime configuration (moodleComponentsListPath). The format of that file is really simple: "plugin,directory" and that's all.
2. If no file has been passed then it loads them from core (without needing to install Moodle!).

And that's the very same approach we have followed with other "guessed" information, needed by some Sniffs to do their work. We also allow moodleRoot to be passed, or also the moodleBranch and, if not passed, we get them from the installation where the code is running (without needing to install a complete Moodle to get it).

So, in the case of the deprecated capabilities... I think we should try something similar:

1. If there is some moodleDeprecatedCapabilitiesListPath, then we read from that file the information about capabilities. BTW note that we need to know in which branch a capability has been deprecated, so the format of the file should be something like "capability,replacement,branch".
2. If that configuration setting has not been passed, then, knowing moodleRoot, we try to load it from the moodle checkout (again, without installing moodle!). And, of course, we know the branch too, it's @ moodleBranch commented above.

So, basically, from current patch... I can see some changes may be needed:

1. Adding the list of deprecated caps to ruleset is not ideal, we are creating here 2 sources of information. Plus, right now, it's missing information, like replacements and, more important, the branch where the deprecation takes effect.
2. The core feature should be executable without having Moodle installed. See how we load components, it works just setting a few variables and calling to the core_component class. We should be able to have something similar for reading all the deprecated caps in core (ensuring it works in the MDL issue).

Also, completely apart from this issue...

3. Surely, we'll need some utility in local_ci able to provide that list of deprecated capabilities (reading them from core too), because when we run the codechecker Sniffs, all the files but the patch ones have been deleted. We already do that for the list of components and will need some parallel "list_deprecated_capabilities" scripts.

But this point is another story (we will need it for CiBoT and how we run it). So, basically, if you're able to get 1 (and specially 2) working... then this 3rd point can be easily done. The main point is to be able to get the list of capabilties deprecated WITHOUT installing Moodle. If that's achieved... then these 1-2-3 points are really easy!

Hope I've explained it clearly, in my brain this is 99% parallel to the "load components" case, that we need for a big number of sniffs (namespaces, packages...). So we need a similar "load deprecated caps" implementation. Just to avoid using different techniques (config settings, ruleset, checkout...) for being able to pass needed information to the sniffs.

Ciao :-)

[andrewnicols]

How are these filled?

[andrewnicols]

What about has_any_capability, has_all_capabilities, and the require_* versions?