Add getIfOwner(), setIfOwner() and deleteIfOwner() methods to TempStore.

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

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 :)

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

Start with a reroll.

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

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

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.

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

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!