Problem/Motivation

General problem

Configuration uses dots as a separator. However currently set() and setData() are not consistent in how they apply dots. For example the following two will do the same thing:

// These do the same thing.
$config->set('foo', array('bar' => 12);
$config->set('foo.bar', 12);

$config->get('foo');
// Will be array('bar' => 12) for both.

The following two will result in totally different things.

// These are totally different.
$config->set('foo', array('bar.baz' => 12);
$config->set('foo.bar.baz', 12);

$config->get('foo');
// Will be array('bar.baz' => 12) for the first but array('bar' => array('baz' => 12)) for the second.

This causes problems in two actual use cases now in core.

Field allowed values

Found this problem in #2293063: Config schema mapping parsing is inconsistently forgiving, does not conform to the interface but the scope was too big to solve it there. Allowed values (for a float or string option field for example) appear like this on the user interface:

0.5|Small value
1.5|Bigger value 

This is then stored in configuration as:

allowed_values:
  0.5: 'Small value'
  1.5: 'Bigger value'

This is not addressable with configuration because as shown above configuration uses dot as level separator in the tree. So allowed_values.0.5 is not a meaningful config value identifier. The bug was found with float option values but may also appear with string option values that may also contain dots.

Migrations data

Testing on this issue found that migrations also use dots in key names, eg:

process:
  entity_type: constants.entity_type
  bundle: type
  field_name:
    plugin: migration
    migration: d6_taxonomy_vocabulary
    source: vid
  'settings.allowed_values.0.vocabulary': @field_name
  'settings.allowed_values.0.parent': constants.parent

Proposed resolution

General problem

Because dots in array keys are ambiguous and lead to unexpected consequences, disallow using dots in config data keys on any level. That means using dots will mean the same thing when addressing configuration. If you attempt to set configuration with dotted keys, it will throw an exception.

Field allowed values

Restructure the allowed values list to be a list of associative pairs:

allowed_values:
  - 
    value: 0.5
    label: 'Small value'
  -
    value: 1.5
    label: 'Bigger value'

May have wide reaching API implications.

Migration keys

Replace dots with slashes. So the above becomes:

process:
  entity_type: 'constants/entity_type'
  bundle: type
  field_name:
    plugin: migration
    migration: d6_taxonomy_vocabulary
    source: vid
  'settings/allowed_values/0/vocabulary': @field_name
  'settings/allowed_values/0/parent': 'constants/parent'

Remaining tasks

Review. Change notice. Commit.

User interface changes

None.

API changes

- Allowed value storage on fields will change. Allowed value function callback results change too.
- The migrate format will minimally change from using dots in tree references to slashes.
- set() and setData() on config now explicitly fails with an exception when dotted keys are used in the data array in any way.

Files: 
CommentFileSizeAuthor
#147 interdiff.txt1.65 KBGábor Hojtsy
#147 field_allowed_values-2293773-147.patch27.54 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,079 pass(es). View
#143 interdiff.txt5.99 KBeffulgentsia
#143 field_allowed_values-2293773-143.patch27.52 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,169 pass(es). View
#142 field_allowed_values-2293773-142.patch25.72 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,350 pass(es). View
#142 interdiff.txt6.44 KBhussainweb
#135 interdiff.txt5.22 KBGábor Hojtsy
#135 2293773-fields-storage-135.patch25.21 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,954 pass(es). View
#128 interdiff.txt2.84 KBGábor Hojtsy
#128 2293773-fields-storage-128.patch25.31 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,222 pass(es). View
#125 2293773-fields-storage-125.patch25.18 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,039 pass(es). View
#125 interdiff.txt15.58 KBGábor Hojtsy
#110 2293773-fields-storage-108-same-as-107.patch19.74 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,255 pass(es). View
#110 2293773-fields-api-108-same-as-100.patch36.61 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,082 pass(es). View
#2 2293773-config-dotted-key-exception-2.patch1.21 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,728 pass(es). View
#3 2293773-config-dotted-key-exception-3.patch1.78 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,680 pass(es), 104 fail(s), and 86 exception(s). View
#3 interdiff.txt1.01 KBGábor Hojtsy
#9 2293773-config-dotted-key-not-allowed-9.patch35.29 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,937 pass(es), 66 fail(s), and 0 exception(s). View
#9 interdiff.txt33.52 KBGábor Hojtsy
#11 2293773-config-dotted-key-not-allowed-11.patch36.59 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,894 pass(es), 65 fail(s), and 0 exception(s). View
#11 interdiff.txt1.3 KBGábor Hojtsy
#13 11-13-interdiff.txt1.23 KBalexpott
#13 2293773-alt.13.patch36.74 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,881 pass(es), 5 fail(s), and 0 exception(s). View
#16 2293773-16.patch37.38 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,749 pass(es), 5 fail(s), and 0 exception(s). View
#16 interdiff.txt1.44 KBGábor Hojtsy
#19 2293773-19.patch36.56 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,750 pass(es), 5 fail(s), and 0 exception(s). View
#20 2293773-20.patch53.25 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2293773-20.patch. Unable to apply patch. See the log in the details link for more information. View
#20 interdiff.txt16.69 KBGábor Hojtsy
#23 2293773-23.patch53.3 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,781 pass(es), 16 fail(s), and 8 exception(s). View
#25 2293773-25.patch61.04 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,931 pass(es), 167 fail(s), and 965 exception(s). View
#25 interdiff.txt13.67 KBGábor Hojtsy
#29 25-28-interdiff.txt9.71 KBalexpott
#29 2293773-new.28.patch69.08 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,796 pass(es). View
#34 28-34-interdiff.txt12.9 KBalexpott
#34 2293773-new.34.patch72.5 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,775 pass(es). View
#35 2293773-new.35.patch74.36 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,824 pass(es). View
#35 interdiff.txt5.03 KBGábor Hojtsy
#36 2293773-new.36.patch90.56 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,063 pass(es), 5 fail(s), and 2 exception(s). View
#36 interdiff.txt29.68 KBGábor Hojtsy
#38 2293773-new.38.patch90.89 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,812 pass(es), 8 fail(s), and 0 exception(s). View
#38 interdiff.txt859 bytesGábor Hojtsy
#42 interdiff.txt1.45 KBpenyaskito
#42 2293773-new.42.patch92.33 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,862 pass(es). View
#46 interdiff.txt3.92 KBGábor Hojtsy
#46 2293773-new.46.patch94.83 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,931 pass(es). View
#63 2293773-new.63.patch94.72 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,897 pass(es). View
#64 interdiff.txt5.57 KBGábor Hojtsy
#64 2293773-new.64.patch94.68 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,192 pass(es), 808 fail(s), and 207 exception(s). View
#66 2293773-new.66.patch94.64 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,911 pass(es). View
#66 interdiff.txt3.06 KBGábor Hojtsy
#75 2293773-new.75-but-will-not-work-at-all.patch70.55 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#75 full-interdiff.txt39.47 KBGábor Hojtsy
#75 interesting-interdiff.txt7.89 KBGábor Hojtsy
#77 FieldConfigStorage.png36.13 KBGábor Hojtsy
#79 2293773-79-migrate-only.patch52.87 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,113 pass(es). View
#79 migrate-only-interdiff.txt638 bytesGábor Hojtsy
#84 2293773-84-migrate-only.patch52.87 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,162 pass(es). View
#84 interdiff.txt1.33 KBGábor Hojtsy
#89 2293773-fields-storage-89-rerolled-from-75-will-not-work.patch17.91 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#89 2293773-fields-api-89-rerolled-from-66.patch42 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,314 pass(es). View
#99 2293773-fields-api-99.patch37.28 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,443 pass(es). View
#100 interdiff.txt2.12 KBGábor Hojtsy
#100 2293773-fields-api-100.patch36.51 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,453 pass(es). View
#102 2293773-fields-storage-102.patch15.66 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#104 2293773-fields-storage-104.patch14.68 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,421 pass(es). View
#104 interdiff.txt7.35 KBGábor Hojtsy
#105 2293773-fields-storage-105.patch14.55 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,558 pass(es). View
#105 interdiff.txt1.39 KBGábor Hojtsy
#106 interdiff.txt3.94 KBGábor Hojtsy
#106 2293773-fields-storage-106.patch17.58 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,619 pass(es). View
#107 2293773-fields-storage-107.patch19.74 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,394 pass(es). View
#107 interdiff.txt2.16 KBGábor Hojtsy
#108 2293773-fields-api-108-same-as-100.patch36.51 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,388 pass(es). View
#108 2293773-fields-storage-108-test-only.patch6.44 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,374 pass(es), 8 fail(s), and 0 exception(s). View
#108 2293773-fields-storage-108-same-as-107.patch19.74 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,449 pass(es). View

Comments

alexpott’s picture

Issue tags: +beta blocker

This is a config data structure change and would require a tricky upgrade path. Therefore changing to be a beta blocker since using dotted keys in configuration is very problematic.

This issue should also add a check to ConfigBase::set() to ensure that we don't have further use of dotted keys.

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
1.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,728 pass(es). View

Note sure we can avoid the use of dotted keys in set(), that one has this code in ConfigBase:

  public function set($key, $value) {
    // The dot/period is a reserved character; it may appear between keys, but
    // not within keys.
    $parts = explode('.', $key);
    if (count($parts) == 1) {
      $this->data[$key] = $value;
    }
    else {
      NestedArray::setValue($this->data, $parts, $value);
    }
    return $this;
  }

I think the place we can add protection is setData(). What about doing that. Just throwing plain exceptions for now, let's see what we get.

Gábor Hojtsy’s picture

FileSize
1.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,680 pass(es), 104 fail(s), and 86 exception(s). View
1.01 KB

@alexpott points out set() may still have dotted keys in $value. Let's ensure that is not happening.

xjm’s picture

Hm, yikes! Agreed, this is definitely critical and beta-blocking.

Status: Needs review » Needs work

The last submitted patch, 3: 2293773-config-dotted-key-exception-3.patch, failed testing.

larowlan’s picture

Another win for config schemas, great find

Gábor Hojtsy’s picture

Title: Field allowed value storage is not configuration system compatible, allows dots » Field allowed values and migrations use dots in key names - not allowed in config
Issue summary: View changes

Retitled and updated with the migrate use case.

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
35.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,937 pass(es), 66 fail(s), and 0 exception(s). View
33.52 KB

Here is a first pass at fixing this for migrations, which is the main body of exceptions/fails. In discussion with @chx, he was fine with changing from dot as the item separator to a slash. I only changed it here for destination keys, not source back-references (because destination keys are what actually is incompatible with config, source back-references are in values not keys). For consistency, we may want to change both.

BTW noticed the funny coexistence of the dotted back-reference with the dotted allowed_values, if both would stay as-is, you could never write a migration for concrete floating point values, ie. settings.allowed_values.0.5.xyz cannot be decoded to belong to the right level. Fun stuff.

Status: Needs review » Needs work

The last submitted patch, 9: 2293773-config-dotted-key-not-allowed-9.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
36.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,894 pass(es), 65 fail(s), and 0 exception(s). View
1.3 KB

The RowTest fail is simple. The others are manipulations not happening on the source values before they end up in destination. I'm not sure where or how those manipulations would happen but the key changes are obviously not making them run anymore. Grepping for parts of the keys did not help...

Status: Needs review » Needs work

The last submitted patch, 11: 2293773-config-dotted-key-not-allowed-11.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
36.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,881 pass(es), 5 fail(s), and 0 exception(s). View

I think this fixes the rest of the migrate tests.

Status: Needs review » Needs work

The last submitted patch, 13: 2293773-alt.13.patch, failed testing.

Gábor Hojtsy’s picture

+++ b/core/modules/migrate/src/Row.php
@@ -198,8 +203,8 @@ public function hasDestinationProperty($property) {
+    $this->rawDestination[str_replace($this::DESTINATION_PROPERTY_SEPARATOR, '.', $property)] = $value;

I see this fixed the fails, but why is this a good fix? Eg. later on we call getRawDestination() and getting slash delimited things, and here we set dot delimited things.

This is very confusing to save *raw* values that are processed from the actual raw values and then have methods that access the actual raw values and not what we saved processed on the raw values array, see:

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
@@ -75,7 +75,7 @@ public function getIds() {
     foreach ($row->getRawDestination() as $property => $value) {
-      $this->updateEntityProperty($entity, explode('.', $property), $value);
+      $this->updateEntityProperty($entity, explode(Row::DESTINATION_PROPERTY_SEPARATOR, $property), $value);

I'll provide a different fix then that converts the separator to dots in the Config destination.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
37.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,749 pass(es), 5 fail(s), and 0 exception(s). View
1.44 KB

Here is the other version of the fix where the raw destination is the raw destination :) BTW thanks for pointing me to the right direction with the patch, helped me figure this out.

alexpott’s picture

How about replacing the / with ][ - yes we lose a character but it is more array like and thats what we need. Plus could there ever be a legitimate usage of /? Actually probably not, they're not legal characters in php variables names - so this comes down to preference. I'm not 100% behind either... dots look so much nicer :)

Status: Needs review » Needs work

The last submitted patch, 16: 2293773-16.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
36.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,750 pass(es), 5 fail(s), and 0 exception(s). View

Reroll folllowing #2293063: Config schema mapping parsing is inconsistently forgiving, does not conform to the interface which fixed a migrate bug. No need to change migrate.migration.d6_vocabulary_field_instance.yml here, no keys in there with dots. No interdiff.

@alexpott: as for the syntax of the array, I don't have strong opinions. It is relatively easy to fix, especially with this patch now making all items use a single quoted key, it will be easy to grep for '.+/.+' in migration.migrate.* to find and change them. There are inconsistencies with missing quotes and double quotes that the patch fixes :)

Gábor Hojtsy’s picture

FileSize
53.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2293773-20.patch. Unable to apply patch. See the log in the details link for more information. View
16.69 KB

There is no lack of change needed for the options/list element either. Here is a first set of changes in the options/list base classes, schemas and the options UI test. This will most probably blow up at several places. Note that this change also allows us to provide types for the actual values for the options (int, string, float, etc. which is kind of neat).

Status: Needs review » Needs work

The last submitted patch, 20: 2293773-20.patch, failed testing.

The last submitted patch, 19: 2293773-19.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
53.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,781 pass(es), 16 fail(s), and 8 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 23: 2293773-23.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
61.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,931 pass(es), 167 fail(s), and 965 exception(s). View
13.67 KB

Now with almost no fails, at least on the things that were failing before.

1. The forum container default settings were not compatible with the new schema, makes sense.
2. Formatters for options were not updated earlier. Essential :)
3. Boolean item config and float item config classes updated.
4. Make the code still conform to the allowed values interface.
5. OptionsFieldTest also had a bunch of structures that need to be updated.
6. BEST of all OptionsFieldUITest.php now needs to provide test data in a type cohesive way, eg. (float) 1 for equal value assertions to pass.

This still produces one fail in OptionsFieldUITest for checking existing values. That is very oddly working for me, not sure why/how... Probably will not resolve it anymore today. Will pick it up tomorrow unless someone beats me to it. Hopefully this one is the last remaining fail.

The main question on this one is when do we convert from/to the structured allowed values vs. a simple array. Now I use a static method on ListItemBase that is called where I think its needed to convert to a simpler form.

effulgentsia’s picture

@alexpott: as for the syntax of the array, I don't have strong opinions.

How about a :, since that's the hierarchy separator character of YAML?

Status: Needs review » Needs work

The last submitted patch, 25: 2293773-25.patch, failed testing.

benjy’s picture

RE: #26, we already moved from ":" to a "." See #2185575: Change : to .

alexpott’s picture

Status: Needs work » Needs review
FileSize
9.71 KB
69.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,796 pass(es). View
alexpott’s picture

+++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
@@ -221,12 +221,42 @@ protected static function extractAllowedValues($string, $has_data) {
   public static function simplifyAllowedValues(array $structured_values) {
     $values = array();
     foreach ($structured_values as $item) {
-      $values[$item['value']] = $item['label'];
+      if (is_array($item['label'])) {
+        $item['label'] = static::simplifyAllowedValues($item['label']);
+      }
+      // Cast the value to a string so that values like 0 and 0.5 are not
+      // treated as equal.
+      $values[(string) $item['value']] = $item['label'];
     }
     return $values;
   }

The casting to string of the key values was the bit that got Drupal\options\Tests\OptionsFieldUITest to pass.

alexpott’s picture

More on #30

  return ListItemBase::simplifyAllowedValues(array(
      array('value' => (float) 0, 'label' => 'Zero'),
      array('value' => (float) 0.5, 'label' => 'Point five')
  ));

without the string cast returns

  array(0 => 'Point five');
chx’s picture

I am unhappy that source and destination now have different separators.

Gábor Hojtsy’s picture

Re #31, this looks to be a PHP thing, as per http://php.net/manual/en/language.types.array.php Floats are also cast to integers, which means that the fractional part will be truncated. E.g. the key 8.7 will actually be stored under 8.. Looks like floats are the only type that loose values. So this was the reason for the fail I was seeing on the Options UI test. Looks like no better solution is available but to cast to string. I think this would be important to note as a code comment because otherwise it looks odd.

  1. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
    @@ -221,12 +221,42 @@ protected static function extractAllowedValues($string, $has_data) {
    +      if (is_array($item['label'])) {
    +        $item['label'] = static::simplifyAllowedValues($item['label']);
    +      }
    

    Under what circumstances would it be like this? That sounds like a very odd situation to embed the whole allowed values array in a 'label' key?!

  2. +++ b/core/modules/options/src/Tests/OptionsFieldUITest.php
    @@ -165,14 +165,14 @@ function testOptionsAllowedValuesFloat() {
    -    $string = "0|Zero\n.5|Point five\n2|Two";
    +    $string = "0|Zero\n0.5|Point five\n2|Two";
    ...
    -    $string = "0|Zero\n.5|Point five";
    +    $string = "0|Zero\n0.5|Point five";
    

    It is strange that we need these? I think part of the test is to prove .5 entered on the UI is canonicalized and gets turned into 0.5:

             // Float keys are represented as strings and need to be disambiguated
             // ('.5' is '0.5').
            $item['value'] = is_numeric($item['value']) ? (string) (float) $item['value'] : $item['value'];
    

@chx, @benjy: we can clearly change the source separator as well, not a problem, although it is definitely not required to fix this bug it will be better consistency. I guess we need to agree on a separator then :)

alexpott’s picture

FileSize
12.9 KB
72.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,775 pass(es). View

1. Have a look at options_test_allowed_values_callback() - we can have groups in the options.
2. Fixed... this turns out to be a can of worms caused by the PHP thing @Gábor Hojtsy mentions in #33.

Gábor Hojtsy’s picture

FileSize
74.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,824 pass(es). View
5.03 KB

The changes make sense, thanks for fixing those. The changes in #34 are especially important because we want a canonical format of the number, and then can only be one of them. Added some more coverage to tests here on that.

Attached updates:

- added docs to why the 'label' array is used for nesting, at least where to look how its structured
- now that duplicate key protection is not native (it is not an array keyed by that), added validation for it
- added tests to integer, float, text, etc. to make sure it fails proper; added floats with different forms .5 and 0.5 to check it fails and text with extra spaces but otherwise same
- the pre-patch codebase does not let text options with string values containing dots for the same reason this is a problem for floats; added tests to validate it now works (and it of course does :)

Would love to move on to the migrate separator question but not 100% sure how to proceed. I can expand the source separator to be a slash as well for now and then we can still change it later :)

Gábor Hojtsy’s picture

FileSize
90.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,063 pass(es), 5 fail(s), and 2 exception(s). View
29.68 KB

All right, applying the property separator to the source values as well. This is not at all required to resolve this issue but I agree it is a DX regression to apply different separators to the source and destination references. This applies / as well as for the destination and quotes those values the same way for consistency. Spot-checked running a few tests which relied on source values and passed. May still fail on things.

Status: Needs review » Needs work

The last submitted patch, 36: 2293773-new.36.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
90.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,812 pass(es), 8 fail(s), and 0 exception(s). View
859 bytes

Fixed the field formatter errors which are also the reason for the D6 general migrate fail. Did not yet figure out the CCK migrate fail, I think that one will fail only now.

Status: Needs review » Needs work

The last submitted patch, 38: 2293773-new.38.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config, +sprint

Also add on D8MI sprint since this affects the allowed values label translatability for floats which is ATM not possible on D8 due to temporary fix in #2293063: Config schema mapping parsing is inconsistently forgiving, does not conform to the interface.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
2.05 KB
92.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,843 pass(es). View

Gábor asked me to try to fix the CCK failures in #38. This is passing locally.

penyaskito’s picture

FileSize
1.45 KB
92.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,862 pass(es). View

#41 had an unrelated change. Please use this one (also the interdiff)

Gábor Hojtsy’s picture

Oh, those ones, I did not find. Good catch! Thanks a lot!

Now reviews and let's get this in :)

chx’s picture

I considered asking for Row::SEPARATOR but that totally looks like a line break. Row::PROPERTY_SEPARATOR is fine (even if long winded a bit; but then again most users never need to know the name for the constant).

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Config/ConfigBase.php
@@ -160,13 +160,39 @@ public function get($key = '') {
+   * @throws \Exception
...
+        throw new \Exception(String::format('@key key contains a dot which is not supported.', array('@key' => $key)));

@@ -176,10 +202,16 @@ public function setData(array $data) {
+   * @throws \Exception

Can we throw an exception that extends ConfigException here? Plus this exception needs tests.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.92 KB
94.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,931 pass(es). View

Very good point on the tests. In fact this uncovered a missing change, Config::setData() did not call the parent, so was not protected. Now it calls the parent code and is tested as working :) This may in fact uncover other issues with setData() that did not fail before. Ha!

Gábor Hojtsy’s picture

Issue summary: View changes

So that proves it works even in test scenarios :) Any other concerns? I am not aware of any.

Updated the issue summary and working on a change notice.

Gábor Hojtsy’s picture

First change record on general Config::set() and Config::setData() changes: https://www.drupal.org/node/2297311 Still doing one for the fields and one for migrations.

Gábor Hojtsy’s picture

Change notice for field allowed values, storage and the API: https://www.drupal.org/node/2297369
Migrations: https://www.drupal.org/node/2297375

Plenty of change notice coverage, right? :) Any other concerns?

penyaskito’s picture

Read the three change records, and they look clear to me.

yched’s picture

Sorry for kicking in this late, esp. with reservations on the approach :-/

  1. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
    @@ -255,4 +255,36 @@ public function instanceSettingsForm(array $form, array &$form_state) {
    +  public static function simplifyAllowedValues(array $structured_values) {
    
    +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
    @@ -271,4 +271,30 @@ public function settingsForm(array &$form, array &$form_state, $has_data);
    +  public static function simplifyAllowedValues(array $structured_values);
    

    I'm really reluctant to put those in such a generic place, as if they made sense for every field.

    - "allowed values" is not a generic Field API feature, it's only something that some field types chose to provide, and each build their "allowed values" in the way that specifically makes sense for them.

    - For the field types provided by options.module (List*Item), this logic happens to be based on an 'allowed_values' setting, containing litteral values, and whose structure happens to be problematic to store in YAML as is.
    So the logic implemented in the patch's FieldItemBase::(simplify|structure)AllowedValues() are nothing generic, but all specific to the 'allowed_values' of List*Item fields.

    FWIW, Taxo terms also have a setting (poorly) named 'allowed_values', but its content and structure have nothing in common, and are not problematic for YAML storage.

    So, if anything, these methods should live in option.module's ListItemBase, and used strictly within options.module ?

  2. +++ b/core/modules/options/options.module
    @@ -99,7 +101,11 @@ function options_field_config_update_forbid(FieldConfigInterface $field, FieldCo
    -    $lost_keys = array_diff(array_keys($prior_allowed_values), array_keys($allowed_values));
    +    $type_definition = \Drupal::typedDataManager()->getDefinition('field_item:' . $field->getType());
    +    $lost_keys = array_diff(
    +      array_keys($type_definition['class']::simplifyAllowedValues($prior_allowed_values)),
    +      array_keys($type_definition['class']::simplifyAllowedValues($allowed_values))
    +    );
    
    +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
    @@ -33,7 +78,8 @@ public function viewElements(FieldItemListInterface $items) {
    -    $allowed_values = options_allowed_values($this->fieldDefinition, $entity);
    +    $type_definition = $this->typedDataManager->getDefinition('field_item:' . $this->fieldDefinition->getType());
    +    $allowed_values = $type_definition['class']::simplifyAllowedValues(options_allowed_values($this->fieldDefinition, $entity));
    

    Ew. Not exactly friendly :-)

    Would be somewhat alleviated if, as mentioned above, the methods lived in ListItemBase, but :

We already have AllowedValuesinterface that is supposed to abstract an API for "working with allowed values". Now, outside of this, the patch introduces two different array-based "formats" for lising allowed values, a "simple' one and a "structured" one, with a painful back and forth dance between the two at various places depending on who needs what at runtime.

Since the issue is strictly related to config storage, it's a bit sad that the spills over to changing runtime structures. Having a way to let field types massage their settings in and out of storage would be very precious :
- list field types could use it to convert their 'allowed values' in a storage-friendly format
- taxo term fields could use it to switch their 'term id' with a UUID (their 'allowed_values' stores a vocabulary name and optionally a term id to restrict to a subtree)
- file / image fields, that use settings to build their own, non generic formulation of "default value", could also switch between a local fid and a uuid.

(the last two points above are currently outstanding "local IDs in config files" issues in HEAD)

hussainweb’s picture

I was just tracing how the patch evolved and found the reason for the issue described by @yched in #51.1. It seems that those methods (simplify|structure)allowedValues did live in ListItemBase initially but they were moved to FieldItemBase in #34. There is no reason mentioned there but as far as I can see from the patch, the reason seems to be that in many places, these methods are called on a class determined by code. See this for one example of such change.

+++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
@@ -34,7 +78,8 @@
-    $allowed_values = ListItemBase::simplifyAllowedValues(options_allowed_values($this->fieldDefinition, $entity));
+    $type_definition = $this->typedDataManager->getDefinition('field_item:' . $this->fieldDefinition->getType());
+    $allowed_values = $type_definition['class']::simplifyAllowedValues(options_allowed_values($this->fieldDefinition, $entity));

I am not too sure but this could be the reason why those methods had to be moved to FieldItemBase?

Having said that, I agree that it either needs to be made generic to live in FieldItemBase (so that it could apply to other scenarios as well) or figure out some other way to use an interface and the above method.

Gábor Hojtsy’s picture

@yched/@hussainweb: The reason it went to FieldItemBase is because not all of the options classes extend ListItemBase. ListFloatItem, ListIntegerItem and ListTextItem do, BUT ListBooleanItem.php don't. If there is no good reason for that, we can make it extend from ListItemBase and then we can move these back there.

@yched:

the patch introduces two different array-based "formats" for lising allowed values, a "simple' one and a "structured" one, with a painful back and forth dance between the two at various places depending on who needs what at runtime.

The "simple" one is not introduced by this patch, it is the current HEAD behaviour. That kind of list is needed for eg. the form API, so we cannot avoid dealing with it. Originally I thought about introducing some kind of conversion layer on reading/writing the setting but that sounded very un-config entities. I mean we don't have a storage concept for config entities between config and the entity to do conversions like that. So when you do $field->getSetting() it would be strange to not get what was stored, no? Especially if you set the setting, that it would store it in a different way. And once the setting is transparent, it would be very strange if the allowed values callback function would use a different format... So it got to the point where we need the simpler array for form API and validation, we use it, otherwise its transparent like the other settings.

I see how it would look simpler on the surface to do the storage reformatting magic inside ListItemBase (if it can be used as a real base class for all the list classes), but is that something people expect would happen?

Berdir’s picture

Yes, ListBooleanItem was explicitly refactored away from ListItemBase, with the goal to merge it with the core BooleanItem field type.

Gábor Hojtsy’s picture

@berdir: Ha! It would be sad to need to postpone this beta blocker on that refactoring though :/

What I found in the meantime is that looks like the main difference between ListBooleanItem and ListItemBase are:

- Nested options not allowed in booleans
- ListItemBase includes the following that would not be useful at all in boolean items:

  abstract protected function allowedValuesDescription();
  public static function validateAllowedValues($element, &$form_state) { ... }
  protected static function extractAllowedValues($string, $has_data) { ... }
  protected static function validateAllowedValue($option) { }
  protected function allowedValuesString($structured_values) { ... }

The implemented methods would probably be fine lingering around, the need to implement the abstract method would be bad for ListBooleanItem.

So if we want to keep them inherit from the same thing,probably a better way is to remove the non-common elements from ListItemBase, extend ListBooleanItem from ListItemBase and add one more common class between the float/int/text classes and ListItemBase to contain their common features. Not sure about the name for that though :D ListItemTextInputBase? All the methods are around dealing with the common text input UI of the fields.

hussainweb’s picture

I agree with the idea of an intermediate class with the necessary methods (including simplify/structure methods). It sounds like a quick fix compared to refactoring once we figure out the name.

Gábor Hojtsy’s picture

@hussainweb: the simplify/structure methods would be on the ListItemBase then, which all types could inherit from including boolean and the non-boolean types would have one more intermediary abstract class.

Another way to approach this is to put the two methods into a trait and use it in ListItemBase and ListBooleanItem (and still override one of the methods as-is now in ListFloatItem).

andypost’s picture

+1 to trait, suppose AllowedValuesTrait

hussainweb’s picture

Yes, I re-read your description and agree that it would be on ListItemBase. On the other hand, I was working out the trait method too. I think it fits the case.

However, if we are implementing it via trait, I think we should declare these methods in an interface. Which interface should it be? I am checking on AllowedValuesInterface to see if adding it there would impact something else (and if it actually makes sense).

hussainweb’s picture

I just checked AllowedValuesInterface and see there are five classes that implement it (including ListItemBase and ListBooleanItem). Will the simplify/structure methods make sense in those classes?

Gábor Hojtsy’s picture

No, we did not modify how AllowedValueInterface behaves/is defined because it goes so much farther. In fact the simplified array is used when returning within methods of that interface for conformance to the interface. This way of storing the allowed values is only applicable to options because they need a key-value pair system.

hussainweb’s picture

@Gábor Hojtsy: Yes, I see. I was just wondering if it's a good idea to implement a trait without matching it's methods from an interface. If adding a new interface is an overkill here, we can just add the intermediate class you mentioned in #55. Thoughts?

Gábor Hojtsy’s picture

FileSize
94.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,897 pass(es). View

First a reroll because #46 did not apply anymore.

Gábor Hojtsy’s picture

FileSize
5.57 KB
94.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,192 pass(es), 808 fail(s), and 207 exception(s). View

Put it into a trait named ListItemAllowedValuesTrait. We can add the intermediate class instead but according to @berdir the code is going in the opposite direction now to decouple as far as I read.

We still need to discuss whether we want to do some storage-adaptation magic instead. Overriding getSetting()/setSetting() in itself may suffice, if we are fine tests needing to set the more complex structure and we can rely on everything working with getSetting()/setSetting().

@yched? @berdir?

Status: Needs review » Needs work

The last submitted patch, 64: 2293773-new.64.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
94.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,911 pass(es). View
3.06 KB

Some things still referred explicitly to FieldItemBase, refer to ListItemBase instead. They all depend on options module so the class will be there.

Gábor Hojtsy’s picture

So looked into how could we encapsulate the "storage specific" settings transformation for allowed values in getSettings()/setSettings(). The main problem seems to be that only getSettings() is on FieldItemBase (and therefore used/inherited by all list fields). setSettings() is directly on FieldDefinition which in turn delegates it to FieldItemDataDefinition which inherits the implementation from DataDefinition. FieldDefinition::create() et. al. do not seem to provide a way to use anything for the field definition but FieldItemDataDefinition().

So one option is to completely subclass FieldDefinition and use ListFieldDefinition or something like that ONLY for the list fields. All other field types seem to be fine using FieldDefinition direct as far as I see(?). Still then the question is how the APIs would be different on ListFieldDefinition vs. FieldDefinition. Eg. setSetting('allowed_values', ...) and getSetting('allowed_values') would do some custom code and only the rest of the settings would be delegated. But what would happen in setSettings(array $settings) and getSettings()? We would single out the allowed values and do conversion there? Is it good DX that you create all fields with FieldDefinition::create() EXCEPT if its a list field when you need to use a subclass?

The other option is to not even make the field settings data structure aware that this is happening and add some code to FieldItemBaseFieldConfig to delegate to the field type on preSave() and postLoad() to convert to/from the data structure for the settings, so the config required data structure is purely only used for right when needed to be stored and right when loaded. Not sure that this forced conversion all the time instead of when the data actually is needed is a good idea but it would be the most magic solution definitely.

yched’s picture

Sorry, hard for me to provide quick feedback atm :-/
Below is about patch #66 and the "simplifyAllowedValues()" approach. Still processing #67.

Yay for moving the methods out of FieldItemBase.
But : actually looking at the trait-based patch, it seems static methods in a utility class in options/lib/Utility would be equally verbose and much simpler (no need to go through Typeddata to get the correct class). A trait with only static methods (when no class needs to override any of the methods, as is the case here) is simpler as a standalone helper class :)
(sorry, it was me who suggested to move the methods into ListItemBase, and I forgot booleans didn't subclass it, which lead to a Trait)

+++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
@@ -33,7 +78,8 @@ public function viewElements(FieldItemListInterface $items) {
-    $allowed_values = options_allowed_values($this->fieldDefinition, $entity);
+    $type_definition = $this->typedDataManager->getDefinition('field_item:' . $this->fieldDefinition->getType());
+    $allowed_values = $type_definition['class']::simplifyAllowedValues(options_allowed_values($this->fieldDefinition, $entity));

The formatter was written before AllowedValuesInterface was introduced, but that's what AllowedValuesInterface::getPossibleValues() is here for.
Now that we have AllowedValuesInterface, any code that needs to know what the "allowed values for a field" should use those methods rather than dedicated code. Here it would result in much simpler code - something like :

if (count($items)) {
  $allowed_values = $items->first()->getPossibleValues();
  foreach ($items ...) { // do your thing, formatter. }
}
yched’s picture

re #67 : yeah, I don't think messing with getSetting() / setSetting() is the right approach.

The other option is to not even make the field settings data structure aware that this is happening and add some code to FieldItemBaseFieldConfig to delegate to the field type on preSave() and postLoad() to convert to/from the data structure for the settings, so the config required data structure is purely only used for right when needed to be stored and right when loaded.

Yup, that's more like what I was thinking of - call static methods on the FieldItem class to massage 'settings' before writing into & after reading from config store. Would be done either in FieldConfig & FieldInstanceConfig, or maybe rather in the corresponding Storage classes (it's strictly a matter of storage...)

At any rate, we don't want to do this as part of Field(Instance)Config::toArray(), since that one is also used when the definitions get serialized into EntityManager's field definitions cache, and we don't want to have to massage back when the definitions are fetched from the cache.

yched’s picture

Regarding ListBooleanItem :
IIRC, it was left out of ListItemBase in #2169983: Move type-specific logic from ListItemBase to the appropriate subclass mostly because it's just much simpler than the other "list" types and would have had to override most of ListItemBase anyway.

Then, yes, it would be really nice to merge it with Core's BooleanItem and stop having almost-duplicate field types (one of which currently has no widget / formatter). Although, looking back at the code here made me realize there's a roadblock I hadn't thought about so far :-/ - added a remark in #2226063: Merge ListBooleanItem from options module into BooleanItem.

Gábor Hojtsy’s picture

@yched: no we DO need to override the static method and we do in ListFloatItem because floats cannot be left their own types to index arrays in PHP. So we cannot move this to a generic utility class, that is why it ended up on FieldItemBase in the first place. So if we are to move this to the storage level class to FieldConfigStorage, we would need to include the type conditional conversions there instead of the type specific field class. Is that what you had in mind?

Gábor Hojtsy’s picture

@yched: thinking about this more, on one hand, you propose this is not a field level feature (ie. it should not be in FieldItemBase), on the other hand you propose it should be in field config storage logic. If that is to be in the generic FieldConfigStorage, that would be even lower level compared to what we did in FieldItemBase, ie. we would hardcode assumptions about options fields specifically in FieldConfigStorage, so we either need to delegate from there or subclass the storage class for options fields. Looking at the FieldType annotation, it does not seem to be possible to specify storage classes per field type? Is that possible? If not, what do we delegate the conversion of the data structure to? The field type somehow would need to specify that.

yched’s picture

Oh, right, sorry, I overlooked ListFloatItem::simplifyAllowedValues(). Damn. I hate ListFloatItem.

if we are to move this to the storage level class to FieldConfigStorage, we would need to include the type conditional conversions there instead of the type specific field class. Is that what you had in mind?

No, we would add new static methods to FieldItemInterface :
preSaveSettings(array &$settings),
postLoadSettings(array &$settings),
preSaveInstanceSettings(array &$settings),
postLoadInstanceSettings(array &$settings),
and FieldConfigStorage / FieldInstanceConfigStorage would call them at the right time on FC / FIC load and save ?

Ultimately, it's still each FieldItem class that knows how its settings should be massaged.
The methods are just more generic than the simplifyAllowedValues() / structureAllowedValues() methods the current patch adds, and will let us solve other issues we currently have besisdes "just" the specific case of 'allowed_values' on List fields.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy

Working on this one.

Gábor Hojtsy’s picture

FileSize
70.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
39.47 KB
7.89 KB

Ok this sounded very simple so I was very confident it would be easy as pie :D Was I so damn wrong :D See interesting-interdiff.txt for the more interesting parts.

1. Its fine to add this very consistent looking extension to FieldItemInterface and base implementations to FieldItemBase, easy.

2. Now the question is where to invoke postLoad; you said bake into FieldConfigStorage, ok that is a ConfigEntityStorage which is an EntityStorageBase, so it has built-in postLoad, cool. BUT that is loaded after the entities are instantiated. We could easily add some code to do this before the entities are instantiated, but no point because we need to delegate all the way to the field item class, which we cannot trace to until we have the config entity and we can inspect the data for it. So we need to have the entity with settings in a form that are not valid in the entity and then convert it quickly. Heh! So they will exist for a little while before their settings go under transformations. Not very logical. Also field config entities do not have provisions to get their configurable settings only, they merge in some default settings, etc, so when I *get* the setting, I get a whole lot of things. There is no setSettings(), which I could still introduce, but then I set the settings back I may overwrite things not intended. So I referenced $entity->settings direct in postLoad which is a big no-no.

3. The preSave is even more of an interesting question. We want to transform the settings *without* changing anything on the runtime entity this time, so we need to bake this into doSave() from ConfigEntityBase somehow, once its just bare config and not an entity anymore. BUT the entity knows its field instance class, so we need the entity as well as the pure data. So the API is very different from the postLoad. So I added a preSaveData() method for this use case. I went back to try to mirror this with postLoadData() but as explained above we do need the entity instantiated to look up its field item class, so no luck there.

4. I am still not convinced this is "storage specific" since we are putting in storage transformation methods into the field item classes which are supposedly storage independent. The code shows how much sweat this requires, we need to make the getFieldItemClass() on the FieldConfig public so we can call it back from the storage so we can reach the field item class and invoke its transformation methods for storage.

4. To implement your suggested API of specific postLoadSettings() and preSaveSettings() methods with a settings array, the storage class *needs* to assume that a setting array exists to begin with (it will get all the fails if the settings array is not there or not an array, etc.). So this bakes in certain knowledge of the field config structure itself to the storage that was not there at all before. At least the very different save and load behaviors can be theoretically masked in FieldConfigStorage so the rest of the API looks unified.

I did not manage to make this work yet, it will fail in huge flames. Also did not restore the new tests in options UI test. It breaks settings arrays somehow I could not figure out :/ But wanted to post for discussion because it is pointless for me to work on this if I'm totally on the wrong path, which I suspect with so many concerns about the hoops that are required in my understanding to implement your suggestion.

Status: Needs review » Needs work

The last submitted patch, 75: 2293773-new.75-but-will-not-work-at-all.patch, failed testing.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned
FileSize
36.13 KB

To illustrate the problem, you propose we add logic into FieldConfigStorage so that transforms can be made right before storage and right after load. Those need to be dependent on logic that is 2 levels up, via the field config entity to the field item itself (code of which depends on the field type on the config entity). So we would keep the storage and field config generic and delegate the ultimately config dependent storage logic all the way up to the field item level that would be generic for storage but not generic for field type because we need a place to have this logic that both depends on field type and storage. (Even more confusing, the settings alteration would run for other storages as well because the pre/post hooks are generic and not something like postLoadSettingsIfStoredInConfigStorage(). So I don't really see that we reach the goal of not transforming it all the time, just maybe that the structure would not show up on other levels but only at very early load and late save.

Review needed.

effulgentsia’s picture

I don't think we need to think of this as a storage-dependent problem. I think it's okay for us to say that part of the Field API is that $settings may not have dots as array keys anywhere within it. Regardless of whether the field is stored in config or not.

Regarding simplifyAllowedValues() and structureAllowedValues() from earlier patches: are those needed as public functions at all? Is there a way we can refactor all consuming code to deal with AllowedValuesInterface instead? Maybe we need to improve on that interface?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
52.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,113 pass(es). View
638 bytes

All right, since the field stuff is largely undecided, let's get the migrate stuff committed ASAP. Here is a roll of ONLY the migrate things. I hope migrate people can help quickly RTBC this since @chx, @benjy, etc. signed off on it above. We can get this committed and then deal with a way smaller patch. I know multiple commits on an issue is unusual but so is the combination of the two related problems here. And by making the patch much easier to review, we make this has more chances to being resolved. So I kindly ask to get in the migrate stuff ASAP and then we can continue posting the field things.

Contains all the migrate things from #75 and only those.

xjm’s picture

@Gábor Hojtsy, would it make sense to split it into a separate issue? Multiple commits cause all kinds of confusion.

benjy’s picture

  1. +++ b/core/modules/migrate/src/Row.php
    @@ -186,7 +191,7 @@ public function freezeSource() {
    +    return NestedArray::keyExists($this->destination, explode($this::PROPERTY_SEPARATOR, $property));
    

    How come we're using $this::PROPERTY_SEPARATOR here?

  2. +++ b/core/modules/migrate/src/Row.php
    @@ -199,7 +204,7 @@ public function hasDestinationProperty($property) {
    +    NestedArray::setValue($this->destination, explode($this::PROPERTY_SEPARATOR, $property), $value, TRUE);
    

    Same here.

  3. +++ b/core/modules/migrate/src/Row.php
    @@ -238,7 +243,7 @@ public function getRawDestination() {
    +    return NestedArray::getValue($this->destination, explode($this::PROPERTY_SEPARATOR, $property));
    

    and again.

  4. +++ b/core/modules/migrate_drupal/config/install/migrate.migration.d6_date_formats.yml
    @@ -14,6 +14,6 @@ process:
    -  'pattern.php': value
    +  pattern: value
     destination:
    

    Is this a mistake or are we fixing another bug here?

Looking good to me.

Gábor Hojtsy’s picture

@xjm: I don't think its less confusing to open yet one more beta blocker, copy over only the relevant parts of the issue summary and commit from there since that will not have any of the discussion and discovery of the things changed while this issue then inevitably needs to remove the then irrelevant issue summary parts. So those finding this issue will find it odd there is discussion of unrelated things, those finding that issue will find there is no discussion but a commit. It needs considerable human parsing either way. I think committing twice from this issue requires less cognitive overload. Also gives us a positive boost that we resolved a quantitatively big part of a beta blocker :)

@benjy:
- 1-3: these are all replacement of explicit explode()s on dots in the destination property names, we did not introduce any new explodes :)
- 4: fixing that to 'pattern/php' here would be the direct fix for this issue at hand, but that would be invalid settings now, see #2276183: Date intl support is broken, remove it where the php key was removed, if you prefer we can fix it to an incorrect value here and open yet one more followup :)

benjy’s picture

1-3, I was referring to the usage of $this:: rather than static::

-4, all good by me, just thought i'd ask the question.

EDIT: Replace self:: with static:: since that is what we're using elsewhere.

Gábor Hojtsy’s picture

FileSize
52.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,162 pass(es). View
1.33 KB

Good eyes, good eyes!

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Couple of people mentioned not liking the "/" compared to "." but that shouldn't hold this beta blocker up.

Patch looks good to me and it will certainly be easier to change the separator character if we change our mind again.

catch’s picture

Title: Field allowed values and migrations use dots in key names - not allowed in config » Field allowed values use dots in key names - not allowed in config
Status: Reviewed & tested by the community » Needs work

This is another strike against migrations as config entities, still don't understand the requirement for that. However I'm also not interested in it blocking the allowed values issue so have gone ahead and committed #84.

Back to CNW for the allowed values changes.

  • catch committed 4ccdb41 on 8.x
    Issue #2293773 by Gábor Hojtsy, alexpott, penyaskito: Fixed migrations...
Gábor Hojtsy’s picture

Published the draft change notice for migrations: https://www.drupal.org/node/2297375. Working on rerolling the patches with only the field changes.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
17.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,314 pass(es). View

Rerolled the API changes from 66 and the storage changes (that still do not work) from 75. So the attached patches are the same as 75 and 66 respectively but now without the migrate part distracting reviewers :) Let's continue discussing which way to go forward then.

chx’s picture

we have been down the "why migrations are config entities" road before: a) we need discover them b) we need to load them c) we are adding groups for drupal 7 and then we need to query them d) who knows, a UI might come along and then you need to save them.

Gábor Hojtsy’s picture

All right, now we are back at discussing the two approaches in #89. Re @effulgentsia in #78, AllowedValuesInterface is used in various other places, eg. constraint validation, with filter formats, etc. So our more convoluted use of a structure is not really generally applicable. There are tests and install functions where the config data structure is directly written and since the config data structure was made more complex, we moved the callback to be in line which also requires the transformation functions. That is why they were made public. See their uses in options_test module (only implementations of allowed value callbacks in core) and feeds for example.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

In discussing this yesterday with @alexpott, @mtift and @xjm, the idea of a dedicated getAllowedValues()/setAllowedValues() method came up which would encapsulate the transformation and avoid the rest of the changes. That is a more mature version of overriding getSetting()/setSetting() on 'allowed_values' only, but it would require throwing exceptions and therefore actually overriding getSetting()/setSetting() for 'allowed_values' also.

It would really be great to get some feedback from @yched and/or other field maintainers on this one. There are various ways we can fix this but it comes down to not agreeing on them and not liking any of them.

yched’s picture

Getting back at this, for now just a note that #2226063: Merge ListBooleanItem from options module into BooleanItem is RTBC, and removes "list_boolean" from the set of problematic field types, which should simplify whichever solution we end up adopting here.

Gábor Hojtsy’s picture

FileSize
37.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,443 pass(es). View

A straight reroll from #89 since that did not apply anymore due to #2226063: Merge ListBooleanItem from options module into BooleanItem landing. We can get rid of the trait right away, so doing that next.

Gábor Hojtsy’s picture

FileSize
2.12 KB
36.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,453 pass(es). View

Now getting rid of the trait and moving the methods back into ListItemBase.

Gábor Hojtsy’s picture

I looked into how can we hide the storage required change in the settings array. The problem is all of the entity_create()s need to use the direct data structure, there is no transformation of data down from the Entity level up through ConfigEntityBase and FieldConfig. So even if we override getSettings()/setSettings() for allowed values or we add specific methods, the entity_create() code would need to use the data structure... :/

Gábor Hojtsy’s picture

FileSize
15.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Also rerolled the storage based patch. Will still fail. This is an alternate track to 100, rolled from 89's storage patch. Because we cannot agree. :P

Status: Needs review » Needs work

The last submitted patch, 102: 2293773-fields-storage-102.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,421 pass(es). View
7.35 KB

Improving on this a bit, doing the data transformation before the instantiation to mirror the save API. This required deeper inspection of the data structure but we make so many assumptions about the field config anyway on the storage level with this :/

Gábor Hojtsy’s picture

FileSize
14.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,558 pass(es). View
1.39 KB

Actually preSaveData() does not need the entity then. True to its name and matching postLoadData().

Gábor Hojtsy’s picture

FileSize
3.94 KB
17.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,619 pass(es). View

Adding instance settings storage possibilities. Although not needed for this use case, @yched suggested we introduce it for completeness.

Gábor Hojtsy’s picture

FileSize
19.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,394 pass(es). View
2.16 KB

And now with the same tests that are in the other approach.

Gábor Hojtsy’s picture

FileSize
19.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,449 pass(es). View
6.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,374 pass(es), 8 fail(s), and 0 exception(s). View
36.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,388 pass(es). View

All right now, we have a solution for the storage based change and one for the API based change and also a test only patch that proves the fixes are worth it :) Should warrant a round of reviews again definitely.

The last submitted patch, 108: 2293773-fields-storage-108-test-only.patch, failed testing.

effulgentsia’s picture

FileSize
36.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,082 pass(es). View
19.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,255 pass(es). View

Rerolled the two #108 patches for #2287727: Rename FieldConfig to FieldStorageConfig.

effulgentsia’s picture

I reviewed both patches, and am pondering a bit more. I think both are workable, and either could be polished into something committable. I'm curious what yched, alexpott, and others think between the two. I'll post my thoughts this weekend as well.

effulgentsia’s picture

I thought about this a bit, and decided I prefer the "api" approach over the "storage" one. Here's why: the difference between the "simplified" representation and the "structured" representation is pretty fundamental: it's the difference between a map and an array of records (using terminology from http://en.wikipedia.org/wiki/Data_structure). Those data structures embody a conceptual difference, not just a "storage detail" difference. Given that, I think it would be a WTF for what you hand-edit in a YAML file to be a conceptually different data structure than what you invoke in code like $field_storage->settings = ... or FieldDefinition::create()->setSetting(...).

OTOH, converting between the two data structures at the point we go from "here's what's in our field definition" to "here's something suitable for assigning to a form element's #options" doesn't seem like a WTF to me. Field definitions and Form API are different systems: it wouldn't surprise me that some conversion function would need to be called at that point.

I think some cleanup is possible to solve some of the ugly parts in #110's "api" patch, but before we start that level of polish, let's get agreement on which approach to follow. Any feedback on my reasoning?

effulgentsia’s picture

Note: where I think the "storage" approach would be appropriate is if we weren't changing data structures around, but instead did something at a more "storage detail" level like escape "." to some other character. But I don't think we should do that: I like changing to a data structure that can be defined in a schema.

yched’s picture

Well, the fields currently structure their runtime "allowed values" in the format that is defined by AllowedValuesInterface, which is the closest to a "canonical format" we have.
The only reason we're considering adding complexity by moving away from that canonical shape and introducing a different, intermediate format, is because of CMI storage, so yeah, this kind of really *is* about storage :-)

I don't really see "what you hand-edit in a YAML file is a different data structure than what you invoke in code like FieldDefinition::create()->setSetting(...)" to be an issue. We don't put a constraint on API structures to be designed 1:1 on database schemas, right ?

Also, as mentioned at the end of #51, we have other cases in other field types where we need to do some field-type-specific massaging of the field-type-specific "settings" anyway...

yched’s picture

Will try to answer @Gabor's points in #75 / #76 more in depth, but the main technical difficulty on the "storage" approach seems to be : "we need a fully instanciated FieldConfig / FieldInstanceConfig $entity in order to determine the right $fieldItemClass".

I don't see why taht would bethe case, though. In the raw config record, there is a 'field_type' entry, that should be enough to determine the corresponding "field type plugin" class ?

Coz, agreed that the transformations should happen before we upcast the raw config data into a full $entity. We don't want to have "non massaged $entities", even short-lived.

chx’s picture

> We don't put a constraint on API structures to be designed 1:1 on database schemas, right ?

Right. Consider how the SQL database rows (leaving out a few values for brevity) [entity_type: 'node', entity_id: 12, delta: 0, value: 'first value'], [entity_type: 'node', entity_id: 12, delta: 1, value: 'second value'] are used for what is a single value, that is $node->somefield. We do not even think about this but SQL can't store arrays so we split up things into multiple rows, multiple tables and never think twice. Every storage has its limitations.

Gábor Hojtsy’s picture

@yched: right, I gave up on that and do not instantiate or require an entity anymore. Instead of calling the methods on the entity, I just figure the field type class out of the confog data and the typed data manager, so the storage classes need to depend on those. See the patches. The instantiation concern is outdated.

effulgentsia’s picture

I don't really see "what you hand-edit in a YAML file is a different data structure than what you invoke in code like FieldDefinition::create()->setSetting(...)" to be an issue. We don't put a constraint on API structures to be designed 1:1 on database schemas, right ?

The difference is that you shouldn't be hand-editing db records. Also, since the db storages are all swappable, there is no canonical schema, only default ones provided by services shipped with core. The same is true with the config active store: there should be no constraint on that being human-friendly to edit. However, the structure of files in config/install is part of our API. We even have schema files for them, and those are not swappable. And, they are intended for humans to hand-edit. A module that wants to install a field_storage config entity can either do so via a config/install/*.yml file, or with a hook_install() implementation that calls entity_create('field_storage_config', $values)->save(): both are legitimate, and I think it's weird for the structure of $values to be radically different depending on which approach the module chooses. Or, when the module wants to update that entity as part of a code upgrade, it then needs to both provide an updated .yml file and implement a hook_update_N() that calls entity_load('field_storage_config', $foo)->makeSomeChanges()->save(). And again, needing to do mental gymnastics between the two representations isn't ideal, IMO.

However, I think the rest of #114 is compelling enough to proceed with the "storage" approach here, because:
1) as yched points, we need the API provided by that for other use cases like local id to uuid conversion.
2) we agree on the changes for how allowed_values needs to be stored within the config.

So my only concern is with keeping List fields returning their current structure when ->getSetting('allowed_values') is called, but since yched doesn't agree to change it, we shouldn't make that part of this issue's scope, and instead, I should open a noncritical followup to change it on DX grounds, and let that part of the discussion continue there.

Gábor Hojtsy’s picture

All right, let's review the storage approach then :) It does as @yched said. Allows to alter data *right* before instantiation and *right* after entity to array conversion in both field configs and instance configs.

So feedback needed on the storage (not the API) patch from #110 then.

effulgentsia’s picture

Some suggestions for cleanup to consider:

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
    @@ -166,7 +166,7 @@ protected function doLoadMultiple(array $ids = NULL) {
    -      $records[$config->get($this->idKey)] = $config->get();
    +      $records[$config->get($this->idKey)] = $this->postLoadData($config->get());
         }
         return $this->mapFromStorageRecords($records);
    

    Instead of introducing a new function, postLoadData(), what about using the already existing mapFromStorageRecords()? i.e., the Field*Storage classes can extend that?

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
    @@ -237,7 +237,7 @@ protected function doSave($id, EntityInterface $entity) {
    -    foreach ($entity->toArray() as $key => $value) {
    +    foreach ($this->preSaveData($entity->toArray(), $entity) as $key => $value) {
    

    For symmetry with ContentEntityDatabaseStorage, what about instead of adding a preSaveData() function, we add a mapToStorageRecord() function, and have ConfigEntityStorage implement that as just the ->toArray(), and then Field*Storage can extend to add its custom logic?

  3. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
    @@ -255,4 +255,24 @@ public function instanceSettingsForm(array $form, array &$form_state) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function preSaveSettings(array &$settings) { }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function postLoadSettings(array &$settings) { }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function preSaveInstanceSettings(array &$settings) { }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function postLoadInstanceSettings(array &$settings) { }
    +
    

    I suggest renaming to something like:
    - settingsToConfigArray()
    - settingsFromConfigArray()
    - instanceSettingsToConfigArray()
    - instanceSettingsFromConfigArray()

    If we like the above terminology, then I don't think they should modify the param by ref, but rather return the converted value.

    Also, within the interface definition, I suggest moving them to the bottom. Any maybe grouping the 8 settings functions into 2 groups of 4. i.e., ordering like this:
    - defaultSettings()
    - settingsFromConfigArray()
    - settingsToConfigArray()
    - settingsForm()
    - repeat above 4 for the "instance" versions.

  4. +++ b/core/modules/field/src/FieldStorageConfigStorage.php
    @@ -156,4 +157,27 @@ public function loadByProperties(array $conditions = array()) {
    +    // @todo Makes too much assumptions but it needs to.
    +    $type_definition = \Drupal::typedDataManager()
    +      ->getDefinition('field_item:' . $data['type']);
    +    $type_definition['class']::postLoadSettings($data['settings']);
    

    What about adding a getFieldTypeClass($type) method to FieldTypePluginManager (which can also benefit from it internally, e.g., in getDefaultSettings())? That plugin manager can then be injected into the storage classes via createInstance()/__construct() just like all the other dependencies it gets that way.

    This could then become $class = $this->fieldTypeManager->getFieldTypeClass($data['type']), which I don't think would need an @todo anymore.

hussainweb’s picture

From the storage patch:

+++ b/core/modules/options/src/Plugin/Field/FieldType/ListFloatItem.php
@@ -90,4 +90,24 @@ protected static function validateAllowedValue($option) {
+  /**
+   * {@inheritdoc}
+   */
+  public static function simplifyAllowedValues(array $structured_values) {
+    $values = array();
+    foreach ($structured_values as $item) {
+      // Nested elements are embedded in the label.
+      // See ListItemBase::structureAllowedValues().
+      if (is_array($item['label'])) {
+        $item['label'] = static::simplifyAllowedValues($item['label']);
+      }
+      // Cast the value to a float first so that .5 and 0.5 are the same value
+      // and then cast to a string so that values like 0.5 can be used as array
+      // keys.
+      // @see http://php.net/manual/en/language.types.array.php
+      $values[(string) (float) $item['value']] = $item['label'];
+    }
+    return $values;
+  }
+

I am just wondering if we should also override structureAllowedValues for ListFloatItem, to normalize float values before writing them to value-label pairs.

effulgentsia’s picture

Re #120.4: we could also consider adding (instance)settings(To|From)ConfigArray() methods to FieldTypePluginManager, just as we have for getDefaultSettings(), if we want that manager to encapsulate the calling of the field type's static methods, which might have some benefits.

yched’s picture

re #118 :
Yeah, sorry if I sounded rigid :-/. Limited personnal bandwidth makes short answers posts & dogmatic statements...

My point is that the "massaging of settings pre/post storage" feature is both :
- perfectly reasonable: a config entity can control how it loads and saves itself based on its own internal structure. but the 'settings' entry in FieldConfig / FieldInstanceConfig is stored on behalf of a 3rd party (the field type class). It's legit to let that 3rd party have a say in the process, since we the generic Field*ConfigStorage knows nothing about what's in there (only : "it exists, and it is an array") and currently just dumbly dumps the content as is. That's just another place where we delegate some field-type-specific processing to the field type, nothing new here.
- needed for other cases anyway.

Then, once the need for this mechanism is established, feels weird to have a different, one-off fix that leaks into runtime for the case at hand here. But if everyone but me thinks it's not a good fit for this case (@eff's argument in #118 about "different format in hook_install() and config/install/*.yml" is a valid one), then I'll live with it. I'll then owe @Gabor a truckload of his beverage of choice for tricking him into implementing a feature we'll use in other bugfixes :-/

To stop derailing this issue further, I'm fine with the plan in #118, though:
- get the "storage" patch in to close the critical here,
- leverage the new API to fix existing issues with deploying local ids in field type settings,
- possibly, reconsider the case of options values.

yched’s picture

So, on the "storage" patch :

@effulgentsia's suggestions in #120 make a lot of sense IMO.

- settingsFromConfigArray() & friends : since both the input and the output are arrays, maybe settingsFromConfigData() ?

- Adding FieldTypemanager->getFieldTypeClass($type) : makes sense, but IMO would be worthwhile more generally as
DiscoveryInterface::getPluginClass() or PluginManagerBase::getPluginClass() - curently a lot of code that need the class call DefaultFactory::getPluginClass(), which is sick.
We probably don't want to add this discussion to the scope of this issue though. Created #2306889: Add a DiscoveryInterface::getPluginClass().
For this issue here, we could simply add FieldTypemanager->getPluginClass() ?

#122:

we could also consider adding (instance)settings(To|From)ConfigArray() methods to FieldTypePluginManager, just as we have for getDefaultSettings()

Less sure about that. FieldTypePluginManager::getDefaultSettings() mostly exists for consistency with (Formatter|Widget)PluginManager, which use the method internally as part of their getInstance() implementation.
But I don't think a FieldTypePluginManager::doSomeConfigStorageStuff() method would make sense. PluginManager have no connection with config storage, and I don't think the Storage class should go through the PluginManager to call a method about storage massaging.

PluginManagers have no vocation to centralize every call to methods on plugins / plugin classes IMO. For example, we don't go through the FieldTypePluginManager to get the schema for a field type; the code that needs the schema calls the FieldItem::schema() method on the right class directly. I see no real reason to do differently here ?

Gábor Hojtsy’s picture

FileSize
15.58 KB
25.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,039 pass(es). View

This resolves all the concerns from @effulgentsia and @yched:

  1. Instead of introducing new postLoadData() and preSaveData() we rely on mapFromStorageRecords() (defined on the EntityStorageBase level) and a new mapToStorageRecord() introduced on the ConfigEntityStorage level. Mapping *to* storage records always starts with one single entity, while mapping *from* storage records always produces an array of entities.
  2. The methods for settings alteration have been renamed to [instance]settings[To|From]ConfigData
  3. These methods now return the modified values do not take the array by reference.
  4. FieldTypePluginManager[Interface] got a new getPluginClass() public method.
  5. The field type manager is injected to both field storage config storage and field instance config storage, and the from/to data mappers use the method to invoke the methods on the right class.

The only thing I did not do here is to reorder the methods because I wanted a reviewable interdiff. We can alphabetise functions later, that is easy. The more important question is if this looks good :) Still passes the tests locally.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Issue summary: View changes
Status: Needs review » Needs work

I really like the use of mapToStorageRecord() and mapFromStorageRecord() - content and config entities using the same methods to do the same thing ++

This looks good to me. I can't rtbc because I've contributed but +1 to doing so once the minor nit below is done.

+        // See structureAllowedValues().

Minor nit. I think the docblocks of simplifyAllowedValues and structureAllowedValues should have @see's to each other. And there are a few // See dotted about which should be @see and fully qualified.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
25.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,222 pass(es). View
2.84 KB

Updated all the docs cross-refs, thanks @alexpott for the review.

hussainweb’s picture

FWIW, +1 from me too once it passes. I am just wondering if there is a reason we are not overriding structureAllowedValues in ListFloatItem like mentioned in #121. It seems like a good idea for consistency. It shouldn't hold this issue up though.

Gábor Hojtsy’s picture

@hussainweb: sorry for not responding to that. So we could override it and force the values in the structured array to floats, but we don't do type enforcement either in the other cases and in fact not dealing with the exact type of data the field type manages when the values come from a settings form submission for example (as opposed to read back from config). The config save process ensures the type casting happens then and then reading back from there results in the exact types. But most of the field types don't do anything special here to force their types. Eg. ListIntegerItem does not override extractAllowedValues() to enforce the values get cast to an int. They will be strings.

We *need* to override the simplifying method for floats because if the values are in fact floats those would not work in a simple PHP array. Whether these item classes should enforce their types is a different question and IMHO not in scope on this issue. It would require several more changes in the implementations (and added tests).

effulgentsia’s picture

Sorry, a bunch of docs nits :(

  1. +++ b/core/lib/Drupal/Core/Config/ConfigBase.php
    @@ -160,13 +160,39 @@ public function get($key = '') {
    +   * @throws \Drupal\Core\Config\ConfigValueException
    +   *   If any key in $data in any depth contains a dot.
        */
       public function setData(array $data) {
    +    $this->validateKeys($data);
    

    Do we need the @throws doc if the throw happens in a called function rather than in this one? Same q for the set() function further down.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigBase.php
    @@ -160,13 +160,39 @@ public function get($key = '') {
    +   * Validate all keys in a passed in config array structure.
    

    s/Validate/Validates/

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
    @@ -246,6 +247,18 @@ protected function doSave($id, EntityInterface $entity) {
    +   * Map an entity to its storage form as needed in this storage.
    

    Let's use the same doc line as in ContentEntityDatabaseStorage.

  4. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
    @@ -183,6 +183,56 @@ public function view($display_options = array());
    +   * Defines behavior for transforming field settings before being saved.
    +   *
    +   * May be used to alter settings to transform them to a storage-friendly form.
    

    Here and for the other methods, I think the docs could be improved.

  5. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
    @@ -48,4 +48,14 @@ public function getDefaultSettings($type);
    +   * Returns the plugin class for a field type.
    

    "Returns the PHP class that implements the field type plugin."

  6. +++ b/core/modules/field/src/Entity/FieldInstanceConfig.php
    @@ -254,7 +254,8 @@ public function __construct(array $values, $entity_type = 'field_instance_config
    +    // schema generation and data altering for storage.
    

    s/data altering for/settings mapping from/

  7. +++ b/core/modules/field/src/Entity/FieldInstanceConfig.php
    @@ -288,7 +289,8 @@ public function getType() {
    +    // allow for altering data by field type.
    

    s/altering data/mapping settings from storage/

  8. +++ b/core/modules/field/src/FieldInstanceConfigStorage.php
    @@ -175,4 +188,25 @@ public function loadByProperties(array $conditions = array()) {
    +    foreach ($records as $key => $record) {
    ...
    +      $records[$key] = $record;
    

    Why not foreach ($records as &$record) { instead?

  9. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
    @@ -237,4 +237,76 @@ protected function allowedValuesString($values) {
    +   * Simplify allowed values to a key-value array from the structured array.
    

    s/Simplify/Simplifies/

effulgentsia’s picture

Re #131.1: I'm not against the extra @throws, and I can see them as helpful. Just wondering if that pattern of redundant docs bubbling will be hard to maintain over time, and if it does/should extend to many other similar places throughout core.

Re the other substitution suggestions, keep in mind those are just my suggestions, and can be tweaked as needed.

Gábor Hojtsy’s picture

The throws docs does not mention if this should appear where it is rethrown (https://www.drupal.org/coding-standards/docs#throws). The reason I documented it on setValue() and set() is that those are the public API of config, and the validation method is protected and internal. I can do the rest of the changes of course. Your point 4 is not clear, what should be improved?

alexpott’s picture

I think the @throws are useful on the public parts of the API.

Gábor Hojtsy’s picture

FileSize
25.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,954 pass(es). View
5.22 KB

Made all changes suggested in 131, except 4 which was not actionable :D As for point 3, ContentEntityDatabaseStorage' map to storage needs to return an stdobj and it supports one more argument, so not possible to copy the docs verbatim, made minor adjustments. Any other concerns? :)

hussainweb’s picture

Thanks @Gabor. I see what you mean and it makes sense that the overall issue of handling types here does not make sense. My point here is that ListFloatItem::simplifyAllowedValues, by doing the cast, will remove duplicates like handled in the test scenario here:

+    // Check that the same key can only be used once.
+    $string = "0.5|Point five\n0.5|Half";
+    $array = array('0.5' => 'Half');
+    $this->assertAllowedValuesInput($string, $array, 'Same value cannot be used multiple times.');

If my understanding is right, the test works because you are writing to config and reading back from it. However, if you are just concerned about what gets written to the config, these duplicates won't be removed. Is this correct?

I am going to do a manual test just to make sure that this is in line with my understanding.

Gábor Hojtsy’s picture

@hussainweb: it is a pre-existing problem that the array keying by value used strings, and floats may have different string representations for the same float value. Prior to this patch, the different float values would not be detected as the same as far as I understand. It is true that now those would only disappear when stored in CMI and not before that. Once again that is a pre-existing problem and not related to the use of dots. A better solution to that may be to make the validation pluggable or add a method that the float items would override that is ran from the value extraction in extractAllowedValues. The way it validates the allowed value with validateAllowedValue() does not lend itself to validating duplicates, because all values are validated in isolation. Once again I think this is a pre-existing problem and it was/is not the task of this issue to resolve that problem. I think its nice if we resolve it, but if it needs a more elaborate apparatus, it would be better to resolve it in another issue (probably once this lands).

hussainweb’s picture

You're right. I guess it makes sense for another issue once this goes.

FYI, I did that test and added two values so:

0.5|Half
.5|Second Half

Once saved, it just remains as:

0.5|Second Half

The config file on export has:

settings:
  allowed_values:
    -
      value: 0.5
      label: 'Second half'
  allowed_values_function: ''
effulgentsia’s picture

Assigned: Unassigned » effulgentsia
Status: Needs review » Needs work

Redocumenting @throws on the innermost public methods makes sense. I'll take a stab on #131.4.

yched’s picture

Awesome :-) @Gabor++

Only found a couple nitpicks :

  1. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
    @@ -48,4 +48,14 @@ public function getDefaultSettings($type);
    +   * @param string $type
    +   *   A field type name.
    +   * @return string
    +   *   Field type plugin class name.
    

    Missing an empty line between @param and @return.

    Same with the other phpdocs for the new methods in FieldItemInterface

  2. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
    @@ -183,6 +183,56 @@ public function view($display_options = array());
    +   * Defines behavior for transforming field settings before being saved.
    +   *
    +   * May be used to alter settings to transform them to a storage-friendly form.
    

    Agreed with @effulgentsia that this could be improved. I'd suggest:

    *ToConfigdata():
    "Transforms field settings before they are saved into a config record.
    This method can be used to massage the runtime settings into a storage-friendly and deployment-friendly shape."

    *FromConfigdata():
    "Transforms field settings after they are loaded from a config record.
    This method can be used to massage back the data written by settingsToConfigdata() into the settings used at runtime."

  3. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
    @@ -183,6 +183,56 @@ public function view($display_options = array());
    +  public static function settingsToConfigData(array $settings);
    ...
    +  public static function settingsFromConfigData(array $settings);
    ...
    +  public static function instanceSettingsToConfigData(array $settings);
    ...
    +  public static function instanceSettingsFromConfigData(array $settings);
    

    Yeah, was mentioned earlier, but now would be a good time to move those next to defaultSettings() / defaultInstanceSettings(),
    rather than between preSave() and insert(), which is a bit arbitrary :-)

  4. +++ b/core/lib/Drupal/Core/Config/ConfigBase.php
    @@ -160,13 +160,39 @@ public function get($key = '') {
    +  protected function validateKeys(array $data) {
    

    Uber nitpick : this is used by both setData() and set(), so it could be added after both methods, keeping them together, rather then in between, splitting them apart from each other ?

yched’s picture

@effulgentsia - "I'll take a stab on #131.4."
oops, crosspost :-) My suggestions are above, but feel free to enhance.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
6.44 KB
25.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,350 pass(es). View

I know @effulgentsia assigned this, but I couldn't resist taking a shot at this, even if it was just moving around code. :)

Attaching the interdiff and updated patch from comments from #141. I have copied @yched's suggestions on #131.4 (almost) exactly. The only new thing I have added is @see declarations for settingsToConfigData / settingsFromConfigData and instanceSettingsToConfigData / instanceSettingsFromConfigData.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
FileSize
27.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,169 pass(es). View
5.99 KB

And here's my attempt at more explicit docs. Interdiff relative to #142.

effulgentsia’s picture

I would have been eligible to RTBC #142, but need someone to ok my interdiff before I can RTBC #143. If there's something there that's not ok, I'd also be ok with RTBC'ing #142 and punting #143 to a followup.

hussainweb’s picture

In my opinion, the doc changes in #143 are great, but need small changes. The wording sometimes throws off the reader and makes it seem more complicated than it actually is. For example:

+++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
@@ -232,60 +232,84 @@
+   * An example of a conversion between representations might be an
+   * "allowed_values" setting that's structured by the field type as a
+   * \Drupal\Core\TypedData\AllowedValuesInterface::getPossibleOptions()
+   * result (i.e., values as keys and labels as values). For such a use case,
+   * in order to comply with the above, this method could convert that

I think it would be simpler to understand if it said "An example is "allowed_values" setting ...". The paragraph above is also somewhat confusing but I couldn't think of a way to make it simpler.

I think this needs more work and maybe a separate issue (so as to not hold up this critical). I am happy with RTBC even on #143 as it has enough we can build upon later in a follow-up issue.

hussainweb’s picture

Oh, and +1 to RTBC for #143 or #142. I don't think I can set the status as RTBC as I have contributed a patch here.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
27.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,079 pass(es). View
1.65 KB

@hussainweb: I don't think you would be disqualified from RTBC since you only moved around a few functions and made no other changes whatsoever. In fact I wrote the whole most of the patch and I will RTBC it myself now on the ground of the following people wanting to RTBC before: @hassainweb in #146, @effulgentsia in #144, @yched in #140 "only found a couple nitpicks", @alexpott in #127. This ought to be enough :D

Gábor Hojtsy’s picture

Updated change notice at https://www.drupal.org/node/2297369, added one more change notice at https://www.drupal.org/node/2307365. With this, the issue has 3 draft change notices all up to date waiting for published and one already published (for migrations). Hopefully all the changes are documented enough :D

yched’s picture

RTBC +1. Thanks a ton, folks :-)

catch’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/core/modules/field/src/FieldInstanceConfigStorage.php
@@ -175,4 +188,24 @@ public function loadByProperties(array $conditions = array()) {
+      $class = $this->fieldTypeManager->getPluginClass($record['field_type']);
+      $record['settings'] = $class::instanceSettingsFromConfigData($record['settings']);
+    }
+    return parent::mapFromStorageRecords($records);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function mapToStorageRecord(EntityInterface $entity) {
+    $record = parent::mapToStorageRecord($entity);
+    $class = $this->fieldTypeManager->getPluginClass($record['field_type']);
+    $record['settings'] = $class::instanceSettingsToConfigData($record['settings']);
+    return $record;
+  }

I'd probably have done configDataToInstanceSettings and instanceSettingsToConfigData since there's less chance of copy-pasting one of those thinking it's the other, but purely preference so not bothered.

That was all I could find to complain about, I never liked the allowed values settings structure and this cleans it up nicely.

Committed/pushed to 8.x, thanks!

  • catch committed f1ed247 on 8.x
    Issue #2293773 by Gábor Hojtsy, alexpott, effulgentsia, penyaskito,...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, amazing! Published all the draft change notices as well. Superb.

andypost’s picture

Status: Fixed » Closed (fixed)

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