From 904fd2d5dc907d658b6a160f80b68ec28b729bad Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Wed, 27 Feb 2013 13:33:36 +1300 Subject: [PATCH 01/10] API Make Object::config use late static binding Can now be used in instance scope, like: $this->config()->db and in static scope, like: Page::config()->db --- core/Object.php | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/core/Object.php b/core/Object.php index bfe7daca5..f313eb9c1 100755 --- a/core/Object.php +++ b/core/Object.php @@ -52,23 +52,12 @@ abstract class Object { */ public $class; - - /** - * @todo Set this via dependancy injection? Can't call it $config, because too many clashes with form elements etc - * @var Config_ForClass - */ - private $_config_forclass = null; - /** * Get a configuration accessor for this class. Short hand for Config::inst()->get($this->class, .....). * @return Config_ForClass|null */ - public function config() { - if (!$this->_config_forclass) { - $this->_config_forclass = Config::inst()->forClass($this->class); - } - - return $this->_config_forclass; + static public function config() { + return Config::inst()->forClass(get_called_class()); } /** From c98621977cc93af1433ea796a8f8c3a13fdd1b2d Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Tue, 26 Feb 2013 16:39:40 +1300 Subject: [PATCH 02/10] Cache the merged version of any Config value in an in-mem LRU cache --- core/Config.php | 175 +++++++++++++++++++++++++++++++------- core/Object.php | 13 +-- tests/core/ConfigTest.php | 52 ++++++++++- 3 files changed, 198 insertions(+), 42 deletions(-) diff --git a/core/Config.php b/core/Config.php index c6b08daac..5a945fb5a 100644 --- a/core/Config.php +++ b/core/Config.php @@ -163,11 +163,14 @@ class Config { $_SINGLETONS['Config'] = $instance; } + protected $cache; + /** * Empty construction, otherwise calling singleton('Config') (not the right way to get the current active config * instance, but people might) gives an error */ public function __construct() { + $this->cache = new Config_LRU(); } /** @var [array] - Array of arrays. Each member is an nested array keyed as $class => $name => $value, @@ -189,6 +192,7 @@ class Config { */ public function pushConfigManifest(SS_ConfigManifest $manifest) { array_unshift($this->manifests, $manifest->yamlConfig); + $this->cache->clean(); // @todo: Do anything with these. They're for caching after config.php has executed $this->collectConfigPHPSettings = true; @@ -342,34 +346,17 @@ class Config { return $res; } - /** - * Get the config value associated for a given class and property - * - * This merges all current sources and overrides together to give final value - * todo: Currently this is done every time. This function is an inner loop function, so we really need to be - * caching heavily here. - * - * @param $class string - The name of the class to get the value for - * @param $name string - The property to get the value for - * @param int $sourceOptions Bitmask which can be set to some combintain of Config::UNINHERITED, - * Config::FIRST_SET, and Config::EXCLUDE_EXTENSIONS. - * - * Config::UNINHERITED does not include parent classes when merging configuration fragments - * Config::FIRST_SET stops inheriting once the first class that sets a value (even an empty value) is encoutered - * Config::EXCLUDE_EXTRA_SOURCES does not include any additional static sources (such as extensions) - * - * Config::INHERITED is a utility constant that can be used to mean "none of the above", equvilient to 0 - * Setting both Config::UNINHERITED and Config::FIRST_SET behaves the same as just Config::UNINHERITED - * - * should the parent classes value be merged in as the lowest priority source? - * @param $result array|scalar Reference to a variable to put the result in. Also returned, so this can be left - * as null safely. If you do pass a value, it will be treated as the highest priority - * value in the result chain - * @param $suppress array Internal use when called by child classes. Array of mask pairs to filter value by - * @return array|scalar The value of the config item, or null if no value set. Could be an associative array, - * sequential array or scalar depending on value (see class docblock) - */ - public function get($class, $name, $sourceOptions = 0, &$result = null, $suppress = null) { + protected $extraConfigSources = array(); + + public function extraConfigSourcesChanged($class) { + unset($this->extraConfigSources[$class]); + $this->cache->clean("__{$class}"); + } + + protected function getUncached($class, $name, $sourceOptions, &$result, $suppress, &$tags) { + $tags[] = "__{$class}"; + $tags[] = "__{$class}__{$name}"; + // If result is already not something to merge into, just return it if ($result !== null && !is_array($result)) return $result; @@ -401,9 +388,16 @@ class Config { $nothing = new stdClass(); $sources = array($class); + // Include extensions only if not flagged not to, and some have been set if (($sourceOptions & self::EXCLUDE_EXTRA_SOURCES) != self::EXCLUDE_EXTRA_SOURCES) { - $extraSources = Object::get_extra_config_sources($class); + // If we don't have a fresh list of extra sources, get it from the class itself + if (!array_key_exists($class, $this->extraConfigSources)) { + $this->extraConfigSources[$class] = Object::get_extra_config_sources($class); + } + + // Update $sources with any extra sources + $extraSources = $this->extraConfigSources[$class]; if ($extraSources) $sources = array_merge($sources, $extraSources); } @@ -421,11 +415,48 @@ class Config { if (($sourceOptions & self::UNINHERITED) != self::UNINHERITED && (($sourceOptions & self::FIRST_SET) != self::FIRST_SET || $result === null)) { $parent = get_parent_class($class); - if ($parent) $this->get($parent, $name, $sourceOptions, $result, $suppress); + if ($parent) $this->getUncached($parent, $name, $sourceOptions, $result, $suppress, $tags); } - if ($name == 'routes') { - print_r($result); die; + return $result; + } + + /** + * Get the config value associated for a given class and property + * + * This merges all current sources and overrides together to give final value + * todo: Currently this is done every time. This function is an inner loop function, so we really need to be + * caching heavily here. + * + * @param $class string - The name of the class to get the value for + * @param $name string - The property to get the value for + * @param int $sourceOptions Bitmask which can be set to some combintain of Config::UNINHERITED, + * Config::FIRST_SET, and Config::EXCLUDE_EXTENSIONS. + * + * Config::UNINHERITED does not include parent classes when merging configuration fragments + * Config::FIRST_SET stops inheriting once the first class that sets a value (even an empty value) is encoutered + * Config::EXCLUDE_EXTRA_SOURCES does not include any additional static sources (such as extensions) + * + * Config::INHERITED is a utility constant that can be used to mean "none of the above", equvilient to 0 + * Setting both Config::UNINHERITED and Config::FIRST_SET behaves the same as just Config::UNINHERITED + * + * should the parent classes value be merged in as the lowest priority source? + * @param $result array|scalar Reference to a variable to put the result in. Also returned, so this can be left + * as null safely. If you do pass a value, it will be treated as the highest priority + * value in the result chain + * @param $suppress array Internal use when called by child classes. Array of mask pairs to filter value by + * @return array|scalar The value of the config item, or null if no value set. Could be an associative array, + * sequential array or scalar depending on value (see class docblock) + */ + public function get($class, $name, $sourceOptions = 0, &$result = null, $suppress = null) { + // Have we got a cached value? Use it if so + $key = sha1($class.$name.$sourceOptions); + + if (($result = $this->cache->get($key)) === false) { + $tags = array(); + $result = null; + $this->getUncached($class, $name, $sourceOptions, $result, $suppress, $tags); + $this->cache->set($key, $result, $tags); } return $result; @@ -452,6 +483,8 @@ class Config { if (!isset($this->overrides[0][$class][$name])) $this->overrides[0][$class][$name] = $val; else self::merge_high_into_low($this->overrides[0][$class][$name], $val); + + $this->cache->clean("__{$class}__{$name}"); } /** @@ -512,6 +545,84 @@ class Config { } +class Config_LRU { + const SIZE = 1000; + + protected $cache; + protected $indexing; + + protected $i = 0; + protected $c = 0; + + public function __construct() { + $this->cache = new SplFixedArray(self::SIZE); + $this->indexing = array(); + } + + public function set($key, $val, $tags = array()) { + // Find an index to set at + $replacing = null; + + // Target count - not always the lowest, but guaranteed to exist (or hit an empty item) + $target = $this->c - self::SIZE + 1; + $i = $stop = $this->i; + + do { + if (!($i--)) $i = self::SIZE-1; + $item = $this->cache[$i]; + + if (!$item) { $replacing = null; break; } + else if ($item->c <= $target) { $replacing = $item; break; } + } + while ($i != $stop); + + if ($replacing) unset($this->indexing[$replacing->key]); + + $this->indexing[$key] = $this->i = $i; + $this->cache[$i] = $obj = new stdClass(); + + $obj->key = $key; + $obj->value = $val; + $obj->tags = $tags; + $obj->c = ++$this->c; + } + + private $hit = 0; + private $miss = 0; + + public function stats() { + return $this->miss ? ($this->hit / $this->miss) : 0; + } + + public function get($key) { + if (isset($this->indexing[$key])) { + $this->hit++; + + $res = $this->cache[$this->indexing[$key]]; + $res->c = ++$this->c; + return $res->value; + } + + $this->miss++; + return false; + } + + public function clean($tag = null) { + if ($tag) { + foreach ($this->cache as $i => $v) { + if ($v && in_array($tag, $v->tags)) { + unset($this->indexing[$v->key]); + $this->cache[$i] = null; + } + } + } + else { + $this->cache = new SplFixedArray(self::SIZE); + $this->indexing = array(); + } + } +} + class Config_ForClass { protected $class; diff --git a/core/Object.php b/core/Object.php index f313eb9c1..a97152288 100755 --- a/core/Object.php +++ b/core/Object.php @@ -483,10 +483,11 @@ abstract class Object { if($subclasses) foreach($subclasses as $subclass) { unset(self::$classes_constructed[$subclass]); unset(self::$extra_methods[$subclass]); - unset(self::$extension_sources[$subclass]); } Config::inst()->update($class, 'extensions', array($extension)); + Config::inst()->extraConfigSourcesChanged($class); + Injector::inst()->unregisterAllObjects(); // load statics now for DataObject classes @@ -523,6 +524,7 @@ abstract class Object { } Config::inst()->remove($class, 'extensions', Config::anything(), $extension); + Config::inst()->extraConfigSourcesChanged($class); // unset singletons to avoid side-effects Injector::inst()->unregisterAllObjects(); @@ -533,7 +535,6 @@ abstract class Object { if($subclasses) foreach($subclasses as $subclass) { unset(self::$classes_constructed[$subclass]); unset(self::$extra_methods[$subclass]); - unset(self::$extension_sources[$subclass]); } } @@ -560,9 +561,6 @@ abstract class Object { // -------------------------------------------------------------------------------------------------------------- - private static $extension_sources = array(); - - // Don't bother checking some classes that should never be extended private static $unextendable_classes = array('Object', 'ViewableData', 'RequestHandler'); static public function get_extra_config_sources($class = null) { @@ -571,9 +569,6 @@ abstract class Object { // If this class is unextendable, NOP if(in_array($class, self::$unextendable_classes)) return; - // If we have a pre-cached version, use that - if(array_key_exists($class, self::$extension_sources)) return self::$extension_sources[$class]; - // Variable to hold sources in $sources = null; @@ -604,7 +599,7 @@ abstract class Object { } } - return self::$extension_sources[$class] = $sources; + return $sources; } public function __construct() { diff --git a/tests/core/ConfigTest.php b/tests/core/ConfigTest.php index 2598bd404..7ed19ed9e 100644 --- a/tests/core/ConfigTest.php +++ b/tests/core/ConfigTest.php @@ -163,5 +163,55 @@ class ConfigTest extends SapphireTest { public function testFragmentOrder() { $this->markTestIncomplete(); } - + + public function testLRUDiscarding() { + $cache = new ConfigTest_Config_LRU(); + + for ($i = 0; $i < Config_LRU::SIZE*2; $i++) $cache->set($i, $i); + $this->assertEquals( + Config_LRU::SIZE, count($cache->indexing), + 'Homogenous usage gives exact discarding' + ); + + $cache = new ConfigTest_Config_LRU(); + + for ($i = 0; $i < Config_LRU::SIZE; $i++) $cache->set($i, $i); + for ($i = 0; $i < Config_LRU::SIZE; $i++) $cache->set(-1, -1); + $this->assertLessThan( + Config_LRU::SIZE, count($cache->indexing), + 'Heterogenous usage gives sufficient discarding' + ); + } + + public function testLRUCleaning() { + $cache = new ConfigTest_Config_LRU(); + + for ($i = 0; $i < Config_LRU::SIZE; $i++) $cache->set($i, $i); + $this->assertEquals(Config_LRU::SIZE, count($cache->indexing)); + + $cache->clean(); + $this->assertEquals(0, count($cache->indexing), 'Clean clears all items'); + $this->assertFalse($cache->get(1), 'Clean clears all items'); + + $cache->set(1, 1, array('Foo')); + $this->assertEquals(1, count($cache->indexing)); + + $cache->clean('Foo'); + $this->assertEquals(0, count($cache->indexing), 'Clean items with matching tag'); + $this->assertFalse($cache->get(1), 'Clean items with matching tag'); + + $cache->set(1, 1, array('Foo', 'Bar')); + $this->assertEquals(1, count($cache->indexing)); + + $cache->clean('Bar'); + $this->assertEquals(0, count($cache->indexing), 'Clean items with any single matching tag'); + $this->assertFalse($cache->get(1), 'Clean items with any single matching tag'); + } +} + +class ConfigTest_Config_LRU extends Config_LRU { + + public $cache; + public $indexing; + } From 6b986cb17d101eecec6e79ba10d2b27389d8b857 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Tue, 26 Feb 2013 16:44:48 +1300 Subject: [PATCH 03/10] Extract statics via code analysis rather than introspection --- core/Config.php | 31 ++- core/Core.php | 6 +- core/manifest/ConfigStaticManifest.php | 309 +++++++++++++++++++++++++ dev/TestRunner.php | 4 + 4 files changed, 341 insertions(+), 9 deletions(-) create mode 100644 core/manifest/ConfigStaticManifest.php diff --git a/core/Config.php b/core/Config.php index 5a945fb5a..95a771550 100644 --- a/core/Config.php +++ b/core/Config.php @@ -181,6 +181,13 @@ class Config { * where value is a config value suppress from any lower priority item */ protected $suppresses = array(); + protected $staticManifests = array(); + + public function pushConfigStaticManifest(SS_ConfigStaticManifest $manifest) { + array_unshift($this->staticManifests, $manifest); + $this->cache->clean(); + } + /** @var [array] - The list of settings pulled from config files to search through */ protected $manifests = array(); @@ -190,7 +197,7 @@ class Config { * WARNING: Config manifests to not merge entries, and do not solve before/after rules inter-manifest - * instead, the last manifest to be added always wins */ - public function pushConfigManifest(SS_ConfigManifest $manifest) { + public function pushConfigYamlManifest(SS_ConfigManifest $manifest) { array_unshift($this->manifests, $manifest->yamlConfig); $this->cache->clean(); @@ -384,9 +391,6 @@ class Config { } } - // Then look at the static variables - $nothing = new stdClass(); - $sources = array($class); // Include extensions only if not flagged not to, and some have been set @@ -401,9 +405,18 @@ class Config { if ($extraSources) $sources = array_merge($sources, $extraSources); } + $value = $nothing = null; + foreach ($sources as $staticSource) { - if (is_array($staticSource)) $value = isset($staticSource[$name]) ? $staticSource[$name] : $nothing; - else $value = Object::static_lookup($staticSource, $name, $nothing); + if (is_array($staticSource)) { + $value = isset($staticSource[$name]) ? $staticSource[$name] : $nothing; + } + else { + foreach ($this->staticManifests as $i => $statics) { + $value = $statics->get($staticSource, $name, $nothing); + if ($value !== $nothing) break; + } + } if ($value !== $nothing) { self::merge_low_into_high($result, $value, $suppress); @@ -412,8 +425,10 @@ class Config { } // Finally, merge in the values from the parent class - if (($sourceOptions & self::UNINHERITED) != self::UNINHERITED - && (($sourceOptions & self::FIRST_SET) != self::FIRST_SET || $result === null)) { + if ( + ($sourceOptions & self::UNINHERITED) != self::UNINHERITED && + (($sourceOptions & self::FIRST_SET) != self::FIRST_SET || $result === null) + ) { $parent = get_parent_class($class); if ($parent) $this->getUncached($parent, $name, $sourceOptions, $result, $suppress, $tags); } diff --git a/core/Core.php b/core/Core.php index 0ff5d2425..541f56122 100644 --- a/core/Core.php +++ b/core/Core.php @@ -288,9 +288,13 @@ if(file_exists(BASE_PATH . '/vendor/autoload.php')) { require_once BASE_PATH . '/vendor/autoload.php'; } +// Now that the class manifest is up, load the configuration +$configManifest = new SS_ConfigStaticManifest(BASE_PATH, false, $flush); +Config::inst()->pushConfigStaticManifest($configManifest); + // Now that the class manifest is up, load the configuration $configManifest = new SS_ConfigManifest(BASE_PATH, false, $flush); -Config::inst()->pushConfigManifest($configManifest); +Config::inst()->pushConfigYamlManifest($configManifest); SS_TemplateLoader::instance()->pushManifest(new SS_TemplateManifest( BASE_PATH, project(), false, isset($_GET['flush']) diff --git a/core/manifest/ConfigStaticManifest.php b/core/manifest/ConfigStaticManifest.php new file mode 100644 index 000000000..0e42ef8a0 --- /dev/null +++ b/core/manifest/ConfigStaticManifest.php @@ -0,0 +1,309 @@ +base = $base; + $this->tests = $includeTests; + + $this->cache = SS_Cache::factory('SS_ConfigStatics', 'Core', array( + 'automatic_serialization' => true, + 'lifetime' => null + )); + + $this->key = 'sc_'.sha1($base . ($includeTests ? '!tests' : '')); + + if(!$forceRegen) { + $this->index = $this->cache->load($this->key); + } + + if($this->index) { + $this->statics = $this->index['$statics']; + } + else { + $this->regenerate($cache); + } + } + + public function get($class, $name, $default) { + if (!isset($this->statics[$class])) { + if (isset($this->index[$class])) { + $info = $this->index[$class]; + $this->statics[$class] = $this->cache->load($this->key.'_'.sha1($class)); + + if (!isset($this->statics[$class])) { + $this->handleFile(null, $info['path'], null); + } + } + else { + $this->statics[$class] = false; + } + } + + $statics = $this->statics; + + if (isset($statics[$class]) && $statics[$class] && array_key_exists($name, $statics[$class])) { + if ($statics[$class][$name]['access'] != T_PRIVATE) { + Deprecation::notice('3.1.0', "Config static $class::\$$name must be marked as private", Deprecation::SCOPE_GLOBAL); + // Don't warn more than once per static + $this->statics[$class][$name]['access'] = T_PRIVATE; + } + + return $statics[$class][$name]['value']; + } + + return $default; + } + + /** + * Completely regenerates the manifest file. + */ + public function regenerate($cache = true) { + $this->statics = array(); + + $finder = new ManifestFileFinder(); + $finder->setOptions(array( + 'name_regex' => '/^([^_].*\.php)$/', + 'ignore_files' => array('index.php', 'main.php', 'cli-script.php'), + 'ignore_tests' => !$this->tests, + 'file_callback' => array($this, 'handleFile') + )); + + $finder->find($this->base); + + $index = array('$statics' => array()); + + foreach ($this->statics as $class => $details) { + $this->cache->save($details, $this->key.'_'.sha1($class)); + + $index[$class] = array( + 'path' => $details['path'], + 'mtime' => filemtime($details['path']) + ); + + if (in_array($class, self::$initial_classes)) { + $index['$statics'][$class] = $details; + } + } + + if($cache) { + $this->cache->save($index, $this->key); + } + } + + public function handleFile($basename, $pathname, $depth) { + $parser = new SS_ConfigStaticManifest_Parser($pathname); + $this->statics = array_merge($this->statics, $parser->parse()); + } + + public function getStatics() { + return $this->statics; + } +} + +/** + * A parser that processes a PHP file, using PHP's built in parser to get a string of tokens, + * then processing them to find the static class variables, their access levels & values + * + * We can't do this using TokenisedRegularExpression because we need to keep track of state + * as we process the token list (when we enter and leave a namespace or class, when we see + * an access level keyword, etc) + */ +class SS_ConfigStaticManifest_Parser { + + protected $statics = array(); + + protected $path; + protected $tokens; + protected $length; + protected $pos; + + function __construct($path) { + $this->path = $path; + $file = file_get_contents($path); + + $this->tokens = token_get_all($file); + $this->length = count($this->tokens); + $this->pos = 0; + } + + /** + * Get the next token to process, incrementing the pointer + * + * @param bool $ignoreWhitespace - if true will skip any whitespace tokens & only return non-whitespace ones + * @return null | int - Either the next token or null if there isn't one + */ + protected function next($ignoreWhitespace = true) { + do { + if($this->pos >= $this->length) return null; + $next = $this->tokens[$this->pos++]; + } + while($ignoreWhitespace && is_array($next) && $next[0] == T_WHITESPACE); + + return $next; + } + + /** + * Parse the given file to find the static variables declared in it, along with their access & values + */ + function parse() { + $depth = 0; $namespace = null; $class = null; $clsdepth = null; $access = 0; + + while($token = $this->next()) { + $type = is_array($token) ? $token[0] : $token; + + if($type == T_CLASS) { + $next = $this->next(); + + if($next[0] != T_STRING) { + user_error("Couldn\'t parse {$this->path} when building config static manifest", E_USER_ERROR); + } + + $class = $next[1]; + } + else if($type == T_NAMESPACE) { + $next = $this->next(); + + if($next[0] != T_STRING) { + user_error("Couldn\'t parse {$this->path} when building config static manifest", E_USER_ERROR); + } + + $namespace = $next[1]; + } + else if($type == '{' || $type == T_CURLY_OPEN || $type == T_DOLLAR_OPEN_CURLY_BRACES){ + $depth += 1; + if($class && !$clsdepth) $clsdepth = $depth; + } + else if($type == '}') { + $depth -= 1; + if($depth < $clsdepth) $class = $clsdepth = null; + if($depth < 0) user_error("Hmm - depth calc wrong, hit negatives", E_USER_ERROR); + } + else if($type == T_PUBLIC || $type == T_PRIVATE || $type == T_PROTECTED) { + $access = $type; + } + else if($type == T_STATIC) { + if($class && $depth == $clsdepth) $this->parseStatic($access, $namespace ? $namespace.'\\'.$class : $class); + } + else { + $access = ''; + } + } + + return $this->statics; + } + + /** + * During parsing we've found a "static" keyword. Parse out the variable names and value + * assignments that follow. + * + * Seperated out from parse partially so that we can recurse if there are multiple statics + * being declared in a comma seperated list + */ + function parseStatic($access, $class) { + $variable = null; + $value = ''; + + while($token = $this->next()) { + $type = is_array($token) ? $token[0] : $token; + + if($type == T_PUBLIC || $type == T_PRIVATE || $type == T_PROTECTED) { + $access = $type; + } + else if($type == T_FUNCTION) { + return; + } + else if($type == T_VARIABLE) { + $variable = substr($token[1], 1); // Cut off initial "$" + } + else if($type == ';' || $type == ',' || $type == '=') { + break; + } + else { + echo "What's this?\n"; + print_r($token); + return; + } + } + + if($token == '=') { + $depth = 0; + + while($token = $this->next(false)){ + $type = is_array($token) ? $token[0] : $token; + + // Track array nesting depth + if($type == T_ARRAY) { + $depth += 1; + } + else if($type == ')') { + $depth -= 1; + } + + // Parse out the assignment side of a static declaration, ending on either a ';' or a ',' outside an array + if($type == T_WHITESPACE) { + $value .= ' '; + } + else if($type == ';' || ($type == ',' && !$depth)) { + break; + } + // Statics can reference class constants with self:: (and that won't work in eval) + else if($type == T_STRING && $token[1] == 'self') { + $value .= $class; + } + else { + $value .= is_array($token) ? $token[1] : $token; + } + } + } + + if(!isset($this->statics[$class])) { + $this->statics[$class] = array( + 'path' => $this->path, + 'statics' => array() + ); + } + + $this->statics[$class][$variable] = array( + 'access' => $access, + 'value' => eval('return '.$value.';') + ); + + if($token == ',') $this->parseStatic($access, $class); + } +} \ No newline at end of file diff --git a/dev/TestRunner.php b/dev/TestRunner.php index cf9395d5e..eed1fc347 100644 --- a/dev/TestRunner.php +++ b/dev/TestRunner.php @@ -88,6 +88,10 @@ class TestRunner extends Controller { SS_TemplateLoader::instance()->pushManifest(new SS_TemplateManifest( BASE_PATH, project(), true, isset($_GET['flush']) )); + + Config::inst()->pushConfigStaticManifest(new SS_ConfigStaticManifest( + BASE_PATH, true, isset($_GET['flush']) + )); } public function init() { From 024a0b90a9af988422f103eca53b94e587aa1052 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Wed, 27 Feb 2013 13:27:40 +1300 Subject: [PATCH 04/10] Add ability to create temporary Config copies --- core/Config.php | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/core/Config.php b/core/Config.php index 95a771550..6cc5aed21 100644 --- a/core/Config.php +++ b/core/Config.php @@ -163,16 +163,44 @@ class Config { $_SINGLETONS['Config'] = $instance; } + /** + * Make the newly active Config be a copy of the current active Config instance. + * + * You can then make changes to the configuration by calling update and remove on the new + * value returned by Config::inst(), and then discard those changes later by calling unnest + */ + static public function nest() { + $current = self::$instance; + + $new = clone $current; + $new->nestedFrom = $current; + self::set_instance($new); + } + + /** + * Change the active Config back to the Config instance the current active Config object + * was copied from + */ + static public function unnest() { + self::set_instance(self::$instance->nestedFrom); + } + protected $cache; /** - * Empty construction, otherwise calling singleton('Config') (not the right way to get the current active config - * instance, but people might) gives an error + * Each copy of the Config object need's it's own cache, so changes don't leak through to other instances */ public function __construct() { $this->cache = new Config_LRU(); } + public function __clone() { + $this->cache = clone $this->cache; + } + + /** @var Config - The config instance this one was copied from when Config::nest() was called */ + protected $nestedFrom = null; + /** @var [array] - Array of arrays. Each member is an nested array keyed as $class => $name => $value, * where value is a config value to treat as the highest priority item */ protected $overrides = array(); From 80bd38e1e9a7fb033531e0ffed2f060dc67630f5 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Wed, 27 Feb 2013 13:34:50 +1300 Subject: [PATCH 05/10] FIX DataObjectSchemaGenerationTest trying to modify config statics directly --- .../model/DataObjectSchemaGenerationTest.php | 33 ++++++++++++------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/tests/model/DataObjectSchemaGenerationTest.php b/tests/model/DataObjectSchemaGenerationTest.php index 8bc2cb749..1385ad3d9 100644 --- a/tests/model/DataObjectSchemaGenerationTest.php +++ b/tests/model/DataObjectSchemaGenerationTest.php @@ -43,9 +43,11 @@ class DataObjectSchemaGenerationTest extends SapphireTest { // Table will have been initially created by the $extraDataObjects setting // Let's insert a new field here - $oldDB = DataObjectSchemaGenerationTest_DO::$db; - DataObjectSchemaGenerationTest_DO::$db['SecretField'] = 'Varchar(100)'; - + Config::nest(); + Config::inst()->update('DataObjectSchemaGenerationTest_DO', 'db', array( + 'SecretField' => 'Varchar(100)' + )); + // Verify that the above extra field triggered a schema update $db->beginSchemaUpdate(); $obj = new DataObjectSchemaGenerationTest_DO(); @@ -55,7 +57,7 @@ class DataObjectSchemaGenerationTest extends SapphireTest { $this->assertTrue($needsUpdating); // Restore db configuration - DataObjectSchemaGenerationTest_DO::$db = $oldDB; + Config::unnest(); } /** @@ -76,9 +78,12 @@ class DataObjectSchemaGenerationTest extends SapphireTest { $this->assertFalse($needsUpdating); // Test with alternate index format, although these indexes are the same - $oldIndexes = DataObjectSchemaGenerationTest_IndexDO::$indexes; - DataObjectSchemaGenerationTest_IndexDO::$indexes = DataObjectSchemaGenerationTest_IndexDO::$indexes_alt; - + Config::nest(); + Config::inst()->remove('DataObjectSchemaGenerationTest_IndexDO', 'indexes'); + Config::inst()->update('DataObjectSchemaGenerationTest_IndexDO', 'indexes', + Config::inst()->get('DataObjectSchemaGenerationTest_IndexDO', 'indexes_alt') + ); + // Verify that it still doesn't need to be recreated $db->beginSchemaUpdate(); $obj2 = new DataObjectSchemaGenerationTest_IndexDO(); @@ -88,7 +93,7 @@ class DataObjectSchemaGenerationTest extends SapphireTest { $this->assertFalse($needsUpdating); // Restore old index format - DataObjectSchemaGenerationTest_IndexDO::$indexes = $oldIndexes; + Config::unnest(); } /** @@ -101,9 +106,13 @@ class DataObjectSchemaGenerationTest extends SapphireTest { // Table will have been initially created by the $extraDataObjects setting // Update the SearchFields index here - $oldIndexes = DataObjectSchemaGenerationTest_IndexDO::$indexes; - DataObjectSchemaGenerationTest_IndexDO::$indexes['SearchFields']['value'] = '"Title"'; - + Config::nest(); + Config::inst()->update('DataObjectSchemaGenerationTest_IndexDO', 'indexes', array( + 'SearchFields' => array( + 'value' => 'Title' + ) + )); + // Verify that the above index change triggered a schema update $db->beginSchemaUpdate(); $obj = new DataObjectSchemaGenerationTest_IndexDO(); @@ -113,7 +122,7 @@ class DataObjectSchemaGenerationTest extends SapphireTest { $this->assertTrue($needsUpdating); // Restore old indexes - DataObjectSchemaGenerationTest_IndexDO::$indexes = $oldIndexes; + Config::unnest(); } } From a6f1a200b60ecce46e1eaa2ba08bda1aacf181b2 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Mon, 4 Mar 2013 09:25:23 +1300 Subject: [PATCH 06/10] Some micro-optimisations for Config --- core/Config.php | 19 +++++--- core/manifest/ConfigStaticManifest.php | 64 ++++++++++++++++---------- 2 files changed, 53 insertions(+), 30 deletions(-) diff --git a/core/Config.php b/core/Config.php index 6cc5aed21..fbc6ddfe6 100644 --- a/core/Config.php +++ b/core/Config.php @@ -493,7 +493,7 @@ class Config { */ public function get($class, $name, $sourceOptions = 0, &$result = null, $suppress = null) { // Have we got a cached value? Use it if so - $key = sha1($class.$name.$sourceOptions); + $key = $class.$name.$sourceOptions; if (($result = $this->cache->get($key)) === false) { $tags = array(); @@ -599,6 +599,13 @@ class Config_LRU { public function __construct() { $this->cache = new SplFixedArray(self::SIZE); + + // Pre-fill with stdClass instances. By reusing we avoid object-thrashing + for ($i = 0; $i < self::SIZE; $i++) { + $this->cache[$i] = new stdClass(); + $this->cache[$i]->key = null; + } + $this->indexing = array(); } @@ -614,7 +621,7 @@ class Config_LRU { if (!($i--)) $i = self::SIZE-1; $item = $this->cache[$i]; - if (!$item) { $replacing = null; break; } + if ($item->key === null) { $replacing = null; break; } else if ($item->c <= $target) { $replacing = $item; break; } } while ($i != $stop); @@ -622,8 +629,8 @@ class Config_LRU { if ($replacing) unset($this->indexing[$replacing->key]); $this->indexing[$key] = $this->i = $i; - $this->cache[$i] = $obj = new stdClass(); + $obj = $this->cache[$i]; $obj->key = $key; $obj->value = $val; $obj->tags = $tags; @@ -653,14 +660,14 @@ class Config_LRU { public function clean($tag = null) { if ($tag) { foreach ($this->cache as $i => $v) { - if ($v && in_array($tag, $v->tags)) { + if ($v->key !== null && in_array($tag, $v->tags)) { unset($this->indexing[$v->key]); - $this->cache[$i] = null; + $this->cache[$i]->key = null; } } } else { - $this->cache = new SplFixedArray(self::SIZE); + for ($i = 0; $i < self::SIZE; $i++) $this->cache[$i]->key = null; $this->indexing = array(); } } diff --git a/core/manifest/ConfigStaticManifest.php b/core/manifest/ConfigStaticManifest.php index 0e42ef8a0..ebd9996ed 100644 --- a/core/manifest/ConfigStaticManifest.php +++ b/core/manifest/ConfigStaticManifest.php @@ -23,9 +23,7 @@ class SS_ConfigStaticManifest { protected $statics; static protected $initial_classes = array( - 'Object', 'ViewableData', 'Injector', 'Director', - 'RequestHandler', 'Controller', 'ContentController', - 'DataObject', 'SiteTree', 'Extension', 'DataExtension', 'Hierarchy', 'Versioned' + 'Object', 'ViewableData', 'Injector', 'Director' ); /** @@ -64,7 +62,10 @@ class SS_ConfigStaticManifest { if (!isset($this->statics[$class])) { if (isset($this->index[$class])) { $info = $this->index[$class]; - $this->statics[$class] = $this->cache->load($this->key.'_'.sha1($class)); + + if ($details = $this->cache->load($this->key.'_'.$info['base'])) { + $this->statics += $details; + } if (!isset($this->statics[$class])) { $this->handleFile(null, $info['path'], null); @@ -75,16 +76,16 @@ class SS_ConfigStaticManifest { } } - $statics = $this->statics; + if (isset($this->statics[$class][$name])) { + $static = $this->statics[$class][$name]; - if (isset($statics[$class]) && $statics[$class] && array_key_exists($name, $statics[$class])) { - if ($statics[$class][$name]['access'] != T_PRIVATE) { + if ($static['access'] != T_PRIVATE) { Deprecation::notice('3.1.0', "Config static $class::\$$name must be marked as private", Deprecation::SCOPE_GLOBAL); // Don't warn more than once per static - $this->statics[$class][$name]['access'] = T_PRIVATE; + $static['access'] = T_PRIVATE; } - return $statics[$class][$name]['value']; + return $static['value']; } return $default; @@ -106,22 +107,37 @@ class SS_ConfigStaticManifest { $finder->find($this->base); - $index = array('$statics' => array()); - - foreach ($this->statics as $class => $details) { - $this->cache->save($details, $this->key.'_'.sha1($class)); - - $index[$class] = array( - 'path' => $details['path'], - 'mtime' => filemtime($details['path']) - ); - - if (in_array($class, self::$initial_classes)) { - $index['$statics'][$class] = $details; - } - } - if($cache) { + $index = array('$statics' => array()); + $bases = array(); + + foreach ($this->statics as $class => $details) { + if (in_array($class, self::$initial_classes)) { + $index['$statics'][$class] = $details; + } + else { + $base = $class; + + do { + $parent = get_parent_class($base); + } + while ($parent != 'Object' && $parent != 'ViewableData' && $parent && ($base = $parent)); + + $base = sha1($base); + $bases[$base][$class] = $details; + + $index[$class] = array( + 'base' => $base, + 'path' => $details['path'], + 'mtime' => filemtime($details['path']), + ); + } + } + + foreach ($bases as $base => $details) { + $this->cache->save($details, $this->key.'_'.$base); + } + $this->cache->save($index, $this->key); } } From c52baae3c88269b599571e88c911ad569f4bc4c1 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Tue, 12 Mar 2013 15:32:46 +1300 Subject: [PATCH 07/10] Add some tests for the static parser --- .../manifest/ConfigStaticManifestTest.php | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 tests/core/manifest/ConfigStaticManifestTest.php diff --git a/tests/core/manifest/ConfigStaticManifestTest.php b/tests/core/manifest/ConfigStaticManifestTest.php new file mode 100644 index 000000000..fa8c49210 --- /dev/null +++ b/tests/core/manifest/ConfigStaticManifestTest.php @@ -0,0 +1,123 @@ +parse(); + } + + return $statics; + } + + public function testParsingAccessLevels() { + $statics = $this->parseSelf(); + + $levels = array( + 'nolevel' => null, + 'public' => T_PUBLIC, + 'public2' => T_PUBLIC, + 'protected' => T_PROTECTED, + 'protected2' => T_PROTECTED, + 'private' => T_PRIVATE, + 'private2' => T_PRIVATE + ); + + foreach($levels as $var => $level) { + $this->assertEquals( + $level, + $statics[__CLASS__][$var]['access'], + 'Variable '.$var.' has '.($level ? token_name($level) : 'no').' access level' + ); + } + } + + public function testParsingValues() { + $statics = $this->parseSelf(); + + // Check assigning values + $values = array( + 'none', + 'null', + 'int', + 'float', + 'string', + 'array', + ); + + $prepends = array( + 's', // Each on it's own + 'o', // All on one line + 'm' // All in on static statement, but each on own line + ); + + foreach ($values as $value) { + foreach ($prepends as $prepend) { + $var = "$prepend$value"; + + $this->assertEquals( + self::$$var, + $statics[__CLASS__][$var]['value'], + 'Variable '.$var.' value is extracted properly' + ); + } + } + } + + public function testIgnoresMethodStatics() { + $statics = $this->parseSelf(); + $this->assertNull(@$statics[__CLASS__]['method_static']); + } + + public function testIgnoresStaticMethods() { + $statics = $this->parseSelf(); + $this->assertNull(@$statics[__CLASS__]['static_method']); + } +} \ No newline at end of file From 943b5cf3a4cea789e15ed92abe2457f13fbf61e8 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Tue, 12 Mar 2013 15:40:12 +1300 Subject: [PATCH 08/10] Remove debug message, any still unexpected token is an error --- core/manifest/ConfigStaticManifest.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/core/manifest/ConfigStaticManifest.php b/core/manifest/ConfigStaticManifest.php index ebd9996ed..f53f9f828 100644 --- a/core/manifest/ConfigStaticManifest.php +++ b/core/manifest/ConfigStaticManifest.php @@ -271,9 +271,7 @@ class SS_ConfigStaticManifest_Parser { break; } else { - echo "What's this?\n"; - print_r($token); - return; + user_error('Unexpected token when building static manifest: '.print_r($token, true), E_USER_ERROR); } } From 7f58730904bf6b712ac5d5f8d018ef7865d0daae Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Tue, 12 Mar 2013 16:52:11 +1300 Subject: [PATCH 09/10] FIX Avoid get_parent_class in ConfigStaticManifest (was loading all classes) --- core/manifest/ConfigStaticManifest.php | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/core/manifest/ConfigStaticManifest.php b/core/manifest/ConfigStaticManifest.php index f53f9f828..ee73da5e0 100644 --- a/core/manifest/ConfigStaticManifest.php +++ b/core/manifest/ConfigStaticManifest.php @@ -63,7 +63,7 @@ class SS_ConfigStaticManifest { if (isset($this->index[$class])) { $info = $this->index[$class]; - if ($details = $this->cache->load($this->key.'_'.$info['base'])) { + if ($details = $this->cache->load($this->key.'_'.$info['key'])) { $this->statics += $details; } @@ -109,33 +109,26 @@ class SS_ConfigStaticManifest { if($cache) { $index = array('$statics' => array()); - $bases = array(); + $keysets = array(); foreach ($this->statics as $class => $details) { if (in_array($class, self::$initial_classes)) { $index['$statics'][$class] = $details; } else { - $base = $class; - - do { - $parent = get_parent_class($base); - } - while ($parent != 'Object' && $parent != 'ViewableData' && $parent && ($base = $parent)); - - $base = sha1($base); - $bases[$base][$class] = $details; + $key = sha1($class); + $keysets[$key][$class] = $details; $index[$class] = array( - 'base' => $base, + 'key' => $key, 'path' => $details['path'], 'mtime' => filemtime($details['path']), ); } } - foreach ($bases as $base => $details) { - $this->cache->save($details, $this->key.'_'.$base); + foreach ($keysets as $key => $details) { + $this->cache->save($details, $this->key.'_'.$key); } $this->cache->save($index, $this->key); From e6352dffbbf8a6deb3f4fc319b1778c4348b303d Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Tue, 12 Mar 2013 17:14:12 +1300 Subject: [PATCH 10/10] FIX Static polution with informational fields --- core/manifest/ConfigStaticManifest.php | 43 +++++++++++-------- .../manifest/ConfigStaticManifestTest.php | 12 +++--- 2 files changed, 32 insertions(+), 23 deletions(-) diff --git a/core/manifest/ConfigStaticManifest.php b/core/manifest/ConfigStaticManifest.php index ee73da5e0..486871e10 100644 --- a/core/manifest/ConfigStaticManifest.php +++ b/core/manifest/ConfigStaticManifest.php @@ -63,7 +63,7 @@ class SS_ConfigStaticManifest { if (isset($this->index[$class])) { $info = $this->index[$class]; - if ($details = $this->cache->load($this->key.'_'.$info['key'])) { + if (isset($info['key']) && $details = $this->cache->load($this->key.'_'.$info['key'])) { $this->statics += $details; } @@ -95,6 +95,7 @@ class SS_ConfigStaticManifest { * Completely regenerates the manifest file. */ public function regenerate($cache = true) { + $this->index = array('$statics' => array()); $this->statics = array(); $finder = new ManifestFileFinder(); @@ -108,22 +109,17 @@ class SS_ConfigStaticManifest { $finder->find($this->base); if($cache) { - $index = array('$statics' => array()); $keysets = array(); foreach ($this->statics as $class => $details) { if (in_array($class, self::$initial_classes)) { - $index['$statics'][$class] = $details; + $this->index['$statics'][$class] = $details; } else { $key = sha1($class); - $keysets[$key][$class] = $details; + $this->index[$class]['key'] = $key; - $index[$class] = array( - 'key' => $key, - 'path' => $details['path'], - 'mtime' => filemtime($details['path']), - ); + $keysets[$key][$class] = $details; } } @@ -131,13 +127,16 @@ class SS_ConfigStaticManifest { $this->cache->save($details, $this->key.'_'.$key); } - $this->cache->save($index, $this->key); + $this->cache->save($this->index, $this->key); } } public function handleFile($basename, $pathname, $depth) { $parser = new SS_ConfigStaticManifest_Parser($pathname); - $this->statics = array_merge($this->statics, $parser->parse()); + $parser->parse(); + + $this->index = array_merge($this->index, $parser->getInfo()); + $this->statics = array_merge($this->statics, $parser->getStatics()); } public function getStatics() { @@ -155,6 +154,7 @@ class SS_ConfigStaticManifest { */ class SS_ConfigStaticManifest_Parser { + protected $info = array(); protected $statics = array(); protected $path; @@ -171,6 +171,14 @@ class SS_ConfigStaticManifest_Parser { $this->pos = 0; } + function getInfo() { + return $this->info; + } + + function getStatics() { + return $this->statics; + } + /** * Get the next token to process, incrementing the pointer * @@ -198,7 +206,6 @@ class SS_ConfigStaticManifest_Parser { if($type == T_CLASS) { $next = $this->next(); - if($next[0] != T_STRING) { user_error("Couldn\'t parse {$this->path} when building config static manifest", E_USER_ERROR); } @@ -233,8 +240,6 @@ class SS_ConfigStaticManifest_Parser { $access = ''; } } - - return $this->statics; } /** @@ -299,13 +304,17 @@ class SS_ConfigStaticManifest_Parser { } } - if(!isset($this->statics[$class])) { - $this->statics[$class] = array( + if (!isset($this->info[$class])) { + $this->info[$class] = array( 'path' => $this->path, - 'statics' => array() + 'mtime' => filemtime($this->path), ); } + if(!isset($this->statics[$class])) { + $this->statics[$class] = array(); + } + $this->statics[$class][$variable] = array( 'access' => $access, 'value' => eval('return '.$value.';') diff --git a/tests/core/manifest/ConfigStaticManifestTest.php b/tests/core/manifest/ConfigStaticManifestTest.php index fa8c49210..ceaa13da1 100644 --- a/tests/core/manifest/ConfigStaticManifestTest.php +++ b/tests/core/manifest/ConfigStaticManifestTest.php @@ -51,14 +51,14 @@ class ConfigStaticManifestTest extends SapphireTest { if ($statics === null) { $parser = new SS_ConfigStaticManifest_Parser(__FILE__); - $statics = $parser->parse(); + $parser->parse(); } - return $statics; + return $parser; } public function testParsingAccessLevels() { - $statics = $this->parseSelf(); + $statics = $this->parseSelf()->getStatics(); $levels = array( 'nolevel' => null, @@ -80,7 +80,7 @@ class ConfigStaticManifestTest extends SapphireTest { } public function testParsingValues() { - $statics = $this->parseSelf(); + $statics = $this->parseSelf()->getStatics(); // Check assigning values $values = array( @@ -112,12 +112,12 @@ class ConfigStaticManifestTest extends SapphireTest { } public function testIgnoresMethodStatics() { - $statics = $this->parseSelf(); + $statics = $this->parseSelf()->getStatics(); $this->assertNull(@$statics[__CLASS__]['method_static']); } public function testIgnoresStaticMethods() { - $statics = $this->parseSelf(); + $statics = $this->parseSelf()->getStatics(); $this->assertNull(@$statics[__CLASS__]['static_method']); } } \ No newline at end of file