Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
now that we have the storage (API) for persistent non-configuration state in, i think we should make it easier to handle FALSE as a value.
this patch just makes ->get($key) return NULL if the key doesn't exist, allowing code that stores boolean values to distinguish between 'the key is there, and the value is FALSE' and 'the key isn't there', which is unpossible right now.
Comment | File | Size | Author |
---|---|---|---|
#12 | drupal8.keyvalue-null.12.patch | 5.4 KB | sun |
#11 | 1790882-11-key-value-booleans.patch | 1.09 KB | Anonymous (not verified) |
key-value-booleans.patch | 542 bytes | Anonymous (not verified) | |
Comments
Comment #1
pounardThen you cannot store NULL, if you choose the patch of using a language constant for telling the key don't exist, the set() method must be changed in order to call delete() when the user attempt a set() with the NULL value.
Another solution is to work as cache does, returning a result object which holds data, or NULL when no data found.
Last, but which can be done alongside any of the two other options, is that you can add a method on the store interface which is something like "getLastResultStatus()" which returns either constants or booleans which means "success" or "failure", this implies the backend needs to keep this state in its internals until another query is done. Likewise, the exact same method can be implemented by passing a "$success" boolean per reference as second parameter of the get() method, but this method is less elegant and less flexible, plus the "getLastResultStatus()" method can be implemented for all operations without modifying their signatures.
Comment #2
catchYeah differentiating between NULL and nothing is a pain. I think returning an object same as cache is a good idea, people are used to it with cache already.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedi intentionally didn't handle NULL. see title. if someone wants to add NULL support, great, follow up issue.
this is a simple patch that puts us at feature parity with variable_get(), so i'd rather see it land without having a big discussion about NULL.
Comment #4
pounardThe NULL problem is the same as FALSE problem, and it needs to be fixed altogether, fixing one without the other this doesn't make any sense because it's exactly the same problem, I think we can do it at the same time, if it takes some more hours, I'd be for the greater good and will make us gain a lot of time later.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedno, they are not the same problem. did you read the code?
with patch, if you get FALSE back, then you can be sure it's because you stored FALSE.
if you get NULL back, you can't be sure. and again, i'm aware that issue exists, but i'm not trying to solve it with this issue. i'm trying to make a small, simple change that brings us up to parity with the system we are replacing.
so, please, if you want to add NULL support, go ahead and create another issue, and i'll review the patch you create.
Comment #6
sunI would agree that a result wrapper object would be the catch-all for all edge-cases.
But apparently, we survived pretty fine with special-casing NULL in the Variable API up until now.
The benefit of NULL is that it still allows for the one-line
state()->get('foo') ?: array()
logic in all cases in which a value that equals FALSE is not expected.Comment #7
pounardThe interface method signature needs PHPdoc update.
Comment #8
pounardSorry cross post.
Comment #9
pounardIf we don't change for a result wrapper, having a utility method such as getLastResultStatus() would be an alternative catch-all use cases.
Comment #10
catchI think we should open a new issue for a result wrapper, but we might want to work on #1748022: Make cache()->get() return a classed CacheItem object first before making that change - if we have an interface then both cache and KeyValue could at least extend the same base.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentednew patch to address #7.
Comment #12
sunI've added tests, which revealed that
- the empty()/reset() logic in ::get() was not explicit enough
- MemoryStorage had to be updated separately (since it's not extending AbstractStorage)
- the expected behavior of getMultiple() is currently different in that it does not return a (array) key for a requested key that does not exist. This inherently means that someone who even needs to additionally differentiate between NULL and no stored value can simply use ::getMultiple() instead of ::get(), along with a array_key_exists() on the returned result set.
I think this is ready to fly.
Comment #13
sunoh, and I intentionally kept the assertFalse() assertions in testCRUD(), since the purpose of that test is limited in scope to CRUD operations; i.e., testing data types is out of scope for it.
I couldn't resist to replace the assertEqual() with assertIdentical() in there though, since a CRUD test should actually verify that what it stores comes back identically.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedsigh.
Comment #15
msonnabaum CreditAttribution: msonnabaum commented#12 looks fine to me.
Comment #16
Dries CreditAttribution: Dries commentedLooks fine to me too. @beejeebus; are you on board with this given your 'sigh' in #14?
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedyep, the patch in #12 is fine by me.
Comment #18
Dries CreditAttribution: Dries commentedCommitted to 8.x.