review-1.4.txt

Review Alexandru 2008-05-16
===========================

[X] the tutorial needs to be updated with the new stackable storage.

[X] run docanalisys.php on Cache:
    - missing method docblocks.
    - missing or wrong parameters.
    - @deprecated tag could be instead @apichange? It is not recognized.

[X] xdebug_start_trace() and xdebug_stop_trace() in complex_stack_test.php on
    line 307 and 309.

[X] investigate which version of APC is needed? Some tests fail with APC 3.0.14
    (I got "missing apc_add() function" but maybe it was not compiled correctly).
	I also needed apc.enable_cli=1 in php.ini which I didn't know - specify this
    somewhere?

[X] 2 failed tests in ezcCacheStorageMemcachePlainTest:

1) testPurgeNoLimit(ezcCacheStorageMemcachePlainTest)
Purged IDs not returned correctly.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ -1,8 +1,3 @@
 Array
 (
-    [0] => ID
-    [1] => Some/Dir/ID
-    [2] => Some/other/Dir/ID/1
-    [3] => Some/other/Dir/ID/2
-    [4] => Some/other/Dir/ID/3
 )

/home/as/dev/ezcomponents/trunk/Cache/tests/storage_memcache_plain_test.php:530

2) testPurgeLimit(ezcCacheStorageMemcachePlainTest)
Purged IDs not returned correctly.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ -1,6 +1,3 @@
 Array
 (
-    [0] => ID
-    [1] => Some/Dir/ID
-    [2] => Some/other/Dir/ID/1
 )

/home/as/dev/ezcomponents/trunk/Cache/tests/storage_memcache_plain_test.php:567


	# I receive these errors, too, but just occasionally. I currently have no
	  clue where they result from. I'll investigate more, later. However, I
	  think it's a Memcache issue.

	  I opened issue #13053 for this, since I need more time to dig into it.

[X] ezcCacheStack: why does pushStorage() use ezcCacheStackStorageConfiguration?
    It could have passed the options right there in the pushStorage() instead of
    creating yet another object. Like:

    $stack->pushStorage( 'cache', $cache, 1, 0.5 );

    instead of:

    $conf = new ezcCacheStackStorageConfiguration( 'cache', $cache, 1, 0.5 );
    $stack->pushStorage( $conf );

	# The object is needed internally to be passed around. Creating the object
	  outside allows to do popStorage() and to push such removed storages back
	  to the stack. Beside that, it can be more convenient when using delayed
	  initialization through the ezcCacheManager.

[X] ezcCacheStack: the cache storages are stored twice: once in $storageStack
    and once in $storageIdMap. Wouldn't it be better to store it only in
    $storageStack and to store only the IDs in $storageIdMap?

	# No. Objects are stored by reference, so it is most likely that this
	  consumes less memory than storing the actual ID twice, which would by
	  copied.

[X] Are all options from ezcCacheStackOptions tested in different combinations?
    LRU, LRU+bubbleUp, LRU+bubbleUp+metaStorage, LRU+metaStorage
    LFU, LFU+bubbleUp, LFU+bubbleUp+metaStorage, LFU+metaStorage

    And in different combinations with ezcCacheStackStorageConfiguration
    parameters, like $itemLimit = 1 and $freeRate = 0.1, $freeRate = 0.99 etc?

	# Since not all of them depend on each other, this seems not necessary to
	  me. Tests are performed for:
      - LRU+bubbleUp
      - LRU+metaStorageConfigured
      - LRU+metaStorageDefault
	  All with concrete method call tests via mock objects. LRU is only a
	  wrapper around internel base strategy.  In addition, all methods of LRU,
	  LFU and the meta data stuff are tested dedicatedly.

    I got a warning:
    Warning: Invalid argument supplied for foreach()
    in /home/as/dev/ezcomponents/trunk/Cache/src/storage/memory.php on line 340

    when I call $cache->store() with this configuration (the cache should? be
    empty but maybe there are items from previous runs):

ezcCacheManager::createCache( 'apc', null, 'ezcCacheStorageApcPlain' );
$apc = ezcCacheManager::getCache( 'apc' );

$options = array( 'host' => 'localhost', 'port' => 11211, 'ttl' => 30 );
ezcCacheManager::createCache( 'mem', null, 'ezcCacheStorageMemcachePlain', $options );
$mem = ezcCacheManager::getCache( 'mem' );

$options = new ezcCacheStackOptions();
$options->replacementStrategy = 'ezcCacheStackLfuReplacementStrategy';
$options->bubbleUpOnRestore = true;
$options->metaStorage = $mem;
ezcCacheManager::createCache( 'cache', null, 'ezcCacheStack', $options );
$cache = ezcCacheManager::getCache( 'cache' );
$conf = new ezcCacheStackStorageConfiguration( 'mem', $mem, 1, 0.9 );
$cache->pushStorage( $conf );
$conf = new ezcCacheStackStorageConfiguration( 'apc', $apc, 1, 0.9 );
$cache->pushStorage( $conf );
$cache = ezcCacheManager::getCache( 'cache' );

[X] Some tests files contain multiple classes (eg complex_stack_test.php). They
    should be separated in different files because it might impede running the
    test case on its own.