Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Status: Active » Needs review
FileSize
8.62 KB
swentel’s picture

Issue tags: +D8MI

Tagging with D8MI as well

swentel’s picture

FileSize
9.99 KB

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

swentel’s picture

Title: Convert field_storage_default and field_language_fallback to CMI » Convert 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.

swentel’s picture

Status: Needs work » Needs review
FileSize
4.28 KB
9.97 KB

Mixing up API's and and epic typo

Status: Needs review » Needs work

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

swentel’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
13.08 KB

Added upgrade path and should fix most of the tests.

Status: Needs review » Needs work

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

Berdir’s picture

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

swentel’s picture

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 ?

yched’s picture

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

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.

yched’s picture

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

yched’s picture

Title: Convert field_storage_default and field_language_fallback to config and state » Convert 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 ? :-/

plach’s picture

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
12.51 KB
15.1 KB

This should be better.

swentel’s picture

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

swentel’s picture

FileSize
1.02 KB
15.78 KB

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
734 bytes
16.49 KB

Missed on test.

yched’s picture

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 ?

plach’s picture

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.

yched’s picture

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.

plach’s picture

Yep, sounds reasonable.

swentel’s picture

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

yched’s picture

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

swentel’s picture

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.

yched’s picture

Separate issue ++

swentel’s picture

swentel’s picture

Issue tags: +Field API

Tagging

swentel’s picture

Assigned: Unassigned » swentel

rerolling now field api cmi is in

swentel’s picture

FileSize
5.57 KB
16.25 KB

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

Status: Needs review » Needs work

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

swentel’s picture

Status: Needs work » Needs review
FileSize
16.25 KB

Ugh, update_8003 -> update_8004

Status: Needs review » Needs work

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

yched’s picture

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

plach’s picture

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

I guess so, in D7 it is.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.41 KB
16.93 KB

Should be good now.

yched’s picture

Status: Needs review » Reviewed & tested by the community

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

swentel’s picture

xjm’s picture

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

swentel’s picture

FileSize
16.06 KB

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.

catch’s picture

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.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.