Moodle
  1. Moodle
  2. MDL-37903

Interface cacheable_object is ignored if it is inside array

    Details

    • Rank:
      47657

      Description

      When caching an array of cacheable_objects, they are cached as if they are normal classes, without initialising cache_cached_object and calling prepare_to_cache() and wake_from_cache()

      It is possible of course to do a workaround by creating another cacheable_object that would contain just a list. But it would be so awesome if cache::set and cache::get were checking for chacheable_object's recursively

        Activity

        Hide
        Sam Hemelryk added a comment -

        Thanks for the idea Marina, definitely one worth doing!

        Show
        Sam Hemelryk added a comment - Thanks for the idea Marina, definitely one worth doing!
        Hide
        Sam Hemelryk added a comment -

        Ok I've a solution up for peer-review now.

        After looking and thinking about this for a while I decided that trying to ascertain handle cacheable_objects within an array is too much of a performance hit to be beneficial. The performance hit would be to any cache using arrays, regardless of the use of cacheable objects or not (as we would still need to inspect all elements before proceeding).

        Instead I think I've found a smart solution to allow the best of both worlds.
        I've introduced a new cacheable_object_array that implements the cacheable_object interface as well.
        Through this means a developer can explicitly say "I have an array of cacheable object" and the cache API will deal with it in a much more satisfactory way.

        As mentioned this solution + a unit test for it are up for peer-review now.
        It'd be interesting to hear your thoughts Marina.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Ok I've a solution up for peer-review now. After looking and thinking about this for a while I decided that trying to ascertain handle cacheable_objects within an array is too much of a performance hit to be beneficial. The performance hit would be to any cache using arrays, regardless of the use of cacheable objects or not (as we would still need to inspect all elements before proceeding). Instead I think I've found a smart solution to allow the best of both worlds. I've introduced a new cacheable_object_array that implements the cacheable_object interface as well. Through this means a developer can explicitly say "I have an array of cacheable object" and the cache API will deal with it in a much more satisfactory way. As mentioned this solution + a unit test for it are up for peer-review now. It'd be interesting to hear your thoughts Marina. Many thanks Sam
        Hide
        Marina Glancy added a comment -

        Hi Sam,
        thanks! this definitely suits my needs so I can't ask for more
        I looked at the code and noticed that you throw and exception if the element is not array in prepare_to_cache(). But will developer actually receive this error?

        Show
        Marina Glancy added a comment - Hi Sam, thanks! this definitely suits my needs so I can't ask for more I looked at the code and noticed that you throw and exception if the element is not array in prepare_to_cache(). But will developer actually receive this error?
        Hide
        Sam Hemelryk added a comment -

        Hi Marina,

        Thanks for looking at that. The exception is purely to aid developers in writing implementations.
        Providing they arn't executing code within a try catch they should get the exception no probs.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Hi Marina, Thanks for looking at that. The exception is purely to aid developers in writing implementations. Providing they arn't executing code within a try catch they should get the exception no probs. Many thanks Sam
        Hide
        Damyon Wiese added a comment -

        Thanks Sam,

        This change looks good - I've added a dev_docs_required label to this issue as it would be good to document the new class.

        I confirmed that the unit test is testing that the cachable_object callbacks are being executed for the items in the cacheable_array.

        Integrated to master.

        Show
        Damyon Wiese added a comment - Thanks Sam, This change looks good - I've added a dev_docs_required label to this issue as it would be good to document the new class. I confirmed that the unit test is testing that the cachable_object callbacks are being executed for the items in the cacheable_array. Integrated to master.
        Hide
        David Monllaó added a comment -

        Passing as running in the CI server

        Show
        David Monllaó added a comment - Passing as running in the CI server
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

        Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

        Thanks, closing as fixed!

        Show
        Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: