From 40cc21c89df9da42dc076ae9ab1e39f75fa01a66 Mon Sep 17 00:00:00 2001
From: sun <sun@unleashedmind.com>
Date: Tue, 14 Feb 2012 14:39:26 +0100
Subject: [PATCH] Fixed Image module tests. Added many critical todos:
- Config values are not casted to strings (despite promised).
- Config keys are not casted to strings (although promised, too).
- XML can be invalid and not parse-able for many reasons.
- Config keys are not validated/sanitized.
- config()->clear() should really be ->unset().
- Configuration must not be additionally cached by a module in any way (static cache / database cache).
- Some modules invoke drupal_alter() on configuration (e.g., image styles).
- Need a way to list config object names/suffixes _after_ a specified prefix.
- Need a way to determine whether a config object exists.
- Some modules might have a valid use-case for retrieving/listing config objects using a wildcard within the name (instead of only searching by prefix).
- The key of a retrieved value is unknown; since you only get the value. Configuration values (or sub-values) may be passed forward to another function/callback, and thus, that function no longer knows about the key of the value. (unless the key is contained in the value, which is a very very wonky duplication)
- Config keys must not contain periods (within a specific key).
---
core/includes/config.inc | 8 +-
.../Drupal/Core/Config/ConfigException.php | 2 +-
core/lib/Drupal/Core/Config/DrupalConfig.php | 10 ++
core/modules/image/image.install | 81 -------------
core/modules/image/image.module | 113 +++++++++++-------
core/modules/image/image.test | 2 +-
6 files changed, 86 insertions(+), 130 deletions(-)
diff --git a/core/includes/config.inc b/core/includes/config.inc
index 7f59ba32cbdb..e60eca897154 100644
--- a/core/includes/config.inc
+++ b/core/includes/config.inc
@@ -188,7 +188,7 @@ function config_xml_to_array($data) {
}
foreach ($xmlObject as $index => $content) {
if (is_object($content)) {
- $out[$index] = config_xml2array($content);
+ $out[$index] = config_xml_to_array($content);
}
}
@@ -219,6 +219,8 @@ function config_encode($data) {
$dom->preserveWhiteSpace = false;
$dom->formatOutput = true;
$dom->loadXML($xml_object->asXML());
+ // @todo The loaded XML can be invalid; throwing plenty of PHP warnings but no
+ // catchable error.
return $dom->saveXML();
}
@@ -246,7 +248,9 @@ function config_array_to_xml($array, &$xml_object) {
}
}
else {
- $xml_object->addChild("$key", "$value");
+ // @todo Cast to string must happen in DrupalConfig::set() already. But
+ // over there, $value may also be an array, so the result is "Array".
+ $xml_object->addChild($key, (string) $value);
}
}
}
diff --git a/core/lib/Drupal/Core/Config/ConfigException.php b/core/lib/Drupal/Core/Config/ConfigException.php
index a115c9352f95..c60a449822f8 100644
--- a/core/lib/Drupal/Core/Config/ConfigException.php
+++ b/core/lib/Drupal/Core/Config/ConfigException.php
@@ -5,4 +5,4 @@
/**
* @todo
*/
-class ConfigException extends Exception {}
+class ConfigException extends \Exception {}
diff --git a/core/lib/Drupal/Core/Config/DrupalConfig.php b/core/lib/Drupal/Core/Config/DrupalConfig.php
index 5cf2c622ead9..121fac7eb615 100644
--- a/core/lib/Drupal/Core/Config/DrupalConfig.php
+++ b/core/lib/Drupal/Core/Config/DrupalConfig.php
@@ -3,6 +3,7 @@
namespace Drupal\Core\Config;
use Drupal\Core\Config\DrupalConfigVerifiedStorageInterface;
+use Drupal\Core\Config\ConfigException;
/**
* Represents the default configuration storage object.
@@ -105,6 +106,13 @@ public function get($key = '') {
* @todo
*/
public function set($key, $value) {
+ // Remove all non-alphanumeric characters from the key.
+ // @todo Reverse this and throw an exception when encountering a key with
+ // invalid name. The identical validation also needs to happen in get().
+ // Furthermore, the dot/period is a reserved character; it may appear
+ // between keys, but not within keys.
+ $key = preg_replace('@[^a-zA-Z0-9_.-]@', '', $key);
+
$parts = explode('.', $key);
if (count($parts) == 1) {
$this->data[$key] = $value;
@@ -119,6 +127,8 @@ public function set($key, $value) {
*
* @param $key
* @todo
+ *
+ * @todo Rename into unset().
*/
public function clear($key) {
$parts = explode('.', $key);
diff --git a/core/modules/image/image.install b/core/modules/image/image.install
index 02be57ca4c5f..91349a92a292 100644
--- a/core/modules/image/image.install
+++ b/core/modules/image/image.install
@@ -22,87 +22,6 @@ function image_uninstall() {
file_unmanaged_delete_recursive(file_default_scheme() . '://styles');
}
-/**
- * Implements hook_schema().
- */
-function image_schema() {
- $schema = array();
-
- $schema['image_styles'] = array(
- 'description' => 'Stores configuration options for image styles.',
- 'fields' => array(
- 'isid' => array(
- 'description' => 'The primary identifier for an image style.',
- 'type' => 'serial',
- 'unsigned' => TRUE,
- 'not null' => TRUE,
- ),
- 'name' => array(
- 'description' => 'The style name.',
- 'type' => 'varchar',
- 'length' => 255,
- 'not null' => TRUE,
- ),
- ),
- 'primary key' => array('isid'),
- 'unique keys' => array(
- 'name' => array('name'),
- ),
- );
-
- $schema['image_effects'] = array(
- 'description' => 'Stores configuration options for image effects.',
- 'fields' => array(
- 'ieid' => array(
- 'description' => 'The primary identifier for an image effect.',
- 'type' => 'serial',
- 'unsigned' => TRUE,
- 'not null' => TRUE,
- ),
- 'isid' => array(
- 'description' => 'The {image_styles}.isid for an image style.',
- 'type' => 'int',
- 'unsigned' => TRUE,
- 'not null' => TRUE,
- 'default' => 0,
- ),
- 'weight' => array(
- 'description' => 'The weight of the effect in the style.',
- 'type' => 'int',
- 'unsigned' => FALSE,
- 'not null' => TRUE,
- 'default' => 0,
- ),
- 'name' => array(
- 'description' => 'The unique name of the effect to be executed.',
- 'type' => 'varchar',
- 'length' => 255,
- 'not null' => TRUE,
- ),
- 'data' => array(
- 'description' => 'The configuration data for the effect.',
- 'type' => 'blob',
- 'not null' => TRUE,
- 'size' => 'big',
- 'serialize' => TRUE,
- ),
- ),
- 'primary key' => array('ieid'),
- 'indexes' => array(
- 'isid' => array('isid'),
- 'weight' => array('weight'),
- ),
- 'foreign keys' => array(
- 'image_style' => array(
- 'table' => 'image_styles',
- 'columns' => array('isid' => 'isid'),
- ),
- ),
- );
-
- return $schema;
-}
-
/**
* Implements hook_field_schema().
*/
diff --git a/core/modules/image/image.module b/core/modules/image/image.module
index 09841c50aad0..851ba141b1cf 100644
--- a/core/modules/image/image.module
+++ b/core/modules/image/image.module
@@ -435,33 +435,30 @@ function image_path_flush($path) {
* @see image_style_load()
*/
function image_styles() {
- $styles = &drupal_static(__FUNCTION__);
-
- // Grab from cache or build the array.
- if (!isset($styles)) {
- if ($cache = cache()->get('image_styles')) {
- $styles = $cache->data;
- }
- else {
+ // @todo Configuration must not be statically cached nor cache-system cached.
+ // However, there's a drupal_alter() involved here.
+
+// $styles = &drupal_static(__FUNCTION__);
+//
+// // Grab from cache or build the array.
+// if (!isset($styles)) {
+// if ($cache = cache()->get('image_styles')) {
+// $styles = $cache->data;
+// }
+// else {
$styles = array();
// Select the styles we have configured.
- foreach (config_get_signed_file_storage_names_with_prefix('image.styles') as $config_name) {
- $style = array();
- $config = config($config_name);
- $style['name'] = $config->get('name');
- $style['effects'] = array();
- foreach ($config->get('effects') as $key => $effect) {
- $definition = image_effect_definition_load($effect['name']);
- $effect = array_merge($definition, $effect);
- $style['effects'][$key] = $effect;
- }
+ $configured_styles = config_get_verified_storage_names_with_prefix('image.styles');
+ foreach ($configured_styles as $config_name) {
+ // @todo Allow to retrieve the name without prefix only.
+ $style = image_style_load(str_replace('image.styles.', '', $config_name));
$styles[$style['name']] = $style;
}
drupal_alter('image_styles', $styles);
- cache()->set('image_styles', $styles);
- }
- }
+// cache()->set('image_styles', $styles);
+// }
+// }
return $styles;
}
@@ -478,16 +475,24 @@ function image_styles() {
* If the image style name is not valid, an empty array is returned.
* @see image_effect_load()
*/
-function image_style_load($name = NULL) {
- $styles = image_styles();
+function image_style_load($name) {
+ $style = config('image.styles.' . $name)->get();
- // If retrieving by name.
- if (isset($name) && isset($styles[$name])) {
- return $styles[$name];
+ // @todo Requires a more reliable + generic method to check for whether the
+ // configuration object exists.
+ if (!isset($style['name'])) {
+ return FALSE;
}
- // Otherwise the style was not found.
- return FALSE;
+ foreach ($style['effects'] as $ieid => $effect) {
+ $definition = image_effect_definition_load($effect['name']);
+ $effect = array_merge($definition, $effect);
+ $style['effects'][$ieid] = $effect;
+ }
+ // Sort effects by weight.
+ uasort($style['effects'], 'drupal_sort_weight');
+
+ return $style;
}
/**
@@ -508,6 +513,8 @@ function image_style_save($style) {
$config->set('effects', array());
}
$config->save();
+ // @todo is_new must only be set when the configuration object did not exist
+ // yet.
$style['is_new'] = TRUE;
// Let other modules update as necessary on save.
@@ -554,8 +561,7 @@ function image_style_delete($style, $replacement_style_name = '') {
* format array('isid' => array()), or an empty array if the specified style
* has no effects.
*
- * @todo I think this function can be deprecated since we automatically
- * load all effects with a style now.
+ * @todo Remove this function; it's entirely obsolete.
*/
function image_style_effects($style) {
return $style['effects'];
@@ -754,11 +760,11 @@ function image_style_flush($style) {
// Let other modules update as necessary on flush.
module_invoke_all('image_style_flush', $style);
- // Clear image style and effect caches.
- cache()->delete('image_styles');
- cache()->deletePrefix('image_effects:');
- drupal_static_reset('image_styles');
- drupal_static_reset('image_effects');
+// // Clear image style and effect caches.
+// cache()->delete('image_styles');
+// cache()->deletePrefix('image_effects:');
+// drupal_static_reset('image_styles');
+// drupal_static_reset('image_effects');
// Clear field caches so that formatters may be added for this style.
field_info_cache_clear();
@@ -899,6 +905,8 @@ function image_effect_definition_load($effect) {
* @return
* An array of all image effects.
* @see image_effect_load()
+ *
+ * @todo Remove after moving/resolving the todo.
*/
function image_effects() {
$effects = &drupal_static(__FUNCTION__);
@@ -907,6 +915,9 @@ function image_effects() {
$effects = array();
// Add database image effects.
+ // @todo Strictly speaking, this is obsolete. However, it demonstrates a
+ // use-case for retrieving/listing configuration objects using a wildcard
+ // within the name (instead of only the suffix).
$result = db_select('image_effects', NULL, array('fetch' => PDO::FETCH_ASSOC))
->fields('image_effects')
->orderBy('image_effects.weight', 'ASC')
@@ -928,13 +939,11 @@ function image_effects() {
/**
* Load a single image effect.
*
- * @param $name
- * The image effect name.
+ * @param $ieid
+ * The image effect ID.
* @param $style_name
* The image style name.
- * @param $include
- * If set, this loader will restrict to a specific type of image style, may be
- * one of the defined Image style storage constants.
+ *
* @return
* An image effect array, consisting of the following keys:
* - "ieid": The unique image effect ID.
@@ -947,9 +956,20 @@ function image_effects() {
* @see image_style_load()
* @see image_effect_definition_load()
*/
-function image_effect_load($name, $style_name) {
- if (($style = image_style_load($style_name)) && isset($style['effects'][$name])) {
- return $style['effects'][$name];
+function image_effect_load($ieid, $style_name) {
+ if (($style = image_style_load($style_name)) && isset($style['effects'][$ieid])) {
+ $effect = $style['effects'][$ieid];
+ $definition = image_effect_definition_load($effect['name']);
+ $effect = array_merge($definition, $effect);
+ // @todo The effect's key name within the style is unknown. It *should* be
+ // identical to the ieid, but that is in no way guaranteed. And of course,
+ // the ieid key *within* the effect is senseless duplication in the first
+ // place. This problem can be eliminated in many places, but especially
+ // for loaded menu arguments like %image_effect, the actual router
+ // callbacks don't have access to 'ieid' anymore (unless resorting to
+ // dirty %index and %map tricks).
+ $effect['ieid'] = $ieid;
+ return $effect;
}
return FALSE;
}
@@ -979,15 +999,18 @@ function image_effect_save($style_name, $effect) {
// The machine name is all the elements of the data array concatenated
// together, delimited by underscores.
$machine_name = $effect['name'];
-
foreach ($effect['data'] as $key => $value) {
$machine_name .= '_' . $value;
}
+ // @todo The machine name must not use any special non-alphanumeric
+ // characters, and may also not contain dots/periods, as that is the
+ // config system's nested key syntax.
+ $machine_name = preg_replace('@[^a-zA-Z0-9_-]@', '', $machine_name);
$effect['ieid'] = $machine_name;
$config->set('effects.' . $machine_name, $effect);
}
$config->save();
- $style = image_style_load(NULL, $style_name);
+ $style = image_style_load($style_name);
image_style_flush($style);
return $effect;
}
diff --git a/core/modules/image/image.test b/core/modules/image/image.test
index 91b5e4faa6e3..8081bf079418 100644
--- a/core/modules/image/image.test
+++ b/core/modules/image/image.test
@@ -468,7 +468,7 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
// Load the style by the new name with the new weights.
drupal_static_reset('image_styles');
- $style = image_style_load($style_name, NULL);
+ $style = image_style_load($style_name);
// Confirm the new style order was saved.
$effect_edits_order = array_reverse($effect_edits_order);
--
GitLab