Stop requiring the file phpdoc block for 1-artifact files

Now we detect all the classes, interfaces and traits in a file and, if there is only one, then we stop requiring the file phpdoc block. This was agreed @ https://tracker.moodle.org/browse/MDLSITE-2804 and #66 was created to implement it. Note this doesn't address the whole agreement, but just the detail about stop requiring the file phpdoc block, because it is specially annoying and recurring in new files (that use to be 1-artifact ones).
moodlehq · Feb 16, 2021 · 67af57c · 67af57c
1 parent b69ad66
commit 67af57c
Show file tree

Hide file tree

Showing 9 changed files with 247 additions and 17 deletions.
diff --git a/file.php b/file.php
@@ -38,6 +38,8 @@ class local_moodlecheck_file {
     protected $tokens = null;
     protected $tokenscount = 0;
     protected $classes = null;
+    protected $interfaces = null;
+    protected $traits = null;
     protected $functions = null;
     protected $filephpdocs = null;
     protected $allphpdocs = null;
@@ -61,6 +63,8 @@ protected function clear_memory() {
         $this->tokens = null;
         $this->tokenscount = 0;
         $this->classes = null;
+        $this->interfaces = null;
+        $this->traits = null;
         $this->functions = null;
         $this->filephpdocs = null;
         $this->allphpdocs = null;
@@ -172,22 +176,27 @@ public function &get_tokens() {
     }
 
     /**
-     * Returns all classes found in file
+     * Returns all artifacts (classes, interfaces, traits) found in file
      *
-     * Returns array of objects where each element represents a class:
-     * $class->name : name of the class
-     * $class->tagpair : array of two elements: id of token { for the class and id of token } (false if not found)
-     * $class->phpdocs : phpdocs for this class (instance of local_moodlecheck_phpdocs or false if not found)
-     * $class->boundaries : array with ids of first and last token for this class
+     * Returns 3 arrays (classes, interfaces and traits) of objects where each element represents an artifact:
+     * ->type : token type of the artifact (T_CLASS, T_INTERFACE, T_TRAIT)
+     * ->name : name of the artifact
+     * ->tagpair : array of two elements: id of token { for the class and id of token } (false if not found)
+     * ->phpdocs : phpdocs for this artifact (instance of local_moodlecheck_phpdocs or false if not found)
+     * ->boundaries : array with ids of first and last token for this artifact.
      *
-     * @return array
+     * @return array with 3 elements (classes, interfaces & traits), each being an array.
      */
-    public function &get_classes() {
+    public function get_artifacts() {
+        $types = array(T_CLASS, T_INTERFACE, T_TRAIT); // We are interested on these.
+        $artifacts = array_combine($types, $types);
         if ($this->classes === null) {
             $this->classes = array();
+            $this->interfaces = array();
+            $this->traits = array();
             $tokens = &$this->get_tokens();
             for ($tid = 0; $tid < $this->tokenscount; $tid++) {
-                if ($this->tokens[$tid][0] == T_CLASS) {
+                if (isset($artifacts[$this->tokens[$tid][0]])) {
                     if ($this->previous_nonspace_token($tid) === "::") {
                         // Skip use of the ::class special constant.
                         continue;
@@ -211,17 +220,41 @@ public function &get_classes() {
                             continue;
                         }
                     }
-                    $class = new stdClass();
-                    $class->tid = $tid;
-                    $class->name = $this->next_nonspace_token($tid);
-                    $class->phpdocs = $this->find_preceeding_phpdoc($tid);
-                    $class->tagpair = $this->find_tag_pair($tid, '{', '}');
-                    $class->boundaries = $this->find_object_boundaries($class);
-                    $this->classes[] = $class;
+                    $artifact = new stdClass();
+                    $artifact->type = $artifacts[$this->tokens[$tid][0]];
+                    $artifact->tid = $tid;
+                    $artifact->name = $this->next_nonspace_token($tid);
+                    $artifact->phpdocs = $this->find_preceeding_phpdoc($tid);
+                    $artifact->tagpair = $this->find_tag_pair($tid, '{', '}');
+                    $artifact->boundaries = $this->find_object_boundaries($artifact);
+                    switch ($artifact->type) {
+                        case T_CLASS:
+                            $this->classes[] = $artifact;
+                            break;
+                        case T_INTERFACE:
+                            $this->interfaces[] = $artifact;
+                            break;
+                        case T_TRAIT:
+                            $this->traits[] = $artifact;
+                            break;
+                    }
                 }
             }
         }
-        return $this->classes;
+        return array(T_CLASS => $this->classes, T_INTERFACE => $this->interfaces, T_TRAIT => $this->traits);
+    }
+
+    /**
+     * Returns all classes found in file
+     *
+     * Returns array of objects where each element represents a class:
+     * $class->name : name of the class
+     * $class->tagpair : array of two elements: id of token { for the class and id of token } (false if not found)
+     * $class->phpdocs : phpdocs for this class (instance of local_moodlecheck_phpdocs or false if not found)
+     * $class->boundaries : array with ids of first and last token for this class
+     */
+    public function &get_classes() {
+        return $this->get_artifacts()[T_CLASS];
     }
 
     /**

diff --git a/rules/phpdocs_basic.php b/rules/phpdocs_basic.php
@@ -70,6 +70,11 @@ function local_moodlecheck_noemptysecondline(local_moodlecheck_file $file) {
  * @return array of found errors
  */
 function local_moodlecheck_filephpdocpresent(local_moodlecheck_file $file) {
+    // This rule doesn't apply if the file is 1-artifact file (see #66).
+    $artifacts = $file->get_artifacts();
+    if (count($artifacts[T_CLASS]) + count($artifacts[T_INTERFACE]) + count($artifacts[T_TRAIT]) === 1) {
+        return array();
+    }
     if ($file->find_file_phpdocs() === false) {
         $tokens = &$file->get_tokens();
         for ($i = 0; $i < 30; $i++) {

diff --git a/tests/fixtures/phpdoc_file_required_no1.php b/tests/fixtures/phpdoc_file_required_no1.php
@@ -0,0 +1,24 @@
+<?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();
+
+/**
+ * This is a dummy class without any tags.
+ */
+class dummy_class_without_tags {
+    // As far as this is an 1-artifact file, the file phpdoc block is not required.
+}
diff --git a/tests/fixtures/phpdoc_file_required_no2.php b/tests/fixtures/phpdoc_file_required_no2.php
@@ -0,0 +1,24 @@
+<?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();
+
+/**
+ * This is a dummy interface without any tags.
+ */
+interface dummy_class_without_tags {
+    // As far as this is an 1-artifact file, the file phpdoc block is not required.
+}
diff --git a/tests/fixtures/phpdoc_file_required_no3.php b/tests/fixtures/phpdoc_file_required_no3.php
@@ -0,0 +1,24 @@
+<?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();
+
+/**
+ * This is a dummy trait without any tags.
+ */
+trait dummy_class_without_tags {
+    // As far as this is an 1-artifact file, the file phpdoc block is not required.
+}
diff --git a/tests/fixtures/phpdoc_file_required_yes1.php b/tests/fixtures/phpdoc_file_required_yes1.php
@@ -0,0 +1,29 @@
+<?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();
+
+/**
+ * This is a dummy class without any tags.
+ */
+class dummy_class_without_tags {
+}
+
+/**
+ * This is another dummy class without any tags. Hence, file phpdoc block is required.
+ */
+class dummy2_class_without_tags {
+}
diff --git a/tests/fixtures/phpdoc_file_required_yes2.php b/tests/fixtures/phpdoc_file_required_yes2.php
@@ -0,0 +1,23 @@
+<?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();
+
+// This is library-style php file, no classes around.
+
+function dummy_function_without_tags() {
+    // No classes, hence the file phpdoc block is required.
+}
diff --git a/tests/fixtures/phpdoc_file_required_yes3.php b/tests/fixtures/phpdoc_file_required_yes3.php
@@ -0,0 +1,29 @@
+<?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();
+
+/**
+ * This is a dummy interface without any tags.
+ */
+interface dummy_class_without_tags {
+}
+
+/**
+ * This is a dummy trait without any tags. Hence, file phpdoc block is required.
+ */
+trait dummy2_class_without_tags {
+}
diff --git a/tests/moodlecheck_rules_test.php b/tests/moodlecheck_rules_test.php
@@ -75,6 +75,45 @@ public function test_constantclass() {
         $this->assertStringNotContainsString('classesdocumented', $result);
     }
 
+    /**
+     * Assert that the file block is required for old files, and not for 1-artifact ones.
+     */
+    public function test_file_block_required() {
+        global $PAGE;
+
+        $output = $PAGE->get_renderer('local_moodlecheck');
+
+        // A file with multiple classes, require the file phpdoc block.
+        $path = new local_moodlecheck_path('local/moodlecheck/tests/fixtures/phpdoc_file_required_yes1.php', null);
+        $result = $output->display_path($path, 'xml');
+        $this->assertStringContainsString('File-level phpdocs block is not found', $result);
+
+        // A file without any class (library-like), require the file phpdoc block.
+        $path = new local_moodlecheck_path('local/moodlecheck/tests/fixtures/phpdoc_file_required_yes2.php', null);
+        $result = $output->display_path($path, 'xml');
+        $this->assertStringContainsString('File-level phpdocs block is not found', $result);
+
+        // A file with one interface and one trait, require the file phpdoc block.
+        $path = new local_moodlecheck_path('local/moodlecheck/tests/fixtures/phpdoc_file_required_yes3.php', null);
+        $result = $output->display_path($path, 'xml');
+        $this->assertStringContainsString('File-level phpdocs block is not found', $result);
+
+        // A file with only one class, do not require the file phpdoc block.
+        $path = new local_moodlecheck_path('local/moodlecheck/tests/fixtures/phpdoc_file_required_no1.php', null);
+        $result = $output->display_path($path, 'xml');
+        $this->assertStringNotContainsString('File-level phpdocs block is not found', $result);
+
+        // A file with only one interface, do not require the file phpdoc block.
+        $path = new local_moodlecheck_path('local/moodlecheck/tests/fixtures/phpdoc_file_required_no2.php', null);
+        $result = $output->display_path($path, 'xml');
+        $this->assertStringNotContainsString('File-level phpdocs block is not found', $result);
+
+        // A file with only one trait, do not require the file phpdoc block.
+        $path = new local_moodlecheck_path('local/moodlecheck/tests/fixtures/phpdoc_file_required_no3.php', null);
+        $result = $output->display_path($path, 'xml');
+        $this->assertStringNotContainsString('File-level phpdocs block is not found', $result);
+    }
+
     /**
      * Assert that classes do not need to have any particular phpdocs tags.
      */