Drupal
/
drupal
mirror of https://git.drupalcode.org/project/drupal.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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).
8.0.x
sun 2012-02-14 14:39:26 +01:00 committed by Greg Dunlap
parent d74f856cd3
commit 40cc21c89d
6 changed files with 85 additions and 129 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
core/includes/config.inc
View File

@ -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);
}
}
}

2
core/lib/Drupal/Core/Config/ConfigException.php
View File

@ -5,4 +5,4 @@ namespace Drupal\Core\Config;
/**
* @todo
*/
class ConfigException extends Exception {}
class ConfigException extends \Exception {}

10
core/lib/Drupal/Core/Config/DrupalConfig.php
View File

@ -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 @@ class DrupalConfig {
* @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 @@ class DrupalConfig {
*
* @param $key
* @todo
*
* @todo Rename into unset().
*/
public function clear($key) {
$parts = explode('.', $key);

81
core/modules/image/image.install
View File

@ -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().
*/

111
core/modules/image/image.module
View File

@ -435,33 +435,30 @@ function image_path_flush($path) {
* @see image_style_load()
*/
function image_styles() {
$styles = &drupal_static(__FUNCTION__);
// @todo Configuration must not be statically cached nor cache-system cached.
// However, there's a drupal_alter() involved here.
// Grab from cache or build the array.
if (!isset($styles)) {
if ($cache = cache()->get('image_styles')) {
$styles = $cache->data;
}
else {
// $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;
}

2
core/modules/image/image.test
View File

@ -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);