Page MenuHomePhabricator

Deprecate and remove $wgObjectCaches[hash]
Closed, DeclinedPublic

Description

See https://gerrit.wikimedia.org/r/294959.

@Krinkle wrote in June 2016:

Is this actually used? [..] Seems there is only 1 use, in AbuseFilter.

Instead of using HashBagOStuff as a central singleton, individual users of it should use their own instance and pass it down where needed. The class requires no parameters or injected fields, so usage should be pretty simple.

For the specific use case of process-caching things from memcaced, WANObjectCache introduced a built-in pcTTL option that should be used instead.

One the problems with the central singleton is that it can grow out of control, and there's no easy way to determine what its maxKeys should be given that usage is unknown.

Event Timeline

Krinkle renamed this task from Deprecate $wgObjectCaches[hash] to Deprecate and remove $wgObjectCaches[hash].May 29 2019, 3:25 PM

Change 530829 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] BadFileLookup to replace wfIsBadImage

https://gerrit.wikimedia.org/r/530829

Change 530829 merged by jenkins-bot:
[mediawiki/core@master] BadFileLookup to replace wfIsBadImage

https://gerrit.wikimedia.org/r/530829

Change #1015096 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/Gadgets@master] Replace 'hash' with CACHE_HASH to improve discovery

https://gerrit.wikimedia.org/r/1015096

As written in the task description, use of CACHE_HASH or 'hash' woud be a mistake in most cases. However, it is also true that "most cases" we use new HashBagOStuff already.

Remaining:
https://codesearch.wmcloud.org/search/?q=%28%28%5BoO%5Dbject%7C%5BcC%5Dache%29.*%5B%22%27%5Dhash%5B%27%22%5D%7CCACHE_HASH%29&files=php%24&excludeFiles=test&repos=

These 5 results all use it the same way:

ObjectCache::getLocalServerInstance( 'hash' )

This is a common idiom to obtain the ApcuBag singleton with fallback to a shared HashBag, which is a fine and easy to memorise pattern. Shared hashing in this case is not only fine, but in fact expected and part of how the ApcuBag would work as well. The main problem I was getting at in this task is when people use ObjectCache::getInstance( 'hash' ) to then store arbitrary data in, which at least today, does not appear to happen anywhere in our codebases.

What is left then is the CACHE_HASH constant and 'hash' singleton itself, which are a valuable part of the the above idiom. Breaking this would be needlessly disruptive for no gain.

In terms of developer UX, I'll focus on these two tasks instead:

Change #1015096 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@master] Replace 'hash' with CACHE_HASH to improve discovery

https://gerrit.wikimedia.org/r/1015096