Skip to content

Commit

Permalink
Make the sniff to also detect unexpected MOODLE_INTERNAL checks
Browse files Browse the repository at this point in the history
Until now, the Sniff was only finding missing MOODLE_INTERNAL checks
(when the file has side effects OR multiple artifacts).

Now, when the check exists and the file does not have side effects
or multiple artifacts, a new MoodleInternalNotNeeded is thrown.

Also, when the old MOODLE_INTERNAL check is detected, a new
MoodleInternalOld warning is thrown.

Covered with tests.

Finally, all the tests related with this Sniff have been moved
to a different test case and all the fixtures moved to new
subdirectory. Part of plans to split the tests completely, with
each sniff having its own testcase.

Fixes #152
  • Loading branch information
stronk7 committed Jan 4, 2022
1 parent 540b12f commit 2b019ce
Show file tree
Hide file tree
Showing 16 changed files with 256 additions and 156 deletions.
59 changes: 42 additions & 17 deletions moodle/Sniffs/Files/MoodleInternalSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,32 +81,57 @@ public function process(File $file, $pointer) {
// Find where real code is and check from there.
$pointer = $this->get_position_of_relevant_code($file, $pointer);

// OK, we've got to the first bit of relevant code.
if ($this->is_moodle_internal_or_die_check($file, $pointer)) {
// There is a MOODLE_INTERNAL check. This file is good, hurrah!
return;
}
if ($this->is_config_php_incluson($file, $pointer)) {
// We are requiring config.php. This file is good, hurrah!
return;
}

if ($this->is_if_not_moodle_internal_die_check($file, $pointer)) {
// It's an old-skool MOODLE_INTERNAL check. This file is good, hurrah!
return;
$hasMoodleInternal = false;
$isOldMoodleInternal = false;
$sideEffectsPointer = $pointer;

// OK, we've got to the first bit of relevant code. It must be the MOODLE_INTERNAL check.
if ($this->is_moodle_internal_or_die_check($file, $pointer)) {
$hasMoodleInternal = true;
// Let's look for side effects after the check.
$sideEffectsPointer = $file->findNext(T_SEMICOLON, $pointer) + 1;

} else if ($this->is_if_not_moodle_internal_die_check($file, $pointer)) {
$hasMoodleInternal = true;
$isOldMoodleInternal = true;
// Let's look for side effects after the check.
$sideEffectsPointer = $file->getTokens()[$pointer]['scope_closer'] + 1;
}

// Got here because, so something is not right.
if ($this->code_changes_global_state($file, $pointer, ($file->numTokens - 1))) {
$hasSideEffects = $this->code_changes_global_state($file, $sideEffectsPointer, ($file->numTokens - 1));
$hasMultipleArtifacts = ($this->count_artifacts($file) > 1);

// Missing MOODLE_INTERNAL and having side effects, error.
if (!$hasMoodleInternal && $hasSideEffects) {
$file->addError('Expected MOODLE_INTERNAL check or config.php inclusion. Change in global state detected.',
$pointer, 'MoodleInternalGlobalState');
} else {
// Only if there are more than one artifact (class, interface, trait), we show the warning.
// (files with only one, are allowed to be MOODLE_INTERNAL free - MDLSITE-5967).
if ($this->count_artifacts($file) > 1) {
$file->addWarning('Expected MOODLE_INTERNAL check or config.php inclusion. Multiple artifacts detected.',
$pointer, 'MoodleInternalMultipleArtifacts');
}
return;
}

// Missing MOODLE_INTERNAL, not having side effects, but having multiple artifacts, warning.
if (!$hasMoodleInternal && !$hasSideEffects && $hasMultipleArtifacts) {
$file->addWarning('Expected MOODLE_INTERNAL check or config.php inclusion. Multiple artifacts detected.',
$pointer, 'MoodleInternalMultipleArtifacts');
return;
}

// Having MOODLE_INTERNAL, not having side effects and not having multiple artifacts, error.
if ($hasMoodleInternal && !$hasSideEffects && !$hasMultipleArtifacts) {
$file->addError('Unexpected MOODLE_INTERNAL check. No side effects or multiple artifacts detected.',
$pointer, 'MoodleInternalNotNeeded');
return;
}

// Having old MOODLE_INTERNAL check, warn.
if ($hasMoodleInternal && $isOldMoodleInternal) {
$file->addWarning('Old MOODLE_INTERNAL check detected. Replace it by "defined(\'MOODLE_INTERNAL\') || die();"',
$pointer, 'MoodleInternalOld');
return;
}
}

Expand Down
184 changes: 184 additions & 0 deletions moodle/tests/files_moodleinternal_test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.

namespace local_codechecker;

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

require_once(__DIR__ . '/../../tests/local_codechecker_testcase.php');

// phpcs:disable moodle.NamingConventions

/**
* Test the MoodleInternalSniff sniff.
*
* @package local_codechecker
* @category test
* @copyright 2013 onwards Eloy Lafuente (stronk7) {@link http://stronk7.com}
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*
* @covers \MoodleCodeSniffer\moodle\Sniffs\Files\MoodleInternalSniff
*/
class files_moodleinternal_test extends local_codechecker_testcase {

public function test_moodle_files_moodleinternal_problem() {
$this->set_standard('moodle');
$this->set_sniff('moodle.Files.MoodleInternal');
$this->set_fixture(__DIR__ . '/fixtures/files/moodleinternal/problem.php');

// Define expected results (errors and warnings). Format, array of:
// - line => number of problems, or
// - line => array of contents for message / source problem matching.
// - line => string of contents for message / source problem matching (only 1).
$this->set_errors([
19 => 'Expected MOODLE_INTERNAL check or config.php inclusion'
]);
$this->set_warnings([]);

$this->verify_cs_results();
}

public function test_moodle_files_moodleinternal_warning() {
$this->set_standard('moodle');
$this->set_sniff('moodle.Files.MoodleInternal');
$this->set_fixture(__DIR__ . '/fixtures/files/moodleinternal/warning.php');

$this->set_errors([]);
$this->set_warnings([
32 => 'Expected MOODLE_INTERNAL check or config.php inclusion. Multiple artifacts'
]);

$this->verify_cs_results();
}

public function test_moodle_files_moodleinternal_nowarning() {
$this->set_standard('moodle');
$this->set_sniff('moodle.Files.MoodleInternal');
$this->set_fixture(__DIR__ . '/fixtures/files/moodleinternal/nowarning.php');

$this->set_errors([]);
$this->set_warnings([]);

$this->verify_cs_results();
}

public function test_moodle_files_moodleinternal_declare_ok() {
$this->set_standard('moodle');
$this->set_sniff('moodle.Files.MoodleInternal');
$this->set_fixture(__DIR__ . '/fixtures/files/moodleinternal/declare_ok.php');

$this->set_errors([]);
$this->set_warnings([]);

$this->verify_cs_results();
}

public function test_moodle_files_moodleinternal_namespace_ok() {
$this->set_standard('moodle');
$this->set_sniff('moodle.Files.MoodleInternal');
$this->set_fixture(__DIR__ . '/fixtures/files/moodleinternal/namespace_ok.php');

$this->set_errors([]);
$this->set_warnings([]);

$this->verify_cs_results();
}

public function test_moodle_files_moodleinternal_no_moodle_cookie_ok() {
$this->set_standard('moodle');
$this->set_sniff('moodle.Files.MoodleInternal');
$this->set_fixture(__DIR__ . '/fixtures/files/moodleinternal/no_moodle_cookie_ok.php');

$this->set_errors([]);
$this->set_warnings([]);

$this->verify_cs_results();
}

public function test_moodle_files_moodleinternal_behat_skipped() {
// Files in /tests/behat/ dirs are ignored.
$this->set_standard('moodle');
$this->set_sniff('moodle.Files.MoodleInternal');
$this->set_fixture(__DIR__ . '/fixtures/files/moodleinternal/tests/behat/behat_mod_workshop.php');

$this->set_errors([]);
$this->set_warnings([]);

$this->verify_cs_results();

// Files in /lib/behat/ dirs are ignored.
$this->set_standard('moodle');
$this->set_sniff('moodle.Files.MoodleInternal');
$this->set_fixture(__DIR__ . '/fixtures/files/moodleinternal/lib/behat/behat_mod_workshop.php');

$this->set_errors([]);
$this->set_warnings([]);

$this->verify_cs_results();
}

public function test_moodle_files_moodleinternal_lang_skipped() {
// Files in lang dirs are ignored.
$this->set_standard('moodle');
$this->set_sniff('moodle.Files.MoodleInternal');
$this->set_fixture(__DIR__ . '/fixtures/files/moodleinternal/lang/en/repository_dropbox.php');

$this->set_errors([]);
$this->set_warnings([]);

$this->verify_cs_results();
}

public function test_moodle_files_moodleinternal_namespace_with_use_ok() {
// An edge case which allows use statements before defined().
$this->set_standard('moodle');
$this->set_sniff('moodle.Files.MoodleInternal');
$this->set_fixture(__DIR__ . '/fixtures/files/moodleinternal/namespace_with_use_ok.php');

$this->set_errors([]);
$this->set_warnings([]);

$this->verify_cs_results();
}

public function test_moodle_files_moodleinternal_old_style_if_die() {
// Old style if statement MOODLE_INTERNAL check.
$this->set_standard('moodle');
$this->set_sniff('moodle.Files.MoodleInternal');
$this->set_fixture(__DIR__ . '/fixtures/files/moodleinternal/old_style_if_die_ok.php');

$this->set_errors([]);
$this->set_warnings([
24 => 'Old MOODLE_INTERNAL check detected. Replace it by',
]);

$this->verify_cs_results();
}

public function test_moodle_files_moodleinternal_unexpected() {
// Old style if statement MOODLE_INTERNAL check.
$this->set_standard('moodle');
$this->set_sniff('moodle.Files.MoodleInternal');
$this->set_fixture(__DIR__ . '/fixtures/files/moodleinternal/unexpected.php');

$this->set_errors([
17 => 'MoodleInternalNotNeeded'
]);
$this->set_warnings([]);

$this->verify_cs_results();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,4 @@
namespace core;
use this\is\that;

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

class something { }
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
namespace core;
defined('MOODLE_INTERNAL') || die();

require_once('something.php');

/**
* Chart line class.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,6 @@

use core\plugininfo\base;

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


class workshopallocation extends base {
public function is_uninstall_allowed() {
if ($this->is_standard()) {
Expand Down
26 changes: 26 additions & 0 deletions moodle/tests/fixtures/files/moodleinternal/unexpected.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.

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

/**
* Chart line class.
*
* @package core
* @copyright 2016 Frédéric Massart - FMCorz.net
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
interface chart { }
Loading

0 comments on commit 2b019ce

Please sign in to comment.