Moodle
  1. Moodle
  2. MDL-37474

Allow definitions to be purged from the cache management screen.

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.5
    • Component/s: Caching
    • Labels:
    • Testing Instructions:
      Hide
      1. Log in as an admin
      2. Turn on perfdebug if you havn't got it on already.
      3. Browse to Admin > Settings > Plugins > Caching > Configuration.
      4. Purge a the string definition, check when the page has finished redirecting that you get a high set count for the file store under string cache in the performance debug output.
      5. Purge the file store and check the set count again is high.
      Show
      Log in as an admin Turn on perfdebug if you havn't got it on already. Browse to Admin > Settings > Plugins > Caching > Configuration. Purge a the string definition, check when the page has finished redirecting that you get a high set count for the file store under string cache in the performance debug output. Purge the file store and check the set count again is high.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-37474-m25
    • Rank:
      47112

      Description

      A smart request has come in.
      On the cache management screen you are able to purge a particular store. It would be great if you were also able to purge a particular definition!

      See https://moodle.org/mod/forum/discuss.php?d=219455

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          OK, putting a solution for this up for peer-review now

          Show
          Sam Hemelryk added a comment - OK, putting a solution for this up for peer-review now
          Hide
          Tim Hunt added a comment -

          You are right, it was easy to implement

          The code looks fine, except that all the uses of PARAM_TEXT in cache/admin.php are wrong. You should clean properly.

          Show
          Tim Hunt added a comment - You are right, it was easy to implement The code looks fine, except that all the uses of PARAM_TEXT in cache/admin.php are wrong. You should clean properly.
          Hide
          Sam Hemelryk added a comment -

          Thanks Tim, I've cleaned up the lazy use of PARAM_TEXT and have pushed this for integration review now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Tim, I've cleaned up the lazy use of PARAM_TEXT and have pushed this for integration review now. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (master only), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
          Hide
          Tim Hunt added a comment -

          I suppose you are not back-porting this because it is a "New features", even though it is very low risk, very useful, and the only people to see UI changes are admins. Oh well.

          If this is not going into 2.4, shouldn't you create a separate security issue for fixing the sloppy PARAM_ types in the 2.4 code?

          Show
          Tim Hunt added a comment - I suppose you are not back-porting this because it is a "New features", even though it is very low risk, very useful, and the only people to see UI changes are admins. Oh well. If this is not going into 2.4, shouldn't you create a separate security issue for fixing the sloppy PARAM_ types in the 2.4 code?
          Hide
          Sam Hemelryk added a comment -

          Hi Tim,

          Indeed, this has hasn't been backported to 2.4 as it is a new feature.

          Regarding the PARAM_ it would be worth backporting those changes.
          I don't think these are a security issue however are there?
          In order to access the page the user must have moodle/site:config and I imagine should a user manage to elevate themselves to a position where they have that capability that they would be able to cause much greater damage. Truthfully I'm not sure what our policy on that sort of thing is. Either way I'll create a bug to backport that change and we'll go from there.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Tim, Indeed, this has hasn't been backported to 2.4 as it is a new feature. Regarding the PARAM_ it would be worth backporting those changes. I don't think these are a security issue however are there? In order to access the page the user must have moodle/site:config and I imagine should a user manage to elevate themselves to a position where they have that capability that they would be able to cause much greater damage. Truthfully I'm not sure what our policy on that sort of thing is. Either way I'll create a bug to backport that change and we'll go from there. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Linking to MDL-37747 to see the PARAM_TEXT change backported.

          Show
          Sam Hemelryk added a comment - Linking to MDL-37747 to see the PARAM_TEXT change backported.
          Hide
          Damyon Wiese added a comment -

          MDL-37747 causes a coding error: e.g. "The requested cache definition does not exist.corestring/" and this patch probably will too...

          Show
          Damyon Wiese added a comment - MDL-37747 causes a coding error: e.g. "The requested cache definition does not exist.corestring/" and this patch probably will too...
          Hide
          Tim Hunt added a comment -

          Of course, since the back-end code already had a cache_helper::purge_by_definition($component, $area); function, it would be easy to consider it a bug that this feature was not exposed in the admin UI. That is certainly where this bug started. To help someone in a forum post, I wanted to say "Go an purge the question definition cache" and then I found that functionality was not exposed, and raised it in Dev chat.

          Oh well, we all have more important things to do than argue over whether something is a bug or a new feature.

          Show
          Tim Hunt added a comment - Of course, since the back-end code already had a cache_helper::purge_by_definition($component, $area); function, it would be easy to consider it a bug that this feature was not exposed in the admin UI. That is certainly where this bug started. To help someone in a forum post, I wanted to say "Go an purge the question definition cache" and then I found that functionality was not exposed, and raised it in Dev chat. Oh well, we all have more important things to do than argue over whether something is a bug or a new feature.
          Hide
          Damyon Wiese added a comment -

          How did the numbers on these 2 issues get so confusing ? MDL-37747 and MDL-37474

          Show
          Damyon Wiese added a comment - How did the numbers on these 2 issues get so confusing ? MDL-37747 and MDL-37474
          Hide
          Sam Hemelryk added a comment -

          Thanks for picking that up Damyon, I've created a small fix and its been integrated now. This is all good for testing once more.

          Show
          Sam Hemelryk added a comment - Thanks for picking that up Damyon, I've created a small fix and its been integrated now. This is all good for testing once more.
          Hide
          Andrew Davis added a comment -

          Works as described. Passing.

          Show
          Andrew Davis added a comment - Works as described. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: