Merge pull request #22 from OSInet/21-undeclared_improvements

Undeclared improvements

Author: fgm

qa.module

@@ -8,52 +8,54 @@
  *
  * @since DRUPAL-4-6
  *
- * @license Licensed under the disjunction of the CeCILL, version 2 and General
- *   Public License version 2 and later
+ * @license GPL-2.0-or-later
  *
- * License note: QA is distributed by OSInet to its customers under the
- * CeCILL 2.0 license. OSInet support services only apply to the module
- * when distributed by OSInet, not by any third-party further down the
- * distribution chain.
+ * License note: OSInet support services only apply to the module when
+ * distributed by OSInet, not by any third-party further down the distribution
+ * chain enabled by GPL.
  *
- * If you obtained QA from drupal.org, that site received it under the
- * GPLv2 license and can therefore distribute it under the GPLv2, and
- * so can you and just anyone down the chain as long as the GPLv2 terms
- * are abided by, the module distributor in that case being the
- * drupal.org organization or the downstream distributor, not OSInet.
+ * If you obtained QA from another source, that source received it under the
+ * GPL, and can therefore distribute it under the GPL, and so can you and just
+ * anyone down the chain as long as the GPL terms are abided by, the module
+ * distributor in that case being that source, not OSInet.
  */
 
 use Drupal\Component\Utility\Xss;
+use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Url;
 use Drupal\qa\BaseControl;
 use Drupal\qa\Exportable;
-use Drupal\qa\Variable\Variable;
+use Drupal\qa\Variable;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 
 /**
- * Page callback for qa/dependencies
+ * Page callback for qa/dependencies.
  *
- * TODO convert to native Image_GraphViz to remove dependency on graphviz_filter
- * XXX convert to Grafizzi to remove dependency on Image_GraphViz
+ * TODO convert to native Image_GraphViz to stop depending on graphviz_filter.
+ * XXX convert to Grafizzi to remove dependency on Image_GraphViz.
  *
  * @return string
+ *   A DOT digraph file.
  */
 function qa_page_dependencies() {
   /** @var \Drupal\qa\Dependencies $qaDep */
   $qaDep = Drupal::service('qa.dependencies');
-  $G = $qaDep->build();
-  // passed by reference: cannot pass a function return
-  return graphviz_filter_render($G);
+  $graph = $qaDep->build();
+  // Passed by reference: cannot pass a function return.
+  return graphviz_filter_render($graph);
 }
 
 /**
  * Batch conclusion callback.
  *
- * @param boolean $success
+ * @param bool $success
+ *   Did the batch succeed ?
  * @param array $results
+ *   The batch steps results.
  * @param array $operations
+ *   The operations performed during the batch.
  */
-function qa_report_finished($success, $results, $operations) {
+function qa_report_finished(bool $success, array $results, array $operations) {
   unset($results['#message']);
   if ($success) {
     $message = Drupal::translation()
@@ -77,7 +79,7 @@ function qa_report_results() {
   if (empty($_SESSION['qa_results'])) {
     return new RedirectResponse(Url::fromRoute('qa.reports'));
   }
-  // Work around incomplete classes
+  // Work around incomplete classes.
   $results = unserialize(serialize($_SESSION['qa_results']));
 
   $header = [
@@ -92,12 +94,12 @@ function qa_report_results() {
     $data[] = [
       $control->title,
       $pass->status
-        ? $r->render([
+      ? $r->render([
         '#theme' => 'image',
         '#path' => 'misc/watchdog-ok.png',
         '#alt' => t('OK'),
       ])
-        : $r->render([
+      : $r->render([
         '#theme' => 'image',
         '#path' => 'misc/watchdog-error.png',
         '#alt' => t('Error'),
@@ -114,14 +116,15 @@ function qa_report_results() {
     ],
     '#attached' => ['library' => ['qa/results']],
   ]);
-  // unset($_SESSION['qa_results']);
+  // unset($_SESSION['qa_results']).
   return $ret;
 }
 
 /**
  * Form builder for QA packages/controls selection form.
  *
  * @return array
+ *   A form array.
  */
 function qa_report_form($form, $form_state) {
   $form = [];
@@ -186,12 +189,14 @@ function qa_report_form($form, $form_state) {
 }
 
 /**
- * Submit handler for QA packages/controls selection form
+ * Submit handler for QA packages/controls selection form.
  *
  * @param array $form
- * @param array $form_state
+ *   The form array.
+ * @param \Drupal\Core\Form\FormStateInterface $form_state
+ *   The form state.
  */
-function qa_report_form_submit($form, &$form_state) {
+function qa_report_form_submit(array $form, FormStateInterface $form_state) {
   $controls = [];
   foreach ($form_state['values'] as $item => $value) {
     if (class_exists($item) && is_subclass_of($item,
@@ -218,10 +223,10 @@ function qa_report_form_submit($form, &$form_state) {
     'operations' => [],
     'title' => t('QA Controls running'),
     'init_message' => t('QA Controls initializing'),
-    // 'progress_message' => t('current: @current, Remaining: @remaining, Total: @total'),
+    'progress_message' => t('current: @current, Remaining: @remaining, Total: @total'),
     'error_message' => t('Error in QA Control'),
     'finished' => 'qa_report_finished',
-    // 'file'             => '', // only if outside module file
+    // 'file'             => '', // only if outside module file.
   ];
 
   foreach ($controls as $item => $value) {
@@ -232,10 +237,8 @@ function qa_report_form_submit($form, &$form_state) {
 
 /**
  * Batch progress step.
- *
- * @return void
  */
-function qa_report_run_pass($class_name, &$context) {
+function qa_report_run_pass($class_name, &$context): void {
   $name_arg = ['@class' => $class_name];
 
   $control = new $class_name();
@@ -263,6 +266,15 @@ function qa_report_run_pass($class_name, &$context) {
   }
 }
 
+/**
+ * Load variable by name.
+ *
+ * @param string $name
+ *   The name of the variable.
+ *
+ * @return \Drupal\qa\Variable|false
+ *   The variable if it was found.
+ */
 function qa_variable_load($name) {
   $variable = new Variable($name);
   if (!$variable->is_set) {

qa.services.yml

@@ -3,6 +3,7 @@ services:
     class: Drupal\qa\Plugin\QaCheckManager
     arguments:
       - '@kernel'
+      - '@extension.list.module'
     parent: default_plugin_manager
 
   logger.channel.qa:

src/Annotation/QaCheck.php

@@ -1,6 +1,6 @@
 <?php
 
-declare(strict_types=1);
+declare(strict_types = 1);
 
 namespace Drupal\qa\Annotation;
 

src/Pass.php

@@ -1,6 +1,6 @@
 <?php
 
-declare(strict_types=1);
+declare(strict_types = 1);
 
 namespace Drupal\qa;
 

src/Plugin/QaCheck/Dependencies/FunctionCallVisitor.php

@@ -1,6 +1,6 @@
 <?php
 
-declare(strict_types=1);
+declare(strict_types = 1);
 
 namespace Drupal\qa\Plugin\QaCheck\Dependencies;
 
@@ -33,9 +33,16 @@ public function __construct() {
    * {@inheritdoc}
    */
   public function enterNode(Node $node) {
-    // Method calls need a different handling, using MethodCall.
-    if ($node instanceof FuncCall) {
-      $this->pad[] = implode('\\', $node->name->parts);
+    // Why this test ?
+    // - Method calls need a different handling, using MethodCall.
+    // - Closure calls are named by the variable holding them, and are
+    //   necessarily defined, so don't need to be tracked.
+    if ($node instanceof FuncCall && isset($node->name->parts)) {
+      $func = implode('\\', $node->name->parts);
+      if (!isset($this->pad[$func])) {
+        $this->pad[$func] = [];
+      }
+      $this->pad[$func][] = $node->getAttribute('startLine') ?? '?';
     }
   }
 

src/Plugin/QaCheck/Dependencies/Undeclared.php

@@ -1,6 +1,6 @@
 <?php
 
-declare(strict_types=1);
+declare(strict_types = 1);
 
 namespace Drupal\qa\Plugin\QaCheck\Dependencies;
 
@@ -149,11 +149,11 @@ protected function functionCalls(string $path): array {
     $traverser = new NodeTraverser();
     $traverser->addVisitor($visitor = new FunctionCallVisitor());
     $traverser->traverse($stmts);
-    $pad = array_unique($visitor->pad);
     // Ignore builtin/extension functions.
-    $pad = array_filter($pad, function ($name) {
-      return empty($this->qam->internalFunctions[$name]);
-    });
+    $pad = array_filter($visitor->pad, function ($name) {
+      $isInternal = isset($this->qam->internalFunctions[$name]);
+      return !$isInternal;
+    }, ARRAY_FILTER_USE_KEY);
     return $pad;
   }
 
@@ -170,19 +170,22 @@ protected function functionCalls(string $path): array {
    */
   protected function moduleActualDependencies(string $name, array $calls): array {
     $modules = [];
-    foreach ($calls as $called) {
+    foreach ($calls as $called => $lines) {
       try {
         $rf = new \ReflectionFunction($called);
       }
       catch (\ReflectionException $e) {
-        $modules[] = "(${called})";
+        $modules["(${called})"][$called] = $lines;
         continue;
       }
 
       // Drupal name-based magic.
       $module = basename($rf->getFileName(), '.module');
       if ($module !== $name) {
-        $modules[] = $module;
+        if (!isset($modules[$module][$called])) {
+          $modules[$module][$called] = [];
+        }
+        $modules[$module][$called] += $lines;
       }
     }
     return $modules;
@@ -198,14 +201,10 @@ protected function moduleActualDependencies(string $name, array $calls): array {
    *   A map of modules names by module name.
    */
   protected function moduleDeclaredDependencies(string $name): array {
-    $info = $this->elm->getExtensionInfo($name);
-    $deps = $info['dependencies'] ?? [];
-    $res = [];
-    foreach ($deps as $dep) {
-      $ar = explode(":", $dep);
-      $res[] = array_pop($ar);
-    }
-    return $res;
+    $ext = $this->elm->get($name);
+    // XXX Undocumented API field.
+    $deps = array_keys($ext->requires);
+    return $deps;
   }
 
   /**
@@ -226,7 +225,7 @@ public function check(): Result {
       $calls = $this->functionCalls($path);
       $actual = $this->moduleActualDependencies($module, $calls);
       $declared = $this->moduleDeclaredDependencies($module);
-      $missing = array_diff($actual, $declared);
+      $missing = array_diff_key($actual, array_flip($declared));
       if (!empty($missing)) {
         $undeclared[$module] = $missing;
       }

src/Plugin/QaCheck/References/Integrity.php

@@ -1,6 +1,6 @@
 <?php
 
-declare(strict_types=1);
+declare(strict_types = 1);
 
 namespace Drupal\qa\Plugin\QaCheck\References;
 

src/Plugin/QaCheck/References/TaxonomyIndex.php

@@ -1,6 +1,6 @@
 <?php
 
-declare(strict_types=1);
+declare(strict_types = 1);
 
 namespace Drupal\qa\Plugin\QaCheck\References;
 

src/Plugin/QaCheck/System/ExternalCode.php

@@ -1,6 +1,6 @@
 <?php
 
-declare(strict_types=1);
+declare(strict_types = 1);
 
 namespace Drupal\qa\Plugin\QaCheck\System;
 

src/Plugin/QaCheckManager.php

@@ -4,6 +4,8 @@
 
 use Drupal\Component\Discovery\DiscoveryException;
 use Drupal\Core\DrupalKernelInterface;
+use Drupal\Core\Extension\Extension;
+use Drupal\Core\Extension\ModuleExtensionList;
 use Drupal\Core\Plugin\DefaultPluginManager;
 use Drupal\Core\Cache\CacheBackendInterface;
 use Drupal\Core\Extension\ModuleHandlerInterface;
@@ -39,6 +41,13 @@ class QaCheckManager extends DefaultPluginManager {
    */
   protected $vendor;
 
+  /**
+   * The extension.list.module service.
+   *
+   * @var \Drupal\Core\Extension\ModuleExtensionList
+   */
+  protected $elm;
+
   /**
    * Constructs a new QaCheckManager object.
    *
@@ -51,15 +60,19 @@ class QaCheckManager extends DefaultPluginManager {
    *   The module handler to invoke the alter hook with.
    * @param \Drupal\Core\DrupalKernelInterface $kernel
    *   The kernel service.
+   * @param \Drupal\Core\Extension\ModuleExtensionList $elm
+   *   The extension.list.module service.
    */
   public function __construct(
     Traversable $namespaces,
     CacheBackendInterface $cache_backend,
     ModuleHandlerInterface $module_handler,
-    DrupalKernelInterface $kernel
+    DrupalKernelInterface $kernel,
+    ModuleExtensionList $elm
   ) {
     parent::__construct('Plugin/QaCheck', $namespaces, $module_handler,
       QaCheckInterface::class, QaCheck::class);
+    $this->elm = $elm;
     $this->root = realpath($kernel->getAppRoot());
     $this->vendor = self::getVendorDir();
 
@@ -176,6 +189,16 @@ public function isInternal($file) {
       $this->vendor,
     ];
 
+    $list = $this->elm->getList();
+    $required = [];
+    array_walk($list, function (Extension $ext) use (&$required) {
+      // XXX Undocumented API field.
+      if (!empty($ext->info['required'])) {
+        $required[] = $this->root . '/' . $ext->getPath();
+      }
+    });
+    $internal = array_merge($internal, $required);
+
     foreach ($internal as $root) {
       if (strpos($file, $root) !== FALSE) {
         return TRUE;