Merge pull request #169 from stronk7/unexpected_moodle_internal

Make the sniff to also detect unexpected MOODLE_INTERNAL checks

moodle/Sniffs/Files/MoodleInternalSniff.php

@@ -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;
         }
     }
 

moodle/tests/files_moodleinternal_test.php

@@ -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();
+    }
+}

moodle/tests/fixtures/moodle_files_moodleinternal/declare_ok.php → moodle/tests/fixtures/files/moodleinternal/declare_ok.php

@@ -29,6 +29,4 @@
 namespace core;
 use this\is\that;
 
-defined('MOODLE_INTERNAL') || die();
-
 class something { }

moodle/tests/fixtures/moodle_files_moodleinternal/namespace_ok.php → moodle/tests/fixtures/files/moodleinternal/namespace_ok.php

@@ -25,6 +25,8 @@
 namespace core;
 defined('MOODLE_INTERNAL') || die();
 
+require_once('something.php');
+
 /**
  * Chart line class.
  *

moodle/tests/fixtures/moodle_files_moodleinternal/namespace_with_use_ok.php → moodle/tests/fixtures/files/moodleinternal/namespace_with_use_ok.php

@@ -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()) {

moodle/tests/fixtures/files/moodleinternal/unexpected.php

@@ -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 { }