Issue #1642062 by tim.plunkett, xjm, chx, merlinofchaos, damiankloip, dawehner, Berdir, aspilicious, Fabianx: Add TempStore for persistent, limited-term storage of non-cache data.

Problem/Motivation

  • It is sometimes useful to have temporary, user-entered data persist across multiple requests.
  • E.g., in a complex or multistep form, a user might enter some data that should persist through multiple page loads or until the next time the user visits the form. The Views UI admin form does this currently.
  • This data store is not a cache, because it is not possible to regenerate the data from a canonical source when the cache is cleared.

Proposed resolution

Remaining tasks

Followup issues:

API changes

  • A setIfNotExists() method is added to KeyValueStoreInterface and must now be implemented by all classes that implement that interface.
  • New KeyValueStoreExpirableInterface extends KeyValueStoreInterface with three methods to set an expiration.
  • DatabaseStorageExpirable is added as the default implementation for expirable k/v core data.
  • A key_value_expire table is added for the storage of DatabaseStorageExpirable data. (#1805094: Decide whether to merge DatabaseStorage and DatabaseStorageExpirable discusses this.)
  • The TempStore class is added to core for the storage of temporary, owned, non-cache data. (See this class's API documentation for details.)
  • The TempStoreFactory class is added as a factory for TempStore objects that belong to a particular user or anonymous user session.
  • The default implementation is registered in the core bundle and can be accessed with drupal_container()->get('user.tempstore').
CommentFileSizeAuthor
#190 tempstore-1642062-189.patch42.81 KBxjm
#190 interdiff.txt5.26 KBxjm
#184 tempstore-1642062-184-tests.patch42.65 KBxjm
#184 tempstore-1642062-184-combined.patch42.66 KBxjm
#184 interdiff-172-184.txt13.56 KBxjm
#172 tempstore-1642062-172.patch36.64 KBxjm
#172 interdiff-164-172.txt6.69 KBxjm
#164 tempstore-1642062-164.patch35.67 KBxjm
#164 interdiff-162-164.txt6.53 KBxjm
#162 tempstore-1642062-162.patch34.92 KBxjm
#162 interdiff-161-162.txt5.84 KBxjm
#161 tempstore-1642062-161.patch29.08 KBxjm
#161 interdiff-158-161.txt1.41 KBxjm
#158 tempstore-1642062-158.patch29.11 KBxjm
#158 interdiff-154-158.txt11.83 KBxjm
#155 interdiff.txt18.32 KBxjm
#154 tempstore-1642062-154.patch26.85 KBxjm
#154 interdiff-127-154.txt26.85 KBxjm
#147 tempstore-1642062-146.patch26.01 KBxjm
#147 interdiff-127-147.patch9.25 KBxjm
#127 tempstore-1642062-127.patch25.65 KBtim.plunkett
#125 1642062-tempstore.patch25.67 KBCrell
#123 1642062-tempstore.patch25.67 KBCrell
#123 interdiff.txt2.28 KBCrell
#122 1642062-122.patch25.37 KBstar-szr
#122 interdiff.txt1.01 KBstar-szr
#118 tempstore-1642062-118.patch25.32 KBtim.plunkett
#118 interdiff.txt3.13 KBtim.plunkett
#112 1642062_112.patch24.33 KBchx
#108 tempstore-1642062-107.patch24.09 KBtim.plunkett
#104 tempstore-1642062-103.patch24.09 KBtim.plunkett
#104 interdiff.txt7.28 KBtim.plunkett
#99 tempstore-1642062-99.patch21.94 KBtim.plunkett
#99 interdiff.txt7.87 KBtim.plunkett
#96 tempstore-1642062-96.patch21.02 KBtim.plunkett
#96 interdiff.txt2.41 KBtim.plunkett
#95 1642062_93.patch20.93 KBchx
#92 1642062_90.patch20.93 KBchx
#92 interdiff.txt4.38 KBchx
#89 1642062_89.patch20.06 KBchx
#88 1642062_88.patch18.95 KBchx
#84 tempstore-1642062-84.patch17.58 KBtim.plunkett
#84 interdiff.txt4.92 KBtim.plunkett
#67 1642062-67.patch20.45 KBdamiankloip
#55 1642062-55.patch18.66 KBdamiankloip
#53 1642062-53.patch18.66 KBdamiankloip
#48 1642062-48.patch18.64 KBdamiankloip
#46 tempstore-1642062-46.patch19.34 KBaspilicious
#44 tempstore-1642062-44.patch19.34 KBaspilicious
#42 tempstore-1642062-42.patch18.67 KBxjm
#40 tempstore-1642062-40.patch18.68 KBxjm
#40 interdiff-40.patch18.87 KBxjm
#33 drupal-1642062-32.patch17.41 KBtim.plunkett
#33 interdiff.txt596 bytestim.plunkett
#25 drupal-1642062-25.patch17.37 KBtim.plunkett
#23 interdiff.txt7.34 KBtim.plunkett
#23 drupal-1642062-23.patch17.25 KBtim.plunkett
#20 drupal-1642062-20.patch17.92 KBtim.plunkett
#20 interdiff.txt10.72 KBtim.plunkett
#19 drupal-1642062-19.patch19.52 KBtim.plunkett
#19 interdiff.txt6.91 KBtim.plunkett
#15 drupal-1642062-15.patch19.5 KBtim.plunkett
#15 interdiff.txt4.05 KBtim.plunkett
#13 drupal-1642062-13.patch19.86 KBtim.plunkett
#13 interdiff.txt8.33 KBtim.plunkett
#11 drupal-1642062-11.patch18.88 KBtim.plunkett
#11 interdiff.txt9.51 KBtim.plunkett
#8 drupal-1642062-8.patch18 KBtim.plunkett
#8 interdiff.txt7.37 KBtim.plunkett
#6 drupal-1642062-6.patch16.88 KBtim.plunkett
#6 interdiff.txt1.9 KBtim.plunkett
#5 interdiff.txt5.67 KBtim.plunkett
#3 1642062-tempstore-2.patch16.6 KBdawehner
#1 1642062-tempstore-1.patch12.84 KBdawehner
temp_store.patch9.87 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

FileSize
12.84 KB

I started with writing some tests for that to be sure it is working as expected.

During the test i observed that "key" as table column is a reserved keyboard of mysql, so it throws wild errors, see http://dev.mysql.com/doc/refman/5.5/en/reserved-words.html

What about using either tkey or temp_key for the storage?

In general it is a bit confusing to have deleteAll to still accept keys, maybe the function name should make it clear
that the function is clearing all the key data for all sessions.

dawehner’s picture

Changed the getLockOwner method to use the $sessionTable information,
so if it is not the sessionTable just the sid will be returned.

dawehner’s picture

FileSize
16.6 KB

.

tim.plunkett’s picture

FileSize
5.67 KB

Posting an interdiff from #0-#3 without the test addition

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +VDC
FileSize
1.9 KB
16.88 KB

Okay, I had to revert some of the changes made in #1, and still return a stdClass of uid/updated from getLockOwner.

#1658068: Convert Views to use core TempStore instead of ctools_object_cache_*() is an implementation of this.

Status: Needs review » Needs work

The last submitted patch, drupal-1642062-6.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.37 KB
18 KB

There were a couple big issues with the last patch.

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,274 @@
+  function __construct($subsystem, $sid = NULL) {
+    $this->subsystem = $subsystem;
+
+    if (!isset($sid)) {
+      $this->sid = session_id();
+      $this->useSessionTable = TRUE;
+    }
+    else {
+      $this->sid = $sid;
+      $this->useSessionTable = FALSE;
+    }

As is, you cannot pass an actual session ID to __construct(). That marks it as using a custom ID system. Simpletest can't use session_id(), it always returns the test runner's session.

So, I split up the code into two classes. This is up for discussion, but I'm hesitant to recombine them without a really good reason. Also, I've had a hard time finding a contrib module using non-session based IDs.

+++ b/core/modules/system/lib/Drupal/system/Tests/TempStore/TempStoreTest.phpundefined
@@ -0,0 +1,143 @@
+    // Change the user, to be sure the tempstore is not returning the same data
+    // for different users.
+    $this->drupalLogin($this->anotherUser);
+    $this->assertFalse($temp_store->get($random_key));

I removed this bit of tests because they were wrong, it SHOULD return the same data. What we have to decide is whether set() should check for blocking or if that is the caller's responsibility. Or, add a helper method to only set if there is no lock.

xjm’s picture

Per @merlinofchaos the purpose of the $sid override is to allow the same user to have the same TempStore across different sessions for that user.

See also: #1123150: object cache: use of session id is somewhat restrictive

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,252 @@
+    // If the current user owns the lock and is excluded, report that the object
+    // is not locked.
+    if ($exclude_owner && isset($lock_owner->uid) && $lock_owner->uid == $GLOBALS['user']->uid) {
+      return;

Reminder for reroll, @todo consider removing this and making Views responsible for checking whether it is the current user instead

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
9.51 KB
18.88 KB

This patch moves code into a new TempStoreBase and gives "better" names to the two extending classes.

merlinofchaos’s picture

Two quick notes on #11:

The __construct method on the abstract base is incorrectly documented. Plus, the $sid paramter is completely ignored; since the sid protected member is used on the base, it should at least be set in the constructor.

The $_SESSION hacky stuff should be on the Session specific inheriting object, since the base shouldn't know about sessions, which means that should override the set() method and whatever else uses that.

Finally, if we completely divorce sessions from the abstract, we should consider renaming sid to something else. I don't have a better suggestion off the top of my head.

tim.plunkett’s picture

FileSize
8.33 KB
19.86 KB

Addressed all three of the two quick notes from #12 ;)

Status: Needs review » Needs work

The last submitted patch, drupal-1642062-13.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.05 KB
19.5 KB

Core commits blowing up my patch!
Also realized I was overzealous in my renaming.

merlinofchaos’s picture

Unless I'm mistaken, as a bare variable it'd be $owner_id not $ownerId as camelCase only applies to classes and methods? (This is my primary gripe about our combining of underscores and uppercase.)

tim.plunkett’s picture

Ugh, I agree, but I checked the docs:

Methods and class properties should use lowerCamel naming.

From http://drupal.org/node/608152

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work

OHHH you said te standalone ones. Whoops yeah.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.91 KB
19.52 KB

Fixed up the ownerId/owner_id usages

tim.plunkett’s picture

FileSize
10.72 KB
17.92 KB

Yet Another Refactoring.

Tested with Views, two logged in users and two anonymous users attempting to edit the same view, and everything worked as expected.

  • Renamed SessionTempStore to UserTempStore
  • Was able to clean out SpecialTempStore, maybe the abstract base isn't needed after all?
  • Killed the exception class
  • getLockOwner() now returns an object with ownerId and updated, not uid and updated
  • added isLocked() which checks getLockOwner() for a different owner
  • cleaned up some docs
merlinofchaos’s picture

Okay, there seems to be no point in UserTempStore; I would say at this point make TempSTore not abstract.

+  /**
+   * Overrides TempStoreBase::__construct().
+   *
+   * The $owner_id is given a default value of NULL.
+   */
+  function __construct($subsystem, $owner_id = NULL) {
+    // If the user isn't anonymous, fall back to session ID.
+    if (!isset($owner_id)) {
+      $owner_id = user_is_logged_in() ? $GLOBALS['user']->uid : session_id();
+    }
+
+    parent::__construct($subsystem, $owner_id);
+  }

This is just thinking, but what if we make the parameter a user object instead of an owner id, and then making it required: IT's easy enough to send the global user in. Because as it is, no one should ever pass an owner id in, the API to do so is kind of annoying.

Though hm, that also kind of fails actually; there's no way to fall back to a session ID if the user is not, in fact, logged in. So maybe that's a bad idea.

merlinofchaos’s picture

Uh. There seems to be no point in *SpecialTempStore*. Sorry about that braino.

tim.plunkett’s picture

FileSize
17.25 KB
7.34 KB

In PHP 5.3, constructors can have different function signatures (type hints and number of required arguments).
But in PHP 5.4, constructors behave as other methods, and have to match.

So, do we add the Drupal\user\User typehint now to make it useful, and tear our hair out when core switches to 5.4?
Or does someone have a better idea of how to make this sane?

merlinofchaos’s picture

Yeah, we can't typehint it. Also do you think actually passing hte user object is even a good idea? I'm feeling like maybe I muddied the water by even suggesting it. :(

tim.plunkett’s picture

FileSize
17.37 KB

Without the ability to type hint, I don't think we gain anything. Just keeping the renaming part of the last change.

Crell’s picture

What about moving those parameters out of the constructor and into a separate method, as defined by an interface? From what I've found, constructors being part of an interface is bad form anyway, so even though there isn't one here (although there should be, IMO) we can take inspiration from that practice. (He says, only skimming the patch so not fully understanding the problem at hand...)

neclimdul’s picture

Status: Needs review » Needs work

Skimmed through the patch. Saw a couple things.

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,250 @@
+   * @return object
+   *   The stored data.

What's the failure condition? It looks like we just return null but that's not documented.

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,250 @@
+      'SELECT * FROM {temp_store} WHERE owner_id = :owner_id AND subsystem = :subsystem AND temp_key = :temp_key',

You should just select data since that's the only value you are using.

There are 3 delete methods and 2 clear methods... That's kinda confusing to me and I didn't immediately know when I was suppose to use what.

vlad.ilyich’s picture

possibly stupid new-to-this-issue question - why is this not just using sessions?

vlad.ilyich’s picture

UserTempStore behaves just like the current session store. only, with a bunch of extra code. i feel like i'm missing a use case here.

existential questions aside - i think this needs transactions. delete-then-insert is a nasty pattern without them.

dawehner’s picture

One use-case for the tempStore is to have locking on these kind of objects, which requires you to store the values in some kind of permanent-like storage.

Crell’s picture

I don't think it's permanent as much as it is shared; memcache would make a perfectly reasonable implementation, for instance. But the need for sharing between users is what eliminates the session as a contender.

vlad.ilyich’s picture

interesting. ok, so, shared state that represent changes to an object n users are editing at the same time. did i get that right as the basic use case?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
596 bytes
17.41 KB

Rerolled to address #27.

merlinofchaos’s picture

Me cache would not make a reasonable implementation as the data must be available and cannot be reconstructed if it disappears.

You can't store it in the session due to size. It would bloat a session record unnecessarily. Plus there must at least be visibility to other sessions to verify lock status.

sun’s picture

I looked at this a couple of times already, and wasn't sure how to comment. This time, I took some paper & pen in order to dissect the proposed change in detail, so as to be able to understand its architecture and intended usage, as well as drawing connotations to other concepts and proposals in Drupal core. This took more than two hours, and so here's my in-depth review. Please note that my perspective on some details might be wrong, so please bear with me and also correct me, in case I did not make the synaptic connections that you might be presuming. :)


I understand the high-level purpose/goal of this proposal, but I have serious problems with its architectural design.

First and foremost, I think the implementation is trying to do too much in a single component/subsystem. It looks like the code is aiming for a general-purpose solution for content-locking. But at the same time, the methods of the component is mixing/combining too much behavior and business logic with regard to the data/object being processed.

If it was about locking only, without any data/object involved, then it might also work for entities. But with the currently proposed "TempStore" code, I do not think we will ever use that for saving entities into that storage/bin. That's what (proper) entity revisions are for.

In general, these are two entirely different things in my mind:

  1. Keeping and storing edits on an object globally.
  2. Locking an object while it's being edited.

Only the first deserves a name that has "store" or similar in it. The second has nothing to do with a "store" - the second is a lock mechanism that, ideally, would be built on the existing Lock component we have already; it's only the context + duration/expiration of the lock that differs.

This is what makes the current implementation really difficult to grok and review/understand. Essentially, there's too much business logic contained in a single component that tries to "override the current data for a thingie while it is being edited" as well as "lock a thingie while it is being edited."

I think we want to cleanly separate those two concepts. One of the main reasons for doing so is that I see the actual "TempStore" component (holding unsaved, in-edit data) not suitable for D8's concept of entities/content, and in my mind, it only applies to "configurable thingies."

Which is, in fact, the original purpose of this code in ctools - unless I'm not aware of "the" contrib module that [ab]uses the functionality for entities. And even if there was such a module, I'm really not sure whether we want to do that for core. One of the reasons against that might be the arbitrary serialization of the data object into the TempStore, which, as we all know, can have very odd and unexpected consequences. (That is, I think we want to investigate content locking, but as mentioned above, as a separate concept for entities/revisions instead.)

The true use-case for this TempStore actually has many parallels to the context plugins and overrides we're architecting for the config system already.

In essence, you want to inject/replace a certain config object - which does not exist in its "TempStore" form - within a certain, clearly predictable context; i.e., the administration or administrative edit form. This config object, stemming from the TempStore, only has its current/in-edit values within that context. Outside of that context (i.e., by default), the original/currently-saved data takes effect (if any).

As such, the TempStore idea definitely delivers the promise of a store that holds temporarily overridden objects being edited (which seems to be the main goal -- kinda resolving the lack of revisions for configuration). But I do think that it is too detached from the general config system architecture and direction (essentially introducing a second DatabaseStorage, just for storing temporary objects, instead of using the existing [pluggable] active store), and that it needs to be brought in line with the general architecture for handling contextual overrides.

That aside:

  1. The three compound primary keys make it hard to understand which TempStore controller actually owns which data objects, given that (the arbitrary) $key is the only argument being actively used. Normally, one would expect that $ownerId AND $key build up a compound key (e.g., colon-delimited) within a single store, so that only $subsystem remains as outer namespace.
  2. But then again, $ownerId is already required to be passed into the constructor. Which inherently means that any caller needs to know the $ownerId in order to retrieve the identical/original object that was created before. Especially because the UserTempStore injects the $uid/$sessionid as $ownerId, I have serious problems to see how the same object could be marked as "locked/in-edit" for all users, since the calling code would have to figure out the original $ownerId that was used to create or edit the object to find the existing record in the first place...?
  3. If one would map what I said above, then the compound key of $subsystem.$key - would essentially equal a config object's [module].[type].[name]. I've the impression that this kind of usage would conflict with the usage that the TempStore class methods assume (since $subsystem + $ownerId are building the namespace, not $subsystem + $key).
  4. As a Core component, it should not use the procedural db_* functions at all. Instead, it should use the Database component and accept the necessary options for it.
  5. The ::clearAll() method seems to duplicate the ::deleteRecords() method.
  6. The @todos about potential static caching should be removed.
  7. The name "TempStore" reads and feels very sub-optimal to me. "EditStore" would be much more appropriate, based on its intended usage.

In general, I think we should first try to separate the involved concerns though. Bringing them in line with the current Entity system and Configuration system efforts could easily mean that the entire implementation will have to look different, or possibly even, that the proposed new TempStore component/subsystem might be unnecessary.

sun’s picture

Tagging for syndication.

merlinofchaos’s picture

Only the first deserves a name that has "store" or similar in it. The second has nothing to do with a "store" - the second is a lock mechanism that, ideally, would be built on the existing Lock component we have already; it's only the context + duration/expiration of the lock that differs.

The existing lock implementation is insufficient:
1) The owner matters
2) Synchronizing a lock with the presence of data will increase work and open us to potential situations where we have a lock with no associated data, or data with no associated lock if some error causes the synchronization to fail or be incorrect.

I think we want to cleanly separate those two concepts. One of the main reasons for doing so is that I see the actual "TempStore" component (holding unsaved, in-edit data) not suitable for D8's concept of entities/content, and in my mind, it only applies to "configurable thingies."

Why is this unsuitable for entities? I fully intended this to be used for entities as well. You say it's unsuitable but I don't see a reason.

As such, the TempStore idea definitely delivers the promise of a store that holds temporarily overridden objects being edited (which seems to be the main goal -- kinda resolving the lack of revisions for configuration). But I do think that it is too detached from the general config system architecture and direction (essentially introducing a second DatabaseStorage, just for storing temporary objects, instead of using the existing [pluggable] active store), and that it needs to be brought in line with the general architecture for handling contextual overrides.

First you say it's not suitable for entities, then you say it's too detached from config. I don't understand this. This is absolutely not a config specific tool. This tool is used extensively within many multi-step edit forms within my stuff, related to any number of things that are often client-custom. It makes no sense to tie the system to config at all.

As such, the TempStore idea definitely delivers the promise of a store that holds temporarily overridden objects being edited (which seems to be the main goal -- kinda resolving the lack of revisions for configuration). But I do think that it is too detached from the general config system architecture and direction (essentially introducing a second DatabaseStorage, just for storing temporary objects, instead of using the existing [pluggable] active store), and that it needs to be brought in line with the general architecture for handling contextual overrides.

You construct a tempstore for a given subsystem, and that tempstore object is always used for that subsystem; then you can have arbitrary keys when calling get() and set() methods. The tempstore is not a singleton; you construct it for your use. There can potentially be mulatiple tempstore objects created during any page request.

But then again, $ownerId is already required to be passed into the constructor. Which inherently means that any caller needs to know the $ownerId in order to retrieve the identical/original object that was created before. Especially because the UserTempStore injects the $uid/$sessionid as $ownerId, I have serious problems to see how the same object could be marked as "locked/in-edit" for all users, since the calling code would have to figure out the original $ownerId that was used to create or edit the object to find the existing record in the first place...?

Since the owner is typically the 'current user' it's pretty easy to calculate this, yes? I don't understand this objection. You only need to know if an object exists with a different ownerId than the one you've specified. You seem to be running down a logic path that doesn't make sense (and does not exist) and saying it doesn't make sense. You're right in that it doesn't make sense, but it's also a logic path that doesn't exist.

The name "TempStore" reads and feels very sub-optimal to me. "EditStore" would be much more appropriate, based on its intended usage.

One other potential use case of this store is for long running batches that build up data during the batch. In that case, the batch id would be used as an owner id. Thus, I disagree with trying to rename this to EditStore.

Crell’s picture

I haven't looked at the implementation here at all, but I did chat with Earl and the other VDC folks at MWDS about it over pizza and ice cream (the best time to discuss architecture). I in general agree that the locking needs here are sufficiently independent of the core locking system's design that I'm fine with it being its own thing (especially with freeze fast approaching). Let's not hold it up on trying to refactor the locking system to handle this case when that already has a stalled refactor attempt: #1225404: Modern event based lock framework proposal (btw, that needs review.)

I'm not convinced on batch, either, though. To me, this seems like a UI-specific tool. It's effectively an adjunct to the Form API, effectively. Batch API needs to go away and just become a wrapper on the Queue system.

Let's keep this focused on the use cases we already know exist, and get 'er done. We don't know how to integrate this with the locking framework, so let's get it working first before we spend time trying to redesign two systems instead of one.

xjm’s picture

Assigned: tim.plunkett » xjm

Working on a reroll/cleanup.

xjm’s picture

The attached patch rerolls so the patch applies and adds some documentation clarifications and code style cleanups.

Fixed:

The @todos about potential static caching should be removed.

Regarding:

The ::clearAll() method seems to duplicate the ::deleteRecords() method.

The former methods are static and apply to all stores, rather than a specific instantiated store. This confused me at first too. For now, I've clarified the docblocks a little.

I'd have similar responses to @merlinofchaos's for the points covered in his post in #37.

Not yet addressed:

The three compound primary keys make it hard to understand which TempStore controller actually owns which data objects, given that (the arbitrary) $key is the only argument being actively used. Normally, one would expect that $ownerId AND $key build up a compound key (e.g., colon-delimited) within a single store, so that only $subsystem remains as outer namespace.

If one would map what I said above, then the compound key of $subsystem.$key - would essentially equal a config object's [module].[type].[name]. I've the impression that this kind of usage would conflict with the usage that the TempStore class methods assume (since $subsystem + $ownerId are building the namespace, not $subsystem + $key).

As a Core component, it should not use the procedural db_* functions at all. Instead, it should use the Database component and accept the necessary options for it.

Status: Needs review » Needs work

The last submitted patch, interdiff-40.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
18.67 KB

Ahem. :)

damiankloip’s picture

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,251 @@
+    db_insert('temp_store')
+      ->fields(array(
+        'owner_id' => $this->ownerID,
+        'subsystem' => $this->subsystem,
+        'temp_key' => $key,
+        'data' => serialize($data),
+        'updated' => REQUEST_TIME,
+      ))
+      ->execute();

I might be missing something, but can we use db_merge here instead of the delete first? Although that might be pointless and If there is a reason for this, sorry :)

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,251 @@
+    if (isset($lock_owner->ownerID) && $this->ownerID != $lock_owner->ownerID) {

Small one, but I always find it easier on the eye if conditions are wrapped: (isset($lock_owner->ownerID) && ($this->ownerID != $lock_owner->ownerID))

Not setting to 'needs work' as both of these points could not have any place.

aspilicious’s picture

FileSize
19.34 KB

fixed #43

Status: Needs review » Needs work

The last submitted patch, tempstore-1642062-44.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
19.34 KB

another try

Status: Needs review » Needs work

The last submitted patch, tempstore-1642062-46.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
18.64 KB

We need a key condition on the merge query I think.

Status: Needs review » Needs work
Issue tags: -Needs architectural review, -VDC

The last submitted patch, 1642062-48.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs architectural review, +VDC

#48: 1642062-48.patch queued for re-testing.

moshe weitzman’s picture

Is anyone available and familiar enough with this feature to do a final code review and work toward RTBC?

aspilicious’s picture

Use full namespace paths in documentation

+ * Overrides TempStore::__construct().

damiankloip’s picture

FileSize
18.66 KB

Rerolled with aspilicious' change in #52.

We are currently using this in views-8.x-3.x at the moment, so this should be a good example for people to see this 'in action' and working.

Status: Needs review » Needs work

The last submitted patch, 1642062-53.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
18.66 KB

Oh, new system_update_N hook added in the mean time.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I agree that this one is ready. I reviewed the code and it is pretty clear, IMO. It adds tests, and they are passing. RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs work

I haven't followed this issue recently, and didn't look at the last few comments, here's a first pass through the patch.

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,249 @@
+class TempStore {

Do we really not need an interface? And is there definitely no reason that this will ever need pluggable storage so people can swap in different storage engines?

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,249 @@
+  /**
+   * The subsystem or module that owns this TempStore.
+   *
+   * @var string
+   */

Why $subsystem here rather than $bin or $namespace?

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,249 @@
+  function get($key) {
+    $data = db_query(
+      'SELECT data FROM {temp_store} WHERE owner_id = :owner_id AND subsystem = :subsystem AND temp_key = :temp_key',

Several patches are trying to inject the db connection into core components rather than using db_query(), but that's not resolved yet so it can be a follow-up.

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,249 @@
+  /**
+   * Writes the data to the store.
+   *
+   * @param string $key
+   *   The key to the object being stored. For objects that already exist in
+   *   the database somewhere else, this is typically the primary key of that
+   *   object. For objects that do not already exist, this is typically 'new'
+   *   or some special key that indicates that the object does not yet exist.
+   * @param mixed $data
+   *   The data to be cached. It will be serialized.
+   *
+   * @todo
+   *   Using 'new' as a key might result in collisions if the same user tries
+   *   to create multiple new objects simultaneously. Document a workaround?
+   */
+  function set($key, $data) {
+    // Store the new data.
+    db_merge('temp_store')
+      ->key(array('temp_key' => $key))
+      ->fields(array(
+        'owner_id' => $this->ownerID,
+        'subsystem' => $this->subsystem,
+        'temp_key' => $key,
+        'data' => serialize($data),
+        'updated' => REQUEST_TIME,
+      ))
+      ->execute();

I'm confused by the already existing data vs. new data - why can't the system itself take care of whether stuff is new or not, the

Also do we really want to say 'cached', shouldn't that be stored?

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,249 @@
+  /**
+   * Removes one or more objects from this store for this owner.
+   *
+   * @param string|array $key
+   *   The key to the stored object, or an array of keys. See
+   *   TempStore::set() for details.
+   */
+  function delete($key) {
+    $this->deleteRecords($key);

This should be deleteMultiple() to be consistent with both entity and cache systems, the delete() method can wrap it, then $key is either a key or an array but not both.

+++ b/core/lib/Drupal/Core/TempStore/UserTempStore.phpundefined
@@ -0,0 +1,44 @@
+  /**
+   * Overrides Drupal\Core\TempStore\TempStore::__construct().
+   *
+   * The $owner_id is given a default value of NULL.
+   */
+  function __construct($subsystem, $owner_id = NULL) {
+    if (!isset($owner_id)) {
+      // If the user is anonymous, fall back to the session ID.
+      $owner_id = user_is_logged_in() ? $GLOBALS['user']->uid : session_id();
+    }
+
+    parent::__construct($subsystem, $owner_id);
+  }
+
+  /**
+   * Overrides TempStore::set().
+   */
+  function set($key, $data) {
+    // Ensure that a session cookie is set for anonymous users.
+    if (!user_is_logged_in()) {
+      // A session is written so long as $_SESSION is not empty. Force this.
+      // @todo This feels really hacky. Is there a better way?
+      // @see http://drupalcode.org/project/ctools.git/blob/refs/heads/8.x-1.x:/includes/object-cache.inc#l69
+      $_SESSION['temp_store_use_session'] = TRUE;
+    }
+
+    parent::set($key, $data);

This seems like use-case-specific logic. Why isn't calling code handling this stuff?

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,249 @@
+class TempStore {

Do we really not need an interface?

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,249 @@
+  /**
+   * The subsystem or module that owns this TempStore.
+   *
+   * @var string
+   */

Why $subsystem here rather than $bin or $namespace?

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,249 @@
+  function get($key) {
+    $data = db_query(
+      'SELECT data FROM {temp_store} WHERE owner_id = :owner_id AND subsystem = :subsystem AND temp_key = :temp_key',

Several patches are trying to inject the db connection into core components rather than using db_query(), but that's not resolved yet so it can be a follow-up.

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,249 @@
+  /**
+   * Writes the data to the store.
+   *
+   * @param string $key
+   *   The key to the object being stored. For objects that already exist in
+   *   the database somewhere else, this is typically the primary key of that
+   *   object. For objects that do not already exist, this is typically 'new'
+   *   or some special key that indicates that the object does not yet exist.
+   * @param mixed $data
+   *   The data to be cached. It will be serialized.
+   *
+   * @todo
+   *   Using 'new' as a key might result in collisions if the same user tries
+   *   to create multiple new objects simultaneously. Document a workaround?
+   */
+  function set($key, $data) {
+    // Store the new data.
+    db_merge('temp_store')
+      ->key(array('temp_key' => $key))
+      ->fields(array(
+        'owner_id' => $this->ownerID,
+        'subsystem' => $this->subsystem,
+        'temp_key' => $key,
+        'data' => serialize($data),
+        'updated' => REQUEST_TIME,
+      ))
+      ->execute();

I'm confused by the already existing data vs. new data - why can't the system itself take care of this?

Also do we really want to say 'cached', shouldn't that be stored?

+++ b/core/lib/Drupal/Core/TempStore/TempStore.phpundefined
@@ -0,0 +1,249 @@
+  /**
+   * Removes one or more objects from this store for this owner.
+   *
+   * @param string|array $key
+   *   The key to the stored object, or an array of keys. See
+   *   TempStore::set() for details.
+   */
+  function delete($key) {
+    $this->deleteRecords($key);

This should be deleteMultiple() to be consistent with both entity and cache systems.

Looking at the patch there's absolutely zero garbage collection from this table - presumably stale items need to be removed at some point. Where does that happen?

sun’s picture

Category: task » feature
Issue tags: +API addition

I haven't re-reviewed the latest code again yet, but overall, I'd like to point out that this might be one of the architectural features that will rather distract us from the real, existing challenges we have for D8 already.

AFAICS, this is pure add-on "entity-locking"-alike + per-user-data-persistence functionality that exists in CTools/Views & Co right now, but doesn't appear to be strictly necessary to get the Views in core effort done.

Just raising a yellow flag. Please correct me where I'm wrong.

effulgentsia’s picture

but doesn't appear to be strictly necessary to get the Views in core effort done

I'm pretty sure it's essential for Views UI. Without it, incremental changes to a View (like adding a field) would get saved to the actual View, but in some cases, that could break that View until some other change is also made. It might be possible to refactor Views UI to do the staging it needs in $form_state as a pseudo-multistep form, but that might be harder to do, and result in a worse UI, than getting this in.

pounard’s picture

Actually this sounds like a good feature, but why not using a cache table without declaring it as a cache bin instead and compute a predictable key just like forms? I'm ok with both methodology of course, no worries, just asking.

tim.plunkett’s picture

Category: feature » task

This is a blocker for VDC. If we need to discuss whether VDC is a feature request or a task itself, that's fine. But as of now, we've been filing issues as tasks.

pounard’s picture

Another note, I think that an application level locking API should be done outside of this patch, it must not be tied to the temporary store API because it could be used outside.

EDIT: I actually wrote this http://drupalcode.org/project/oox.git/tree/refs/heads/7.x-1.x:/lib/Lock some years ago, and I think this gives enough flexibility to start as an application-level locking API. I guess that a new core issue should be opened for this.

tim.plunkett’s picture

@pounard I agree this could be improved, but it would be best as a follow-up. I think @damiankloip is going to address catch's concerns in a new patch soon.

pounard’s picture

Doing this as a follow-up seems like doing it in the wrong way around IMO, it's a recurent need a feature I'm talking about since a long time (for example, node form protection when another user is using it would be great to achieve this way). It will be harder to change than to do it right on the first shot, and I don't think the future person that will do this patch will want to do patch a whole set of other APIs at the same time.

tim.plunkett’s picture

This is a OO rewrite/port of the existing code used by Views, and Views has already been converted to use this. So the idea of consumers of this API having to change less if it's done before commit is invalid.

pounard’s picture

What?
In all cases, I really think that the proper course of action is to fraction this patch in two before getting it commited.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
20.45 KB

ok, this patch addresses the following of catch's issues in #57:

  • 'subsystem' has been replaced by 'namespace'
  • An interface 'TempStoreInterface' has been implemented (with method docs from TempStore moving to there)
  • deleteMultiple stuff has been used instead

What I'm not as sure about:

  • Are you suggesting we just remove the UserTempStore class?
  • Should we change the current patch to not use db_merge but know it's state better? (i.e. not a follow up issue)

Status: Needs review » Needs work

The last submitted patch, 1642062-67.patch, failed testing.

effulgentsia’s picture

I agree with #66. I suggest removing getLockOwner() and isLocked() from this issue. Correct me if I'm wrong, but I think that Views can proceed with a subclass (e.g., ViewsTempStore or LockableTempStore) until we figure out what kind of object locking API we want in core.

merlinofchaos’s picture

As I've said previously, I don't feel that separating the locking aspect out of the temp store and trying to come up with a 'larger lock framework' is a good idea.

This is a *very* simple locking system that does what it does, and it simply uses the presence of temporary data, if you need it, as a signal that someone else is doing the editing. Moving to another locking framework just adds extra work, extra code, and extra potential points of failure. One of the benefits of this system is that we automatically 'removed' locks when the sessions they were tied to were deleted (which happens way in the background where it's difficult to react to) by joining to that table. There was a nice simplicity to that mechanism that made the system work very well. I fear way too much energy is being spent on the fact that this system includes a couple of methods to allow simplified locking and the only answers are to increase complexity for minimal actual benefit.

pounard’s picture

It still would be a duplicate feature, because node form is already trying to do the exact same thing, without the temp store. As soon as this gets in as-is, we have a dup.

I think this kind of dup should block Views and is a good reason to stop and rethink this kind of inconsistent tiny pieces of core. There is already many in core, if we don't do it while integrating such huge thing as Views, we will never do it: refactorizing and joining after that will be impossible. Core has more than 200,000 lines of code, Views will almost double it, it gets unsustainable to refactor for a couple of human beings once done if we don't it upstream, prior to the integration.

catch’s picture

One of the benefits of this system is that we automatically 'removed' locks when the sessions they were tied to were deleted (which happens way in the background where it's difficult to react to) by joining to that table.

Do you mean joining on {session}?

tim.plunkett’s picture

Core has more than 200,000 lines of code, Views will almost double it, it gets unsustainable to refactor for a couple of human beings once done if we don't it upstream, prior to the integration.

Please educate yourself. FUD is unwelcome here. Views is just over 40,000 lines of code, and 6000 of that is integration with core modules that would be moved into those modules anyway.

If there are components of views you think are refactorable before moving into core, please open an issue and tag it VDC.

pounard’s picture

Sorry, I was doing a guess using the archive size, which is probably wrong. Problem remains, 40k lines is give or take 20% of core size. It's huge. It's not something you can include in haste. Symfony is probably huge too, but the whole goal of Symfony components is to remove core code, not add new code. If Views is to remain optional, those 40k added lines won't remove any line from core, but just be pure addition. So, in that regard, we have to factorize what needs to be factorize, and I'm just giving you one good example, and in the right issue, which is already tagged VDC. EDIT: And if we don't, Views is IMO as much unwelcomed as FUD.

tim.plunkett’s picture

Assigned: xjm » tim.plunkett

I'm going to follow-up along the lines of #69, i.e. trimming it down and extending it in Views.

pounard’s picture

It sounds like a good and better plan to me.

tim.plunkett’s picture

Issue tags: +CTools dependency

Tagging.

Lars Toomre’s picture

I am confused about how this storage area is different from the state API being worked on in #1175054: Add a storage (API) for persistent non-configuration state. Both this and that effort seem to want to find a way of temporarily storing non-configuration (state) data.

Can someone please explain the difference? Or should the two efforts be combined? Thanks.

tim.plunkett’s picture

@Lars Toomre I followed up on the other issue, but yes I hope to strip this down to a point where it *could* use the state API.

chx’s picture

It seems to me that the big difference to the key-value storage already added to core is a) the key is more complicated here b) there is a range query for the timestamp. I do not have a feel whether retreivieng all objects within a collection (that's the term k-v uses for namespacing) and deleting old ones is a feasible operation or that's abstraction madness and we should go with a different interface here. Maybe we should go the other way around and the K-V store should be an implementation of tempstore, one that does not do cleanup? Note that I have no skin in this game -- I work with MongoDB which can do all the basic operations necessary here aplently. I will get msonnabaum to comment from a Redis angle.

Note: batch table could be converted to this store because it's the same structure (k-v store with a range query for cleanup) and then if this store becomes pluggable so becomes batch.

pounard’s picture

#80 From my point of view, I proposed a long time ago (almost a year) a full K/V API with expiration time on a key basis, and wildcard selection operations [EDIT: Ok this won't be usefull here]. Almost everyone said it was too complex. From what I can remember all K/V stores that I know support all the operations I proposed at the time [EDIT: Memcache is not a key/value database, it's a key/value in-memory array with no persistence], including the one you need right now [EDIT: I am wrong here too]. You rejected it and refused to listen, and implemented a totally useless bastard and overly simple implementation [EDIT: I still think this is true, temp store is now a good example that K/V don't fit at all here] now you need it, and you don't know what to do [EDIT: In fact, I'm all wrong here, the K/V just don't have any sense in this issue], and you are trying to figure out an extremely complex solution where the K/V store become a specialization of the TempStore. This is a huge WTF, and I wanted to tell you: I told you so. So long and thanks for all the fish. [EDIT: This is humour]

chx’s picture

To the contrary: the simple API we have right now is very useful, the whole system table removal works with it, it supports state so it does what we wanted. Those do not need time-based expire -- it's harmful to them, so why provide a method for their API? As for wildcards, I have no idea what are you talking about: neither the memcached protocol (which is widely used for persistent K-V stores, hell, MySQL 5.6 provides you with a memcache interface into InnoDB) nor redis support wildcard searches (and do not tell me to use KEYS, it's a debug command which blocks the whole Redis instance).

Now that's done, let's get back to our regular programming and figure out how tempstore and K-V relates.

pounard’s picture

EDIT: Deleted. Enough is enough. Just get back to your "regular programming". Re-EDIT: I was indeed wrong in this issue, because the K/V store does not fit at all with this temp store, because the temp store requires way more features than the k/v store.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
4.92 KB
17.58 KB

Okay, this removes getLockOwner() and isLocked() as requested in #69.
I've subclassed it in Views, and that works fine for our needs.
So as far as VDC is concerned, this is good.

msonnabaum’s picture

For the record, the interface with locking as is won't work with ANY k/v store, which is why I haven't advocated the use of KeyValueStore here. If locking is separated later on, I've always intended to add a KeyValueStoreExpirable interface once we had a good use case, so we can revisit that in a followup. Almost anywhere k/v could be used can be refactored to use it later, so let's not discuss it anymore in this issue.

I agree that the TempStore class as is has an odd set of responsibilities, but they work well for views, so whatever we can do to keep this interface for views and refactor later seems sensible to me.

chx’s picture

All i can say: totally agreed with #85 and will find timplunkett to discuss the split as followups and once we have a clear vision of it then I will RTBC this. Hopefully, tomorrow.

chx’s picture

Assigned: Unassigned » chx
Status: Needs review » Needs work
Issue tags: -Needs architectural review

[10:40] <timplunkett> chx: you edit an existing view, and you make several changes. you are able to preview those changes in the admin UI, but it does not affect the actual view in use on the site
[10:40] <timplunkett> chx: you can continue to make changes, over multiple requests, or hours, or days, and they persist until you hit cancel or save
[10:40] <chx> so far so good
[10:40] <timplunkett> both cancel and save remove it from the tempstore, with save writing it to the active store
[10:41] <timplunkett> that's what the current patch provides
[10:41] <chx> but then why do you have this weird deleteAll method which can delete *everyone*'s edits?
[10:41] <timplunkett> the old one, and the subclass living in views, allows for the temporary state to be tied to a user
[10:42] <timplunkett> chx: views doesn't use deleteAll. it was just in the API from ctools
[10:47] <chx> timplunkett: i am going to assign the issue to myself and remove deleteAll and the testing for it and the , $all = FALSE
[10:47] <timplunkett> chx: sounds good to me
[10:48] <chx> timplunkett: because then this is the keyValueExpireStore with a namespace of namespace:owner_id , right?
[10:49] <chx> timplunkett: that's where msonnabaum and myself were headed
[10:49] <timplunkett> chx: hm, yeah!

chx’s picture

Status: Needs work » Needs review
FileSize
18.95 KB

Written from ground up without using any of the previous code, solely based on the requirements timplunkett gave me and guidance on the class hiearchy from msonnabaum.

chx’s picture

FileSize
20.06 KB

OK that was not correct. But this is.

tim.plunkett’s picture

For the record, I am extremely +1 on this new approach.

tim.plunkett’s picture

Assigned: chx » tim.plunkett

Assigning to myself for some real world testing with Views.

chx’s picture

FileSize
4.38 KB
20.93 KB

Very small fixes: delete doesn't loop just tries twice to get the lock and throws an exception. Added a test for expired items. Reorganized the test a little with an additional helper. No functional change. The added unit test (yeah! fast!) covers these use cases:

  1. Two users, one after the edit tries to edit a view.
  2. The second user breaks the lock and edits the view.
  3. The second user updates the view.
  4. The first user attempts to update the view.
  5. After the item expired, it no longer can be retrieved.
Crell’s picture

chx asked me to look this over for architectural cleanliness. I likely won't have a chance until Monday night or Tuesday night, but I will try to look it over ASAP.

pounard’s picture

The KeyValueStoreExpireInterface has no use of living on its own, the setExpire() method can live in the KeyValueExpireInterface quite easily: for backends that don't support it natively storing a data structure with a timestamp that we check at the get call is enough to reproduce this feature on the API side.

Please avoid to create such interface only in order add one single method, where the feature could be merged to all backends without any huge effort, even if they don't support it natively.

I'm not convinced the setIfNotExists() is necessary, since it's just a shortcurt for exists() and set() calls altogether. This is has an advantage thought: this makes sense to have it for pure optimization purposes. Nevertheless you are introducing high level functions that don't match any real backend feature that I know of.

[Pure nitpicking] Stuff such as:

      $this->connection->delete($this->table)
        ->condition('name', array_splice($keys, 0, 1000))
        ->condition('collection', $this->collection)
        ->execute();

Could be rewrote as such:

      $this
        ->connection
        ->delete($this->table)
        ->condition('name', array_splice($keys, 0, 1000))
        ->condition('collection', $this->collection)
        ->execute();
chx’s picture

FileSize
20.93 KB

Last reroll for tonight :) had a small problem in the drupal_container register.

tim.plunkett’s picture

FileSize
2.41 KB
21.02 KB

In our use case, we need to know not only who owns it, but when it was created.

The interdiff reflects that, as well as an issue with the registration.

It needs a docs clean-up, if chx/msonnabaum are okay with the change I made, I can do that today.

Status: Needs review » Needs work

The last submitted patch, tempstore-1642062-96.patch, failed testing.

chx’s picture

Works for me, I expect people to add setExpire to their factories if they want to change it anyways so relying it being constant is fine. Please change getOwner to getMetadata and adjust the test. Be very thoughful if you decide to test the returned expire in getMetadata to not add a random test failure.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
7.87 KB
21.94 KB

Okay, this renames the method, fixes the test, and cleans up the docs/coding style.

I'm 100% happy with this as is, it works great for Views.

Lars Toomre’s picture

Given that this issue looks close to commit stage, here is a detailed review of #99 from a coding and documentation perspective.

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueStoreExpireInterface.phpundefined
@@ -0,0 +1,23 @@
+  /**
+   * Sets the expiration time for this key/value store collection.
+   *
+   * @param int $expire
+   *   The expiration time in milliseconds.
+   */
+  function setExpire($expire);

I think that you mean seconds here, not milliseconds.

+++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwner.phpundefined
@@ -0,0 +1,101 @@
+  /**
+   * @param KeyValueStoreExpireInterface $storage
+   * @param Drupal\Core\Lock\LockBackendInterface $lockBackend
+   * @param mixed $owner

Each of these @param directives need an explanation as well.

+++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwner.phpundefined
@@ -0,0 +1,101 @@
+  /**
+   * Returns the stored value for the default key.
+   *
+   * @return mixed

This and other functions in class KeyValueStoreWithOwner are missing @param directives.

+++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwnerFactory.phpundefined
@@ -0,0 +1,31 @@
+class KeyValueStoreWithOwnerFactory {
+
+  function __construct(Connection $connection, LockBackendInterface $lockBackend) {
+    $this->connection = $connection;
+    $this->lockBackend = $lockBackend;
+  }

This is missing a docblock.

Also it appears that ->connection and ->lockBackend are missing docblocks as well.

+++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwnerFactory.phpundefined
@@ -0,0 +1,31 @@
+  /**
+   * Create a KeyValueStoreDefaultExpire stored in the database.
+   *
+   * @param Drupal\Core\Database\Connection $connection
+   *
+   * @return Drupal\Core\KeyValueStore\KeyValueStoreDefaultExpire

I think this should start with 'Creates'. Also $connection is not name of variable, $namespace is used in function. Finally, don't both @param and @return directives require an explanation?

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
@@ -0,0 +1,141 @@
+   * @param $uid
+   *   A user ID.
+   * @return Drupal\user\KeyValueStoreWithOwner
+   */
+  protected function getStorePerUid($uid) {

I think @return needs an explanation as well as a blank line above.

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
@@ -0,0 +1,141 @@
+  }
+
+  protected function assertIdenticalObject($object1, $object2) {
+    $identical = TRUE;
+    foreach ($object1 as $key => $value) {

Missing docblock for this method.

pounard’s picture

Moving the setExpire() method onto the KeyValueStoreInterface sounds a good thing to do. All known backend will be able to implement this easily.

chx’s picture

I do not support merging the keyValueStoreExpireInterface into keyValueStoreInterface which is what #101 suggests. Having two interfaces separately is the best of both worlds: if you are writing, say, a Redis backend you can supply one backend implementing two interfaces. The database backend, however, can be much faster if we do not need to support expire on state(), module schema and similar things so two backends make sense.

And, if we decide later this doesn't make sense it's a lot lot easier to merge them than to separate. So, I strongly recommend continuing with two interfaces.

pounard’s picture

Status: Needs review » Needs work

Set of other notes:

  • user module may be a required module, having the KeyValueStoreWithOwnerFactory registered by the CoreBundle adds yet another user module dependency over the Drupal\Core namespace (and this is a circular dependency because the component depends on core components).
  • Having two separate implementations DatabaseStorage and DatabaseStorageExpire makes no sense and double the code to maintain, while having a single DatabaseStorage alone that implements KeyValueStoreExpireInterface would make a lot of sense [EDIT: This would need the setExpire() method being changed as explained a bit lower]
  • The KeyValueStoreExpireInterface with the expire infix sounds wrong, neither correct english neither natural technical name, and pollutes the DatabaseStorageExpire name for only one added method
  • Having a default expire value in the backend, and not having anyway to set it on a per key basis sounds a use case we won't need anywhere else: it doesn't worth to be implemented as a core public API. Having a the setExpire() method being an alias of the set() method with a lifetime as second parameter would make much more sense.
  • Are you really going to use the table parameter of DatabaseStorageExpire::expire()? Wouldn't it be more accurate and consistent to consider that the table name is based on the collection name and fix the DatabaseStorage accordingly?

This patch seems highly inconsistent to me because the covered use case does not seem to be useful outside of the temp store. It would be much more efficient to implement it in a much more generic way, closer to what cache does with expiration timestamp.

I don't know what are the needs of the temp store, but it would sound nice to be able to select data stuff on a per user basis?

The lock usage seems wrong here, what is needed is an applicative level lock (higher locking API, different from actual core mutex system). The actual implementation may be fine until a better implementation is done, but it definitely can be improved.

Temp store is using a very specific derivative of k/v store just like a cache backend. So close that it sound odd than both exists in core. It's right in the middle between a business specific API (which would make a lot of sense) and a simple cache usage (which can make sense too). It's also a variant of how forms are storing their state, it sounds weird to duplicate this kind of code, we have now three methods for doing the same thing, and a very specialized weird k/v store that fit a very specific business need but which is not flexible.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.28 KB
24.09 KB

Thanks, I only went through the Drupal\Core\KeyValueStore classes, I forgot to overhaul the docs for the classes added to Drupal\user.

Status: Needs review » Needs work

The last submitted patch, tempstore-1642062-103.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

I don't know what are the needs of the temp store, but it would sound nice to be able to select data stuff on a per user basis?

No, it's not needed by temp store, and we're not making up new use cases because they "sound nice".

Temp store is using a very specific derivative of k/v store just like a cache backend. So close that it sound odd than both exists in core. It's right in the middle between a business specific API (which would make a lot of sense) and a simple cache usage (which can make sense too). It's also a variant of how forms are storing their state, it sounds weird to duplicate this kind of code, we have now three methods for doing the same thing, and a very specialized weird k/v store that fit a very specific business need but which is not flexible.

This indicates that you do not understand the use case, or the needs it presents. chx took the time to discuss and address all of our concerns, and worked with @msonnabaum to reuse the existing core classes and concepts as much as possible.

No one liked the original solution (any patch above #87), because it didn't do all of the things this class does specifically. I would welcome a more theoretically pure solution, but I don't think one exists, nor will it materialize before December 1st.

pounard’s picture

I do not support merging the keyValueStoreExpireInterface into keyValueStoreInterface which is what #101 suggests. Having two interfaces separately is the best of both worlds: if you are writing, say, a Redis backend you can supply one backend implementing two interfaces. The database backend, however, can be much faster if we do not need to support expire on state(), module schema and similar things so two backends make sense.

Expire if not used in database is just a integer column that will be silent, and add no extra overhaul; Plus if we consider the already existing key column, which is a varchar index, adding a integer index is the least of your troubles. A lot of backends will support expire natively, and some that don't, e.g. mongo, will be so fast you won't even see a difference even with millions of records. In all cases, having two different interfaces forces to maintain the double amount of code, because the new one is extremelly wrong: having a default lifetime, which is the same for every key, does not sound wise while most use cases of an expiration time will need to be able to set a per key expiration time.

tim.plunkett’s picture

FileSize
24.09 KB

Had a period instead of a semicolon.

Lars Toomre’s picture

Thanks @tim! I reviewed the patch in #104 and I can confirm that you have addressed all of my concerns from #100. From a docs and code standards perspective, I believe that #104 is good to go!

pounard’s picture

This indicates that you do not understand the use case, or the needs it presents. chx took the time to discuss and address all of our concerns, and worked with @msonnabaum to reuse the existing core classes and concepts as much as possible.

I wasn't speaking in term of business, but in term of implementation. You set a key, it expires. I quite well understood the use case, I did wrote equivalent features some years ago in client projects, based upon an applicative-leveled user/token based locking API with possible lock sharing amongs sessions for the same user, possibility for him to revoke/renew locks manually, providing a full UI and possible automatic expiration with user session, providing a temporary store for editing complex business objects. So I guess this is exactly the use case you're trying to implement.

No one liked the original solution (any patch above #87), because it didn't do all of the things this class does specifically. I would welcome a more theoretically pure solution, but I don't think one exists, nor will it materialize before December 1st.

I just gave you a strong path regarding the k/v store code, but I won't for temp store. But things I proposed upper would actually work for you.

dawehner’s picture

One small thing which i didn't liked was that KeyValueStoreWithOwnerFactory currently
always use DatabaseStorageExpire even there is really no reason to do so. Just add
the storage from outside as well totally helps.

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
@@ -0,0 +1,151 @@
+    $this->storeFactory = new KeyValueStoreWithOwnerFactory(Database::getConnection(), new DatabaseLockBackend());

If you already change the file it would be cool to document storeFactory

chx’s picture

FileSize
24.33 KB
msonnabaum’s picture

#112 looks good to me.

pounard’s picture

This patch addresses numeours concerns I emited, and after this long chat with chx and msonnabaum, I think this is a good step forward. Thanks.

Small nitpicking: About the @todo in catch blocks, because the k/v store is persistent (unlike cache which is volatile) we should consider letting the exceptions being raised, because a backend downtime means a potential site break, returning an empty array can break serious business stuff upper which may think the values are not set and behave wrongly instead of throwing an error and stop their work.

Note for later: I think that having only class for database would be good in the future.

EDIT: I won't RTBC, I think Tim should, because the issue is about the temp store.

chx’s picture

Note for later: I think that having only class for database would be good in the future. <= totally agreed. This needs some DIC untangling (cos the regular K-V thing needs to be in bootstrap DIC for early state() ) and also can happen past Dec 1, it's just implementation.

Status: Needs review » Needs work
Issue tags: -API addition, -VDC, -CTools dependency

The last submitted patch, 1642062_112.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
Issue tags: +API addition, +VDC, +CTools dependency

#112: 1642062_112.patch queued for re-testing.

tim.plunkett’s picture

FileSize
3.13 KB
25.32 KB

I did another docs pass.

I'm perfectly happy with this, but I've worked on it too much to RTBC.

Lars Toomre’s picture

I can confirm based on the interdiff and patch itself that the incremental docs are all good. I will leave it to someone else to RTBC.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

This looks great to me. Excellent collaboration!

cweagans’s picture

This has signoff from effulgentsia, tim.plunkett, msonnabaum, pounard, and chx, plus the patch looks great to me. +1 for RTBC.

star-szr’s picture

FileSize
1.01 KB
25.37 KB

Extremely minor docs updates, still RTBC.

Created a followup to rename/update Drupal\user\Tests\TempStoreDatabaseTest, that shouldn't hold up committing this.
#1800600: Rename/update Drupal\user\Tests\TempStoreDatabaseTest

Crell’s picture

FileSize
2.28 KB
25.67 KB

A few minor nits, but otherwise this passes my cleanliness smell test:

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.php
@@ -80,6 +89,23 @@ class DatabaseStorage extends StorageBase {
+  public function setIfNotExists($key, $value) {
+    $result = db_merge($this->table)
+      ->insertFields(array(
+        'collection' => $this->collection,
+        'name' => $key,
+        'value' => $value,
+      ))
+      ->condition('collection', $this->collection)
+      ->condition('name', $key)
+      ->condition('value', $value)
+      ->execute();
+    return $result == Merge::STATUS_INSERT;
+  }

This needs a @todo to convert it to an injected object once that's feasible.

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php
@@ -0,0 +1,145 @@
+      $result = $this->connection->query('SELECT name, value, expire FROM {' . db_escape_table($this->table) . '} WHERE expire > :now AND name IN (:keys) AND collection = :collection',
+        array(
+          ':now' => REQUEST_TIME,
+          ':keys' => $keys,
+          ':collection' => $this->collection,
+      ))->fetchAllAssoc('name');
+      foreach ($keys as $key) {

db_escape_table() should be $this->connection->escapeTable().

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php
@@ -0,0 +1,145 @@
+    $result = $this->connection->query('SELECT name, value FROM {' . db_escape_table($this->table) . '} WHERE collection = :collection AND expire > :now', array(':collection' => $this->collection, ':now' => REQUEST_TIME));

Ibid.

Attached patch fixes the above. No other changes. No need to credit me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1642062-tempstore.patch, failed testing.

Crell’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
25.67 KB

Grumble grumble remember to fetch --all first grumble grumble.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1642062-tempstore.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
25.65 KB

Fixed the hook_update_N conflict.

msonnabaum’s picture

Status: Reviewed & tested by the community » Needs work

It's fine if we want to change the db_merge to Database::getConnection()->merge() or whatever for autoloading purposes, but there is absolutely no reason it needs to be injected into a class that's specifically implementing a dbtng backend for k/v.

msonnabaum’s picture

Status: Needs work » Reviewed & tested by the community

Crosspost, moving back to RTBC.

Love the patch, just not a supporter of injecting all the things.

xjm’s picture

Do we have followup issues for the multiplicity of @todo in the RTBC patch? Let's file them and link them in the summary. #128 could go in a followup as well.

xjm’s picture

Issue summary: View changes

updated issue summary

xjm’s picture

Priority: Normal » Major

Alright, so #128 is actually just in reply to the @todo from @Crell's patch:

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.phpundefined
@@ -7,15 +7,28 @@
+ * @todo This class still calls db_* functions directly because it's needed
+ *   very early, pre-Container.  Once the early bootstrap dependencies are
+ *   sorted out, switch this to use an injected database connection.

And the other @todo are already in core; one is just a copypaste of another that we are <soapbox>reformatting out of scope here</soapbox>. So that's just one followup issue.

Berdir’s picture

A number of questions about the code, which looks nice and could also replace the current cache_update table I think. I'm not sure if my questions have been addressed/discussed already, so not changing the status.

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.phpundefined
@@ -0,0 +1,145 @@
+    return $result == MERGE::STATUS_INSERT;

This shouldn't be uppercase. Class names are case sensitive, or am I wrong? At least Netbeans isn't able resolve "MERGE" and also shows the use statement as unused.

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueStoreExpirableInterface.phpundefined
@@ -0,0 +1,52 @@
+interface KeyValueStoreExpirableInterface extends KeyValueStoreInterface {

This means that it is also possible to call set() *without* an expiration on an expirable key storage. What exactly is going to happen in this case? Is this defined/supposed to be possible?

+++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwner.phpundefined
@@ -0,0 +1,155 @@
+  /**
+   * The time to live for items in seconds. Defaults to a week.
+   *
+   * @var int
+   */
+  protected $expire = 604800;

The description says it *defaults* to a week but there is no way to change it?

That doesn't make much sense to me. Either it should be changeable somehow (I'm not sure if subclassing counts, looks rather hardcoded..) or the comment shouldn't say that it's just a default :)

xjm’s picture

This shouldn't be uppercase. Class names are case sensitive, or am I wrong? At least Netbeans isn't able resolve "MERGE" and also shows the use statement as unused.

Agreed, I think that should be Merge::STATUS_INSERT. It will probably choke on some environments I think? But should be changed.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Gave this a review. Most if it is minor, apart from terminology check/docs on this "owner" concept (I have not kept up on this issue so I have no idea what that is, and the docs don't tell me unfortunately), as well as question about how we're implementing this expiration column.

+++ b/core/modules/system/system.install
@@ -832,6 +832,43 @@ function system_schema() {
+  $schema['key_value_expire'] = array(
+    'description' => 'Generic key-value storage table with an expire.',

It seems exceedingly odd to re-create the schema for key_value only to add a column to it. I asked Tim about this in IRC and he pointed out that we don't want key/value store implementors to have to schema alter key/value in order to add more stuff. That's probably fair enough, but it seems odd that an expire column isn't standard, run-of-the mill column available. I could see many contributed modules replicating this in lots of incompatible ways (expiration, expire, timestamp, etc.)

If we do need to do it this way, I think the least we can do is make a wrapper function to get you the default key/value store table definition, similar to what we do for cache tables. This helps ensure better consistency.

+++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwner.php
@@ -0,0 +1,155 @@
+/**
+ * Stores and retrieves values from a key/value store with a default key.
+ */
+class KeyValueStoreWithOwner {

Hm. That does not really explain to me why this is called "owner"

+++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwnerFactory.php
@@ -0,0 +1,60 @@
+/**
+ * Provides a factory for key/value storage that needs an owner.
+ */
+class KeyValueStoreWithOwnerFactory {

What is an "owner"? When do I need this one over the standard KeyValueStore?

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.php
@@ -0,0 +1,165 @@
+use stdClass;

AFAIK we don't "use stdClass" but rather do \stdClass in code. I would not normally nit-pick this but it can actually cause significant problems if a "use" statement isn't done in a given file.

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.php
@@ -0,0 +1,165 @@
+    // Create two users and two objects for testing.
+    for ($i = 0; $i <= 3; $i++) {

That looks like 4 to me, no? Let's just strike the number, or replace it with "a few"

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.php
@@ -0,0 +1,165 @@
+  /**
+   * Generates a random PHP object.
+   *
+   * @param int $size
+   *   The number of random keys to add to the object.
+   *
+   * @return stdClass
+   *   The generated object, with the specified number of random keys. Each key
+   *   has a random string value.
+   */
+  public function randomObject($size = 4) {
+    $object = new stdClass();
+    for ($i = 0; $i < $size; $i++) {
+      $random_key = $this->randomName();
+      $random_value = $this->randomString();
+      $object->{$random_key} = $random_value;
+    }
+    return $object;
+  }
...
+  /**
+   * Checks to see if two objects are identical.
+   *
+   * @param object $object1
+   *   The first object to check.
+   * @param object $object2
+   *   The second object to check.
+   */
+  protected function assertIdenticalObject($object1, $object2) {
+    $identical = TRUE;
+    foreach ($object1 as $key => $value) {
+      $identical = $identical && isset($object2->$key) && $object2->$key === $value;
+    }
+    $this->assertTrue($identical, format_string('!object1 is identical to !object2', array(
+      '!object1' => var_export($object1, TRUE),
+      '!object2' => var_export($object2, TRUE),
+    )));
+  }

These look like useful general functions. Can we pull them out into WebTestBase or similar?

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.php
@@ -0,0 +1,165 @@
+  public function testUserTempStore() {

Awesome work on the tests in this function. Very easy to follow.

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.php
@@ -0,0 +1,165 @@
+    // This relies on the logged user uid!!
+    return $this->storeFactory->get($this->collection);

Why the EXCLAMATION POINTS!!!! :D

It seems like this is important; if so, we should expand this a bit to explain why.

29 days to next Drupal core point release.

xjm’s picture

Assigned: Unassigned » xjm

I'll do another pass on the docs.

chx’s picture

AFAIK we don't "use stdClass" but rather do \stdClass in code. <= That policy was put in place today.

We are using two tables because , as I said above, I am afraid of the get performance. K-V runs a constant query straight against the table primary key but the KVE get runs a range query. However, this is indeed very likely to be premature optimization as the PK, as pounard points out, is strings, the range query is still indexed and to add, there won't be many items there. So, yeah, we can merge. Let's make the expire default 2147483647 (2**31-1, 2038 january 18) and then the items stored in the normal K-V store indeed never expire.

Berdir’s picture

I've started working on replacing cache_update using this API (the KeyValueStorageExpirable API, not temp store).

I've handled the wildcard stuff by using different collations so I can use getAll() but there is no deleteAll() and I need that to be able to replace http://api.drupal.org/api/drupal/modules%21update%21update.module/functi....

As an alternative, @msonnabaum suggested that we could also add a keys() method and then do $storage->deleteAll($storage->keys()). Or both.

This doesn't need to happen in here, if there's any sort of disagreement. No need to block this issue, we can discuss it in a follow-up or in the cache_update issue. Just wanted to write this down.

chx’s picture

I recommend adding methods in the issues where we need them like this issue adds setIfNotExists. The number of possible methods is close to infinite (do we want to increment, decrement, append, prepend, CAS, etc) so we add them on a needed basis.

Berdir’s picture

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.phpundefined
@@ -80,6 +93,23 @@ public function set($key, $value) {
+  public function setIfNotExists($key, $value) {
+    $result = db_merge($this->table)
+      ->insertFields(array(
+        'collection' => $this->collection,
+        'name' => $key,
+        'value' => $value,
+      ))
+      ->condition('collection', $this->collection)
+      ->condition('name', $key)
+      ->condition('value', $value)

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.phpundefined
@@ -0,0 +1,145 @@
+  /**
+   * Implements Drupal\Core\KeyValueStore\KeyValueStoreInterface::setWithExpireIfNotExists().
+   */
+  function setWithExpireIfNotExists($key, $value, $expire) {
+    $this->garbageCollection();
+    $result = $this->connection->merge($this->table)
+      ->insertFields(array(
+        'collection' => $this->collection,
+        'name' => $key,
+        'value' => serialize($value),
+        'expire' => REQUEST_TIME + $expire,
+      ))
+      ->condition('collection', $this->collection)
+      ->condition('name', $key)
+      ->execute();

setIfNotExists() does not serialize(). Also, only setIfNotExists() has the value condition. Which one is correct?

chx’s picture

Having a condition on value is wrong. And the serialize stuff needs to be added and apparently tested.

chx’s picture

Why the EXCLAMATION POINTS!!!! :D

Because relying on a global in a class is WRONG!!!! but i have nothing better.

Berdir’s picture

Another note: As suspected, currently, DatabaseStorageExpirable::set() will set it to expire 0, which means that get will never return it. The suggested fix by chx to set expire to the end of time (more or less ;)) will fix that.

pounard’s picture

#132

This shouldn't be uppercase. Class names are case sensitive, or am I wrong? At least Netbeans isn't able resolve "MERGE" and also shows the use statement as unused.

#133

Agreed, I think that should be Merge::STATUS_INSERT. It will probably choke on some environments I think? But should be changed.

We all agree that per convention, we need to keep the right case for class names. Actually, PHP is not case sensitive for class names (and for a whole lot of other stuffs, FALSE vs. false for example). But the PSR-0 autoloaders are, which means that if the Merge class has not been loaded before hitting this code, the autoloader will fail and code crash: but if the Merge query has been loaded before, it will work flawlessly. Stupid PHP is stupid. This is not environment dependent, it's a language feature.

chx’s picture

Oh dear, let's not waste too much breath on that, Merge::STATUS_INSERT is the correct one, end of story, i guess i got stuck on capslock prepping to type the constant.

pounard’s picture

It's just a small detail of PHP, a lot of people are often surprised by this so I like to clarify a bit (I have surprised myself when I figured that out). But that's not important to the patch itself, just pure brain food.

xjm’s picture

Dreditor is tripping out on me, so pasting some notes for my cleanup here.

Architectural

  1. +++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwnerFactory.phpundefined
    @@ -0,0 +1,60 @@
    +   * Creates a Drupal\user\KeyValueStoreWithOwner stored in the database.
    +   *
    +   * @param string $namespace
    +   *   The namespace to use for this key/value store.
    +   *
    +   * @return Drupal\user\KeyValueStoreWithOwner
    +   *   An instance of the the key/value store.
    +   */
    +  function get($namespace) {
    +    $storage = new DatabaseStorageExpirable($namespace, array('connection' => $this->connection));
    +    return new KeyValueStoreWithOwner($storage, $this->lockBackend, $GLOBALS['user']->uid ?: session_id());
    +  }
    
    +++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
    @@ -0,0 +1,165 @@
    +  protected function getStorePerUid($uid) {
    +    $GLOBALS['user']->uid = $uid;
    +    // This relies on the logged user uid!!
    +    return $this->storeFactory->get($this->collection);
    +  }
    

    So, chx is right that the fact that we have to set the user ID global for testing is kind of gross. The fact that it does rely on the logged-in user ID is by design, but one possibility might be to add an optional owner ID argument to the get() method.

    Also, this method should probably be getStorePerUID() per http://drupal.org/node/608152 (see also #1627350: Patch for: Case of acronyms in class names (SomethingXSSClassName versus SomethingXssClassName)).

  2. +++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
    @@ -0,0 +1,165 @@
    +  public function randomObject($size = 4) {
    ...
    +  protected function assertIdenticalObject($object1, $object2) {
    

    In some ways I wonder if these two methods should be on TestBase.

  3. +++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
    @@ -0,0 +1,165 @@
    +use stdClass;
    ...
    +   * An array of random stdClass objects.
    ...
    +   * @return stdClass
    ...
    +    $object = new stdClass();
    

    I'll correct this per #1614186: Coding standards for using "native" PHP classes like stdClass in namespaced code. (The use of \Exception is already correct in the patch.)

Incomplete documentation

  1. +++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwner.phpundefined
    @@ -0,0 +1,155 @@
    + * Stores and retrieves values from a key/value store with a default key.
    + */
    +class KeyValueStoreWithOwner {
    ...
    +  /**
    +   * The owner key to store along with the data.
    +   *
    +   * @var mixed
    +   */
    +  protected $owner;
    

    We should clarify what an "owner" is here, as per @webchick's feedback.

  2. +++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwner.phpundefined
    @@ -0,0 +1,155 @@
    +   * The time to live for items in seconds. Defaults to a week.
    +   *
    +   * @var int
    +   */
    +  protected $expire = 604800;
    

    I'll clarify documentation here.

  3. +++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwner.phpundefined
    @@ -0,0 +1,155 @@
    +   * Returns the stored value for the default key.
    ...
    +   * Adds the value for the default key if it doesn't exist yet.
    ...
    +   * Sets the value for the default key.
    

    Where's the "default" part of it? We're passing in $key, so the word "default" confuses me.

  4. +++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwner.phpundefined
    @@ -0,0 +1,155 @@
    +   * Deletes the value of the default key.
    +   *
    +   * @param string $key
    +   *   The key of the data to store.
    +   */
    

    This docblock seems inconsistent/incorrect/confusing.

  5. +++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
    @@ -0,0 +1,165 @@
    +   * The name of the collection to set and retrieve.
    

    Collection of what?

  6. +++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
    @@ -0,0 +1,165 @@
    +    module_load_install('system');
    +    $schema = system_schema();
    +    db_create_table('semaphore', $schema['semaphore']);
    +    db_create_table('key_value_expire', $schema['key_value_expire']);
    

    An inline comment explaining why we need to do this would be good.

Code style and grammar nitpicks

  1. +++ b/core/modules/system/system.installundefined
    @@ -832,6 +832,43 @@ function system_schema() {
    +    'description' => 'Generic key-value storage table with an expire.',
    
    @@ -2102,6 +2139,50 @@ function system_update_8022() {
    +    'description' => 'Generic key-value storage table with an expire.',
    

    "With an expire" sounds ungrammatical to me. Maybe "that expires" or "with an expiration"?

  2. +++ b/core/modules/system/system.installundefined
    @@ -832,6 +832,43 @@ function system_schema() {
    +        'description' => 'The key of the key-value pair. As KEY is a SQL reserved keyword, name was chosen instead.',
    
    @@ -2102,6 +2139,50 @@ function system_update_8022() {
    +        'description' => 'The key of the key-value pair. As KEY is a SQL reserved keyword, name was chosen instead.',
    

    I think this is a bit too much info for the context (the "was chosen" part in particular is unnecessary). Maybe an inline comment instead?

  3. +++ b/core/modules/system/system.installundefined
    @@ -832,6 +832,43 @@ function system_schema() {
    +        'description' => 'The value.',
    
    @@ -2102,6 +2139,50 @@ function system_update_8022() {
    +        'description' => 'The value.',
    

    Maybe "The value of the key-value pair" for clarity.

  4. +++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwner.phpundefined
    @@ -0,0 +1,155 @@
    +   *   An object with the owner and updated time if the key has a value, NULL
    +   *   otherwise.
    

    Either the comma should be replaced with a semicolon, or there should be a conjuction ("or"). There's a couple other places with this as well.

  5. +++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwnerException.phpundefined
    @@ -0,0 +1,13 @@
    + * Exception thrown when KeyValueStoreWithOwner can not acquire a lock.
    

    The docblock should start with a third-person verb.

  6. +++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
    @@ -0,0 +1,165 @@
    +   * An array of (fake) user ids.
    

    Should be "user IDs".

  7. +++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
    @@ -0,0 +1,165 @@
    +    // First test that only one setIfNotExists succeeds.
    

    setIfNotExists() should have parens.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Status: Needs work » Needs review
FileSize
9.25 KB
26.01 KB

Attached just cleans up the minor issues. Next I'll add some more thorough documentation.

Spoke to @tim.plunkett a bit about how the term "TempStore" is not actually used anywhere anymore other than the tests and DIC. I'll rename the test class and add docs explaining what user.tempstore is, and why you'd want to use it.

xjm’s picture

Title: Add TempStore for persistent, limited-term storage of non-cache data » Add persistent, limited-term storage of non-cache data
Status: Needs review » Needs work
+++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwner.phpundefined
@@ -0,0 +1,155 @@
+class KeyValueStoreWithOwner {

+++ b/core/modules/user/lib/Drupal/user/KeyValueStoreWithOwnerFactory.phpundefined
@@ -0,0 +1,60 @@
+class KeyValueStoreWithOwnerFactory {

I think these two classes are misnamed. Edit 2: Tim and I decided to go back to TempStore, with TempStoreFactory.

xjm’s picture

Status: Needs work » Needs review
FileSize
26.85 KB
26.85 KB

Attached fixes some docs in addition to renaming the classes and the minor cleanups. The interdiff is against the previously reviewed patch in #127 since some of the previous ones were broken.

xjm’s picture

FileSize
18.32 KB

Ugh, and the real interdiff. Sorry for the noise.

xjm’s picture

Not yet addressed:

  1. Edit: I did not yet add a way to override the session for testing purposes.
  2. There's still no way to override the default expiration short of extending the whole class.
  3. #137 through #142.
  4. I didn't move the random object methods to TestBase yet. Anyone have an opinion on that? Edit: and JUST now I see that @webchick actually said the same thing. I'll move them on the next reroll.
  5. +++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.phpundefined
    @@ -7,15 +7,28 @@
    + * @todo This class still calls db_* functions directly because it's needed
    + *   very early, pre-Container.  Once the early bootstrap dependencies are
    + *   sorted out, switch this to use an injected database connection.
    

    Could probably say "consider switching" to address @msonnabaum's concern.

  6. +++ b/core/modules/user/lib/Drupal/user/TempStoreFactory.phpundefined
    @@ -0,0 +1,64 @@
    +   * @param string $namespace
    +   *   The namespace to use for this key/value store.
    ...
    +  function get($namespace) {
    

    We should probably rename this to $collection to be consistent with the interface, explain what it's for, and give some examples.

  7. Edit: The methods in DatabaseStorageExpirable have no inline documentation. Maybe they don't need to, but it'd be nice.
  8. Edit: TempStore itself doesn't actually have any relationship to user; only TempStoreFactory does.
Lars Toomre’s picture

re: 156.4 I too agree that the random object methods should be moved to TestBase.

xjm’s picture

Attached:

  1. Updates and clarifies more documentation.
  2. Moves the random object methods to TestBase.
  3. Allows the $owner to be overridden in TempStoreFactory::get() so that it is more testable. This also could have other applications.
  4. Adds several @todo for points that seemed questionable in TempStore when I went through it carefully.

Not yet addressed:

  1. The @todo:
    +++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
    @@ -54,6 +53,9 @@ class TempStore {
    +   * @todo Currently, this property is not exposed anywhere, and so the only
    +   *   way to override it is by extending the class.
        */
       protected $expire = 604800;
    
    @@ -111,14 +115,25 @@ function setIfNotExists($key, $value) {
    +   * @todo Should we throw an exception here if the lock cannot be acquired
    +   *   like we do in delete()?
    +   * @todo Should we return something here so we know whether the operation
    +   *   was successful?
        */
       function set($key, $value) {
    
    @@ -154,12 +169,17 @@ function getMetadata($key) {
    +   * @todo Why does this ignore whether we're the data owner or not?
        */
       function delete($key) {
    
  2. @chx and @berdir in #136 and #142:

    We are using two tables because , as I said above, I am afraid of the get performance. K-V runs a constant query straight against the table primary key but the KVE get runs a range query. However, this is indeed very likely to be premature optimization as the PK, as pounard points out, is strings, the range query is still indexed and to add, there won't be many items there. So, yeah, we can merge. Let's make the expire default 2147483647 (2**31-1, 2038 january 18) and then the items stored in the normal K-V store indeed never expire.

    Another note: As suspected, currently, DatabaseStorageExpirable::set() will set it to expire 0, which means that get will never return it. The suggested fix by chx to set expire to the end of time (more or less ;)) will fix that.

  3. @berdir and @chx in #139 and #140:

    setIfNotExists() does not serialize(). Also, only setIfNotExists() has the value condition. Which one is correct?

    Having a condition on value is wrong. And the serialize stuff needs to be added and apparently tested.

Let's file followups for #137 / #138.

Berdir’s picture

I am not yet fully sold on merging those two tables.

This means that those two key stores share the same table/data. So if you set() something in the expirable key store, get() of the normal key store will return it, even if it's expired (obviously, because it doesn't care about it). And if you set something in the non-expirable keystore, the other one might or might not return it, depending on how it's implemented*. We would have to add an additional column to identify who added it there, which would make the indexes bigger.

This isn't good or bad, but we need to decide if we really want that. It's also tricky if someone would rely (no idea why, but people are creative...) on it but then you use a different backend for the kevalue store and it suddenly doesn't work like that anymore?

* The expire column could have a default value of 0/NULL, but then both 0 and MAX_TIMESTAMP would mean does-not-expire, or we could set the default value of that column to MAX_TIMESTAMP but then the non-expire keyvaluestore entries would a non-zero expiration date, which is a bit weird too.

I'm tired, not sure if what I'm writing is understandable...

Status: Needs review » Needs work

The last submitted patch, tempstore-1642062-158.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
29.08 KB

Attached fixes the test failure above and corrects the bugs described in #139. I'll file a followup for the expiration stuff.

The reason DatabaseStorage::setIfNotExists() doesn't have test coverage is that we override it for the TempStore implementation; I'll look at the k/v tests and see what we can add.

xjm’s picture

Alrighty, the attached modifies the k/v tests to test serialization more thoroughly, and adds test coverage for setIfNotExists() there. So the only outstanding non-followup issues are then these questions of mine:

+++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
@@ -111,14 +115,25 @@ function setIfNotExists($key, $value) {
+   * @todo Should we throw an exception here if the lock cannot be acquired
+   *   like we do in delete()?
+   * @todo Should we return something here so we know whether the operation
+   *   was successful?
    */
   function set($key, $value) {

@@ -154,12 +169,17 @@ function getMetadata($key) {
+   * @todo Why does this ignore whether we're the data owner or not?
    */
   function delete($key) {
xjm’s picture

Talked to @tim.plunkett in IRC about #162 and we made decisions there. Final (?) patch coming shortly.

xjm’s picture

Assigned: xjm » Unassigned
FileSize
6.53 KB
35.67 KB

Alright, I think this is it!

xjm’s picture

+++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
@@ -125,25 +122,25 @@ function setIfNotExists($key, $value) {
    * @todo Should we throw an exception here if the lock cannot be acquired
    *   like we do in delete()?

Oops, missed removing this @todo, but that's a quick reroll.

xjm’s picture

Oh, I forgot to document here -- @tim.plunkett and I decided that it's up to the caller or implementation to decide whether and when one owner should be able to delete another owner's data. (That's what our views implementation already did anyway). So that check is removed, and the codified behavior in the tests is changed to simply ensure the users can both access the data and see the correct owner.

Berdir’s picture

Status: Needs review » Needs work

Went through the patch in detail, here's some feedback.

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.phpundefined
@@ -7,15 +7,28 @@
   /**
+   * The name of the SQL table to use.
+   *
+   * @var string
+   */
+  protected $table;

If you're wondering about this, usage of $this->table (e.g. the constructor) was added in another patch but that patch was missing this.

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.phpundefined
@@ -0,0 +1,147 @@
+      $result = $this->connection->query('SELECT name, value, expire FROM {' . $this->connection->escapeTable($this->table) . '} WHERE expire > :now AND name IN (:keys) AND collection = :collection',
+        array(
+          ':now' => REQUEST_TIME,
+          ':keys' => $keys,
+          ':collection' => $this->collection,
+      ))->fetchAllAssoc('name');
+      foreach ($keys as $key) {
+        if (isset($result[$key])) {
+          $values[$key] = unserialize($result[$key]->value);
+        }

I believe this can be simplified quite a bit.

- Why are we selecting expire here?
- As we just need to return the value and not an object, we can use fetchAllKeyed() after expire is gone. That already gives us an array($name => $value).
- I believe the cache backend receives the keys by reference and removes those that found returned so that you easily know which ones are missing. That's where the foreach ($keys as $key) loop is coming from I think. We currently don't do that. Either we should do that or we can simplify the loop and don't need the condition.
- Actually we can even use array_map(). I confirmed that this passes the tests:

      $values = $this->connection->query('SELECT name, value FROM {' . $this->connection->escapeTable($this->table) . '} WHERE expire > :now AND name IN (:keys) AND collection = :collection',
        array(
          ':now' => REQUEST_TIME,
          ':keys' => $keys,
          ':collection' => $this->collection,
      ))->fetchAllKeyed();
$values = array_map('unserialize', $values);

Not sure if array_map() is actually faster or easier to understand, we can also leave that out.

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.phpundefined
@@ -0,0 +1,147 @@
+    $values = array();
+
+    foreach ($result as $item) {
+      if ($item) {
+        $values[$item->name] = unserialize($item->value);
+      }

Same as above with fetchAllKeyed(). And can someone explain me under which contain if ($item) could ever be false? :) If that would be possible, then every single query + loop in core would have to do this?

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueStoreExpirableInterface.phpundefined
@@ -0,0 +1,52 @@
+interface KeyValueStoreExpirableInterface extends KeyValueStoreInterface {

Do you plan to deal with KeyValueStoreExpirableInterface::set() in a follow-up?

We should be able to solve this easily by setting the default value of the expire column to 2147483647. Or overwrite set() in DatabaseStorageExpirable and call setWithExpire($key, $value, 2147483647).

It would also be easy to test. Just do a set and then a get on an expirable KV and we're good.

+++ b/core/modules/user/lib/Drupal/user/TempStoreFactory.phpundefined
@@ -0,0 +1,72 @@
+    // Store the data for this collection in the database.
+    $storage = new DatabaseStorageExpirable($collection, array('connection' => $this->connection));

For the record, I'm introducing a keyvaluestore.expire service in my update_cache issue so that will make this switchable without having to override this factory. So we don't need to discuss that here :)

xjm’s picture

Assigned: Unassigned » xjm

Do you plan to deal with KeyValueStoreExpirableInterface::set() in a follow-up?

I still don't quite understand this bit, but I'll clean up the other points, and file the followup for whether or not to merge DatabaseStorageExpirable back into DatabaseStorage. I also realized there should be a DatabaseStorageExpirableTest so I'll add that.

Berdir’s picture

The set() problem is quite easy. DatabaseStorageExpirable needs to support all methods from KeyValueStorageInterface as well. But when you call set(), it is stored with an expire of 0. Which mean that it's already expired and get() will never return it. The suggested fix by chx is to set expire to the max possible value, so it won't expire before 2038 :)

Simply add a set() and get() in the test class that you plan to write for expirable and you'll see the problem.

xjm’s picture

So one thing that's really confusing me here is that we explicitly specify the database connection in DatabaseStorageExpirable but not DatabaseStorage.

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review

Alright, the attached addresses everything in #167 plus adds more basic test coverage for DatabaseStorageExpirable. For now I'm leaving the bit with the database connection alone, except to add a default value of the current connection. Berdir says we could also remove it and then add it back for both classes once #1764474: Make Cache interface and backends use the DIC is fixed.

xjm’s picture

Berdir’s picture

Slight misunderstanding. What I meant is not remove the connection argument completely but simply not add a default value. That will be taken care by adding a generic keyvaluestore.expirable service, this happens in my cache_update issue. Then the default is provided through the service definition.

Edit: As explained by @xjm, this actually does make sense because of the way the test class is currently structured.

xjm’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageExpirableTest.phpundefined
@@ -0,0 +1,42 @@
+/**
+ * Tests the key-value database storage.
+ */
+class DatabaseStorageExpirableTest extends StorageTestBase {

Oh BTW, for the uninitiated who are asking "well that doesn't look like much of a test". StorageTestBase is a bit "clever" in that it defines test methods on the abstract base class, which are then of course automatically run for every subclass and therefore for each storage method. I personally think it would be better to simply provide helpers on the base class and then call the helpers on the actual child classes in their test methods, but it's outside the scope of this issue.

WRT the default for the connection, I'd say we can leave it in there until it's solved for both classes.

Berdir’s picture

Agreed, the interdiff looks good to me, I think this is RTBC. But I've written a part of the code in there myself, so someone else should probably look over it again.

xjm’s picture

Title: Add persistent, limited-term storage of non-cache data » Add TempStore for persistent, limited-term storage of non-cache data

Here's the followup I keep promising: #1805094: Decide whether to merge DatabaseStorage and DatabaseStorageExpirable

And here's a commit message:
Issue #1642062 by tim.plunkett, xjm, chx, merlinofchaos, damiankloip, dawehner, Berdir, aspilicious, Cottser: Add TempStore for persistent, limited-term storage of non-cache data.

Fabianx’s picture

Title: Add TempStore for persistent, limited-term storage of non-cache data » Add persistent, limited-term storage of non-cache data
+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.phpundefined
@@ -7,15 +7,28 @@
+  protected $table;

@@ -78,6 +91,22 @@ public function set($key, $value) {
+    $result = db_merge($this->table)

Uhm, what happens if this remains NULL?

Wouldn't this fail then?

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.phpundefined
@@ -0,0 +1,135 @@
+    return array_map('unserialize', $values);

This is really nice! :-)

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.phpundefined
@@ -0,0 +1,135 @@
+      $this->set($key, $value, $expire);

Why is this calling set and not setWithExpire?

I don't think there is a Test for setMultipleWithExpire.

+++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
@@ -0,0 +1,181 @@
+ * requests. Each TempStore belongs to a particular owner (e.g. a user,

The key itself needs to be secure for this to be secure?

Because the owner seems not to be checked when receiving objects. So I guess this is like a form token.

Would modules know that this is "insecure" / shared across users?

+++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
@@ -0,0 +1,181 @@
+   * @todo Currently, this property is not exposed anywhere, and so the only

I assume this will be a follow up issue? to maybe add a getter/setter?

+++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
@@ -0,0 +1,181 @@
+    return $this->storage->setWithExpireIfNotExists($key, $value, $this->expire);

Why is this not in critical section?

Huh! No locking?

I can see that maybe the db_merge() does not need the DB lock, but this should be documented as this could be not necessarily true for other implementations of the TempStorage. I think.

+++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
@@ -0,0 +1,181 @@
+      unset($object->data);

Why is this necessary? Ah, to unset the data ...

This means getMetadata is as fast/slow as get()?

That could be documented, so that people don't assume this is faster.

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
@@ -0,0 +1,148 @@
+  protected function setUp() {

Where is the tearDown for this?

Like in DatabaseStorageExpirableTest?

Or is that not needed?

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
@@ -0,0 +1,148 @@
+      // setIfNotExists() should fail the second time ($i = 1).

What does that mean?

Not totally intuitive, I would propose: $i = 1 => !$i = FALSE, but that is nit picking.

---

Overall a great patch, I read it from bottom up and it got clearer and clearer. I love the concept.

I found some things that were unclear and especially the purpose got not totally clear from the viewing of the comments in the patch itself, so I'd say some nice docs on this as followup would be great on how to (securely) use that.

(Disclaimer: I have not read the issue summary or any comments.)

Waiting for answer to my comments, but this seems close to RTBC.

Fabianx’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

  1. .

    +++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.phpundefined
    @@ -7,15 +7,28 @@
    + protected $table;

    @@ -78,6 +91,22 @@ public function set($key, $value) {
    + $result = db_merge($this->table)

    Uhm, what happens if this remains NULL?

    Wouldn't this fail then?

    It's set in the constructor, but we could move the default to the property I guess?

  2. .

    Why is this calling set and not setWithExpire?

    The above is in reference to DatabaseStorageExpirable::setMultipleWithExpire(), and yes, that looks like a bug. Perhaps a few more lines of tests specifically for the WithExpire methods are needed, since they aren't in the parent's implementation.

  3. .

    I assume this will be a follow up issue? to maybe add a getter/setter?

    Yep, that's correct, once we decide what to do with the expiration stuff. There's a patch Berdir is working on that adds a way to override this. I'll add that note to the summary.

  4. .

    Why is this not in critical section?

    Huh! No locking?

    I can see that maybe the db_merge() does not need the DB lock, but this should be documented as this could be not necessarily true for other implementations of the TempStorage. I think.

    setIfNotExists() does not need locking because by design it only happens if the object doesn't exist yet. So there's no chance of colliding with another operation on it.

  5. .

    Where is the tearDown for this?

    Like in DatabaseStorageExpirableTest?

    Or is that not needed?

    Most tests don't need tearDown() because the parent implementation is sufficient. Though actually in this case we could probably add it to delete the table, since we are extending UnitTestBase. Good catch!

  6. .

    Why is this necessary? Ah, to unset the data ...

    This means getMetadata is as fast/slow as get()?

    That could be documented, so that people don't assume this is faster.

    IMO it's not really necessary to explain; the difference is slight.

  7. .

    What does that mean?

    Not totally intuitive, I would propose: $i = 1 => !$i = FALSE, but that is nit picking.

    Er... I'll just type it out and say "when $i is 1". :)

  8. .

    I found some things that were unclear and especially the purpose got not totally clear from the viewing of the comments in the patch itself, so I'd say some nice docs on this as followup would be great on how to (securely) use that.

    The docblock for TempStore is the place to look for that. We could file a followup to add additional docs, but I don't want to block it on that.

xjm’s picture

Assigned: Unassigned » xjm

I'll clean these things up.

Fabianx’s picture

Title: Add persistent, limited-term storage of non-cache data » Add TempStore for persistent, limited-term storage of non-cache data
Status: Needs review » Needs work

After discussion in IRC:

Okay, then its only 2) and 5) from #178 plus typos and a little more docs.

Nice!

xjm’s picture

Title: Add TempStore for persistent, limited-term storage of non-cache data » Add persistent, limited-term storage of non-cache data
Status: Needs work » Needs review

Notes for myself for tomorrow:

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.phpundefined
@@ -42,9 +55,9 @@ public function getMultiple(array $keys) {
-      // @todo: Perhaps if the database is never going to be available,
-      // key/value requests should return FALSE in order to allow exception
-      // handling to occur but for now, keep it an array, always.
+      // @todo Perhaps if the database is never going to be available,
+      //   key/value requests should return FALSE in order to allow exception
+      //   handling to occur but for now, keep it an array, always.

This hunk is now completely out of scope.

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.phpundefined
@@ -0,0 +1,135 @@
+ * This is Drupal's default key/value store implementation. It uses the database
+ * to store key/value data with an expire date.
+ */
+class DatabaseStorageExpirable extends DatabaseStorage implements KeyValueStoreExpirableInterface {

First sentence here is misleading (copypastaed?)

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/StorageTestBase.phpundefined
@@ -123,4 +137,30 @@ public function testNonExistingKeys() {
+  public function testSetIfNotExists() {
+
+    $key = $this->randomName();
...
+    // Verify that the other collection is still not affected.
+    $this->assertFalse($this->store2->get($key));
+
+  }

Extra blank lines here.

+++ b/core/modules/system/system.installundefined
@@ -2102,6 +2140,51 @@ function system_update_8022() {
+      'expire' => array(
+        'description' => 'The time since Unix epoch in seconds when this item expires.',

This needs the extra bit of description that's already in $schema.

+++ b/core/modules/system/system.installundefined
@@ -2102,6 +2140,51 @@ function system_update_8022() {
+        'default' => 2147483647,

Maybe this wants to be a constant.

+++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
@@ -0,0 +1,181 @@
+ * A TempStore can be used to make temporary, non-cache data available across
+ * requests. Each TempStore belongs to a particular owner (e.g. a user,
+ * session, or process).TempStore data expires automatically after a given
+ * timeframe.

Missing space after the period there. We could also clarify a little more how "owner" is used (to indicate that it does not imply "private" or restricted data).

xjm’s picture

Status: Needs review » Needs work

xpost

xjm’s picture

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.phpundefined
@@ -0,0 +1,135 @@
+  /**
+   * Implements Drupal\Core\KeyValueStore\KeyValueStoreExpireInterface::setWithExpire().
+   */
+  function setWithExpire($key, $value, $expire) {
+    $this->garbageCollection();
...
+  /**
+   * Implements Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface::setWithExpireIfNotExists().
+   */
+  function setWithExpireIfNotExists($key, $value, $expire) {
+    $this->garbageCollection();
...
+  /**
+   * Implements Drupal\Core\KeyValueStore\KeyValueStoreExpirablInterface::setMultipleWithExpire().
+   */
+  function setMultipleWithExpire(array $data, $expire) {
+    foreach ($data as $key => $value) {
+      $this->set($key, $value, $expire);
+    }
+  }
...
+  /**
+   * Implements Drupal\Core\KeyValueStore\KeyValueStoreInterface::deleteMultiple().
+   */
+  public function deleteMultiple(array $keys) {
+    $this->garbageCollection();
+    parent::deleteMultiple($keys);
+  }
...
+  /**
+   * Deletes expired items.
+   */
+  protected function garbageCollection() {
+    $this->connection->delete($this->table)
+      ->condition('expire', REQUEST_TIME, '<')
+      ->execute();
+  }

In the process of writing tests, I noticed that garbage collection is being done on every set operation, and in fact once for every key on setMultiple() operations. That could add up to a LOT of queries. I think garbage collection should only be done on deletions, and then the application can also decide to do it on cron or whatever.

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
FileSize
13.56 KB
42.66 KB
42.65 KB

Attached addresses everything in #177 through #183, plus adds a whole bunch more test coverage. The new tests exposed the bug @Fabianx found (just like the last batch exposed the bug @Berdir found). I've also attached a "test-only" patch with everything but that fix to demonstrate the coverage. The interdiff is a complete interdiff from my last patch.

Two outstanding things in this patch:

  • I noticed it might be nice to add some whateverIfOwner() methods in TempStore, but that can also be a followup.
  • I'm not entirely sure what to do with the garbage collection; we need to be deleting expired items on cron or something. I also realized that garbageCollection() was protected. I made it public to test it. Conceptually it seems like it should be static, but it depends on the table and connection defined. This could also be a followup.
xjm’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageExpirableTest.phpundefined
@@ -39,4 +39,130 @@ protected function tearDown() {
+    $this->verbose($expire);

Oops, leftover debug.

xjm’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/StorageTestBase.phpundefined
@@ -142,11 +142,11 @@ public function testNonExistingKeys() {
   public function testSetIfNotExists() {
...
     for ($i = 0; $i <= 1; $i++) {
-      // setIfNotExists() should fail the second time ($i = 1).
+      // setIfNotExists() should be TRUE the first time (when $i is 0) and
+      // FALSE the second time (when $i is 1).

And this comment change should also go in the TempStore tests, I think.

xjm’s picture

Issue summary: View changes

.

star-szr’s picture

Thanks xjm! I removed my name from the commit message in the issue summary, no need for credit.

Fabianx’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageExpirableTest.phpundefined
@@ -39,4 +39,130 @@ protected function tearDown() {
+    $key = $this->randomName();
+    for ($i = 0; $i <= 1; $i++) {
+      // setIfNotExists() should be TRUE the first time (when $i is 0) and
+      // FALSE the second time (when $i is 1).
+      $this->assertEqual(!$i, $this->store1->setWithExpireIfNotExists($key, $this->objects[$i], $expire));

Should this be setWithExpireIfNotExists() instead of just setIfNotExists() in the comment?

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageExpirableTest.phpundefined
@@ -39,4 +39,130 @@ protected function tearDown() {
+    // Remove the item and try to set it again.
+    $this->store1->delete($key);
+    $this->store1->setIfNotExists($key, $this->objects[1]);
+    // This time it should succeed.
+    $this->assertIdenticalObject($this->objects[1], $this->store1->get($key));
+    // Verify that the other collection is still not affected.

Shouldn't this be setWithExpireIfNotExists() to test that case again?

+++ b/core/modules/user/lib/Drupal/user/TempStore.phpundefined
@@ -14,15 +14,29 @@
+ *
+ * Each TempStore belongs belongs to a particular owner (e.g. a user, session,
+ * or process). Multiple owners may use the same key/value collection, and the
+ * owner is stored along with the key/value pair.

There is a "belongs belongs".

Only docs, issues and maybe one non-critical test. That can be cleaned up and then RTBC - finally.

Fabianx’s picture

Issue summary: View changes

Updated issue summary.

Fabianx’s picture

Title: Add persistent, limited-term storage of non-cache data » Add TempStore for persistent, limited-term storage of non-cache data

Re-titling ...

xjm’s picture

Title: Add TempStore for persistent, limited-term storage of non-cache data » Add persistent, limited-term storage of non-cache data
FileSize
5.26 KB
42.81 KB

Attached cleans up #185 - #188 and randomizes the expiration time in the CRUD test for more robust coverage.

xjm’s picture

Title: Add persistent, limited-term storage of non-cache data » Add TempStore for persistent, limited-term storage of non-cache data

LOL

xjm’s picture

Issue summary: View changes

Updated issue summary.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Re-read, looks really nice.

I assume test bot will green this, so:

Reviewed and Tested By many many many in The whole Drupal Community.

RTBC

:-)

Fabianx’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Very small nit... The value 299792458 appears many times within this patch. Should it perhaps be a member with an initial value of 299792458 or even a constant? It would be helpful to identify where such a seemingly arbitrary value came from.

The one draw back with using such a wide range of 100 .. 299792458 is that most values of the default expire range will be way, way in the future. Does it makes sense to use a upper end value like a couple of weeks or a month (in seconds of course)?

Lars Toomre’s picture

Issue summary: View changes

.

xjm’s picture

@Lars Toomre: The number appears as a maximum value in a call to rand(). It's completely arbitrary. It's just an upper limit. We aren't actually even testing expiration in that method, just CRUD, so the only point is to ensure that no two are the same.

webchick’s picture

Title: Add TempStore for persistent, limited-term storage of non-cache data » Change notice: Add TempStore for persistent, limited-term storage of non-cache data
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Ok, awesome. This patch got cleaned up a lot since last time I looked. Great work. The naming and docs are much better, and the one piece of feedback that wasn't addressed (merging these two stores together) is covered in a follow-up at #1805094: Decide whether to merge DatabaseStorage and DatabaseStorageExpirable.

Therefore.......

Committed and pushed to 8.x. WOOHOO!

Let's get a change notice for this. I could see this being very helpful in other places in core, such as Field UI (and perhaps my very special dream of "true" node previews? :D)

Lars Toomre’s picture

Thanks for the explanation @xjm. Like I did when looking at the patch in #190, I suspect that some in the future will wonder where the value of 299792458 came from. However, it is a very small nit.

xjm’s picture

For the curious, it's the value of the speed of light in meters per second. ;) But it's just an upper limit of a random number in an automated test.

xjm’s picture

Issue summary: View changes

.

xjm’s picture

Issue summary: View changes

.

xjm’s picture

Issue summary: View changes

.

xjm’s picture

Assigned: Unassigned » xjm

Working on the change notification.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Title: Change notice: Add TempStore for persistent, limited-term storage of non-cache data » Add TempStore for persistent, limited-term storage of non-cache data
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs change record

Never mind on that last issue.

I added the change notification: http://drupal.org/node/1805940

Status: Fixed » Closed (fixed)

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

xjm’s picture

Assigned: xjm » Unassigned
fubhy’s picture

fubhy’s picture

Issue summary: View changes

Updated issue summary.

fubhy’s picture

fubhy’s picture

Issue summary: View changes

Updated issue summary.

Andre-B’s picture

sorry for hijacking this issue - but this is related: is there already something similar for d7?

Andre-B’s picture

Issue summary: View changes

Updated issue summary.