Posted by swentel on March 13, 2013 at 9:47pm
11 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | field system |
| Category: | task |
| Priority: | normal |
| Assigned: | swentel |
| Status: | closed (fixed) |
| Issue tags: | Configuration system, D8MI, Field API |
Issue Summary
This will convert field_language_default and storage_default to field.settings.
Related #1953528: Move 'view modes' part out of field_bundle_settings()
Comments
#1
#2
Tagging with D8MI as well
#3
Missed the 'extra_weights' example in field.api.php (although they are not used in actual code though) and moved language_default to state.
#4
Better title
#5
The last submitted patch, 1942346-3.patch, failed testing.
#6
Mixing up API's and and epic typo
#7
The last submitted patch, 1942346-6.patch, failed testing.
#8
Added upgrade path and should fix most of the tests.
#9
The last submitted patch, 1942346-8.patch, failed testing.
#10
Looks great. Might be possible to remove some variable tables from those DUBT tests?
#11
could be, I'll look into it, but will be for tomorrow. Other two tests need the field config as well, I'm wondering whether we should just simply install field config always by default in the base unit test ?
#12
+++ b/core/modules/edit/lib/Drupal/edit/Tests/EditTestBase.phpundefined@@ -32,7 +32,7 @@ function setUp() {
// Set default storage backend.
- variable_set('field_storage_default', $this->default_storage);
+ config('field.settings')->set('storage_default', $this->default_storage)->save();
- in EditTestBase and FieldUnitTestBase: Looks like this should be replaced by $this->installConfig(array('field)), and var $default_storage can go away as a class member
- in FieldTestBase: this seems unneeded to begin with, since that is a WebTest, the config will be applied as part of installing field.module ?
+++ b/core/modules/field/field.moduleundefined@@ -468,8 +468,7 @@ function field_entity_field_info($entity_type) {
* By default this is called by field_field_language_alter(), but this
- * behavior can be disabled by setting the 'field_language_fallback'
- * variable to FALSE.
The comment doesn't apply anymore now that this is in state ?
Also, the existence of that comment (encouraging site admins to control the value of the field_language_fallback variable) makes me question whether this should really be in state() :-/
The current code in HEAD overwrites that value on every language enable / disable, but maybe setting $conf['field_language_fallback'] in settings.php was a feature.
We probably want to ping @plach about that.
+++ b/core/modules/field/field.api.phpundefined@@ -1191,11 +1191,11 @@ function hook_field_attach_create_bundle($entity_type, $bundle) {
- $extra_weights = variable_get('field_extra_weights', array());
+ $extra_weights = config('field.settings')->get('extra_weights');
Yeah, so this variable is now long gone, it doesn't seem a good idea to keep a misleading reference to it in sample code, makes you think field module has such a config value.
We should replace the sample code in hook_field_attach_rename_bundle() / hook_field_attach_delete_bundle() with an imaginary use case. Something like :
<?phpfunction hook_field_attach_rename_bundle($entity_type, $bundle_old, $bundle_new) {
// Update the settings associated with the bundle in my_module.settings.
if ($bundle_old !== $bundle_new) {
$config = config('my_module.settings');
$bundle_settings = $config->get('bundle_settings');
if (isset($bundle_settings[$entity_type][$bundle_old])) {
$bundle_settings[$entity_type][$bundle_new] = $bundle_settings[$entity_type][$bundle_old];
unset($bundle_setting[$entity_type][$bundle_old]);
$config->set('bundle_settings', $bundle_settings);
}
}
}
?>
Also, sigh at field_attach_(create|rename|delete)_bundle() and associated hooks still being part of Field API...
We really need to move forward on #1187784: Standardize adding a new bundle to an entity type.
#13
Hm, I guess we'd now need to add the new config value to the config schema that got added in #1919164: Create configuration schemas for field module...
+ Minor: looking at the setting name now, I guess 'default_storage' would make more more sense than 'storage_default' ?
#14
Re-title. A bit less accurate, but easier to find :-)
Also, while #1735118: Change notice: Convert Field API to CMI is not the place to start adding installConfig('field') in all DUTB tests that enable field.module, I guess this issue is ? :-/
#15
Yep, in D7 ET exposes a UI to set the value of that variable. We might want to do that in D8 too, hence I definitely convert it to a configuration setting rather than storing in the state. It's totally something we may want to migrate across environments.
#16
This should be better.
#17
Bah forgot the config schema - give me a few moments.
#18
And the schema - also moved the settings in alphabetical order in the file.
#19
The last submitted patch, 1942346-18.patch, failed testing.
#20
Missed on test.
#21
Thanks for the feedback @plach.
What worries me a bit is to have language enable / disable resulting in changing the value of another setting in field.settings. Config is config, state is state. Such coupling & side effects sound like a recipe for messy deployments.
What I understand from #15 is :
- we want a field.settings.language_fallback config entry; It's just configuration, aka "does the site admin want field language fallback to ever trigger when applicalble". Doesn't ever change unless the admin changes it.
- at runtime, language fallback is triggered or not based on the condition :
(field.settings.language_fallback == TRUE) && (there are more than two languages on the site).
i.e
(a condition on the config defined by the admin) && (a condition on the current runtime state)
Whether the second condition is read directly from language config or precomputed and placed in state(), I don't know for sure.
But IMO we shouldn't denormalize those two conditions in a single value written back in config. Config should be normalized, non redundant, non coupled.
@plach, thoughts ?
#22
Totally agreed, I missed that change. I think what we want is a new API function, something like:
<?phpfunction field_language_fallback_enabled() {
return language_multilingual() && config('field.settings')->get('language_fallback');
}
?>
Then we can remove those two
hook_language_*implementations.#23
Sounds fine.
In this scenario, though, I guess the upgrade path should always set the new field.settings.language_fallback value to TRUE, then.
The sites that will want this to FALSE are the D7 sites that were using settings.php to override the value computed from the state of enabled languages, and they will have to update this piece anyway.
#24
Yep, sounds reasonable.
#25
@yched - do we also want to kill field_bundle_settings variable with this one ?
#26
Hmm - field_bundle_settings still holds what would go in EntityFormDisplays for extra fields, right ?
Then maybe let's not kill it right now ?
#27
Partly - field api cmi will kill the 'display' part, entityform display the 'form' part, but we're still left with the 'view modes' entry, i.e which one has custom settings or not. Maybe we should create a dedicated issue for that, because it might be more complex conversion, depending on how we approach it.
#28
Separate issue ++
#29
done - #1953528: Move 'view modes' part out of field_bundle_settings()
#30
Tagging
#31
rerolling now field api cmi is in
#32
New patch with the new API function, is that ok ?
#33
The last submitted patch, 1942346-32.patch, failed testing.
#34
Ugh, update_8003 -> update_8004
#35
The last submitted patch, 1942346-34.patch, failed testing.
#36
Looks good - well, aside from the test fail :-)
+++ b/core/modules/field/config/field.settings.ymlundefined@@ -1 +1,3 @@
+language_fallback: false
Don't remember exactly, but shouldn't this be true now ?
+++ b/core/modules/field/field.multilingual.incundefined@@ -186,6 +151,13 @@ function field_content_languages() {
/**
+ * Checks whether field language fallback is configured.
Maybe rather "... is enabled."?
#37
I guess so, in D7 it is.
#38
Should be good now.
#39
Looks good to me :-).
Temptative RTBC if it comes back green.
#40
if this one goes in first, #1966998: Add a constant OR use TRUE/FALSE for active storage vs. inactive storage and other properties needs a re-roll.
#41
@@ -35,11 +35,11 @@* - weight: The default weight of the element.
* - visible: The default visibility of the element. Only for 'display'
* context.
- * - edit: (optional) String containing markup (normally a link) used as the
- * element's 'edit' operation in the administration interface. Only for
+ * - edit: (optional) String containing markup (normally a link) used as the
+ * element's 'edit' operation in the administration interface. Only for
* 'form' context.
- * - delete: (optional) String containing markup (normally a link) used as the
- * element's 'delete' operation in the administration interface. Only for
+ * - delete: (optional) String containing markup (normally a link) used as the
+ * element's 'delete' operation in the administration interface. Only for
* 'form' context.
*/
It took me a few reads to realize that these are out-of-scope whitespace cleanups.
+++ b/core/modules/field/field.installundefined@@ -481,6 +481,19 @@ function field_update_8003() {
+ * Moves field_storage_default and field_language_fallback to config.
+ *
+ * @ingroup config_upgrade
+ */
+function field_update_8004() {
+ update_variable_set('field_language_fallback', TRUE);
+ update_variables_to_config('field.settings', array(
+ 'field_storage_default' => 'default_storage',
+ 'field_language_fallback' => 'language_fallback',
+ ));
Does this need an upgrade path test?
+++ b/core/modules/field/field.multilingual.incundefined@@ -64,41 +64,6 @@
-function field_language_insert() {
...
- variable_set('field_language_fallback', TRUE);
...
-function field_language_delete() {
...
- variable_set('field_language_fallback', FALSE);
For the reason these are being removed, see #21, #22, and #23. Does this behavior have test coverage to ensure it works the same as it did previously? Also, I don't actually see where the setting ever actually gets set now?
#42
Re-roll after #1966998: Add a constant OR use TRUE/FALSE for active storage vs. inactive storage and other properties got in (trailing spaces in field.api.php were fixed there).
I *think* we have tests for this behavior yes, but I was hoping that plach could confirm this.
#43
Agreed with the helper for dealing with the language_multilingual case and this is straightforward otherwise.
Committed/pushed to 8.x.
#44
Automatically closed -- issue fixed for 2 weeks with no activity.