Consider creating a new "moodle-for-plugins" standard

stronk7 commented 9 months ago

Commented @ #91 maybe it would be nice to have a new standard, similar to the moodle-extra one for easier use by tools that are checking plugins and not core, namely CiBoT and moodle-plugin-ci.

That way, each time that we introduce a new Sniff in the "moodle" standard, if it's not suitable for plugins, we just can disable it in the new moodle-for-plugins standard and done. Or modify any behaviour (error vs warning), or whatever.

Right now, when those exceptions happen, we have to create issues like:

CiBoT: https://github.com/moodlehq/moodle-local_ci/issues/289
moodle-plugin-ci: https://github.com/moodlehq/moodle-plugin-ci/issues/266

And set a custom configuration there, that is slow and hard.

Instead, if we create the new standard for plugins, everything will work automagically, the tools just will use that standard and done.

Disclaimer: the moodle-for-plugins name is not a proposal, just the concept, heh.

jrchamp commented 9 months ago

moodle-plugins makes a lot of sense to me. Right now, I'm using moodle-extra and like the idea of avoiding core-specific rules, so hopefully this plan will also create moodle-plugins-extra or moodle-extra-plugins.

andrewnicols commented 9 months ago

Another option for this is to have an option set in the plugin's own phpcs.xml, for example:

<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="MoodleCore">
  <config name="moodle-type" value="plugin" /> 
</ruleset>

This can be accessed within the relevant sniffs:

public function process(File $file, $pointer) {
    if ($file->config->getConfigData('moodle-type') === 'plugin') {
        return;
    }

    // ...

This has the benefit that the same standard sniffs can be used without doubling up.

stronk7 commented 8 months ago

Yeah, both are valid points,

if there is a moodle-for-plugins, we'll need a moodle-for-plugins-extra.
we can avoid the duplication by using some config setting controlling sniffs behaviour.

I think that having a (two) new standards is not much complex as far as they would include their parents and just enable/disable exceptional stuff.

In the other side, making it via configuration can be, also, a very good solution and the tools using phpcs (CiBoT, moodle-plugin-ci, codechecker, ... ) just have to apply for it or no.

Just to see the problem from another angle, not discarding anything of the above, I've the feeling that we should have a clear policy / process about how to manage the evolution of the moodle standard(s). And then, apply for it always. Instead, right now, we have a bunch of policies that are applied here and there and, although we try to do it "correctly", it's really tricky to decide between them:

Enable some Sniff only for version vX.Y and up.
Make the Sniff to become an error after 1-year as warning.
moodle vs moodle-extra
disable completely a Sniff for plugins (in the tools using it, or via config or via new standard).
modify a Sniff behaviour for plugins (in the tools using it, or via config or via new standard).
... (and sure that we have applied for something different in the past).

So, ideally, we should find which of the above (or new) strategies we agree to apply, for which cases each, make it public and do not look back.

I think that, long term, it's impossible to continue work-arrounding / delaying the new Sniffs (if they are objectively / un-opinionated part of the coding style) to apply to plugins. I know that it creates a lot of noise on everybody's plugins, but it's for a good reason.

Then, there are the opinionated ones, that, maybe, are the trickier ones (for a current example, see #103). It's a good change, but it's not a must (or part of the coding style, or part of a future requirement of PHPUnit).

Those are, surely the ones we should find a path for.

Ciao :-)

timhunt commented 8 months ago

jsut to suggest an alternative. Instead of "moodle-for-plugins" we could instead

keep moodle-cs as the vast majority of rules that apply to all Moodle-related code
have moodle-cs-core which is that, plus the few extra rules which related to things in core, like TODO comments must have a MDL- number.

Basically the same idea, just different split and naming.

andrewnicols commented 8 months ago

@timhunt ,

They would all be under the moodlehq/moodle-cs composer package. The current core ruleset is called moodle, and the extra one is moodle-extra.

If we were to add additional rulesets then they would be a part of the same package.

Whatever we do we would need to create a phpcs.xml file in each plugin because by default it will inherit from the parent directories (therefore moodle). If we were to rename the core one, it would still be the default for plugins because it would be set in the Moodle directory.

We have a couple of options:

ask developers to create their own phpcs.xml defining their preferred rulesets/configs
move the list of standard plugins to something more easily consumed (like lib/plugins.json) and disable some rules for any plugin which is not a standard plugin. I have already created MDL-81084 for this one.

timhunt commented 8 months ago

Thanks for continuing to think about this.

Here is another thought: phpcs.xml is not the only way to specify the ruleset to use (I think - I could well be wrong). I think you can also specify it on the command line (https://github.com/squizlabs/PHP_CodeSniffer/wiki/Usage). Could we specify moodle in the main phpcs.xml, but then get CiBoT to specify moodle-extra on the command-line when checking core changes.

(You probably already thought of that, and it won't work for some reason. Also, it seem moodle-plugin-ci already just works, so that is fine.)

andrewnicols commented 8 months ago

Here is another thought: phpcs.xml is not the only way to specify the ruleset to use (I think - I could well be wrong).

You are correct, the options are (in descending order of precedence):

CLI arg standard: phpcs --standard=moodle
.phpcs.xml
phpcs.xml
.phpcs.xml.dist
phpcs.xml.dist

Source: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage

Moodle already ships with a phpcs.xml.dist file which specifies the default ruleset (moodle) and the compatible PHP versions (testVersion). That file was introduced via MDL-76727 for 3.11.12, 4.0.6, 4.1.1, and 4.2.

We also generate a phpcs.xml file using the grunt ignorefiles command to ignore things like vendor, and any libs listed in a thirdpartylibs.xml file -- that is site specific, which is why we generate it via a grunt command. If it is not present, the site still gets the benefit of the default ruleset, but if it has been run, then phpcs will ignore files we don't control.

That leaves .phpcs.xml for a moodle install to do their own thing and specify a more stringent standard. That is documented here: https://moodledev.io/general/development/tools/phpcs#configuration

An individual plugin can also just create their own version of any of these files however it will only be used if phcps was run from that directory.

Could we specify moodle in the main phpcs.xml, but then get CiBoT to specify moodle-extra on the command-line when checking core changes.

I don't think we need to really. We already do specify moodle in our phpcs.xml.dist file, and we already use that (not moodle-extra) in cibot. The issue is that we include some sniffs which upset people who are calling it in a plugin and don't want certain sniffs (like the TODO one).

Additionally our moodle-plugin-ci tooling allows you to override the --standard option in your GHA workflow.

Ultimately I'm not sure what else we can actually do in that regard because of how phpcs picks up config files.

moodlehq / moodle-cs

Consider creating a new "moodle-for-plugins" standard #92