summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAlex Pott2015-09-26 10:54:37 (GMT)
committerAlex Pott2015-09-26 10:54:37 (GMT)
commita49d543008e37999435e316e203fc14426b34668 (patch)
tree8f033967feac68bb5e834174364b201d13d3d6fe
parentae70ed6f445df5a27dc9dd69c0d9dc0d58aca180 (diff)
Issue #2236903 by yched, tstoeckler, swentel, Jalandhar, nlisgo, andypost, fago, sun, amateescu: setSettings() on field definitions can unintentionally clear default values
-rw-r--r--core/lib/Drupal/Core/Field/BaseFieldDefinition.php42
-rw-r--r--core/lib/Drupal/Core/Field/FieldConfigBase.php2
-rw-r--r--core/lib/Drupal/Core/Field/FieldConfigInterface.php24
-rw-r--r--core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php8
-rw-r--r--core/modules/field/src/Entity/FieldStorageConfig.php2
-rw-r--r--core/modules/field/src/FieldStorageConfigInterface.php26
-rw-r--r--core/modules/menu_link_content/src/Entity/MenuLinkContent.php8
-rw-r--r--core/modules/system/src/Tests/Entity/EntityFieldTest.php8
-rw-r--r--core/modules/system/src/Tests/Field/FieldSettingsTest.php120
-rw-r--r--core/modules/views/tests/src/Unit/EntityViewsDataTest.php2
10 files changed, 208 insertions, 34 deletions
diff --git a/core/lib/Drupal/Core/Field/BaseFieldDefinition.php b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
index 67cc15d..7df3ec1 100644
--- a/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
+++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
@@ -146,16 +146,36 @@ class BaseFieldDefinition extends ListDataDefinition implements FieldDefinitionI
}
/**
- * Sets field settings.
+ * {@inheritdoc}
*
- * @param array $settings
- * The value to set.
+ * Note that the method does not unset existing settings not specified in the
+ * incoming $settings array.
*
- * @return static
- * The object itself for chaining.
+ * For example:
+ * @code
+ * // Given these are the default settings.
+ * $field_definition->getSettings() === [
+ * 'fruit' => 'apple',
+ * 'season' => 'summer',
+ * ];
+ * // Change only the 'fruit' setting.
+ * $field_definition->setSettings(['fruit' => 'banana']);
+ * // The 'season' setting persists unchanged.
+ * $field_definition->getSettings() === [
+ * 'fruit' => 'banana',
+ * 'season' => 'summer',
+ * ];
+ * @endcode
+ *
+ * For clarity, it is preferred to use setSetting() if not all available
+ * settings are supplied.
*/
public function setSettings(array $settings) {
- $this->getItemDefinition()->setSettings($settings);
+ // Assign settings individiually, in order to keep the current values
+ // of settings not specified in $settings.
+ foreach ($settings as $setting_name => $setting) {
+ $this->getItemDefinition()->setSetting($setting_name, $setting);
+ }
return $this;
}
@@ -167,15 +187,7 @@ class BaseFieldDefinition extends ListDataDefinition implements FieldDefinitionI
}
/**
- * Sets a field setting.
- *
- * @param string $setting_name
- * The field setting to set.
- * @param mixed $value
- * The value to set.
- *
- * @return static
- * The object itself for chaining.
+ * {@inheritdoc}
*/
public function setSetting($setting_name, $value) {
$this->getItemDefinition()->setSetting($setting_name, $value);
diff --git a/core/lib/Drupal/Core/Field/FieldConfigBase.php b/core/lib/Drupal/Core/Field/FieldConfigBase.php
index 3d6613e..da0bca8 100644
--- a/core/lib/Drupal/Core/Field/FieldConfigBase.php
+++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
@@ -350,7 +350,7 @@ abstract class FieldConfigBase extends ConfigEntityBase implements FieldConfigIn
* {@inheritdoc}
*/
public function setSettings(array $settings) {
- $this->settings = $settings;
+ $this->settings = $settings + $this->settings;
return $this;
}
diff --git a/core/lib/Drupal/Core/Field/FieldConfigInterface.php b/core/lib/Drupal/Core/Field/FieldConfigInterface.php
index 1357a1e..b802221 100644
--- a/core/lib/Drupal/Core/Field/FieldConfigInterface.php
+++ b/core/lib/Drupal/Core/Field/FieldConfigInterface.php
@@ -55,7 +55,29 @@ interface FieldConfigInterface extends FieldDefinitionInterface, ConfigEntityInt
public function setTranslatable($translatable);
/**
- * Sets field settings (overwrites existing settings).
+ * Sets field settings.
+ *
+ * Note that the method does not unset existing settings not specified in the
+ * incoming $settings array.
+ *
+ * For example:
+ * @code
+ * // Given these are the default settings.
+ * $field_definition->getSettings() === [
+ * 'fruit' => 'apple',
+ * 'season' => 'summer',
+ * ];
+ * // Change only the 'fruit' setting.
+ * $field_definition->setSettings(['fruit' => 'banana']);
+ * // The 'season' setting persists unchanged.
+ * $field_definition->getSettings() === [
+ * 'fruit' => 'banana',
+ * 'season' => 'summer',
+ * ];
+ * @endcode
+ *
+ * For clarity, it is preferred to use setSetting() if not all available
+ * settings are supplied.
*
* @param array $settings
* The array of field settings.
diff --git a/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
index 3d15bca..8321f7d 100644
--- a/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
+++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
@@ -57,19 +57,23 @@ interface FieldStorageDefinitionInterface extends CacheableDependencyInterface {
public function getType();
/**
- * Returns the field settings.
+ * Returns the storage settings.
*
* Each field type defines the settings that are meaningful for that type.
* For example, a text field can define a 'max_length' setting, and an image
* field can define a 'alt_field_required' setting.
*
+ * The method always returns an array of all available settings for this field
+ * type, possibly with the default values merged in if values have not been
+ * provided for all available settings.
+ *
* @return mixed[]
* An array of key/value pairs.
*/
public function getSettings();
/**
- * Returns the value of a given field setting.
+ * Returns the value of a given storage setting.
*
* @param string $setting_name
* The setting name.
diff --git a/core/modules/field/src/Entity/FieldStorageConfig.php b/core/modules/field/src/Entity/FieldStorageConfig.php
index 5fff1aa..3da118a 100644
--- a/core/modules/field/src/Entity/FieldStorageConfig.php
+++ b/core/modules/field/src/Entity/FieldStorageConfig.php
@@ -565,7 +565,7 @@ class FieldStorageConfig extends ConfigEntityBase implements FieldStorageConfigI
* {@inheritdoc}
*/
public function setSettings(array $settings) {
- $this->settings = $settings;
+ $this->settings = $settings + $this->settings;
return $this;
}
diff --git a/core/modules/field/src/FieldStorageConfigInterface.php b/core/modules/field/src/FieldStorageConfigInterface.php
index 116517a..3f44818 100644
--- a/core/modules/field/src/FieldStorageConfigInterface.php
+++ b/core/modules/field/src/FieldStorageConfigInterface.php
@@ -96,10 +96,32 @@ interface FieldStorageConfigInterface extends ConfigEntityInterface, FieldStorag
public function setSetting($setting_name, $value);
/**
- * Sets field settings (overwrites existing settings).
+ * Sets field storage settings.
+ *
+ * Note that the method does not unset existing settings not specified in the
+ * incoming $settings array.
+ *
+ * For example:
+ * @code
+ * // Given these are the default settings.
+ * $storage_definition->getSettings() === [
+ * 'fruit' => 'apple',
+ * 'season' => 'summer',
+ * ];
+ * // Change only the 'fruit' setting.
+ * $storage_definition->setSettings(['fruit' => 'banana']);
+ * // The 'season' setting persists unchanged.
+ * $storage_definition->getSettings() === [
+ * 'fruit' => 'banana',
+ * 'season' => 'summer',
+ * ];
+ * @endcode
+ *
+ * For clarity, it is preferred to use setSetting() if not all available
+ * settings are supplied.
*
* @param array $settings
- * The array of field settings.
+ * The array of storage settings.
*
* @return $this
*/
diff --git a/core/modules/menu_link_content/src/Entity/MenuLinkContent.php b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
index abbc694..7d51ae2 100644
--- a/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -261,9 +261,7 @@ class MenuLinkContent extends ContentEntityBase implements MenuLinkContentInterf
->setDescription(t('The text to be used for this link in the menu.'))
->setRequired(TRUE)
->setTranslatable(TRUE)
- ->setSettings(array(
- 'max_length' => 255,
- ))
+ ->setSetting('max_length', 255)
->setDisplayOptions('view', array(
'label' => 'hidden',
'type' => 'string',
@@ -279,9 +277,7 @@ class MenuLinkContent extends ContentEntityBase implements MenuLinkContentInterf
->setLabel(t('Description'))
->setDescription(t('Shown when hovering over the menu link.'))
->setTranslatable(TRUE)
- ->setSettings(array(
- 'max_length' => 255,
- ))
+ ->setSetting('max_length', 255)
->setDisplayOptions('view', array(
'label' => 'hidden',
'type' => 'string',
diff --git a/core/modules/system/src/Tests/Entity/EntityFieldTest.php b/core/modules/system/src/Tests/Entity/EntityFieldTest.php
index 5414730..5d93c78 100644
--- a/core/modules/system/src/Tests/Entity/EntityFieldTest.php
+++ b/core/modules/system/src/Tests/Entity/EntityFieldTest.php
@@ -30,7 +30,7 @@ class EntityFieldTest extends EntityUnitTestBase {
*
* @var array
*/
- public static $modules = array('filter', 'text', 'node', 'user');
+ public static $modules = array('filter', 'text', 'node', 'user', 'field_test');
/**
* @var string
@@ -688,10 +688,8 @@ class EntityFieldTest extends EntityUnitTestBase {
->save();
$definition = BaseFieldDefinition::create('entity_reference')
->setLabel('Test entity')
- ->setSettings(array(
- 'target_type' => 'node',
- 'target_bundle' => 'article',
- ));
+ ->setSetting('target_type', 'node')
+ ->setSetting('target_bundle', 'article');
$reference_field = \Drupal::TypedDataManager()->create($definition);
$reference = $reference_field->appendItem(array('entity' => $node))->get('entity');
$violations = $reference->validate();
diff --git a/core/modules/system/src/Tests/Field/FieldSettingsTest.php b/core/modules/system/src/Tests/Field/FieldSettingsTest.php
new file mode 100644
index 0000000..2c2a8e9
--- /dev/null
+++ b/core/modules/system/src/Tests/Field/FieldSettingsTest.php
@@ -0,0 +1,120 @@
+<?php
+
+/**
+ * @file
+ * Contains Drupal\system\Tests\Field\FieldSettingsTest.
+ */
+
+namespace Drupal\system\Tests\Field;
+
+use Drupal\Core\Field\BaseFieldDefinition;
+use Drupal\field\Entity\FieldConfig;
+use Drupal\field\Entity\FieldStorageConfig;
+use Drupal\system\Tests\Entity\EntityUnitTestBase;
+
+/**
+ * Tests field settings methods on field definition structures.
+ *
+ * @group Field
+ */
+class FieldSettingsTest extends EntityUnitTestBase {
+
+ /**
+ * Modules to enable.
+ *
+ * @var array
+ */
+ public static $modules = array('field', 'field_test');
+
+ /**
+ * @covers \Drupal\Core\Field\BaseFieldDefinition::getSettings()
+ * @covers \Drupal\Core\Field\BaseFieldDefinition::setSettings()
+ */
+ public function testBaseFieldSettings() {
+ $base_field = BaseFieldDefinition::create('test_field');
+
+ // Check that the default settings have been populated.
+ $expected_settings = [
+ 'test_field_storage_setting' => 'dummy test string',
+ 'changeable' => 'a changeable field storage setting',
+ 'unchangeable' => 'an unchangeable field storage setting',
+ 'translatable_storage_setting' => 'a translatable field storage setting',
+ 'test_field_setting' => 'dummy test string',
+ 'translatable_field_setting' => 'a translatable field setting',
+ ];
+ $this->assertEqual($base_field->getSettings(), $expected_settings);
+
+ // Change one single setting using setSettings(), and check that the other
+ // expected settings are still present.
+ $expected_settings['test_field_setting'] = 'another test string';
+ $base_field->setSettings(['test_field_setting' => $expected_settings['test_field_setting']]);
+ $this->assertEqual($base_field->getSettings(), $expected_settings);
+ }
+
+ /**
+ * @covers \Drupal\field\Entity\FieldStorageConfig::getSettings()
+ * @covers \Drupal\field\Entity\FieldStorageConfig::setSettings()
+ */
+ public function testConfigurableFieldStorageSettings() {
+ $field_storage = FieldStorageConfig::create([
+ 'field_name' => 'test_field',
+ 'entity_type' => 'entity_test',
+ 'type' => 'test_field'
+ ]);
+
+ // Check that the default settings have been populated.
+ $expected_settings = [
+ 'test_field_storage_setting' => 'dummy test string',
+ 'changeable' => 'a changeable field storage setting',
+ 'unchangeable' => 'an unchangeable field storage setting',
+ 'translatable_storage_setting' => 'a translatable field storage setting',
+ ];
+ $this->assertEqual($field_storage->getSettings(), $expected_settings);
+
+ // Change one single setting using setSettings(), and check that the other
+ // expected settings are still present.
+ $expected_settings['test_field_storage_setting'] = 'another test string';
+ $field_storage->setSettings(['test_field_storage_setting' => $expected_settings['test_field_storage_setting']]);
+ $this->assertEqual($field_storage->getSettings(), $expected_settings);
+ }
+
+ /**
+ * @covers \Drupal\field\Entity\FieldStorageConfig::getSettings()
+ * @covers \Drupal\field\Entity\FieldStorageConfig::setSettings()
+ */
+ public function testConfigurableFieldSettings() {
+ $field_storage = FieldStorageConfig::create([
+ 'field_name' => 'test_field',
+ 'entity_type' => 'entity_test',
+ 'type' => 'test_field'
+ ]);
+ $field = FieldConfig::create([
+ 'field_storage' => $field_storage,
+ 'bundle' => 'entity_test'
+ ]);
+ // Note: FieldConfig does not populate default settings until the config
+ // is saved.
+ // @todo Remove once https://www.drupal.org/node/2327883 is fixed.
+ $field->save();
+
+ // Check that the default settings have been populated. Note: getSettings()
+ // returns both storage and field settings.
+ $expected_settings = [
+ 'test_field_storage_setting' => 'dummy test string',
+ 'changeable' => 'a changeable field storage setting',
+ 'unchangeable' => 'an unchangeable field storage setting',
+ 'translatable_storage_setting' => 'a translatable field storage setting',
+ 'test_field_setting' => 'dummy test string',
+ 'translatable_field_setting' => 'a translatable field setting',
+ 'field_setting_from_config_data' => TRUE,
+ ];
+ $this->assertEqual($field->getSettings(), $expected_settings);
+
+ // Change one single setting using setSettings(), and check that the other
+ // expected settings are still present.
+ $expected_settings['test_field_setting'] = 'another test string';
+ $field->setSettings(['test_field_setting' => $expected_settings['test_field_setting']]);
+ $this->assertEqual($field->getSettings(), $expected_settings);
+ }
+
+}
diff --git a/core/modules/views/tests/src/Unit/EntityViewsDataTest.php b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
index b8db4fe..9a65d6e 100644
--- a/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
+++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
@@ -485,7 +485,7 @@ class EntityViewsDataTest extends UnitTestCase {
$base_field_definitions = $this->setupBaseFields(EntityTestMul::baseFieldDefinitions($this->baseEntityType));
$base_field_definitions['type'] = BaseFieldDefinition::create('entity_reference')
->setLabel('entity test type')
- ->setSettings(array('target_type' => 'entity_test_bundle'))
+ ->setSetting('target_type', 'entity_test_bundle')
->setTranslatable(TRUE);
$base_field_definitions = $this->setupBaseFields($base_field_definitions);
$user_base_field_definitions = [