Updated: Comment #1

Problem/Motivation

A variety of plugins will want to know abotu the current user (e.g. for access checks, getting the user name or ID, etc)

We've added some useful methods to \Drupal\Core\Plugin\PluginBase like t() so far, let's add currentUser() too.

Proposed resolution

Add currentUSer() method to \Drupal\Core\Plugin\PluginBase and pull the object from Drupal::currentUser() if it's not set.

Remaining tasks

patch

User interface changes

none

API changes

none

#1972990: Convert tracker_page() to a Controller

Files: 
CommentFileSizeAuthor
#23 plugin-2105123-23.patch7.58 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,607 pass(es).
[ View ]
#19 PluginBase.png61.62 KBandypost
#6 2105123-6.patch1.59 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,402 pass(es).
[ View ]
#6 2105123-5-6.increment.txt880 bytespwolanin
#5 2105123-5.patch1.68 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#5 2105123-4-5.increment.txt449 bytespwolanin
#4 2105123-4.patch1.89 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#4 2105123-3-4.increment.txt903 bytespwolanin
#3 2105123-3.patch1.01 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#3 2105123-1-3.increment.txt976 bytespwolanin
#1 2105123-1.patch1.31 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,407 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,407 pass(es).
[ View ]

  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

StatusFileSize
new976 bytes
new1.01 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

ok, here's a another version.

As a follow-up we could e.g. used it in \Drupal\comment\Plugin\field\formatter\CommentDefaultFormatter also.

StatusFileSize
new903 bytes
new1.89 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Adding a use (and fix) in UserSearch.

StatusFileSize
new449 bytes
new1.68 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

no longer need to +use Drupal\Core\Session\AccountInterface;

StatusFileSize
new880 bytes
new1.59 KB
PASSED: [[SimpleTest]]: [MySQL] 58,402 pass(es).
[ View ]

ugh, missed a usage in UserSearch of the variable - putting the name back per Tim's suggestion.

Status:Needs review» Reviewed & tested by the community

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.

Status:Reviewed & tested by the community» Fixed
Issue tags:+DX (Developer Experience)

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.

<?php
// 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.

Status:Fixed» Active

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.

StatusFileSize
new61.62 KB

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.
PluginBase.png

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!

Title:Add a currentUser() method to \Drupal\Core\Plugin\PluginBaseAdd a currentUser() method to plugin base classes that need it
Status:Active» Needs review
StatusFileSize
new7.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,607 pass(es).
[ View ]

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:

<?php
// 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.

Issue summary:View changes

Updated issue summary.