From 67af57c862bc662ba7d48f89bd1fe8cc9f339c6a Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Tue, 16 Feb 2021 18:03:43 +0100 Subject: [PATCH] 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 https://github.com/moodlehq/moodle-local_moodlecheck/issues/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). --- file.php | 67 +++++++++++++++----- rules/phpdocs_basic.php | 5 ++ tests/fixtures/phpdoc_file_required_no1.php | 24 +++++++ tests/fixtures/phpdoc_file_required_no2.php | 24 +++++++ tests/fixtures/phpdoc_file_required_no3.php | 24 +++++++ tests/fixtures/phpdoc_file_required_yes1.php | 29 +++++++++ tests/fixtures/phpdoc_file_required_yes2.php | 23 +++++++ tests/fixtures/phpdoc_file_required_yes3.php | 29 +++++++++ tests/moodlecheck_rules_test.php | 39 ++++++++++++ 9 files changed, 247 insertions(+), 17 deletions(-) create mode 100644 tests/fixtures/phpdoc_file_required_no1.php create mode 100644 tests/fixtures/phpdoc_file_required_no2.php create mode 100644 tests/fixtures/phpdoc_file_required_no3.php create mode 100644 tests/fixtures/phpdoc_file_required_yes1.php create mode 100644 tests/fixtures/phpdoc_file_required_yes2.php create mode 100644 tests/fixtures/phpdoc_file_required_yes3.php diff --git a/file.php b/file.php index 2c63472..ba8ddeb 100644 --- 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 index 722543f..096b8f0 100644 --- 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 new file mode 100644 index 0000000..98f1a57 --- /dev/null +++ b/tests/fixtures/phpdoc_file_required_no1.php @@ -0,0 +1,24 @@ +. + +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 new file mode 100644 index 0000000..1c40b9a --- /dev/null +++ b/tests/fixtures/phpdoc_file_required_no2.php @@ -0,0 +1,24 @@ +. + +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 new file mode 100644 index 0000000..b1b6323 --- /dev/null +++ b/tests/fixtures/phpdoc_file_required_no3.php @@ -0,0 +1,24 @@ +. + +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 new file mode 100644 index 0000000..e4fc5d8 --- /dev/null +++ b/tests/fixtures/phpdoc_file_required_yes1.php @@ -0,0 +1,29 @@ +. + +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 new file mode 100644 index 0000000..8cc73f9 --- /dev/null +++ b/tests/fixtures/phpdoc_file_required_yes2.php @@ -0,0 +1,23 @@ +. + +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 new file mode 100644 index 0000000..70a593c --- /dev/null +++ b/tests/fixtures/phpdoc_file_required_yes3.php @@ -0,0 +1,29 @@ +. + +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 index c1820f7..8f88889 100644 --- 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. */