This will convert field_language_default and storage_default to field.settings.

Related #1953528: Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable

Files: 
CommentFileSizeAuthor
#42 1942346-42.patch16.06 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 54,945 pass(es).
[ View ]
#38 1942346-38.patch16.93 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 54,689 pass(es).
[ View ]
#38 interdiff.txt2.41 KBswentel
#34 1942346-34.patch16.25 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 54,550 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#32 1942346-32.patch16.25 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field/field.install.
[ View ]
#32 interdiff.txt5.57 KBswentel
#20 1942346-20.patch16.49 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 53,422 pass(es).
[ View ]
#20 interdiff.txt734 bytesswentel
#18 1942346-18.patch15.78 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 53,350 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#18 interdiff.txt1.02 KBswentel
#16 1942346-16.patch15.1 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 53,403 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#16 interdiff.txt12.51 KBswentel
#8 1942346-8.patch13.08 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 53,170 pass(es), 0 fail(s), and 8 exception(s).
[ View ]
#8 interdiff.txt3.11 KBswentel
#6 1942346-6.patch9.97 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 52,910 pass(es), 24 fail(s), and 232 exception(s).
[ View ]
#6 interdiff.txt4.28 KBswentel
#3 1942346-3.patch9.99 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 45,435 pass(es), 243 fail(s), and 575 exception(s).
[ View ]
#1 1942346-1.patch8.62 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new8.62 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Issue tags:+D8MI

Tagging with D8MI as well

StatusFileSize
new9.99 KB
FAILED: [[SimpleTest]]: [MySQL] 45,435 pass(es), 243 fail(s), and 575 exception(s).
[ View ]

Missed the 'extra_weights' example in field.api.php (although they are not used in actual code though) and moved language_default to state.

Title:Convert field_storage_default and field_language_fallback to CMIConvert field_storage_default and field_language_fallback to config and state

Better title

Status:Needs review» Needs work

The last submitted patch, 1942346-3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.28 KB
new9.97 KB
FAILED: [[SimpleTest]]: [MySQL] 52,910 pass(es), 24 fail(s), and 232 exception(s).
[ View ]

Mixing up API's and and epic typo

Status:Needs review» Needs work

The last submitted patch, 1942346-6.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.11 KB
new13.08 KB
FAILED: [[SimpleTest]]: [MySQL] 53,170 pass(es), 0 fail(s), and 8 exception(s).
[ View ]

Added upgrade path and should fix most of the tests.

Status:Needs review» Needs work

The last submitted patch, 1942346-8.patch, failed testing.

Looks great. Might be possible to remove some variable tables from those DUBT tests?

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 ?

+++ 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 :

<?php
function 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.

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' ?

Title:Convert field_storage_default and field_language_fallback to config and stateConvert Field API variables to CMI

Re-title. A bit less accurate, but easier to find :-)

Also, while #1735118: 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 ? :-/

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.

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.

Status:Needs work» Needs review
StatusFileSize
new12.51 KB
new15.1 KB
FAILED: [[SimpleTest]]: [MySQL] 53,403 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

This should be better.

Bah forgot the config schema - give me a few moments.

StatusFileSize
new1.02 KB
new15.78 KB
FAILED: [[SimpleTest]]: [MySQL] 53,350 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

And the schema - also moved the settings in alphabetical order in the file.

Status:Needs review» Needs work

The last submitted patch, 1942346-18.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new734 bytes
new16.49 KB
PASSED: [[SimpleTest]]: [MySQL] 53,422 pass(es).
[ View ]

Missed on test.

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 ?

Totally agreed, I missed that change. I think what we want is a new API function, something like:

<?php
function field_language_fallback_enabled() {
  return
language_multilingual() && config('field.settings')->get('language_fallback');
}
?>

Then we can remove those two hook_language_* implementations.

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.

Yep, sounds reasonable.

@yched - do we also want to kill field_bundle_settings variable with this one ?

Hmm - field_bundle_settings still holds what would go in EntityFormDisplays for extra fields, right ?
Then maybe let's not kill it right now ?

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.

Separate issue ++

Issue tags:+Field API

Tagging

Assigned:Unassigned» swentel

rerolling now field api cmi is in

StatusFileSize
new5.57 KB
new16.25 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field/field.install.
[ View ]

New patch with the new API function, is that ok ?

Status:Needs review» Needs work

The last submitted patch, 1942346-32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new16.25 KB
FAILED: [[SimpleTest]]: [MySQL] 54,550 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Ugh, update_8003 -> update_8004

Status:Needs review» Needs work

The last submitted patch, 1942346-34.patch, failed testing.

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."?

Don't remember exactly, but shouldn't this be true now ?

I guess so, in D7 it is.

Status:Needs work» Needs review
StatusFileSize
new2.41 KB
new16.93 KB
PASSED: [[SimpleTest]]: [MySQL] 54,689 pass(es).
[ View ]

Should be good now.

Status:Needs review» Reviewed & tested by the community

Looks good to me :-).
Temptative RTBC if it comes back green.

@@ -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?

StatusFileSize
new16.06 KB
PASSED: [[SimpleTest]]: [MySQL] 54,945 pass(es).
[ View ]

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.

Status:Reviewed & tested by the community» Fixed

Agreed with the helper for dealing with the language_multilingual case and this is straightforward otherwise.

Committed/pushed to 8.x.

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

Updated issue summary.