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.

CommentFileSizeAuthor
#12 drupal8.keyvalue-null.12.patch5.4 KBsun
#11 1790882-11-key-value-booleans.patch1.09 KBAnonymous (not verified)
key-value-booleans.patch542 bytesAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pounard’s picture

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

catch’s picture

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

Anonymous’s picture

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

pounard’s picture

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

Anonymous’s picture

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

sun’s picture

Title: make it easier to handle FALSE in the key value store » Allow to store value FALSE in the key value store
Issue tags: +KeyValueStore

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

pounard’s picture

Title: Allow to store value FALSE in the key value store » make it easier to handle FALSE in the key value store
Status: Needs review » Needs work
Issue tags: -KeyValueStore

The interface method signature needs PHPdoc update.

pounard’s picture

Title: make it easier to handle FALSE in the key value store » Allow to store value FALSE in the key value store
Issue tags: +KeyValueStore

Sorry cross post.

pounard’s picture

If we don't change for a result wrapper, having a utility method such as getLastResultStatus() would be an alternative catch-all use cases.

catch’s picture

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

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.09 KB

new patch to address #7.

sun’s picture

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

sun’s picture

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

Anonymous’s picture

sigh.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

#12 looks fine to me.

Dries’s picture

Looks fine to me too. @beejeebus; are you on board with this given your 'sigh' in #14?

Anonymous’s picture

yep, the patch in #12 is fine by me.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x.

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