From dc0fb7d2b4712a0d86d3904297369ad6ba35766c Mon Sep 17 00:00:00 2001
From: "Edward Z. Yang" <edwardzyang@thewritingpot.com>
Date: Sat, 23 Jun 2007 14:05:09 +0000
Subject: [PATCH] [2.0.1] DefinitionCache related bug-fixes - Fixed bug where
 manually modified definitions were not saved via cache (mostly harmless,
 except for the fact that it would be a little slower) - Configuration objects
 with different serials do not clobber each others when revision numbers are
 unequal . DefinitionCache keys reordered to reflect precedence: version
 number, hash, then revision number

git-svn-id: http://htmlpurifier.org/svnroot/htmlpurifier/trunk@1204 48356398-32a2-884e-a903-53898d9a118a
---
 NEWS                                          |  6 ++++
 library/HTMLPurifier/Config.php               |  7 +++-
 library/HTMLPurifier/DefinitionCache.php      | 17 ++++++----
 .../DefinitionCache/Serializer.php            | 11 ++++++-
 tests/HTMLPurifier/ConfigTest.php             |  3 +-
 .../DefinitionCache/SerializerTest.php        | 32 +++++++++++++++++--
 tests/HTMLPurifier/DefinitionCacheTest.php    | 22 ++++++++-----
 7 files changed, 78 insertions(+), 20 deletions(-)

diff --git a/NEWS b/NEWS
index f97e4389..199b8cc8 100644
--- a/NEWS
+++ b/NEWS
@@ -13,8 +13,14 @@ NEWS ( CHANGELOG and HISTORY )                                     HTMLPurifier
 - Clean up special case code for <script> tags
 - Reorder includes for DefinitionCache decorators, fixes a possible
   missing class error
+- Fixed bug where manually modified definitions were not saved via cache
+  (mostly harmless, except for the fact that it would be a little slower)
+- Configuration objects with different serials do not clobber each
+  others when revision numbers are unequal
 . Rewire some test-cases to swallow errors rather than expect them
 . HTMLDefinition printer updated with some of the new attributes
+. DefinitionCache keys reordered to reflect precedence: version number,
+  hash, then revision number
 
 2.0.0, released 2007-06-20
 # Completely refactored HTMLModuleManager, decentralizing safety
diff --git a/library/HTMLPurifier/Config.php b/library/HTMLPurifier/Config.php
index 15d47044..14da7dcd 100644
--- a/library/HTMLPurifier/Config.php
+++ b/library/HTMLPurifier/Config.php
@@ -155,11 +155,15 @@ class HTMLPurifier_Config
     /**
      * Returns a md5 signature of a segment of the configuration object
      * that uniquely identifies that particular configuration
+     * @note Revision is handled specially and is removed from the batch
+     *       before processing!
      * @param $namespace Namespace to get serial for
      */
     function getBatchSerial($namespace) {
         if (empty($this->serials[$namespace])) {
-            $this->serials[$namespace] = md5(serialize($this->getBatch($namespace)));
+            $batch = $this->getBatch($namespace);
+            unset($batch['DefinitionRev']);
+            $this->serials[$namespace] = md5(serialize($batch));
         }
         return $this->serials[$namespace];
     }
@@ -270,6 +274,7 @@ class HTMLPurifier_Config
             if (!empty($this->definitions[$type])) {
                 if (!$this->definitions[$type]->setup) {
                     $this->definitions[$type]->setup($this);
+                    $cache->set($this->definitions[$type], $this);
                 }
                 return $this->definitions[$type];
             }
diff --git a/library/HTMLPurifier/DefinitionCache.php b/library/HTMLPurifier/DefinitionCache.php
index b83115e4..f21b0354 100644
--- a/library/HTMLPurifier/DefinitionCache.php
+++ b/library/HTMLPurifier/DefinitionCache.php
@@ -38,8 +38,8 @@ class HTMLPurifier_DefinitionCache
      */
     function generateKey($config) {
         return $config->version . '-' . // possibly replace with function calls
-               $config->get($this->type, 'DefinitionRev') . '-' .
-               $config->getBatchSerial($this->type);
+               $config->getBatchSerial($this->type) . '-' .
+               $config->get($this->type, 'DefinitionRev');
     }
     
     /**
@@ -49,11 +49,16 @@ class HTMLPurifier_DefinitionCache
      * @param $config Instance of HTMLPurifier_Config to test against
      */
     function isOld($key, $config) {
-        list($version, $revision, $hash) = explode('-', $key, 3);
+        list($version, $hash, $revision) = explode('-', $key, 3);
         $compare = version_compare($version, $config->version);
-        if ($compare > 0) return false;
-        if ($compare == 0 && $revision >= $config->get($this->type, 'DefinitionRev')) return false;
-        return true;
+        // version mismatch, is always old
+        if ($compare != 0) return true;
+        // versions match, ids match, check revision number
+        if (
+            $hash == $config->getBatchSerial($this->type) &&
+            $revision < $config->get($this->type, 'DefinitionRev')
+        ) return true;
+        return false;
     }
     
     /**
diff --git a/library/HTMLPurifier/DefinitionCache/Serializer.php b/library/HTMLPurifier/DefinitionCache/Serializer.php
index 64b79eba..8272a5af 100644
--- a/library/HTMLPurifier/DefinitionCache/Serializer.php
+++ b/library/HTMLPurifier/DefinitionCache/Serializer.php
@@ -88,9 +88,18 @@ class HTMLPurifier_DefinitionCache_Serializer extends
      * @note No trailing slash
      */
     function generateDirectoryPath($config) {
+        $base = $this->generateBaseDirectoryPath($config);
+        return $base . '/' . $this->type;
+    }
+    
+    /**
+     * Generates path to base directory that contains all definition type
+     * serials
+     */
+    function generateBaseDirectoryPath($config) {
         $base = $config->get('Cache', 'SerializerPath');
         $base = is_null($base) ? dirname(__FILE__) . '/Serializer' : $base;
-        return $base . '/' . $this->type;
+        return $base;
     }
     
     /**
diff --git a/tests/HTMLPurifier/ConfigTest.php b/tests/HTMLPurifier/ConfigTest.php
index 5387a3c4..e79a81a0 100644
--- a/tests/HTMLPurifier/ConfigTest.php
+++ b/tests/HTMLPurifier/ConfigTest.php
@@ -252,9 +252,10 @@ class HTMLPurifier_ConfigTest extends UnitTestCase
         
         // test retrieval of raw definition
         $config->set('HTML', 'DefinitionID', 'HTMLPurifier_ConfigTest->test_getHTMLDefinition()');
+        $config->set('HTML', 'DefinitionRev', 3);
         $def =& $config->getHTMLDefinition(true);
         $this->assertNotEqual($def, $def2);
-        $this->assertFalse($def->setup);
+        $this->assertEqual(false, $def->setup);
         
         // auto initialization
         $config->getHTMLDefinition();
diff --git a/tests/HTMLPurifier/DefinitionCache/SerializerTest.php b/tests/HTMLPurifier/DefinitionCache/SerializerTest.php
index 99867b03..00766df3 100644
--- a/tests/HTMLPurifier/DefinitionCache/SerializerTest.php
+++ b/tests/HTMLPurifier/DefinitionCache/SerializerTest.php
@@ -14,7 +14,7 @@ class HTMLPurifier_DefinitionCache_SerializerTest extends HTMLPurifier_Definitio
         $config->setReturnValue('get', 2, array('Test', 'DefinitionRev'));
         $config->version = '1.0.0';
         
-        $config_md5   = '1.0.0-2-serial';
+        $config_md5   = '1.0.0-serial-2';
         
         $file = realpath(
             $rel_file = dirname(__FILE__) .
@@ -138,6 +138,32 @@ class HTMLPurifier_DefinitionCache_SerializerTest extends HTMLPurifier_Definitio
         
     }
     
+    function testCleanupOnlySameID() {
+        
+        $cache = new HTMLPurifier_DefinitionCache_Serializer('Test');
+        
+        $config1 = $this->generateConfigMock('serial1');
+        $config1->version = '1.0.0';
+        $config1->setReturnValue('get', 1, array('Test', 'DefinitionRev'));
+        $def1 = $this->generateDefinition(array('info' => 1));
+        
+        $config2 = $this->generateConfigMock('serial2');
+        $config2->version = '1.0.0';
+        $config2->setReturnValue('get', 34, array('Test', 'DefinitionRev'));
+        $def2 = $this->generateDefinition(array('info' => 3));
+        
+        $cache->set($def1, $config1);
+        $cache->cleanup($config1);
+        $this->assertEqual($def1, $cache->get($config1)); // no change
+        
+        $cache->set($def2, $config2);
+        $cache->cleanup($config2);
+        $this->assertEqual($def1, $cache->get($config1));
+        $this->assertEqual($def2, $cache->get($config2));
+        
+        $cache->flush($config1);
+    }
+    
     /**
      * Asserts that a file exists, ignoring the stat cache
      */
@@ -164,9 +190,9 @@ class HTMLPurifier_DefinitionCache_SerializerTest extends HTMLPurifier_Definitio
         
         $def_original = $this->generateDefinition();
         $cache->add($def_original, $config);
-        $this->assertFileExist($dir . '/Test/1.0.0-1-serial.ser');
+        $this->assertFileExist($dir . '/Test/1.0.0-serial-1.ser');
         
-        unlink($dir . '/Test/1.0.0-1-serial.ser');
+        unlink($dir . '/Test/1.0.0-serial-1.ser');
         rmdir( $dir . '/Test');
         
     }
diff --git a/tests/HTMLPurifier/DefinitionCacheTest.php b/tests/HTMLPurifier/DefinitionCacheTest.php
index 702712ff..196943a3 100644
--- a/tests/HTMLPurifier/DefinitionCacheTest.php
+++ b/tests/HTMLPurifier/DefinitionCacheTest.php
@@ -15,16 +15,22 @@ class HTMLPurifier_DefinitionCacheTest extends UnitTestCase
         HTMLPurifier_ConfigSchema::defineNamespace('Test', 'Test namespace');
         HTMLPurifier_ConfigSchema::define('Test', 'DefinitionRev', 1, 'int', 'Definition revision.');
         
-        $config = HTMLPurifier_Config::createDefault();
-        $config->version = '1.0.0';
-        $config->set('Test', 'DefinitionRev', 10);
+        generate_mock_once('HTMLPurifier_Config');
+        $config = new HTMLPurifier_ConfigMock();
+        $config->version = '1.0.0'; // hopefully no conflicts
+        $config->setReturnValue('get', 10, array('Test', 'DefinitionRev'));
+        $config->setReturnValue('getBatchSerial', 'hash', array('Test'));
         
-        $this->assertIdentical($cache->isOld('1.0.0-10-hashstuffhere', $config), false);
-        $this->assertIdentical($cache->isOld('1.5.0-1-hashstuffhere', $config), false);
+        $this->assertIdentical($cache->isOld('1.0.0-hash-10', $config), false);
+        $this->assertIdentical($cache->isOld('1.5.0-hash-1', $config), true);
         
-        $this->assertIdentical($cache->isOld('0.9.0-1-hashstuffhere', $config), true);
-        $this->assertIdentical($cache->isOld('1.0.0-1-hashstuffhere', $config), true);
-        $this->assertIdentical($cache->isOld('1.0.0beta-11-hashstuffhere', $config), true);
+        $this->assertIdentical($cache->isOld('0.9.0-hash-1', $config), true);
+        $this->assertIdentical($cache->isOld('1.0.0-hash-1', $config), true);
+        $this->assertIdentical($cache->isOld('1.0.0beta-hash-11', $config), true);
+        
+        $this->assertIdentical($cache->isOld('0.9.0-hash2-1', $config), true);
+        $this->assertIdentical($cache->isOld('1.0.0-hash2-1', $config), false); // if hash is different, don't touch!
+        $this->assertIdentical($cache->isOld('1.0.0beta-hash2-11', $config), true);
         
         HTMLPurifier_ConfigSchema::instance($old_copy);