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
#42 interdiff-2105123-31-42.txt965 bytesSharique
#42 add_a_currentuser-2105123-42.patch3.31 KBSharique
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,785 pass(es), 192 fail(s), and 237 exception(s).
[ 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

pwolanin’s picture

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

  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

pwolanin’s picture

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.

pwolanin’s picture

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.

pwolanin’s picture

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;

pwolanin’s picture

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.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Nice and easy ;)
Thanks @pwolanin

dawehner’s picture

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.

webchick’s picture

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?

dawehner’s picture

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());
  }
}
?>
damiankloip’s picture

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.

webchick’s picture

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.

damiankloip’s picture

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.

webchick’s picture

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.

webchick’s picture

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?

webchick’s picture

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.

dawehner’s picture

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.

dawehner’s picture

andypost’s picture

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

webchick’s picture

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.

tim.plunkett’s picture

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.

webchick’s picture

Ok cool. :) Make it so!

tim.plunkett’s picture

Title:Add a currentUser() method to \Drupal\Core\Plugin\PluginBase» Add 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 ]
tstoeckler’s picture

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.

tim.plunkett’s picture

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

dawehner’s picture

  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.

dawehner’s picture

Issue summary:View changes

Updated issue summary.

jhedstrom’s picture

Issue summary:View changes
Status:Needs review» Needs work
Issue tags:+Needs reroll
Nitesh Sethia’s picture

Status:Needs work» Needs review
StatusFileSize
new120.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,001 pass(es), 459 fail(s), and 182 exception(s).
[ View ]

Rerolled the patch sucessfully.

Status:Needs review» Needs work

The last submitted patch, 28: add_a_currentuser-2105123-28.patch, failed testing.

saki007ster’s picture

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

Nitesh Sethia’s picture

Status:Needs work» Needs review
StatusFileSize
new3.75 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 77,251 pass(es), 547 fail(s), and 235 exception(s).
[ View ]

Rerolled the patch successfully.

Status:Needs review» Needs work

The last submitted patch, 31: add_a_currentuser-2105123-31.patch, failed testing.

hitesh-jain’s picture

Assigned:Unassigned» hitesh-jain
Status:Needs work» Active
Sharique’s picture

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.

hitesh-jain’s picture

Assigned:hitesh-jain» Unassigned

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, 31: add_a_currentuser-2105123-31.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 31: add_a_currentuser-2105123-31.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 31: add_a_currentuser-2105123-31.patch, failed testing.

Sharique’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new3.31 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,785 pass(es), 192 fail(s), and 237 exception(s).
[ View ]
new965 bytes

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

Status:Needs review» Needs work

The last submitted patch, 42: add_a_currentuser-2105123-42.patch, failed testing.

andypost’s picture

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

Xano’s picture

Status:Needs work» Closed (won't fix)

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.