Moodle
  1. Moodle
  2. MDL-36768

Cache store plugins should extend an abstract class.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Caching
    • Labels:
    • Rank:
      46280

      Description

      It's come to realize that cache store plugins really ought to be extending an abstract base class as well as implementing the interface.
      The reason being that should we need to extend the API in the future we cannot do it via the interface. We'd need to optionally support things in order not to break cache store plugins every time we make a change to their API.
      As such I'm purposing to sew an abstract base class into core now, it'll be very basic but will allow us to expand in the future much more readily.

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Hi Eloy + Matteo,

          Eloy I've made you integrator of this issue, and I've put it up for integration now. This is the issue I talked to you about regarding introducing a new abstract cache store plugin base class.

          Matteo, I've added you here just to make you aware of this issue. It introduces an abstract class for store plugins to extend. The idea behind it is that after 2.4 has been released this will allow us to make any required API changes without breaking any custom store plugins people may have created. A requirement as adding to the cache_store interface will break things for sure.

          This patch introduces the base class and modifies the existing store plugins to make use of it.
          I've initially moved a handful of ultra-safe methods from the stores to the base class, Eloy what's been moved should make you smile.
          The base class isn't a requirement, but it is a recommendation. I'll need to update docs if this gets integrated to reflect that.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy + Matteo, Eloy I've made you integrator of this issue, and I've put it up for integration now. This is the issue I talked to you about regarding introducing a new abstract cache store plugin base class. Matteo, I've added you here just to make you aware of this issue. It introduces an abstract class for store plugins to extend. The idea behind it is that after 2.4 has been released this will allow us to make any required API changes without breaking any custom store plugins people may have created. A requirement as adding to the cache_store interface will break things for sure. This patch introduces the base class and modifies the existing store plugins to make use of it. I've initially moved a handful of ultra-safe methods from the stores to the base class, Eloy what's been moved should make you smile. The base class isn't a requirement, but it is a recommendation. I'll need to update docs if this gets integrated to reflect that. Many thanks Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I like it a lot, so I'd integrate it immediately.

          The only point preventing me to do so is that, if we introduce the base class, then the interface seems a bit redundant IMO (or abstract class or interface).

          Of course, I'm ok for "feaure" interfaces, not problem with those.

          Just I find the base class far better to "define" the cache store (can contain common methods, declare any visibility, have members...). And that renders the interface not really useful anymore, in my mind.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I like it a lot, so I'd integrate it immediately. The only point preventing me to do so is that, if we introduce the base class, then the interface seems a bit redundant IMO (or abstract class or interface). Of course, I'm ok for "feaure" interfaces, not problem with those. Just I find the base class far better to "define" the cache store (can contain common methods, declare any visibility, have members...). And that renders the interface not really useful anymore, in my mind. Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          Thanks for looking, indeed I had contemplated removing the cache_store interface and perhaps it is best to.
          I've produced a patch now that removes cache_store interface, renames the abstract class to cache_store, and reintroduces a new cache_store_interface for the public static methods that must be present on the cache store plugins that because of strict standards cannot be declared on the abstract class.
          All yours to consider once more.

          The old branch is still there btw, branch name is wip-MDL-36768-m24

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, Thanks for looking, indeed I had contemplated removing the cache_store interface and perhaps it is best to. I've produced a patch now that removes cache_store interface, renames the abstract class to cache_store, and reintroduces a new cache_store_interface for the public static methods that must be present on the cache store plugins that because of strict standards cannot be declared on the abstract class. All yours to consider once more. The old branch is still there btw, branch name is wip- MDL-36768 -m24 Many thanks Sam
          Hide
          Matteo Scaramuccia added a comment -

          @Sam: I'll thought at it (not rembering me as watcher of this issue too) while working on MDL-36363 where instance_deleted() could be a good candidate to be "implemented" at specific store class level, only when override is really required (there, just for cachestore_file).

          Once the decision will be taken i.e. integrated, I'll change the things in MDL-36363 according to that.

          Show
          Matteo Scaramuccia added a comment - @Sam: I'll thought at it (not rembering me as watcher of this issue too) while working on MDL-36363 where instance_deleted() could be a good candidate to be "implemented" at specific store class level, only when override is really required (there, just for cachestore_file ). Once the decision will be taken i.e. integrated, I'll change the things in MDL-36363 according to that.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ah, I thought your were going to do something like:

           public static are_requirements_met() {
                  throw new coding_exception('are_requirements_met() method needs to be overridden in each subclass of cache_store');
              }
          

          Instead of keeping the interface. But I'm not strong against your solution (in fact I think I made some "dual" use of both interfaces and "coding exception" methods in some parts of backup, so just for your consideration. I'm happy with any.

          Another little thing is that some time ago we decided to not allow phpdoc templates (http://docs.moodle.org/dev/Coding_style#Properties) and surely we should take them out. There are still some occurrences in codebase, but I think we should not be introducing more.

          Apart from that I think everything is looking ok. I've seen some changes about how the has() method behaves under ttl and key awareness, but I trust you about that.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ah, I thought your were going to do something like: public static are_requirements_met() { throw new coding_exception('are_requirements_met() method needs to be overridden in each subclass of cache_store'); } Instead of keeping the interface. But I'm not strong against your solution (in fact I think I made some "dual" use of both interfaces and "coding exception" methods in some parts of backup, so just for your consideration. I'm happy with any. Another little thing is that some time ago we decided to not allow phpdoc templates ( http://docs.moodle.org/dev/Coding_style#Properties ) and surely we should take them out. There are still some occurrences in codebase, but I think we should not be introducing more. Apart from that I think everything is looking ok. I've seen some changes about how the has() method behaves under ttl and key awareness, but I trust you about that. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks for taking rid of the phpdoc block templates. Integrated!

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks for taking rid of the phpdoc block templates. Integrated!
          Hide
          Sam Hemelryk added a comment -

          Thanks Eloy, much appreciated

          Show
          Sam Hemelryk added a comment - Thanks Eloy, much appreciated
          Hide
          Adrian Greeve added a comment -

          I ran unit tests,
          I logged in as an admin,
          and then I purged my caches.
          No errors found.
          Test passed.

          Show
          Adrian Greeve added a comment - I ran unit tests, I logged in as an admin, and then I purged my caches. No errors found. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Y E S !

          Closing as fixed, many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: