Filing this under 'entity system' as the original issue #1642062: Add TempStore for persistent, limited-term storage of non-cache data was filed under that too.

There is still a @todo in the class saying that we might want to add these methods if we need them. I strongly believe that we WILL need them. It definitely increases protection against potential security vulnerabilities if we make it convenient for implementations to check for the owner during get/set. Actually, I am currently working on a D7 module which leverages a lightweight backport of the tempstore in several places and can therefore verify that it definitely improves DX if we have those methods.

CommentFileSizeAuthor
#11 2008806-11.patch17.09 KBdamiankloip
#8 2008806-8.patch6.41 KBdamiankloip
#6 2008806-6.patch8.5 KBdamiankloip
#6 interdiff-2008806-6.txt6 KBdamiankloip
#6 interdiff-2008806-6-2.txt1.03 KBdamiankloip
#1 tempstore-if-owner-2008806-2.patch3.74 KBfubhy

Comments

fubhy’s picture

fubhy commented
Status: Active » Needs review
StatusFileSize
new tempstore-if-owner-2008806-2.patch3.74 KB

Status: Needs review » Needs work

The last submitted patch, tempstore-if-owner-2008806-2.patch, failed testing.

fubhy’s picture

fubhy commented
Status: Needs work » Needs review

#1: tempstore-if-owner-2008806-2.patch queued for re-testing.

fubhy’s picture

fubhy commented

#1: tempstore-if-owner-2008806-2.patch queued for re-testing.

damiankloip’s picture

damiankloip commented 
+++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
@@ -107,6 +104,24 @@ function get($key) {
+  function getIfOwner($key) {

@@ -129,6 +144,33 @@ function setIfNotExists($key, $value) {
+  function setIfOwner($key, $value) {

@@ -194,4 +236,27 @@ function delete($key) {
+  function deleteIfOwner($key) {

public

+++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
@@ -107,6 +104,24 @@ function get($key) {
+    if ($object && $object->owner == $this->owner) {

Wrap the second part in parenthesis? I like that for readability. Really it's a matter of taste though.

+++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
@@ -194,4 +236,27 @@ function delete($key) {
+   * Deletes data from the store for a given key and releases the lock on it.

Do we need the lock part here?

+++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
@@ -194,4 +236,27 @@ function delete($key) {
+    if (!$object = $this->storage->get($key)) {

You're using the if($var = something()) {} pattern here but not in the other two methods, I think we should make it consistent.

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
@@ -134,6 +134,11 @@ public function testUserTempStore() {
+    $this->assertNull($stores[0]->getIfOwner($key));
+    $this->assertFalse($stores[0]->setIfOwner($key, $this->objects[1]));
+    $this->assertFalse($stores[0]->deleteIfOwner($key));

We should add a bit more coverage that tests the new methods working too.

damiankloip’s picture

damiankloip commented
StatusFileSize
new interdiff-2008806-6-2.txt1.03 KB
new interdiff-2008806-6.txt6 KB
new 2008806-6.patch8.5 KB

It's been a while, so getting this back on track. Made the changes from my review in #5, I also added visibility to Tempstore methods - seems we might as well whilst we're there? I also converted the test to DUTB and added some assertions to check the new methods work when the user is the current owner.

I have added 2 interdiffs, purely out of laziness....sorry. The second one is just for the test assertions. That was quickest :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Component: entity system » user system
Issue summary: View changes
damiankloip’s picture

damiankloip commented
StatusFileSize
new 2008806-8.patch6.41 KB

Let's revive this one. Not sure how it got lost for so long.

Start with a reroll.

Status: Needs review » Needs work

The last submitted patch, 8: 2008806-8.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review

8: 2008806-8.patch queued for re-testing.

damiankloip’s picture

damiankloip commented
StatusFileSize
new 2008806-11.patch17.09 KB

Added some unit tests for the TempStore class. Needed a couple of fixes like trying to call ->collection directly on to get the collection name, and using format_string for exception messages.

fubhy’s picture

fubhy commented
Status: Needs review » Reviewed & tested by the community

The additional tests look good. Thanks

+++ b/core/modules/user/lib/Drupal/user/TempStore.php
@@ -85,7 +83,7 @@ class TempStore {
-  function __construct(KeyValueStoreExpirableInterface $storage, LockBackendInterface $lockBackend, $owner) {
+  public function __construct(KeyValueStoreExpirableInterface $storage, LockBackendInterface $lockBackend, $owner) {

@@ -100,13 +98,30 @@ function __construct(KeyValueStoreExpirableInterface $storage, LockBackendInterf
-  function get($key) {
+  public function get($key) {

@@ -117,7 +132,7 @@ function get($key) {
-  function setIfNotExists($key, $value) {
+  public function setIfNotExists($key, $value) {

@@ -129,18 +144,46 @@ function setIfNotExists($key, $value) {
-  function set($key, $value) {
+  public function set($key, $value) {
...
-        throw new TempStoreException(format_string("Couldn't acquire lock to update item %key in %collection temporary storage.", array(
+        throw new TempStoreException(String::format("Couldn't acquire lock to update item %key in %collection temporary storage.", array(
...
-          '%collection' => $this->storage->collection,
+          '%collection' => $this->storage->getCollectionName(),

@@ -164,7 +207,7 @@ function set($key, $value) {
-  function getMetadata($key) {
+  public function getMetadata($key) {

@@ -180,13 +223,13 @@ function getMetadata($key) {
-  function delete($key) {
+  public function delete($key) {
...
-        throw new TempStoreException(format_string("Couldn't acquire lock to delete item %key from %collection temporary storage.", array(
+        throw new TempStoreException(String::format("Couldn't acquire lock to delete item %key from %collection temporary storage.", array(
...
-          '%collection' => $this->storage->collection,
+          '%collection' => $this->storage->getCollectionName(),

All those changes are a little bit scope-creep but it really doesn't hurt to fix it right here. Not sure what the commiters think though.

damiankloip’s picture

damiankloip commented

fubhy! Where have you been all this time?! :)

Thanks. I guess they could be considered out of scope, but they are necessary for working unit tests :) So I think that is a fair trade off. Not just added in for the sake of it.

catch’s picture

catch
he/him
Primary language English
commented 
  1. +++ b/core/modules/user/lib/Drupal/user/TempStore.php
@@ -129,18 +144,46 @@ function setIfNotExists($key, $value) {
+  public function setIfOwner($key, $value) {

    Isn't this technically setIfNotExistsAndOwner()? I can't really bring myself to suggest changing that, but it crossed my mind. 

  2. +++ b/core/modules/user/lib/Drupal/user/TempStore.php
@@ -194,4 +237,28 @@ function delete($key) {
+  public function deleteIfOwner($key) {

    The get then delete here doesn't make me feel 100% comfortable, but there's already locking and no choice anyway.

damiankloip’s picture

damiankloip commented

Isn't this technically setIfNotExistsAndOwner()? I can't really bring myself to suggest changing that, but it crossed my mind.

I know what you mean. I think this is kind of an implicit logic that makes sense though. If you are setting IfOwner, if it doesn't exist, it owner doesn't matter so should work. I think this makes makes a nicer, more care free API for devs.

I can also appreciate what you mean about the deleting. I think this makes sense for how TempStore is currently implemented though. We need to get to be able to check the owner etc..

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

OK. I'll think about the implementation some more, it's not fixable without making the whole thing not a real k/v store, and not sure that's worth doing for this particular case since this ought to be low concurrency stuff anyway.

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.