I'm getting CI errors like global $PAGE cannot be used in block classes. Use $this->page. (moodle.PHP.ForbiddenGlobalUse.BadGlobal) in a class block_form class block_form extends \core\form\persistent

This doesn't look correct as basically we can't have any classes containing 'block'.

Our CI found the following error with a lib.php file today.
The lib file definitely contains functions.

FILE: /home/runner/work/cfz/cfz/theme/cfz/lib.php

FOUND 1 ERROR AFFECTING 1 LINE

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

While reviewing #90, @vmdef has raised a couple of interesting things to review related to the included files in index.php and run.php:

if (is_file(__DIR__ . '/phpcs/autoload.php') === true) {
    include_once(__DIR__ . '/phpcs/autoload.php');
} else {
    include_once('PHP/CodeSniffer/autoload.php');
}
// PHPCompatibility autoloading.
require_once('PHPCSAliases.php');

1. "PHP/CodeSniffer/autoload.php" include might not be required any more.
2. PHPCSAliases.php is included through PHPCompatibility/ruleset.xml, so we don't need to include it here.

This issue is about the check if this piece of code can be improved and some of these includes can be removed definitively.

Since https://tracker.moodle.org/browse/MDLSITE-2800, the phpcompatibility standard supports passing a given PHP version ("--runtime-set testVersion X.Y.Z") and that produces multiple sniffs (deprecated, removed, non-existing... function/classes) to be run specifically for that version.

This is about to enable the codechecker UI to, optionally, specify a PHP version to run the phpcompatibility sniffs against.

The basic idea is to show all versions between:

1. the required @ admin/environment.xml for current site.
2. the current php version being used.

And to allow the developer to pick one (can default to 1).

Note: This should be supported both by the web UI and the run.php CLI script.

Reporter: Eloy Lafuente
Original issue: https://tracker.moodle.org/browse/CONTRIB-5158

Code checker is not working after upgrading to the latest release:

Warning: simplexml_load_file(): I/O warning : failed to load external entity "/moodle/lib//thirdpartylibs.xml" in /moodle/local/codechecker/locallib.php on line 194

Reporter: Yuliya Bozhko
Original issue: https://tracker.moodle.org/browse/CONTRIB-5160

Starting with PHPUnit 8.5, the @expectedExceptionXXX annotations are deprecated and, instead of the ->expectExceptionXXX() methods should be used, exactly before the line that is going to throw the exception.

This issue is about to enable moodlecheck (or, if easier, codechecker) to be aware of this deprecation, starting to warn about it. Ideally, only for 310 and up, if that's possible.

Reporter: Eloy Lafuente
Original issue: https://tracker.moodle.org/browse/CONTRIB-8264

The codechecker plugin contains a sniff (IncludingFileSniff.php) that enforces the inclusion of redundant parenthesis around the path to be included.

e.g. "require_once" must be immediately followed by an open parenthesis

There is a common misconception about the nature of include, require, include_once, and require_once.
None of these language constructs require parenthesis, and it can be argued that enforcing this behaviour as a matter of course is bad practice.

_PHP manual - see examples #4 and #5 http://php.net/manual/en/function.include.php

"Because include is a special language construct, parentheses are not needed around its argument. Take care when comparing return value."

_Pear coding standards - http://pear.php.net/manual/en/standards.including.php

"include_once and require_once are statements, not functions. Parentheses should not surround the subject filename."

It is for a similar reason echo and print do not require the use of parenthesis (and their use can potentially break normal use of echo).

Note: List of PHP keywords http://php.net/manual/en/reserved.keywords.php

Reporter: David Aylmer
Original issue: https://tracker.moodle.org/browse/CONTRIB-7099

In https://tracker.moodle.org/browse/MDLSITE-5967 it was decided that it's ok to omit the MOODLE_INTERNAL check for 1-artifact files without side effects.

And we did that in the codechecker (and coding style details).

Now it's time to introduce a new warning about that, aka detected not needed uses. When a file is 1-artifact and without side effects, better don't add the MOODLE_INTERNAL check.

The other side of the agreement, basically. So we stop getting too many, now unnecessary, checks in the new files.

Ciao :-)

If we do check any plugin via code checker and have file like this

https://moodledev.io/docs/4.4/apis/commonfiles#libphp

even

file like - https://moodledev.io/docs/4.4/apis/commonfiles#locallibphp

It does not showing any warning / error. There should be some checking about the legacy thing.

Kindly let me know if this feature is already there via some config etc.

Now (July 2022), the unique source for the Moodle coding style standard is moodle-cs.

So, local_codechecker isn't anymore the source, but just one more consumer of the standard.

In order to keep existing integrations with IDEs and other tools working... as part of #193 , we created a "moodle" alias to point to the imports of the new moodle-cs standard.

We are going to give people 1 year to move those existing integrations from local_codechecker to moodle-cs. Will be announced everywhere:

• codechecker release notes.
• dev chat
• dev forum
• integration exposed
• ...

And, then (July 2023), we will proceed to:

• delete the moodle alias from local_codechecker.
• delete the phpcs.xml file (see #207), because we don't need it anymore.
• amend thirdpartylibs.xml and remove the alias from there too.
• verify that everything works under unix and windows.

Ciao :-)

This result seems incorrect to me, it does not appear to recognising the array correctly.

#78: ········$banner·=$generator->create_banner(['position'·=>·1]);
Expected 1 space after "="; 0 found

This is happening in the latest version of the master branch

As result, it does not complain on violations similar to

class api
{

or when there are spaces before or after brace

class api       {

Issue has been found by @dravek (thanks!)

Mentioned by @stronk7 - we should check whether the file contains a /behat/ folder in it's path, rather than it's immediate descendant having that name:

See MDL-72696 (fails for file lib/behat/form_field/behat_form_inplaceeditable_select.php)

This comes from MDL-69936, where it was agreed to deprecate print_error() and start to use, exclusively, moodle_exception().

We need to add this to our standard ASAP, so uses are detected and informed (as warnings while the function is deprecated, and as errors once the function is removed).

The issue to effectively deprecate the function from core is MDL-71062. In that issue we'll decide when it becomes deprecated and, in followup, removed.

Ciao :-)

Since some months ago, core includes a .phpcs.xml file, really useful that instructs to any PHP_CodeSniffer about which standard must be used by default.

And the standard is "moodle" how now.

Problem is that PHP_CodeSniffer first looks for local (pwd) directories with that name ("moodle") and, in the case of codechecker, that's currently a soft alias, pointing to the new moodle-cs standard, and kept on purpose to allow other tools to continue working.

While codechecker's bundled PHP_CodeSniffer doesn't need that file at all (because the standards are perfectly configured via .conf file), its default behaviour is to look to those .phpcs.xml files. Then it reads "moodle" from there and resolves it to the soft alias. And soft aliases don't work on Windows, causing an error.

An error occurred during processing; checking has been aborted. The error message was:
Ruleset C:\...\local\codechecker\moodle is not valid - On line 1, column 1: Start tag expected, '<' not found

We need to ensure that codechecker's bundled PHP_CodeSniffer stops looking to core .phpcs.xml, surely by introducing its own .phpcs.xml file (that will cause it to stop looking upwards). At least that's the theory.

Let's see. Ciao :-)

Hi all,

today (24th Nov 2023), we have proceeded to rename the "master" branch of this repository to "main".

You can find more details about the migration @ https://tracker.moodle.org/browse/MDLSITE-7418

For some guidance about how to proceed with your clones you can follow the information that GitHub provides in the home page of the repo and, also, take a look to this (still draft!) document:

https://gist.github.com/stronk7/d007be57062ebef2dd6549010954a378
(you're welcome to help amending it with your experiences, feel free to comment there)

Once ready, the document will be moved to https://moodledev.io

Sorry for the trouble and ciao :-)

PS: This issue will be closed in a few weeks.

if i have a foreach like this:

/** @var table_header_item $columninfo */
foreach ($this->columninfo as $columninfo)

The plugin validates as OK.

But when it has three parameters it reports error

/** @var table_header_item $columninfo */
foreach ($this->column as $key => $column)

"Inline doc block type-hinting for '$column' does not match next foreach() as variable"

Add some sniffs to check file and line termination according to MDLSITE-6397.

I've got a file of constants (they're all just define(blah, blah)) with an mdef check at the top.

Code checker complains with:

--------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------
 25 | WARNING | Unexpected MOODLE_INTERNAL check. No side effects or multiple artifacts detected.
--------------------------------------------------------------------------------------------------

But if I remove the mdef check, it complains with:

------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------
 1 | ERROR | Expected MOODLE_INTERNAL check or config.php inclusion. Change in global state detected.
------------------------------------------------------------------------------------------------------

constants.txt

Testing a module I decided to "Exclude" from the check each txt file.
Because of this I added
".txt", "/.txt" and some other different way to describe what I had in mind
but codechecker never jumped my *.txt files.
Was it my fault? Was I supposed to write the path in a different way?

Reporter: Daniele Cordella
Original issue: https://tracker.moodle.org/browse/CONTRIB-6345

According to this article on DEV.to and many other sites on the Net, Visual Studio Code is now one of the most popular code editors in the world. Once configured with extensions like PHPCS and others, this cross-platform editor is often considered to be like an IDE by many developers.

It would be helpful and so much appreciated if you could please provide instructions in the most recent version of codechecker's README.md on for VSCode to make it use the ruleset of moodle-local_codechecker and make it aware of the Moodle coding guidelines.

I was 90% sure that codechecker was detecting this already... but it seems that it isn't. It has happened @ MDL-73719, for example.

Reference (see bad examples) : https://docs.moodle.org/dev/Coding_style#Inline_comments

Basically inline comments must be, always, // comments. And, only exceptionally, to type-hint about a variable or so /** comments. But never # or /* comments.

Verify current behavior of all the above and add the missing checks as needed.

Hello,

Why this message?

"require" and "require_once" are statements, not functions

You should allow both and prefer the version without "()".

Thank you,

Olivier

We don't usually test codechecker under Windows, not many have that OS at hand. A very least we should guarantee that GHA performs one Windows run to detect regressions better.

See for example #207, that was not detected because everything is working ok under unix.

Ciao :-)

btw... was thinking if maybe we could try to keep the moodlerooms/moodle-coding-standard auto-maintained, so any change in the standard is automatically populated there. I see you've the sync + manual review approach there.

Source: open-lms-open-source/moodle-plugin-ci#18

It would be nice if it were auto-maintained, though I think an even better approach would be to replace it with Moodle's own official coding standard, distributed via Composer. The reason why I created the moodlerooms/moodle-coding-standard project was simply to make it easier to manage my dependencies for the moodle-plugin-ci project and also to know, for certain, that if the project is installed, then I don't have to install additional items in order to run some of the commands, EG: ensure local/codechecker is installed into the Moodle it is running against.

Two things I have encountered with the moodlerooms/moodle-coding-standard project:

• Must always depend on an official release of PHP_CodeSniffer. Recently I upgraded to a specific version in their master branch, and that worked well until I tried to pull the coding standard into another project. Composer doesn't like that I used dev-master and considered that release of the standard to be unstable.
• I had to remove the tests because they depend on Moodle's PHPUnit framework. I would hope this is a minor barrier.

So, if Moodle had its own official coding standard, distributed by Composer, then it can be easily reused by other projects, including Moodle itself. We could put it in Moodle's composer.json making it trivial for developers to run it against their code (or useful for CI). Another bonus, you can do something like this in the composer.json:

  "scripts": {
    "post-install-cmd": [
      "vendor/bin/phpcs --config-set installed_paths ../../moodlehq/moodle-coding-standard",
      "vendor/bin/phpcs --config-set default_standard moodle",
      "vendor/bin/phpcs --config-set show_progress 1",
      "vendor/bin/phpcs --config-set colors 1",
      "vendor/bin/phpcs --config-set severity 1",
      "vendor/bin/phpcs --config-set report_width auto",
      "vendor/bin/phpcs --config-set encoding utf-8"
    ],
    "post-update-cmd": [
      "vendor/bin/phpcs --config-set installed_paths ../../moodlehq/moodle-coding-standard",
      "vendor/bin/phpcs --config-set default_standard moodle",
      "vendor/bin/phpcs --config-set show_progress 1",
      "vendor/bin/phpcs --config-set colors 1",
      "vendor/bin/phpcs --config-set severity 1",
      "vendor/bin/phpcs --config-set report_width auto",
      "vendor/bin/phpcs --config-set encoding utf-8"
    ]
  }

Now you can use vendor/bin/phpcs AND vendor/bin/phpcbf and both default to the correct coding standard and our other various defaults. The only thing that is missing is some of the magic that local/codechecker does to ignore 3rd party libraries which you might be able to make another light weight package to house that logic and add it to the composer.json as well.

Anyways, just some ideas I have been thinking about. I know this will be disruptive for developers because it changes their workflow, but like most change, hopefully it is for the better.

Reporter: Mark Nielsen
Original issue: https://tracker.moodle.org/browse/CONTRIB-6209

We should enforce order for 'abstract', 'final' and 'static' declarations.

When present, the abstract and final declarations MUST precede the visibility declaration.
When present, the static declaration MUST come after the visibility declaration.

https://www.php-fig.org/psr/psr-12/#46-abstract-final-and-static (4.6 abstract, final, and static)

https://docs.fileformat.com/spreadsheet/csv/ says "Each record is located on a separate line, delimited by a line break (CRLF)."

CodeChecker is telling me

"Windows (CRLF) line ending instead of just LF (reporting only first occurrence)"

on my CSV file. I think CodeChecker is wrong.

Default install, v5.0.4, build 2024012600

Cannot use the code checker at all anymore, it just dies with this message for every file.

Moodle:
4.3+ Build: 20231013
PHP 8.1.27 (FPM/FastCGI)

Form @ewallah, in recently fixed #179 (PR #183):

The loadCoreComponent function has to fail if dirroot is not set. First a new empty class is created, the next step evaluates the dirroot of this (empty) class. (/codechecker/moodle/Util/MoodleUtil.php)

if (!isset($CFG->dirroot)) { // No defined, let's start from scratch.
        $CFG = new \stdClass();
}
if ($CFG->dirroot !== $moodleRoot) { // Different, set the minimum required.

We need to revisit that "illogical" code (just noting that, normally it uses to be set, but should work if not, too).

Thanks for the report!

Ciao :-)

If I add the following:

phpcs:disable moodle.PHPUnit.TestCaseNames.Missing

I still get the error showing up:

moodle.PHPUnit.TestCaseNames.Missing

Use case for not wanting this sniff:

I have an abstract class with all my tests in it.
The abstract class will test according to the value of a constant set on the class.
I have two sub classes of the abstract class which have different constants to make the tests behave differently.
There is no need for me to have test methods in the sub classes.

We should support inline use of:

/** @var [type] */

With debugging set to DEVELOPER mode, I get the following error messages when the source code file is empty:

Notice: Undefined offset: 0 in /local/codechecker/locallib.php on line 197

Line 197 is:

CodeChecker does report the issue correctly but it should be able to handle the issue without reporting the above-mentioned notice.

Let me know if you have any questions.

Best regards,

Michael Milette

We are using 18.04 that is deprecated, have to move up.

In dfff50b we disabled in our {{.travis.yml}} file the execution of the codechecker (because codechecker 2 cannot check codechecker 3). This is because moodle-plugin-ci continues pointing to codechecker 2.9.8.

moodlehq/moodle-plugin-ci#49 has been created to update moodle-plugin-ci to codechecker 3 final release. Once that's done and there is a compatible version... we'll revert the commit above and codechecker codebase itself will be checked by codechecker.

Ciao :-)

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.

It seems as part of this latest change line 79 barfs when $CFG->dirroot is not set because we make $CFG into a new \stdClass which does not have dirroot in it as a property.

In Moodle coding style,

$array [] = $newelement;

Is wrong.

It should be

$array[] = $newelement;

But, e.g. in https://tracker.moodle.org/browse/MDL-73808, we fail to detect that.

http://php.net/manual/en/class.jsonserializable.php

Reporter: Dan Poltawski
Original issue: https://tracker.moodle.org/browse/CONTRIB-6469

In #149 we have make mariadb to be sticky to 10.5, because of some incompatible rowformat changes.

Once https://tracker.moodle.org/browse/MDL-72131 is fixed, we have to un-stick it. this issue is about that

I have been trying to figure out how to get the, Exclude, part of Codechecker to work. In the Path(s) to check I am using mod/mootyper and I want to Exclude the subfolder, lessons because it only contains simple text files with many lines longer than 132 characters, so they generate many bogus errors. Every combination of, lessons, I have tried gets ignored and the text files in the folder are scanned and included in the results anyway. In fact, if I include a comma after the word lessons, the contents of the lessons folder is the only thing scanned and listed in the results.

While CodeSniffer is already a great tool I think augmenting it with PHP CS Fixer will allow to catch even more style violations and possible errors.

https://tracker.moodle.org/browse/MDL-43804, deprecates r0 and r1 specific css from core, however we need a way to inform about this to community developers. Would be nice if code checker could look for these in the css files and throw a warning.

Reporter: Ankit Agarwal
Original issue: https://tracker.moodle.org/browse/CONTRIB-4849