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().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Status: Active » Needs review
Issue tags: +Needs tests, +VDC
FileSize
657 bytes

Quite simple really for a nice little DX improvement.

Still need to add tests for it.

xjm’s picture

+++ b/core/modules/user/user.moduleundefined
@@ -3182,3 +3182,16 @@ function user_library_info() {
+  return drupal_container()->get('user.tempstore')->get($container);

Yeah obviously that's supposed to be $collection. ;)

xjm’s picture

Issue tags: -Needs tests
FileSize
2.68 KB

This should be all that's needed for tests, since TempStore itself is already tested quite thoroughly.

andypost’s picture

Issue tags: +Needs tests

I 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

xjm’s picture

Issue tags: -Needs tests

@andypost, this is not just for Views. We expect the TempStore to be very widely used for all sorts of things.

xjm’s picture

FileSize
2.9 KB

Hmm, that wasn't the right patch. Sorry. This is.

xjm’s picture

+++ b/core/modules/user/lib/Drupal/user/Tests/UserTempStoreTest.phpundefined
@@ -0,0 +1,64 @@
+    $this->assertIdenticalObject($this->storeFactory->get($this->collection)->get($key1), $this->objects[1]);

This one assertion is unnecessary.

xjm’s picture

FileSize
2.8 KB

So 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 a TempStore instantiation.

xjm’s picture

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

Alright, I've been convinced this is a bad idea. :)

sun’s picture

Component: base system » user system
Status: Closed (won't fix) » Needs review

Can 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.

xjm’s picture

Well, see #1783196: Get the views plugin manager from the DIC when possible. I asked Crell to leave feedback.

tim.plunkett’s picture

This 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.

Crell’s picture

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

Pretty 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.

xjm’s picture

Assigned: xjm » Unassigned