wikimedia/mediawiki-extensions-TitleBlacklist
1
0
Fork 0
mirror of https://gerrit.wikimedia.org/r/mediawiki/extensions/TitleBlacklist synced 2024-11-23 21:53:40 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix flipped version number comparison for the whitelist

Browse source 
I don't know much about this codebase, but I'm pretty sure this is
a mistake. The point of such a version number is usually that we can
invalidate old data from the cache in case we make changes to the
format. Let's say the data in the cache is stored with version 3. Now
we increase it in the code to 4. The next time the cache is read the
numbers don't match and the cache is repopulated with new data,
including the new version number.

I think what the != comparison did was the opposite: The code is
constantly throwing away perfectly fine data and re-creating it over
and over again, except when the version number is increased. Only
then old (but now outdated, possibly even incompatible) data is used.
I don't know if it's ever invalidated in this case. Do objects in the
WANObjectCache get refreshed even if they are constantly in use?

Maybe we are lucky and this never had any consequences, except for
performance.

This bug exists ever since this code was added in 2007, see esp.
https://phabricator.wikimedia.org/rETBLb7bc8af6
That patch even mentions this exact issue when it says "make cache
version check really work", but fixes it only in one of the two
places.

Change-Id: I48ab5d5abd6018a846349c5d4c4ed041dd925389
This commit is contained in:
thiemowmde 2024-08-16 15:36:25 +02:00
parent 52d708f9b3
commit 01985564fa
1 changed files with 9 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
includes/TitleBlacklist.php
View file

@ -33,7 +33,7 @@ class TitleBlacklist {
/** @var TitleBlacklist|null */
protected static $instance = null;
/** Blacklist format */
/** Increase this to invalidate the cached copies of both blacklist and whitelist */
public const VERSION = 4;
/**
@ -73,9 +73,11 @@ class TitleBlacklist {
$cache = MediaWikiServices::getInstance()->getMainWANObjectCache();
// Try to find something in the cache
/** @var TitleBlacklistEntry[]|false $cachedBlacklist */
$cachedBlacklist = $cache->get( $cache->makeKey( 'title_blacklist_entries' ) );
if ( is_array( $cachedBlacklist ) && count( $cachedBlacklist ) > 0
&& ( $cachedBlacklist[0]->getFormatVersion() == self::VERSION )
if ( $cachedBlacklist &&
is_array( $cachedBlacklist ) &&
$cachedBlacklist[0]->getFormatVersion() == self::VERSION
) {
$this->mBlacklist = $cachedBlacklist;
return;
@ -103,9 +105,11 @@ class TitleBlacklist {
global $wgTitleBlacklistCaching;
$cache = MediaWikiServices::getInstance()->getMainWANObjectCache();
/** @var TitleBlacklistEntry[]|false $cachedWhitelist */
$cachedWhitelist = $cache->get( $cache->makeKey( 'title_whitelist_entries' ) );
if ( is_array( $cachedWhitelist ) && count( $cachedWhitelist ) > 0
&& ( $cachedWhitelist[0]->getFormatVersion() != self::VERSION )
if ( $cachedWhitelist &&
is_array( $cachedWhitelist ) &&
$cachedWhitelist[0]->getFormatVersion() == self::VERSION
) {
$this->mWhitelist = $cachedWhitelist;
return;