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

Allow definitions to be purged from the cache management screen.

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: 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

      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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            samhemelryk Sam Hemelryk added a comment -

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

            Show
            samhemelryk Sam Hemelryk added a comment - OK, putting a solution for this up for peer-review now
            Hide
            timhunt 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
            timhunt 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
            samhemelryk 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
            samhemelryk 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
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (master only), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
            Hide
            timhunt 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
            timhunt 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk Sam Hemelryk added a comment -

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

            Show
            samhemelryk Sam Hemelryk added a comment - Linking to MDL-37747 to see the PARAM_TEXT change backported.
            Hide
            damyon 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 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
            timhunt 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
            timhunt 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 Damyon Wiese added a comment -

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

            Show
            damyon Damyon Wiese added a comment - How did the numbers on these 2 issues get so confusing ? MDL-37747 and MDL-37474
            Hide
            samhemelryk 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
            samhemelryk 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
            andyjdavis Andrew Davis added a comment -

            Works as described. Passing.

            Show
            andyjdavis Andrew Davis added a comment - Works as described. Passing.
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  14/May/13