Add a currentUser() method to plugin base classes that need it

1. +++ b/core/lib/Drupal/Core/Plugin/PluginBase.php
   @@ -23,6 +24,39 @@
   +  public function currentUser() {
   

   This needs to be protected

2. +++ b/core/lib/Drupal/Core/Plugin/PluginBase.php
   @@ -23,6 +24,39 @@
   +  public function setCurrentUser(AccountInterface $user) {
   

   Please do not add a setter. We don't need it and it's very existence is confusing

Nice and easy ;)
Thanks @pwolanin

In contrast to controllers plugins contain a lot of logic which can and should be tested. In contrast to t() the current user is often needed for the logic.
We have to stop at some points to provide magic which is hard to test.

Committed and pushed to 8.x. Thanks!

Daniel, can you explain why this can't be tested once it's in the base class?

Daniel, can you explain why this can't be tested once it's in the base class?

It is not untestable but just way harder to test.

// Our actual code we want to test.
class Tested {

  public function __construct(AccountInterface $user) {
    $this->user = $user;
  }

  public function test() {
    return $user->id() == '123';
  }

}

// The unit test using injection via the DIC.
class Test extends UnitTestCase {

  public function testTest() {
    $user = $this->getMock('Drupal\Core\Session\AccountInterface');
    $tested = new Tested($user);

    $this->assertEqual(123, $tested->test());
  }

}

use Drupal\Core\DependencyInjection\Container;

// The unit test using injection via the drupal class.
class Test2 extends UnitTestCase {


  // We don't do that in core yet, though we maybe should to avoid side effects.
  public function tearDown() {
    \Drupal::setContainer(NULL);
  }

  public function testTest() {
    $user = $this->getMock('Drupal\Core\Session\AccountInterface');
    $container = new ContainerBuilder();
    $container->set('current_user', $user);
    \Drupal::setContainer($container);

    $tested = new Tested();

    $this->assertEqual(123, $tested->test());
  }
}

Yeah, I don't really think this is a good idea because any plugin using the current user service also has to mock the container, as daniel pointed out above. This is not that cool. I don't think the DX win is really worth it because test DX suffers and we don't really have a self contained class.

IMO we need to stop using Drupal (class) in our classes.

When we have a trade-off between module developer DX and test author DX though, we must optimize for the module developer. 100% of module developers are module developers. 0.0005% of them are unit test authors. We hope that percentage will increase to 2% or possibly even as high as 10% but it is definitely the minority case.

That said, I'm obviously happy to discuss ways we could make unit testing easier as well.

I don't even think using the user in plugins is close to being the most common use case. This affects every single plugin that extends cores PluginBase class. If people want to use this in their plugins they might as well just call \Drupal::current user() directly?

We shouldn't be scaring people away from unit testing but it seems like a bit of a catch 22.

OK, rolled back for now so we can discuss more.

That'd be a place to start then, is trying to figure out how common code like this:

-    $user_account = $this->request->attributes->get('_account');

...is within plugins (or, more importantly, *will* be in contrib). Because that code is *absolutely awful* to do something so simple and common as work with the currently logged in user.

Also, since I don't really grok PHPUnit, can you explain why the code in the second example is so much harder to grok than the code in the first example? Are you saying it's because you have to mock the container as well and take care that it's removed at the end of the test? If so, is that something we could move into testbase (and/or a helper function for it), or would that kind of eliminate the point?

Because, at a glance, an extra 4 lines of code for unit test authors to have to contend with way outweighs the downside of "weekend hacker types" (who, let's face it, are never ever going to write a unit test in their lives) having to read and type code like #14 on a regular basis.

When we have a trade-off between module developer DX and test author DX though, we must optimize for the module developer. 100% of module developers are module developers. 0.0005% of them are unit test authors. We hope that percentage will increase to 2% or possibly even as high as 10% but it is definitely the minority case.

The most cases where you need the user access are access methods. As realized in other issues we want to pass the current user to the access(). These is probably the common usecase. I came to the conclusion by search for user_access() and looking at the plugins.
For other cases putting dependencies into the constructor way better describe the dependencies needed by the object.

Added a subissue for views plugins: #2109433: Replace user_access() through injected user accounts in views.

Another very common case to take into account #1951386: Extend BlockPluginInterface::access to allow passing in an account
Block plugins needs 2 access types:
1) access to block plugin itself - the BlockPluginInterface::access()
2) $account or current user service to check additional access within block plugin methods

For me the second part applies to views plugins and search plugins. Actions and image effects don't.
Fields (types, widgets, formatters) mostly needs the same as views.
Filters needs the first one.

So better to check all implementations and only then make this change.

PS: The translation manager already make additional call (wrapper) for each plugin string

Just brainstorming: Do we maybe need two types of PluginBase? One from which views, search, field, etc. derive that has currentUser service, and the other more "barebones" for things like image styles, actions, etc. that does not?

Only thing is I've no idea how we'd differentiate from those in name enough to make it obvious to people which one they should use.

If you look at the plugins that actually need this, it further supports @damiankloip's point:

comment/lib/Drupal/comment/Plugin/views/field/NodeNewComments.php
history/lib/Drupal/history/Plugin/views/field/HistoryUserTimestamp.php
history/lib/Drupal/history/Plugin/views/filter/HistoryUserTimestamp.php
user/lib/Drupal/user/Plugin/views/argument_default/CurrentUser.php
user/lib/Drupal/user/Plugin/views/argument_validator/User.php
user/lib/Drupal/user/Plugin/Search/UserSearch.php
editor/lib/Drupal/editor/Plugin/InPlaceEditor/Editor.php
statistics/lib/Drupal/statistics/Plugin/Block/StatisticsPopularBlock.php
tracker/lib/Drupal/tracker/Plugin/Menu/UserTrackerTab.php

options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.php
views/lib/Drupal/views/Plugin/views/cache/CachePluginBase.php
views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
views/lib/Drupal/views/Plugin/views/filter/FilterPluginBase.php
entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteWidgetBase.php

Obviously those 5 base classes should get their own method.

The other plugins are all specifically for user-related things. Out of 300+ plugins, this is not many. (Editor is the only one that doesn't seem user specific, but it needs the filter formats available to the current user).

So I propose we just add this method to those 5 base classes, add an @todo about how traits would make this not a problem, and move on.

Ok cool. :) Make it so!

While I totally agree with putting this on less generic classes if possible, I've never understood, why we don't simply provide optional set()ters for the function where get()ter accesses \Drupal. That would make unit testing so much easier:

// Test with constructor injection
$account = $this->getMock('AccountInterface');
$plugin = new Plugin($account);

// Test with setter injection
$account = $this->getMock('AccountInterface');
$plugin = new Plugin();
$plugin->setAccount($account);

I guess this should be a separate issue, because IMO we should be doing that in ControllerBase and FormBase, etc. as well, but since this point was specifically discussed here, I thought I'd bring it up.

It would also make our classes in general less tied to the \Drupal class and to Drupal in general, which I think is a worthy goal.

Please discuss the idea of requiring setters that would only be used by tests in another issue, thanks.

1. --- a/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteWidgetBase.php
   +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteWidgetBase.php
   

   I would consider that autocomplete wiedget is not a plugin base class but just a common base class for two other classes in core. This feels like it should not have it. Btw. there is some code which uses the user which should be changed.

2. +++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.php
   @@ -123,7 +130,7 @@ public static function validateElement(array $element, array &$form_state) {
   +      $options = $item->getSettableOptions($this->currentUser());
   

   WOW: AllowedValuesInterface

3. +++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.php
   index 1b948f1..b2ffe00 100644
   --- a/core/modules/views/lib/Drupal/views/Plugin/views/cache/CachePluginBase.php
   
   --- a/core/modules/views/lib/Drupal/views/Plugin/views/cache/CachePluginBase.php
   +++ b/core/modules/views/lib/Drupal/views/Plugin/views/cache/CachePluginBase.php
   

   The views issue adds a user object on the view executable so we might should just inject it form there when we init the cache plugin base. I guess that many parts of CachePluginBase is independent from the current user, so let's make at least this dependency optional.

4. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/cache/CachePluginBase.php
   index 1a96c60..9462273 100644
   --- a/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
   
   --- a/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
   +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
   

   I don't think we need this as the access() method will get the user all the time at the moment.

Updated issue summary.

Rerolled the patch sucessfully.

@Nitesh Sethia your patch has some deleted files as well, please remove them.

Rerolled the patch successfully.

The tests are failing due to following error.

Fatal error: Class Drupal\views\Plugin\views\filter\StringFilter contains 2 abstract methods and must therefore be declared abstract or implement the remaining methods (Drupal\views\Plugin\CacheablePluginInterface::isCacheable, Drupal\views\Plugin\CacheablePluginInterface::getCacheContexts) in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/src/Plugin/views/filter/StringFilter.php on line 22

This error does not looks relevant to this. Let wait couple of more days and run tests again.

Somebody accidentally deleted 2 methods from FilterPluginBase class, added them again. Tests should be green now.

That looks a candidate for CurrentUserTrait

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsWidgetBase.php
@@ -114,10 +121,7 @@ public static function validateElement(array $element, FormStateInterface $form_
-      $options = $this->fieldDefinition
-        ->getFieldStorageDefinition()
-        ->getOptionsProvider($this->column, $entity)
-        ->getSettableOptions(\Drupal::currentUser());
+      $options = $item->getSettableOptions($this->currentUser());

this change is wrong

If any class still needs the current user at this point, they'll need to use constructor injection. We shouldn't pollute the system any further by adding more service location.