From 284241ecb9675c71519c4dd9670e6781ac4a8d93 Mon Sep 17 00:00:00 2001 From: webchick Date: Sat, 4 Jan 2014 10:53:30 -0800 Subject: [PATCH] Issue #2162013 by alexpott, Berdir, sun, pwolanin: Critical performance regression due to config schema + TypedData lookups in installer. --- core/core.services.yml | 2 +- core/includes/install.core.inc | 3 +- core/lib/Drupal/Core/Config/Config.php | 36 ++-------------- .../lib/Drupal/Core/Config/Schema/Mapping.php | 26 ++++++++---- .../Schema/SchemaIncompleteException.php | 14 +++++++ .../Drupal/Core/Config/Schema/Sequence.php | 4 +- .../Drupal/Core/Config/TypedConfigManager.php | 41 ++++++++++++++++--- .../lib/Drupal/locale/LocaleConfigManager.php | 7 +++- .../locale/Tests/LocaleConfigManagerTest.php | 3 +- core/modules/locale/locale.services.yml | 2 +- 10 files changed, 83 insertions(+), 55 deletions(-) create mode 100644 core/lib/Drupal/Core/Config/Schema/SchemaIncompleteException.php diff --git a/core/core.services.yml b/core/core.services.yml index a2d373cffd7..1449b3bbf22 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -107,7 +107,7 @@ services: class: Drupal\Core\Config\InstallStorage config.typed: class: Drupal\Core\Config\TypedConfigManager - arguments: ['@config.storage', '@config.storage.schema'] + arguments: ['@config.storage', '@config.storage.schema', '@cache.config'] database: class: Drupal\Core\Database\Connection factory_class: Drupal\Core\Database\Database diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc index 1dbfec695b2..5594343985c 100644 --- a/core/includes/install.core.inc +++ b/core/includes/install.core.inc @@ -390,7 +390,8 @@ function install_begin_request(&$install_state) { $container->register('config.typed', 'Drupal\Core\Config\TypedConfigManager') ->addArgument(new Reference('config.storage')) - ->addArgument(new Reference('config.storage.schema')); + ->addArgument(new Reference('config.storage.schema')) + ->addArgument(new Reference('cache.config')); $container->register('config.factory', 'Drupal\Core\Config\ConfigFactory') ->addArgument(new Reference('config.storage')) diff --git a/core/lib/Drupal/Core/Config/Config.php b/core/lib/Drupal/Core/Config/Config.php index f04bbe0c69d..e0267172292 100644 --- a/core/lib/Drupal/Core/Config/Config.php +++ b/core/lib/Drupal/Core/Config/Config.php @@ -11,6 +11,7 @@ use Drupal\Component\Utility\NestedArray; use Drupal\Component\Utility\String; use Drupal\Core\Config\ConfigNameException; use Drupal\Core\Config\Context\ContextInterface; +use Drupal\Core\Config\Schema\SchemaIncompleteException; use Drupal\Core\TypedData\PrimitiveInterface; use Drupal\Core\TypedData\Type\FloatInterface; use Drupal\Core\TypedData\Type\IntegerInterface; @@ -516,37 +517,6 @@ class Config { return $this->schemaWrapper; } - /** - * Gets the definition for the configuration key. - * - * @param string $key - * A string that maps to a key within the configuration data. - * - * @return \Drupal\Core\Config\Schema\Element - * - * @throws \Drupal\Core\Config\ConfigException - * Thrown when schema is incomplete. - */ - protected function getSchemaForKey($key) { - $parts = explode('.', $key); - $schema_wrapper = $this->getSchemaWrapper(); - if (count($parts) == 1) { - $schema = $schema_wrapper->get($key); - } - else { - $schema = clone $schema_wrapper; - foreach ($parts as $nested_key) { - if (!is_object($schema) || !method_exists($schema, 'get')) { - throw new ConfigException(String::format("Incomplete schema for !key key in configuration object !name.", array('!name' => $this->name, '!key' => $key))); - } - else { - $schema = $schema->get($nested_key); - } - } - } - return $schema; - } - /** * Casts the value to correct data type using the configuration schema. * @@ -564,7 +534,7 @@ class Config { } elseif (is_scalar($value)) { try { - $element = $this->getSchemaForKey($key); + $element = $this->getSchemaWrapper()->get($key); if ($element instanceof PrimitiveInterface) { // Special handling for integers and floats since the configuration // system is primarily concerned with saving values from the Form API @@ -586,7 +556,7 @@ class Config { $value = $element->getString(); } } - catch (\Exception $e) { + catch (SchemaIncompleteException $e) { // @todo throw an exception due to an incomplete schema. Only possible // once https://drupal.org/node/1910624 is complete. } diff --git a/core/lib/Drupal/Core/Config/Schema/Mapping.php b/core/lib/Drupal/Core/Config/Schema/Mapping.php index 92e9a90f53f..256076fe343 100644 --- a/core/lib/Drupal/Core/Config/Schema/Mapping.php +++ b/core/lib/Drupal/Core/Config/Schema/Mapping.php @@ -7,10 +7,8 @@ namespace Drupal\Core\Config\Schema; -use Drupal\Component\Utility\NestedArray; use Drupal\Core\TypedData\ComplexDataInterface; -use Drupal\Core\TypedData\TypedDataInterface; -use \InvalidArgumentException; +use Drupal\Component\Utility\String; /** * Defines a mapping configuration element. @@ -36,17 +34,29 @@ class Mapping extends ArrayElement implements ComplexDataInterface { /** * Implements Drupal\Core\TypedData\ComplexDataInterface::get(). + * + * Since all configuration objects are mappings the function will except a dot + * delimited key to access nested values, for example, 'page.front'. */ public function get($property_name) { + $parts = explode('.', $property_name); + $root_key = array_shift($parts); $elements = $this->getElements(); - if (isset($elements[$property_name])) { - return $elements[$property_name]; + if (isset($elements[$root_key])) { + $element = $elements[$root_key]; } else { - throw new InvalidArgumentException(format_string("The configuration property @key doesn't exist.", array( - '@key' => $property_name, - ))); + throw new SchemaIncompleteException(String::format("The configuration property @key doesn't exist.", array('@key' => $property_name))); } + + // If $property_name contained a dot recurse into the keys. + foreach ($parts as $key) { + if (!is_object($element) || !method_exists($element, 'get')) { + throw new SchemaIncompleteException(String::format("The configuration property @key does not exist.", array('@key' => $property_name))); + } + $element = $element->get($key); + } + return $element; } /** diff --git a/core/lib/Drupal/Core/Config/Schema/SchemaIncompleteException.php b/core/lib/Drupal/Core/Config/Schema/SchemaIncompleteException.php new file mode 100644 index 00000000000..34a458e26bf --- /dev/null +++ b/core/lib/Drupal/Core/Config/Schema/SchemaIncompleteException.php @@ -0,0 +1,14 @@ +getItemDefinition(); + $definition = $this->getItemDefinition(); $elements = array(); foreach ($this->value as $key => $value) { $elements[$key] = $this->parseElement($key, $value, $definition); @@ -60,7 +60,7 @@ class Sequence extends ArrayElement implements ListInterface { * Typed configuration element. */ public function get($key) { - $elements = $this->parse(); + $elements = $this->getElements(); return $elements[$key]; } diff --git a/core/lib/Drupal/Core/Config/TypedConfigManager.php b/core/lib/Drupal/Core/Config/TypedConfigManager.php index 71aa0bcfb5c..09ec0c5b739 100644 --- a/core/lib/Drupal/Core/Config/TypedConfigManager.php +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php @@ -11,13 +11,20 @@ use Drupal\Component\Plugin\Exception\PluginException; use Drupal\Component\Plugin\PluginManagerBase; use Drupal\Component\Utility\NestedArray; use Drupal\Component\Utility\String; -use Drupal\Core\Config\TypedConfigManagerInterface; +use Drupal\Core\Cache\CacheBackendInterface; /** * Manages config type plugins. */ class TypedConfigManager extends PluginManagerBase implements TypedConfigManagerInterface { + /** + * The cache ID for the definitions. + * + * @var string + */ + const CACHE_ID = 'typed_config_definitions'; + /** * A storage controller instance for reading configuration data. * @@ -39,6 +46,13 @@ class TypedConfigManager extends PluginManagerBase implements TypedConfigManager */ protected $definitions; + /** + * Cache backend for the definitions. + * + * @var \Drupal\Core\Cache\CacheBackendInterface + */ + protected $cache; + /** * Creates a new typed configuration manager. * @@ -46,10 +60,13 @@ class TypedConfigManager extends PluginManagerBase implements TypedConfigManager * The storage controller object to use for reading schema data * @param \Drupal\Core\Config\StorageInterface $schemaStorage * The storage controller object to use for reading schema data + * @param \Drupal\Core\Cache\CacheBackendInterface $cache + * The cache backend to use for caching the definitions. */ - public function __construct(StorageInterface $configStorage, StorageInterface $schemaStorage) { + public function __construct(StorageInterface $configStorage, StorageInterface $schemaStorage, CacheBackendInterface $cache) { $this->configStorage = $configStorage; $this->schemaStorage = $schemaStorage; + $this->cache = $cache; } /** @@ -157,22 +174,34 @@ class TypedConfigManager extends PluginManagerBase implements TypedConfigManager } /** - * Implements Drupal\Component\Plugin\Discovery\DiscoveryInterface::getDefinitions(). + * {@inheritdoc} */ public function getDefinitions() { if (!isset($this->definitions)) { - $this->definitions = array(); - foreach ($this->schemaStorage->listAll() as $name) { - if ($schema = $this->schemaStorage->read($name)) { + if ($cache = $this->cache->get($this::CACHE_ID)) { + $this->definitions = $cache->data; + } + else { + $this->definitions = array(); + foreach ($this->schemaStorage->readMultiple($this->schemaStorage->listAll()) as $schema) { foreach ($schema as $type => $definition) { $this->definitions[$type] = $definition; } } + $this->cache->set($this::CACHE_ID, $this->definitions); } } return $this->definitions; } + /** + * {@inheritdoc} + */ + public function clearCachedDefinitions() { + $this->definitions = NULL; + $this->cache->delete($this::CACHE_ID); + } + /** * Gets fallback metadata name. * diff --git a/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.php b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.php index e1469369c5f..9284444d2fa 100644 --- a/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.php +++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.php @@ -7,6 +7,7 @@ namespace Drupal\locale; +use Drupal\Core\Cache\CacheBackendInterface; use Drupal\Core\Language\Language; use Drupal\Core\Config\TypedConfigManager; use Drupal\Core\Config\StorageInterface; @@ -47,10 +48,12 @@ class LocaleConfigManager extends TypedConfigManager { * data. * @param \Drupal\locale\StringStorageInterface $localeStorage * The locale storage to use for reading string translations. + * @param \Drupal\Core\Cache\CacheBackendInterface $cache + * The cache backend to use for caching the definitions. */ - public function __construct(StorageInterface $configStorage, StorageInterface $schemaStorage, StorageInterface $installStorage, StringStorageInterface $localeStorage) { + public function __construct(StorageInterface $configStorage, StorageInterface $schemaStorage, StorageInterface $installStorage, StringStorageInterface $localeStorage, CacheBackendInterface $cache) { // Note we use the install storage for the parent constructor. - parent::__construct($configStorage, $schemaStorage); + parent::__construct($configStorage, $schemaStorage, $cache); $this->installStorage = $installStorage; $this->localeStorage = $localeStorage; } diff --git a/core/modules/locale/lib/Drupal/locale/Tests/LocaleConfigManagerTest.php b/core/modules/locale/lib/Drupal/locale/Tests/LocaleConfigManagerTest.php index f6f2900f378..af87dc968aa 100644 --- a/core/modules/locale/lib/Drupal/locale/Tests/LocaleConfigManagerTest.php +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleConfigManagerTest.php @@ -46,7 +46,8 @@ class LocaleConfigManagerTest extends DrupalUnitTestBase { $this->container->get('config.storage.installer'), $this->container->get('config.storage.schema'), $this->container->get('config.storage.installer'), - $this->container->get('locale.storage') + $this->container->get('locale.storage'), + $this->container->get('cache.config') ); $language = new Language(array('id' => 'de')); diff --git a/core/modules/locale/locale.services.yml b/core/modules/locale/locale.services.yml index f8319893dc8..568f21d701f 100644 --- a/core/modules/locale/locale.services.yml +++ b/core/modules/locale/locale.services.yml @@ -11,7 +11,7 @@ services: arguments: ['@entity.manager'] locale.config.typed: class: Drupal\locale\LocaleConfigManager - arguments: ['@config.storage', '@config.storage.schema', '@config.storage.installer', '@locale.storage'] + arguments: ['@config.storage', '@config.storage.schema', '@config.storage.installer', '@locale.storage', '@cache.config'] locale.storage: class: Drupal\locale\StringDatabaseStorage arguments: ['@database']