Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
- Add a class to core that is similar to the CTools "object cache" to allow this temporary storage.
Remaining tasks
Followup issues:
- #1805094: Decide whether to merge DatabaseStorage and DatabaseStorageExpirable
- Standardize connection handling for
DatabaseStorage
andDatabaseStorageExpirable
once the above and #1764474: Make Cache interface and backends use the DIC is resolved. - Add additional methods as they are needed by a specific implementation.
TempStore
itself does not rely on the User module; onlyTempStoreFactory
does. We might consider moving it in the future.- #1658068: Convert Views to use core TempStore instead of ctools_object_cache_*()
- #1805412: TempStore: Enable garbage collection on all temp store objects via hook_cron() or similar / #1805444: garbageCollection followups to TempStore
- #2008806: Add getIfOwner(), setIfOwner() and deleteIfOwner() methods to TempStore.
- #2008884: Provide an interface for TempStore implementations
API changes
- A
setIfNotExists()
method is added toKeyValueStoreInterface
and must now be implemented by all classes that implement that interface. - New
KeyValueStoreExpirableInterface
extendsKeyValueStoreInterface
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 ofDatabaseStorageExpirable
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 forTempStore
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')
.
Comment | File | Size | Author |
---|---|---|---|
#190 | tempstore-1642062-189.patch | 42.81 KB | xjm |
#190 | interdiff.txt | 5.26 KB | xjm |
#184 | tempstore-1642062-184-tests.patch | 42.65 KB | xjm |
#184 | tempstore-1642062-184-combined.patch | 42.66 KB | xjm |
#184 | interdiff-172-184.txt | 13.56 KB | xjm |
Comments
Comment #1
dawehnerI 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.
Comment #2
dawehnerChanged the getLockOwner method to use the $sessionTable information,
so if it is not the sessionTable just the sid will be returned.
Comment #3
dawehner.
Comment #5
tim.plunkettPosting an interdiff from #0-#3 without the test addition
Comment #6
tim.plunkettOkay, 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.
Comment #8
tim.plunkettThere were a couple big issues with the last patch.
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.
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.
Comment #9
xjmPer @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
Comment #10
tim.plunkettReminder for reroll, @todo consider removing this and making Views responsible for checking whether it is the current user instead
Comment #11
tim.plunkettThis patch moves code into a new TempStoreBase and gives "better" names to the two extending classes.
Comment #12
merlinofchaos CreditAttribution: merlinofchaos commentedTwo 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.
Comment #13
tim.plunkettAddressed all three of the two quick notes from #12 ;)
Comment #15
tim.plunkettCore commits blowing up my patch!
Also realized I was overzealous in my renaming.
Comment #16
merlinofchaos CreditAttribution: merlinofchaos commentedUnless 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.)
Comment #17
tim.plunkettUgh, I agree, but I checked the docs:
From http://drupal.org/node/608152
Comment #18
tim.plunkettOHHH you said te standalone ones. Whoops yeah.
Comment #19
tim.plunkettFixed up the ownerId/owner_id usages
Comment #20
tim.plunkettYet Another Refactoring.
Tested with Views, two logged in users and two anonymous users attempting to edit the same view, and everything worked as expected.
Comment #21
merlinofchaos CreditAttribution: merlinofchaos commentedOkay, there seems to be no point in UserTempStore; I would say at this point make TempSTore not abstract.
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.
Comment #22
merlinofchaos CreditAttribution: merlinofchaos commentedUh. There seems to be no point in *SpecialTempStore*. Sorry about that braino.
Comment #23
tim.plunkettIn 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?
Comment #24
merlinofchaos CreditAttribution: merlinofchaos commentedYeah, 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. :(
Comment #25
tim.plunkettWithout the ability to type hint, I don't think we gain anything. Just keeping the renaming part of the last change.
Comment #26
Crell CreditAttribution: Crell commentedWhat 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...)
Comment #27
neclimdulSkimmed through the patch. Saw a couple things.
What's the failure condition? It looks like we just return null but that's not documented.
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.
Comment #28
vlad.ilyich CreditAttribution: vlad.ilyich commentedpossibly stupid new-to-this-issue question - why is this not just using sessions?
Comment #29
vlad.ilyich CreditAttribution: vlad.ilyich commentedUserTempStore 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.
Comment #30
dawehnerOne 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.
Comment #31
Crell CreditAttribution: Crell commentedI 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.
Comment #32
vlad.ilyich CreditAttribution: vlad.ilyich commentedinteresting. 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?
Comment #33
tim.plunkettRerolled to address #27.
Comment #34
merlinofchaos CreditAttribution: merlinofchaos commentedMe 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.
Comment #35
sunI 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:
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:
$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).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.
Comment #36
sunTagging for syndication.
Comment #37
merlinofchaos CreditAttribution: merlinofchaos commentedThe 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.
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.
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.
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.
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.
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.
Comment #38
Crell CreditAttribution: Crell commentedI 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.
Comment #39
xjmWorking on a reroll/cleanup.
Comment #40
xjmThe attached patch rerolls so the patch applies and adds some documentation clarifications and code style cleanups.
Fixed:
Regarding:
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:
Comment #42
xjmAhem. :)
Comment #43
damiankloip CreditAttribution: damiankloip commentedI 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 :)
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.
Comment #44
aspilicious CreditAttribution: aspilicious commentedfixed #43
Comment #46
aspilicious CreditAttribution: aspilicious commentedanother try
Comment #48
damiankloip CreditAttribution: damiankloip commentedWe need a key condition on the merge query I think.
Comment #50
tim.plunkett#48: 1642062-48.patch queued for re-testing.
Comment #51
moshe weitzman CreditAttribution: moshe weitzman commentedIs anyone available and familiar enough with this feature to do a final code review and work toward RTBC?
Comment #52
aspilicious CreditAttribution: aspilicious commentedUse full namespace paths in documentation
+ * Overrides TempStore::__construct().
Comment #53
damiankloip CreditAttribution: damiankloip commentedRerolled 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.
Comment #55
damiankloip CreditAttribution: damiankloip commentedOh, new system_update_N hook added in the mean time.
Comment #56
moshe weitzman CreditAttribution: moshe weitzman commentedI agree that this one is ready. I reviewed the code and it is pretty clear, IMO. It adds tests, and they are passing. RTBC.
Comment #57
catchI haven't followed this issue recently, and didn't look at the last few comments, here's a first pass through the patch.
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?
Why $subsystem here rather than $bin or $namespace?
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.
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?
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.
This seems like use-case-specific logic. Why isn't calling code handling this stuff?
Do we really not need an interface?
Why $subsystem here rather than $bin or $namespace?
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.
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?
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?
Comment #58
sunI 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.
Comment #59
effulgentsia CreditAttribution: effulgentsia commentedI'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.
Comment #60
pounardActually 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.
Comment #61
tim.plunkettThis 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.
Comment #62
pounardAnother 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.
Comment #63
tim.plunkett@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.
Comment #64
pounardDoing 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.
Comment #65
tim.plunkettThis 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.
Comment #66
pounardWhat?
In all cases, I really think that the proper course of action is to fraction this patch in two before getting it commited.
Comment #67
damiankloip CreditAttribution: damiankloip commentedok, this patch addresses the following of catch's issues in #57:
What I'm not as sure about:
Comment #69
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #70
merlinofchaos CreditAttribution: merlinofchaos commentedAs 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.
Comment #71
pounardIt 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.
Comment #72
catchDo you mean joining on {session}?
Comment #73
tim.plunkettPlease 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.
Comment #74
pounardSorry, 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.
Comment #75
tim.plunkettI'm going to follow-up along the lines of #69, i.e. trimming it down and extending it in Views.
Comment #76
pounardIt sounds like a good and better plan to me.
Comment #77
tim.plunkettTagging.
Comment #78
Lars Toomre CreditAttribution: Lars Toomre commentedI 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.
Comment #79
tim.plunkett@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.
Comment #80
chx CreditAttribution: chx commentedIt 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.
Comment #81
pounard#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]Comment #82
chx CreditAttribution: chx commentedTo 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.
Comment #83
pounardEDIT: 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.
Comment #84
tim.plunkettOkay, 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.
Comment #85
msonnabaum CreditAttribution: msonnabaum commentedFor 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.
Comment #86
chx CreditAttribution: chx commentedAll 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.
Comment #87
chx CreditAttribution: chx commentedComment #88
chx CreditAttribution: chx commentedWritten 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.
Comment #89
chx CreditAttribution: chx commentedOK that was not correct. But this is.
Comment #90
tim.plunkettFor the record, I am extremely +1 on this new approach.
Comment #91
tim.plunkettAssigning to myself for some real world testing with Views.
Comment #92
chx CreditAttribution: chx commentedVery 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:
Comment #93
Crell CreditAttribution: Crell commentedchx 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.
Comment #94
pounardThe
KeyValueStoreExpireInterface
has no use of living on its own, thesetExpire()
method can live in theKeyValueExpireInterface
quite easily: for backends that don't support it natively storing a data structure with a timestamp that we check at theget
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 forexists()
andset()
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:
Could be rewrote as such:
Comment #95
chx CreditAttribution: chx commentedLast reroll for tonight :) had a small problem in the drupal_container register.
Comment #96
tim.plunkettIn 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.
Comment #98
chx CreditAttribution: chx commentedWorks 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.
Comment #99
tim.plunkettOkay, 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.
Comment #100
Lars Toomre CreditAttribution: Lars Toomre commentedGiven that this issue looks close to commit stage, here is a detailed review of #99 from a coding and documentation perspective.
I think that you mean seconds here, not milliseconds.
Each of these @param directives need an explanation as well.
This and other functions in class KeyValueStoreWithOwner are missing @param directives.
This is missing a docblock.
Also it appears that ->connection and ->lockBackend are missing docblocks as well.
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?
I think @return needs an explanation as well as a blank line above.
Missing docblock for this method.
Comment #101
pounardMoving the setExpire() method onto the KeyValueStoreInterface sounds a good thing to do. All known backend will be able to implement this easily.
Comment #102
chx CreditAttribution: chx commentedI 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.
Comment #103
pounardSet of other notes:
KeyValueStoreWithOwnerFactory
registered by theCoreBundle
adds yet another user module dependency over theDrupal\Core
namespace (and this is a circular dependency because the component depends on core components).DatabaseStorage
andDatabaseStorageExpire
makes no sense and double the code to maintain, while having a singleDatabaseStorage
alone that implementsKeyValueStoreExpireInterface
would make a lot of sense [EDIT: This would need the setExpire() method being changed as explained a bit lower]KeyValueStoreExpireInterface
with the expire infix sounds wrong, neither correct english neither natural technical name, and pollutes theDatabaseStorageExpire
name for only one added methodDatabaseStorageExpire::expire()
? Wouldn't it be more accurate and consistent to consider that the table name is based on the collection name and fix theDatabaseStorage
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.
Comment #104
tim.plunkettThanks, I only went through the Drupal\Core\KeyValueStore classes, I forgot to overhaul the docs for the classes added to Drupal\user.
Comment #106
tim.plunkettNo, it's not needed by temp store, and we're not making up new use cases because they "sound nice".
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.
Comment #107
pounardExpire 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.
Comment #108
tim.plunkettHad a period instead of a semicolon.
Comment #109
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @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!
Comment #110
pounardI 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.
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.
Comment #111
dawehnerOne 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.
If you already change the file it would be cool to document storeFactory
Comment #112
chx CreditAttribution: chx commentedComment #113
msonnabaum CreditAttribution: msonnabaum commented#112 looks good to me.
Comment #114
pounardThis 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.
Comment #115
chx CreditAttribution: chx commentedNote 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.
Comment #117
chx CreditAttribution: chx commented#112: 1642062_112.patch queued for re-testing.
Comment #118
tim.plunkettI did another docs pass.
I'm perfectly happy with this, but I've worked on it too much to RTBC.
Comment #119
Lars Toomre CreditAttribution: Lars Toomre commentedI 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.
Comment #120
effulgentsia CreditAttribution: effulgentsia commentedThis looks great to me. Excellent collaboration!
Comment #121
cweagansThis has signoff from effulgentsia, tim.plunkett, msonnabaum, pounard, and chx, plus the patch looks great to me. +1 for RTBC.
Comment #122
star-szrExtremely 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
Comment #123
Crell CreditAttribution: Crell commentedA few minor nits, but otherwise this passes my cleanliness smell test:
This needs a @todo to convert it to an injected object once that's feasible.
db_escape_table() should be $this->connection->escapeTable().
Ibid.
Attached patch fixes the above. No other changes. No need to credit me.
Comment #125
Crell CreditAttribution: Crell commentedGrumble grumble remember to fetch --all first grumble grumble.
Comment #127
tim.plunkettFixed the hook_update_N conflict.
Comment #128
msonnabaum CreditAttribution: msonnabaum commentedIt'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.
Comment #129
msonnabaum CreditAttribution: msonnabaum commentedCrosspost, moving back to RTBC.
Love the patch, just not a supporter of injecting all the things.
Comment #130
xjmDo 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.
Comment #130.0
xjmupdated issue summary
Comment #131
xjmAlright, so #128 is actually just in reply to the @todo from @Crell's patch:
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.Comment #132
BerdirA 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.
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.
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?
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 :)
Comment #133
xjmAgreed, I think that should be
Merge::STATUS_INSERT
. It will probably choke on some environments I think? But should be changed.Comment #134
webchickGave 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.
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.
Hm. That does not really explain to me why this is called "owner"
What is an "owner"? When do I need this one over the standard KeyValueStore?
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.
That looks like 4 to me, no? Let's just strike the number, or replace it with "a few"
These look like useful general functions. Can we pull them out into WebTestBase or similar?
Awesome work on the tests in this function. Very easy to follow.
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.
Comment #135
xjmI'll do another pass on the docs.
Comment #136
chx CreditAttribution: chx commentedAFAIK 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.
Comment #137
BerdirI'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.
Comment #138
chx CreditAttribution: chx commentedI 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.
Comment #139
BerdirsetIfNotExists() does not serialize(). Also, only setIfNotExists() has the value condition. Which one is correct?
Comment #140
chx CreditAttribution: chx commentedHaving a condition on value is wrong. And the serialize stuff needs to be added and apparently tested.
Comment #141
chx CreditAttribution: chx commentedWhy the EXCLAMATION POINTS!!!! :D
Because relying on a global in a class is WRONG!!!! but i have nothing better.
Comment #142
BerdirAnother 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.
Comment #143
pounard#132
#133
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.
Comment #144
chx CreditAttribution: chx commentedOh 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.
Comment #145
pounardIt'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.
Comment #146
xjmDreditor is tripping out on me, so pasting some notes for my cleanup here.
Architectural
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)).In some ways I wonder if these two methods should be on
TestBase
.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
We should clarify what an "owner" is here, as per @webchick's feedback.
I'll clarify documentation here.
Where's the "default" part of it? We're passing in
$key
, so the word "default" confuses me.This docblock seems inconsistent/incorrect/confusing.
Collection of what?
An inline comment explaining why we need to do this would be good.
Code style and grammar nitpicks
"With an expire" sounds ungrammatical to me. Maybe "that expires" or "with an expiration"?
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?
Maybe "The value of the key-value pair" for clarity.
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.
The docblock should start with a third-person verb.
Should be "user IDs".
setIfNotExists()
should have parens.Comment #146.0
xjmUpdated issue summary.
Comment #147
xjmAttached 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.Comment #149
xjmI think these two classes are misnamed. Edit 2: Tim and I decided to go back to TempStore, with TempStoreFactory.
Comment #154
xjmAttached 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.
Comment #155
xjmUgh, and the real interdiff. Sorry for the noise.
Comment #156
xjmNot yet addressed:
Could probably say "consider switching" to address @msonnabaum's concern.
We should probably rename this to
$collection
to be consistent with the interface, explain what it's for, and give some examples.DatabaseStorageExpirable
have no inline documentation. Maybe they don't need to, but it'd be nice.TempStore
itself doesn't actually have any relationship touser
; onlyTempStoreFactory
does.Comment #157
Lars Toomre CreditAttribution: Lars Toomre commentedre: 156.4 I too agree that the random object methods should be moved to TestBase.
Comment #158
xjmAttached:
TestBase
.$owner
to be overridden inTempStoreFactory::get()
so that it is more testable. This also could have other applications.@todo
for points that seemed questionable inTempStore
when I went through it carefully.Not yet addressed:
@todo
:Let's file followups for #137 / #138.
Comment #159
BerdirI 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...
Comment #161
xjmAttached 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 theTempStore
implementation; I'll look at the k/v tests and see what we can add.Comment #162
xjmAlrighty, 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:Comment #163
xjmTalked to @tim.plunkett in IRC about #162 and we made decisions there. Final (?) patch coming shortly.
Comment #164
xjmAlright, I think this is it!
Comment #165
xjmOops, missed removing this @todo, but that's a quick reroll.
Comment #166
xjmOh, 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.
Comment #167
BerdirWent through the patch in detail, here's some feedback.
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.
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:
Not sure if array_map() is actually faster or easier to understand, we can also leave that out.
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?
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.
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 :)
Comment #168
xjmI 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.Comment #169
BerdirThe 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.
Comment #170
xjmSo one thing that's really confusing me here is that we explicitly specify the database connection in
DatabaseStorageExpirable
but notDatabaseStorage
.Comment #171
xjmAlright, 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.Comment #172
xjmComment #173
BerdirSlight 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.
Comment #174
xjmOh 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.
Comment #175
BerdirAgreed, 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.
Comment #176
xjmHere'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.
Comment #177
Fabianx CreditAttribution: Fabianx commentedUhm, what happens if this remains NULL?
Wouldn't this fail then?
This is really nice! :-)
Why is this calling set and not setWithExpire?
I don't think there is a Test for setMultipleWithExpire.
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?
I assume this will be a follow up issue? to maybe add a getter/setter?
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.
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.
Where is the tearDown for this?
Like in DatabaseStorageExpirableTest?
Or is that not needed?
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.
Comment #177.0
Fabianx CreditAttribution: Fabianx commentedUpdated issue summary.
Comment #178
xjmIt's set in the constructor, but we could move the default to the property I guess?
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.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.
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.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 extendingUnitTestBase
. Good catch!IMO it's not really necessary to explain; the difference is slight.
Er... I'll just type it out and say "when $i is 1". :)
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.Comment #179
xjmI'll clean these things up.
Comment #180
Fabianx CreditAttribution: Fabianx commentedAfter discussion in IRC:
Okay, then its only 2) and 5) from #178 plus typos and a little more docs.
Nice!
Comment #181
xjmNotes for myself for tomorrow:
This hunk is now completely out of scope.
First sentence here is misleading (copypastaed?)
Extra blank lines here.
This needs the extra bit of description that's already in
$schema
.Maybe this wants to be a constant.
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).
Comment #182
xjmxpost
Comment #183
xjmIn 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.
Comment #184
xjmAttached 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:
whateverIfOwner()
methods inTempStore
, but that can also be a followup.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.Comment #185
xjmOops, leftover debug.
Comment #186
xjmAnd this comment change should also go in the TempStore tests, I think.
Comment #186.0
xjm.
Comment #187
star-szrThanks xjm! I removed my name from the commit message in the issue summary, no need for credit.
Comment #188
Fabianx CreditAttribution: Fabianx commentedShould this be setWithExpireIfNotExists() instead of just setIfNotExists() in the comment?
Shouldn't this be setWithExpireIfNotExists() to test that case again?
There is a "belongs belongs".
Only docs, issues and maybe one non-critical test. That can be cleaned up and then RTBC - finally.
Comment #188.0
Fabianx CreditAttribution: Fabianx commentedUpdated issue summary.
Comment #189
Fabianx CreditAttribution: Fabianx commentedRe-titling ...
Comment #190
xjmAttached cleans up #185 - #188 and randomizes the expiration time in the CRUD test for more robust coverage.
Comment #191
xjmLOL
Comment #191.0
xjmUpdated issue summary.
Comment #192
Fabianx CreditAttribution: Fabianx commentedRe-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
:-)
Comment #192.0
Fabianx CreditAttribution: Fabianx commentedUpdated issue summary.
Comment #193
Lars Toomre CreditAttribution: Lars Toomre commentedVery 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)?
Comment #193.0
Lars Toomre CreditAttribution: Lars Toomre commented.
Comment #194
xjm@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.Comment #195
webchickOk, 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)
Comment #196
Lars Toomre CreditAttribution: Lars Toomre commentedThanks 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.
Comment #197
xjmFor 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.
Comment #197.0
xjm.
Comment #197.1
xjm.
Comment #197.2
xjm.
Comment #198
xjmWorking on the change notification.
Comment #198.0
xjmUpdated issue summary.
Comment #199
xjmPosted #1805854: Add user_tempstore() as a convenience wrapper.
Comment #199.0
xjmUpdated issue summary.
Comment #199.1
xjmUpdated issue summary.
Comment #200
xjmNever mind on that last issue.
I added the change notification: http://drupal.org/node/1805940
Comment #202
xjmComment #203
fubhy CreditAttribution: fubhy commentedAdded a follow-up task: #2008806: Add getIfOwner(), setIfOwner() and deleteIfOwner() methods to TempStore.
Comment #203.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #204
fubhy CreditAttribution: fubhy commentedAlso opened #2008884: Provide an interface for TempStore implementations.
Comment #204.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #205
Andre-Bsorry for hijacking this issue - but this is related: is there already something similar for d7?
Comment #205.0
Andre-BUpdated issue summary.