Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37472

Change cache::get's return value from false to null.

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Won't Fix
    • Affects Version/s: 2.4
    • Fix Version/s: None
    • Component/s: Caching
    • Labels:
    • Affected Branches:
      MOODLE_24_STABLE

      Description

      Presently cache::get($key) returns false if the key was not found in any of the assigned caches and could not be loaded (if a loader is available).

      Its been suggested several times now that returning null instead of false would be a better idea.

      Have a read of the conversation David and myself had about this in dev chat (quoted here because linking to HQ chat logs means some can't see it)

      (10:12:59) DavidMudrak: hmm
      (10:13:21) DavidMudrak: $data = $cache->get('key'); returns false if the key was not found. I'm wondering why NULL has not been chosen instead.
      (10:15:11) DavidMudrak: samhemelryk?
      (10:16:22) mchurch left the room.
      (10:26:59) DavidMudrak: Otherwise I must say I am pretty impressed with the MUC API. Well done Sam!
      (10:27:09) samhemelryk: Thanks
      (10:28:29) aborrow left the room.
      (10:28:46) samhemelryk: False was perhaps not the right choice, I know Tim H suggested changing it to NULL at hackfest but I never got time to properly consider it/make the change
      (10:30:24) samhemelryk: Originally I had chosen false because it is commonly used by PHP cache extensions as the return value when they don't have the requested value.
      (10:30:48) samhemelryk: APC and memcached extensions for example
      (10:31:33) samhemelryk: I'm also not really sure whether there is a good use case for storing false (causing a collision of interest)
      (10:32:33) samhemelryk: But then at the time of thinking that I was more focused on the application cache
      (10:32:51) DavidMudrak: Right.
      (10:32:53) samhemelryk: *cache idea
      (10:33:04) DavidMudrak: I know that many PHP core function return FALSE in such cases
      (10:33:21) DavidMudrak: It's just that in my mind, the NULL is exactly this - unknown value.
      (10:33:22) samhemelryk: That was really my driving reason in this instance
      (10:33:35) samhemelryk: Yip - that is what Tim H pointed out to me as well
      (10:33:46) DavidMudrak: False is something known. Imagine caching of an array of flags.
      (10:33:48) samhemelryk: NULL would be more commonly used within our code as well I imagine
      (10:34:42) DavidMudrak: Anyway, I agree on the point of FALSE being used by common cache products.
      (10:35:22) DavidMudrak: Still. It is not late to change it IMHO. Better now (than never ).
      (10:36:29) DavidMudrak: See for example how isset() works.
      (10:36:30) samhemelryk: Certainly if we are going to change it sooner would be better
      (10:37:06) DavidMudrak: $a = false; $b = null; print_r(isset($a)); print_r(isset($b));
      (10:38:08) samhemelryk: I see that as an example of a php function that returns null, but I don't see its relevance to the fetching something from a cache
      (10:38:10) DavidMudrak: and as we have set() method ...
      (10:38:28) DavidMudrak: yeah
      (10:38:54) samhemelryk: I think null is worth looking into, but I think to change it at this point we would be best to have a really good reason to
      (10:39:16) DavidMudrak: I mean - it makes sense to store FALSE value in a cache (like a flag) but it does not make sense to store NULL.
      (10:40:22) DavidMudrak: $a = null; $cache->set('a', $a); // I can even imagine this throwing exception
      (10:41:11) samhemelryk: But can you think of a use case for storing false in an application cache, that would apply to everyone and wouldn't be better suited as a config variable? (I am just curious because I struggle to think of one)
      (10:41:37) samhemelryk: Hehe I wonder if it throws an exception if you try to set $a = false presently!
      (10:42:07) samhemelryk: Nope it would permit it
      (10:43:04) DavidMudrak: The relevance in my mind was that if isset($a) returns false, it makes no sense to call $cache->set('a', $a) - you can't call set() with a value that is not set
      (10:43:39) samhemelryk: Hmm that is a pretty good point
      (10:44:45) samhemelryk: I'll create an issue to convert the return from false to null and add a few watchers who may be interested
      (10:45:03) DavidMudrak: And regarding the use case - imagine cache of flags like $cache->set('update_available', false)
      (10:45:55) DavidMudrak: Using FALSE effectively disables caching of bool values, unless FALSE is the default (fallback)

      Lets gets some ideas and votes going here.
      If we are going to make this change it has to be done ASAP!

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              samhemelryk Sam Hemelryk
              Reporter:
              samhemelryk Sam Hemelryk
              Participants:
              Component watchers:
              Matteo Scaramuccia, Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Ilya Tregubov, Sara Arjona (@sarjona)
              Votes:
              3 Vote for this issue
              Watchers:
              9 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: