Removing a file store cache instance should remove the folder too, at least remove the files representing the cached items

I'm not sure about my patch proposal, that's the reason why the branch name starts w/ wip_.

I'm thinking about the reason to create an ad hoc definition which actually affects the purge operation by creating an extra subfolder under 'path' (ref.: cachestore_file::initialise()): my proposal doesn't get the expected result due to that extra subfolder.
Besides that subfolder is named adhoc_core_cache_purge and I'm not able right now (started today to look at the MUC code) to understand if purging a file store will ever work if the file store cache instance is initialised in a "normal way".

I'm pretty sure that https://github.com/scara/moodle/commit/5a26d449a73d1130a2dfa78471db2930478a6c24#L1R290 is not correct: it gets into the store logics without abstraction.

What about adding a new bool cache_store ::destroy() method which implements destruction on per-store basis? Besides a new property, hash, should be added in cachestore_file to cleanup the implementation of initialise, improving a possible implementation of destroy() charged to remove anything under path.

I'm guessing if this kind of re-working should wait for the implementation of MDL-36120.

See also MDL-36512: I'll work on this using that proposal as the starting point.
Whyle studying MUC code I've found that my proposal of destroy could be based on cleanup which stops at the definition level in removing folders - imposed with initialise - or, maybe, to remove the base path of the file store just after actually purging the store (MDL-36512).

I've modified my first proposal, https://github.com/scara/moodle/commit/5a26d449a73d1130a2dfa78471db2930478a6c24 in branch wip_m24MDL-36363Remove_path_when_removing_a_file_store_cache.
I keep on wondering if it could be considered AS-IS without abstracting the explicit destruction required for just a file store.

Hi Matteo,

Thanks for the creating reporting the issue and providing a patch.
Certainly 100% this is a good improvement.
Having looked at the patch I think a better approach would be to ask the store to clean itself up before being deleted rather than addressing the file store personally.
The reason for this being that other stores, memcache, apc, database whatever will also want a chance to clean up.
What do you think of the idea of implementing two new methods against the cache_store interface:

• instance_created : called when a store instance is first created.
• instance_deleted : called when the store instance is deleted.

If you like the idea and would be happy to implement it I will leave it up for you, otherwise I can jump on this one myself.

Many thanks
Sam

Hi Sam,
it sounds good - kind of BC, is it possible when being in beta? For sure there are good reasons -: if you have enough time (not busy for other urgent tasks for 2.4beta) and you think it is urgent, please jump on it.
I'm happy in contributing but I must be honest: I'm a bit slow not only for lacking of spare time and not being a subject matter expert but for flu. Being Friday, time for integration has gone and probably my slowness shouldn't be an issue if a proposal will land the next week.

Fever has gone , I've updated the branch with your proposal BUT I'm not confident at all about MY first proposal with a full deletion so I'll improve it with another commit to restrict the deletion to what the file store is supposed to create, regardless the specific definition.
Then obviously up to you to define what's better, even to keep your idea about the two new method but w/o doing anything in file store too i.e. a third commit, the safer moving this issue as just an improvement of the API for future use.

Pushed:
a. Commit 1+2: my proposal
b. Commit 1+2+3: the safest option, with two small improvement: $filestorepath and purge() implementation in cachestore_file, to play its role when path has been defined.

Squashing will be required for both options.

Fixed a missing TNX to PHPunit (phpunit core_cache cache/tests/cache_test.php) telling about the missing instance_*() implementations in dummystore.php, cachestore_dummy.

Hi Sam,
back again, while looking at MDL-36322 I found an issue with file store:

...
            if ($path !== false && !is_writable($path)) {
                $path = false;
...
        $this->isready = $path !== false;
...
    public function is_ready() {
        return ($this->path !== null);
    }
...

which means that is_ready will not tells about any failure during the creation of the instance when attempting to create the path if required. I noticed it just now, when looking for a reason for MDL-36322 and looking at my proposal here where I've used the property and not the function.
Since this is really a separate issue do I need to open a new one or it could be added here or (IMHO better) in MDL-36322?

EDIT@20121128T0732CEST: the is_ready() issue has been fixed in MDL-36825.

Hi Matteo,

Looks like my time today has already been filled sorry but I'll hopefully look to this tomorrow and will have some feedback/patch to share.

Many thanks
Sam

Rebased against 2.4rc1 and cleaned up using a different set of commits no more linked to the strategy used to present the proposal in the "old" branch, m24_MDL-36363_Remove_path_when_removing_a_file_store_cache: now the commits history should be preserved.

Hi Matteo, sorry it took me so long to get to these issues.
I'm onto them today.

Thanks Matteo.

I've looked this over and having thought about it for quite some time I've made a couple of changes.
Rather than have both cleanup and instance_deleted methods I opted for just instance_deleted. Reading back over comments and documentation I am sure that cleanup was intended to clean up deleted stores anyway.
I also fixed up the memcache and mongodb instance_deleted methods so that they would work if the cache had not been initialised.
I've made my changes as an additional change on top of your and am putting this up for integration review now.

Many thanks
Sam

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

Only one tiny detail... if we mark the function as deprecated for 2.5 ... shouldn't it be calling the newer one instead of debugging and doing nothing? I mean, that's not deprecation, it's kill.

Hi Eloy,

It is sort of kill, essentially it won't cause any harm it just won't purge the cache.
The only people affected would be those who would have instantiated a cache store instance themselves and have written code to call cleanup manually. Hopefully (and I imagine to be the case) no one has done that.
Instead I opted to make instance_deleted call cleanup if it exists. This I feel is a much better option, I'm sure others have created cache store plugins and have written cleanup methods. This way existing cache stores won't be break, however they will see a debugging notice.
It also means however that I can't make cleanup call instance_deleted... not easily anyway.
I'd rather this stays as is but am happy to look at other solutions if you desire.

Many thanks
Sam

Alright, integrated, thanks!

while testing:

Step 4: First time I test this issue, I get: Fatal error: Allowed memory size of 2097152000 bytes exhausted (tried to allocate 76 bytes) in /integration/24/cache/classes/helper.php on line 476 for for both Master and 2.4
Then I re-run the test again by removing all folders within the cache folder in moodledata, I didn't get the above error, however there's no theme applied for the page.

Then when I continue on to step 5 and noticed that I didn't get any other file/folder except 'core string' folder.

Could you confirm that the above error and behaviour are the expected result, before I continue on to the next step?

Thanks
Rosie

Thanks for reporting that Rosie.

I tracked it down this morning to the cache key being parsed multiple times if there are stacked loaders (you've mapped two stores to a single definition).
I've created a fix for this that is now ready for integration:

Branches:

• wip-MDL-36363-m24-integration
• wip-MDL-36363-m25-integration

Diffs

• https://github.com/samhemelryk/moodle/commit/wip-MDL-36363-m24-integration
• https://github.com/samhemelryk/moodle/commit/wip-MDL-36363-m25-integration

All your's once more Eloy.

Thanks
Sam

Pushed the extra 2 commits. Now this can be tested again and verify the memory problem doesn't happen anymore and everything works as expected.

Thanks!

Re-tested this issue with the updated patch.

It works as expected for 2.4 and master

Test passed.

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

I've removed the dev_docs_required label. The API still needs to be documented as part of MDL-25290.