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.
Problem/Motivation
In the process of writing the change notice for #1642062: Add TempStore for persistent, limited-term storage of non-cache data, I noticed that Views is doing the following a lot:
drupal_container()->get('user.tempstore')->get('views')->doSomething()
Proposed resolution
It might be nice to add a convenience wrapper similar to what we have for state()
, something like:
function user_tempstore($container) {
return drupal_container()->get('user.tempstore')->get($container);
}
Then we could do:
user_tempstore('views')->doSomething();
user_tempstore('node')->doSomething();
etc.
Patch forthcoming.
API changes
user_tempstore()
is added as a wrapper for drupal_container()->get('user.tempstore')->get()
.
Comment | File | Size | Author |
---|---|---|---|
#8 | user-1805854-8.patch | 2.8 KB | xjm |
#6 | user-1805854-6.patch | 2.9 KB | xjm |
#3 | user-1805854-3.patch | 2.68 KB | xjm |
#1 | user-1805854-1.patch | 657 bytes | xjm |
Comments
Comment #1
xjmQuite simple really for a nice little DX improvement.
Still need to add tests for it.
Comment #2
xjmYeah obviously that's supposed to be
$collection
. ;)Comment #3
xjmThis should be all that's needed for tests, since TempStore itself is already tested quite thoroughly.
Comment #4
andypostI think it's not good idea to make wrappers for container items. state() is very special case.
Views can use it's own wrapper
PS Probably KeyValue store could be useful for #347988: Move $user->data into own table but having wrapper here is a bad idea
Comment #5
xjm@andypost, this is not just for Views. We expect the TempStore to be very widely used for all sorts of things.
Comment #6
xjmHmm, that wasn't the right patch. Sorry. This is.
Comment #7
xjmThis one assertion is unnecessary.
Comment #8
xjmSo this should be the minimum needed, basically confirming that getting and setting data with
user_tempstore()
works, and that it returns the same results as interacting with the default database storage manually with aTempStore
instantiation.Comment #9
xjmAlright, I've been convinced this is a bad idea. :)
Comment #10
sunCan you explain why?
The user_tempstore() helper looks actually useful to me. That is, as long as we make such excessive usage of drupal_container(), which doesn't seem to be avoidable for D8 at this point.
The added tests are unnecessary and can be removed though.
Comment #11
xjmWell, see #1783196: Get the views plugin manager from the DIC when possible. I asked Crell to leave feedback.
Comment #12
tim.plunkettThis saves 32 characters and adds a function call.
We're going to be using the DIC in so many places, it's not worth it IMO. People are going to have to get used to seeing that pattern.
I'd also like to avoid starting a trend of adding a procedural wrapper for every service in the DIC.
Won't fix for me.
Comment #13
Crell CreditAttribution: Crell commentedPretty much what Tim said. The longer-term goal is to eliminate drupal_container() sometime in the Drupal 9 timeframe, and all of these wrapper functions with it. We should not be hiding that fact from developers. They're getting services from the container. That fact should not be hidden from them. The only reason we have so many wrapper functions now is for BC. Any wrapper functions that did not exist in Drupal 7 shouldn't exist, and if any have been added should be removed. (There's a scant few exceptions to that; this is not one of them.)
If writing drupal_container()->get('foo') feels too icky for someone, they should refactor their code to itself be a service that can accept 'foo' as a parameter in the first place. Then they don't need to call the wrapper and can just call $this->foo.
Comment #14
xjm