Make \Drupal\views\Plugin\views\HandlerBase unit-testable

Initial patch. There will likely be more changes.

I added wrappers to get the module handler and Views data services, and replaced the calls to procedural string manipulation functions with calls to their OOP replacements.

3: drupal_2174021_3.patch queued for re-testing.

Isn't the policy to wrap calls to \Drupal in protected methods, store the retrieved services in protected properties, and test by building a custom container in the test (#2171315: Ensure a NULL container in UnitTestCase::setUP proves that's a bad idea) or mock the class under test and let the wrapper methods return mocked services rather than calling \Drupal::service()?

I agree on using setter injection if we really can't use constructor injection, but seeing as such setters are only relevant for tests and this patch does not actually introduce a test, I'm honestly not sure whether we should add those setters in this patch.

which we could do in our plugin/handler manager automatically

Adding implementation logic to plugin managers sounds like a Very Bad Idea™, especially because that would also require us to extend interfaces with setters, which completely defeats the purpose of having those interfaces in the first place (separation of contract and implementation).

On the longrun I would suggest to use setter injection here, which we could do in our plugin/handler manager automatically

If the manager calls methods on plugins, those plugins must implement an interface that defines those methods, otherwise we would be relying on implementation details. Setter methods for dependencies should not belong on interfaces, as dependencies are more often than not implementation details, and we can't couple those to an interface.

Back to the last patch, what is your verdict?

Re-roll.

So much for decoupling interfaces and implementations, but if this it what it takes to make this unit-testable, let's get it over with.

Sure. I just don't think I'm allowed to RTBC this, seeing as I wrote most of the code. The honor is therefore all yours :)

Victory!

Re-roll.

diff a/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.php b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.php	(rejected hunks)
@@ -169,11 +186,11 @@ public function isOptional() {
    */
   public function adminLabel($short = FALSE) {
     if (!empty($this->options['admin_label'])) {
-      $title = check_plain($this->options['admin_label']);
+      $title = String::checkPlain($this->options['admin_label']);
       return $title;
     }
     $title = ($short && isset($this->definition['title short'])) ? $this->definition['title short'] : $this->definition['title'];
-    return t('!group: !title', array('!group' => $this->definition['group'], '!title' => $title));
+    return $this->t('!group: !title', array('!group' => $this->definition['group'], '!title' => $title));
   }
 
   /**

RTBC, provided the tests pass.

Re-roll. RTBC, provided that the tests pass.

RTBC, as this was the reject:

diff a/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.php b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.php	(rejected hunks)
@@ -11,16 +11,33 @@
 use Drupal\Component\Utility\Unicode;
 use Drupal\Component\Utility\Url;
 use Drupal\Component\Utility\Xss;
+use Drupal\Core\Extension\ModuleHandlerInterface;
+use Drupal\Core\Render\Element;
 use Drupal\Core\Session\AccountInterface;
 use Drupal\views\Plugin\views\display\DisplayPluginBase;
 use Drupal\views\Plugin\views\PluginBase;
 use Drupal\views\ViewExecutable;
 use Drupal\Core\Database\Database;
 use Drupal\views\Views;
+use Drupal\views\ViewsData;
 
 abstract class HandlerBase extends PluginBase {
 
   /**
+   * The module handler.
+   *
+   * @var \Drupal\Core\Extension\ModuleHandlerInterface
+   */
+  protected $moduleHandler;
+
+  /**
+   * The views data service.
+   *
+   * @var \Drupal\views\ViewsData
+   */
+  protected $viewsData;
+
+  /**
    * Where the $query object will reside:
    *
    * @var \Drupal\views\Plugin\views\query\QueryPluginBase

Re-roll. RTBC, under the condition that the tests pass, because the rejects were only about injecting the module handler into ViewsHandlerManager, which is already done in HEAD these days.

The rejects:

diff a/core/modules/views/views.services.yml b/core/modules/views/views.services.yml	(rejected hunks)
@@ -4,10 +4,10 @@ services:
     arguments: [access, '@container.namespaces', '@cache.views_info', '@language_manager', '@module_handler']
   plugin.manager.views.area:
     class: Drupal\views\Plugin\ViewsHandlerManager
-    arguments: [area, '@container.namespaces', '@views.views_data']
+    arguments: [area, '@container.namespaces', '@views.views_data', '@module_handler']
   plugin.manager.views.argument:
     class: Drupal\views\Plugin\ViewsHandlerManager
-    arguments: [argument, '@container.namespaces', '@views.views_data']
+    arguments: [argument, '@container.namespaces', '@views.views_data', '@module_handler']
   plugin.manager.views.argument_default:
     class: Drupal\views\Plugin\ViewsPluginManager
     arguments: [argument_default, '@container.namespaces', '@cache.views_info', '@language_manager', '@module_handler']
@@ -28,13 +28,13 @@ services:
     arguments: [exposed_form, '@container.namespaces', '@cache.views_info', '@language_manager', '@module_handler']
   plugin.manager.views.field:
     class: Drupal\views\Plugin\ViewsHandlerManager
-    arguments: [field, '@container.namespaces', '@views.views_data']
+    arguments: [field, '@container.namespaces', '@views.views_data', '@module_handler']
   plugin.manager.views.filter:
     class: Drupal\views\Plugin\ViewsHandlerManager
-    arguments: [filter, '@container.namespaces', '@views.views_data']
+    arguments: [filter, '@container.namespaces', '@views.views_data', '@module_handler']
   plugin.manager.views.join:
     class: Drupal\views\Plugin\ViewsHandlerManager
-    arguments: [join, '@container.namespaces', '@views.views_data']
+    arguments: [join, '@container.namespaces', '@views.views_data', '@module_handler']
   plugin.manager.views.pager:
     class: Drupal\views\Plugin\ViewsPluginManager
     arguments: [pager, '@container.namespaces', '@cache.views_info', '@language_manager', '@module_handler']
@@ -43,13 +43,13 @@ services:
     arguments: [query, '@container.namespaces', '@cache.views_info', '@language_manager', '@module_handler']
   plugin.manager.views.relationship:
     class: Drupal\views\Plugin\ViewsHandlerManager
-    arguments: [relationship, '@container.namespaces', '@views.views_data']
+    arguments: [relationship, '@container.namespaces', '@views.views_data', '@module_handler']
   plugin.manager.views.row:
     class: Drupal\views\Plugin\ViewsPluginManager
     arguments: [row, '@container.namespaces', '@cache.views_info', '@language_manager', '@module_handler']
   plugin.manager.views.sort:
     class: Drupal\views\Plugin\ViewsHandlerManager
-    arguments: [sort, '@container.namespaces', '@views.views_data']
+    arguments: [sort, '@container.namespaces', '@views.views_data', '@module_handler']
   plugin.manager.views.style:
     class: Drupal\views\Plugin\ViewsPluginManager
     arguments: [style, '@container.namespaces', '@cache.views_info', '@language_manager', '@module_handler']

diff a/core/modules/views/lib/Drupal/views/Plugin/ViewsHandlerManager.php b/core/modules/views/lib/Drupal/views/Plugin/ViewsHandlerManager.php	(rejected hunks)
@@ -9,9 +9,11 @@
 
 use Drupal\Component\Plugin\Exception\PluginException;
 use Drupal\Component\Plugin\PluginManagerBase;
+use Drupal\Core\Extension\ModuleHandlerInterface;
 use Drupal\Core\Plugin\Factory\ContainerFactory;
 use Drupal\Core\Plugin\Discovery\CacheDecorator;
 use Drupal\views\Plugin\Discovery\ViewsHandlerDiscovery;
+use Drupal\views\Plugin\views\HandlerBase;
 use Drupal\views\ViewsData;
 
 /**
@@ -44,9 +53,11 @@ class ViewsHandlerManager extends PluginManagerBase {
    *   An object that implements \Traversable which contains the root paths
    *   keyed by the corresponding namespace to look for plugin implementations,
    * @param \Drupal\views\ViewsData $views_data
-    *   The views data cache.
+   *   The views data cache.
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
+   *   The module handler.
    */
-  public function __construct($handler_type, \Traversable $namespaces, ViewsData $views_data) {
+  public function __construct($handler_type, \Traversable $namespaces, ViewsData $views_data, ModuleHandlerInterface $module_handler) {
     $this->discovery = new ViewsHandlerDiscovery($handler_type, $namespaces);
     $this->discovery = new CacheDecorator($this->discovery, "views:$handler_type", 'views_info');
 
@@ -54,6 +65,7 @@ public function __construct($handler_type, \Traversable $namespaces, ViewsData $
 
     $this->viewsData = $views_data;
     $this->handlerType = $handler_type;
+    $this->moduleHandler = $module_handler;
   }

As far as I know, serializing plugin instances is a Bad Idea™ and unsupported by design. If you want to reproduce an instance later, save its ID and configuration instead.

We can either not store dependencies in plugin class properties (easy solution), or prevent view objects from serializing the plugins they contain.

Re-roll, because of recent additions to Unicode. Reject log:

diff a/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.php b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.php	(rejected hunks)
@@ -259,11 +276,11 @@ protected function caseTransform($string, $option) {
       default:
         return $string;
       case 'upper':
-        return drupal_strtoupper($string);
+        return Unicode::strtoupper($string);
       case 'lower':
-        return drupal_strtolower($string);
+        return Unicode::strtolower($string);
       case 'ucfirst':
-        return drupal_strtoupper(drupal_substr($string, 0, 1)) . drupal_substr($string, 1);
+        return Unicode::strtoupper(Unicode::substr($string, 0, 1)) . Unicode::substr($string, 1);
       case 'ucwords':
         if (Unicode::getStatus() == Unicode::STATUS_MULTIBYTE) {
           return mb_convert_case($string, MB_CASE_TITLE);

RTBC, under the condition that the tests pass.

50: drupal_2174021_50.patch queued for re-testing.

The tests failed because of a random segmentation fault. RTBC'ing as per #33 and #51.

Re-roll. Reject:

diff a/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.php b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.php	(rejected hunks)
@@ -259,9 +276,9 @@ protected function caseTransform($string, $option) {
       default:
         return $string;
       case 'upper':
-        return drupal_strtoupper($string);
+        return Unicode::strtoupper($string);
       case 'lower':
-        return drupal_strtolower($string);
+        return Unicode::strtolower($string);
       case 'ucfirst':
         return Unicode::ucfirst($string);
       case 'ucwords':

RTBC as per #54, under the condition that the tests pass.