Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff-2105123-31-42.txt | 965 bytes | Sharique |
#42 | add_a_currentuser-2105123-42.patch | 3.31 KB | Sharique |
#19 | PluginBase.png | 61.62 KB | andypost |
#6 | 2105123-6.patch | 1.59 KB | pwolanin |
#6 | 2105123-5-6.increment.txt | 880 bytes | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedComment #2
tim.plunkettThis needs to be protected
Please do not add a setter. We don't need it and it's very existence is confusing
Comment #3
pwolanin CreditAttribution: pwolanin commentedok, here's a another version.
As a follow-up we could e.g. used it in \Drupal\comment\Plugin\field\formatter\CommentDefaultFormatter also.
Comment #4
pwolanin CreditAttribution: pwolanin commentedAdding a use (and fix) in UserSearch.
Comment #5
pwolanin CreditAttribution: pwolanin commentedno longer need to +use Drupal\Core\Session\AccountInterface;
Comment #6
pwolanin CreditAttribution: pwolanin commentedugh, missed a usage in UserSearch of the variable - putting the name back per Tim's suggestion.
Comment #7
tim.plunkettNice and easy ;)
Thanks @pwolanin
Comment #8
dawehnerIn 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.
Comment #9
webchickCommitted and pushed to 8.x. Thanks!
Daniel, can you explain why this can't be tested once it's in the base class?
Comment #10
dawehnerIt is not untestable but just way harder to test.
Comment #11
damiankloip CreditAttribution: damiankloip commentedYeah, 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.
Comment #12
webchickWhen 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.
Comment #13
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #14
webchickOK, 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:
...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.
Comment #15
webchickAlso, 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?
Comment #16
webchickBecause, 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.
Comment #17
dawehnerThe 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.
Comment #18
dawehnerAdded a subissue for views plugins: #2109433: Replace user_access() through injected user accounts in views.
Comment #19
andypostAnother 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 methodsFor 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
Comment #20
webchickJust 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.
Comment #21
tim.plunkettIf you look at the plugins that actually need this, it further supports @damiankloip's point:
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.
Comment #22
webchickOk cool. :) Make it so!
Comment #23
tim.plunkettComment #24
tstoecklerWhile 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:
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.
Comment #25
tim.plunkettPlease discuss the idea of requiring setters that would only be used by tests in another issue, thanks.
Comment #26
dawehnerI 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.
WOW: AllowedValuesInterface
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.
I don't think we need this as the access() method will get the user all the time at the moment.
Comment #26.0
dawehnerUpdated issue summary.
Comment #27
jhedstromComment #28
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedRerolled the patch sucessfully.
Comment #30
saki007ster@Nitesh Sethia your patch has some deleted files as well, please remove them.
Comment #31
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedRerolled the patch successfully.
Comment #33
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedComment #34
Sharique CreditAttribution: Sharique as a volunteer commentedThe tests are failing due to following error.
This error does not looks relevant to this. Let wait couple of more days and run tests again.
Comment #35
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedComment #42
Sharique CreditAttribution: Sharique as a volunteer commentedSomebody accidentally deleted 2 methods from FilterPluginBase class, added them again. Tests should be green now.
Comment #44
andypostThat looks a candidate for
CurrentUserTrait
this change is wrong
Comment #45
XanoIf 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.