Problem

  • Lots of integers, Booleans, and even octal numbers in config object files, but all of them are converted to strings.

Goal

  • Research whether it is feasible to remove the enforced type-casting to strings for all config values.

Details

  • The reason we are force-casting everything into a string is because that is the common denominator for all possible storage engines.
  • There are considerations to change the active store implementation to essentially store each key within the config object as a separate item/record.
  • However, right now, we're dealing with YAML and serialized PHP. Both of them support primitive data types.
  • For modules that have to work on other data types than strings, we're implanting lots of data type conversions currently.

Proposed solution

  1. Remove the enforced type-casting to strings.
  2. Update all config files accordingly.

Notes

  • This destroys native support for alternative serialization formats, such as JSON and XML.

Related Issues


Original report:

During the discussions around #1650778: Convert default views into YAML files for CMI we've come up against the issue of data typing. Yaml does support preserving types when saving. However, currently the DrupalConfig object casts all values to strings. Should we do this?

In my opinion config should be a dumb as possible - which includes returning exactly what was given. So if given the data:

  array(
    'test' => 2,
    3 => 1,
  );

We should return an identical array.

Currently we return

  array(
    'test' => '2',
    3 => '1', // Yes key the remains numeric!
  );

Sub-tasks #

Modules #

Module Issue
action #2105905: Make sure all YML files in Action module has no type-casting to string.
aggregator #2105913: Make sure all YML files in Aggregator module has no type-casting to string.
block #2105915: Make sure all YML files in Block module has no type-casting to string. Assigned to: foxtrotcharlie
book #2105917: Make sure all config yml files in Book module has no type-casting to string.
comment #2105921: Make sure all YML files in Comment module has no type-casting to string.
config #2105925: Make sure all YML files in Config module has no type-casting to string.
field #2105951: Make sure all YML files in Field module has no type-casting to string.
file #2105957: Make sure all YML files in File module has no type-casting to string.
locale #2105969: Make sure all YML files in Locale module has no type-casting to string + update schema
menu #2105971: Make sure all YML files in Menu module has no type-casting to string.
node #2105977: Make sure all YML files in Node module has no type-casting to string. Assigned to: alexander_danilenko
rest #2105979: Make sure all YML files in REST module has no type-casting to string. Assigned to: alexander_danilenko
shortcut #2105987: Make sure all YML files in Shortcut module has no type-casting to string. Assigned to: alexander_danilenko
simpletest #2105989: Make sure all YML files in Simpletest module has no type-casting to string. Assigned to: alexander_danilenko
statistics #2105991: Make sure all YML files in Statistics module has no type-casting to string. Assigned to: nonsie
system #2105993: Make sure all Config yml files in System module has no type-casting to string.
taxonomy #2105997: Make sure all YML files in Taxonomy module has no type-casting to string. Assigned to: alexander_danilenko
toolbar #2106001: Make sure all YML files in Toolbar module has no type-casting to string. Assigned to: alexander_danilenko
tour #2106005: Make sure all YML files in Tour module has no type-casting to string + update stings with quotes Assigned to: nonsie
tracker #2106007: Make sure all YML files in Tracker module has no type-casting to string. Assigned to: alexander_danilenko
update #2106009: Make sure all YML files in Update module has no type-casting to string. Assigned to: alexander_danilenko
user #2106011: Make sure all YML files in User module has no type-casting to string.
views #2106021: Make sure all YML files in Views module has no type-casting to string.
views_ui #2106025: Make sure all YML files in Views UI module has no type-casting to string. Assigned to: alexander_danilenko

Themes #

Theme Issue
bartik #2106033: Make sure all YML files in bartik theme has no type-casting to string. Assigned to: alexander_danilenko
serven #2106035: Make sure all YML files in Seven theme has no type-casting to string. Assigned to: alexander_danilenko
stark #2106037: Make sure all YML files in Stark theme has no type-casting to string. Assigned to: alexander_danilenko

Profiles #

Profile Issue
minimal #2106031: Make sure all YML files in Minimal profile has no type-casting to string. Assigned to: alexander_danilenko
standard #2106029: Make sure all YML files in Standard profile has no type-casting to string. Assigned to: Barrett

Sub-tasks completed#

Modules #

Module Issue
breakpoint #2105919: Make sure all YML files in Breakpoint module has no type-casting to string.
contact #2105927: Complete config of Contact module
content_translation #2105931: Make sure all YML files in Content Translation module has no type-casting to string.
dblog #2105933: Make sure all YML files in Dblog Translation module has no type-casting to string.
edit #2105937: Make sure all YML files in Edit module has no type-casting to string.
editor #2105939: Make sure all YML files in Editor module has no type-casting to string + config system changes broke image uploading in editors Assigned to: Wim Leers
entity_reference #2105947: Make sure all config yml files in Entity Reference module has no type-casting to string. Assigned to: japerry
field_ui #2105955: Make sure all YML files in Field UI module has no type-casting to string.
filter #2105959: Make sure all YML files in Filter module has no type-casting to string.
forum #2105963: Make sure all YML files in Forum module has no type-casting to string. Assigned to: annikaC
image #2105965: Make sure all YML files in Image module has no type-casting to string.
language #2105967: Make sure all YML files in Language module has no type-casting to string. Assigned to: nonsie
search #2105983: Make sure all YML files in Search module have no type-casting to string.
xmlrpc #2106027: Make sure all YML files in xmlrpc module has no type-casting to string. Assigned to: nonsie
CommentFileSizeAuthor
#149 config-1653026-149.patch69.39 KBkrishnan.n
#146 config-1653026-146.patch326.51 KBkrishnan.n
#124 config-strings-1653026-124.patch23.93 KBtim.plunkett
#124 interdiff.txt761 bytestim.plunkett
#122 config-1653026-122.patch23.95 KBtim.plunkett
#118 interdiff.txt2.86 KBtim.plunkett
#118 config-1653026-118.patch24.92 KBtim.plunkett
#115 interdiff.txt1.67 KBtim.plunkett
#115 config-1653026-115.patch22.06 KBtim.plunkett
#111 1653026-111.patch21.42 KBdamiankloip
#111 interdiff-1653026-111.txt2.49 KBdamiankloip
#109 1653026-109.patch18.96 KBdamiankloip
#109 interdiff-1653026-109.txt4.52 KBdamiankloip
#107 1653026-107.patch14.44 KBmtift
#105 1653026-105.patch14.41 KBdamiankloip
#102 interdiff.txt2.45 KBdawehner
#102 drupal-1653026-102.patch14.35 KBdawehner
#99 1653026-99.patch12.62 KBdamiankloip
#99 interdiff-1653026-99.txt1.38 KBdamiankloip
#96 1653026-96.patch11.86 KBdamiankloip
#65 1653026-65.patch11.78 KBdamiankloip
#65 interdiff-1653026-65.txt782 bytesdamiankloip
#63 1653026-63.patch11.76 KBdamiankloip
#60 1653026-60.patch12.31 KBdamiankloip
#59 1653026-59.patch11.75 KBdamiankloip
#59 interdiff.txt937 bytesdamiankloip
#51 1653026-51.patch11.54 KBdamiankloip
#51 interdiff.txt4.47 KBdamiankloip
#49 1653026-48.patch7.07 KBdamiankloip
#48 1653026-47.patch7.1 KBdamiankloip
#48 interdiff.txt2.57 KBdamiankloip
#46 1653026-46.patch6.66 KBdamiankloip
#36 config.datatypes.36.patch6.64 KBsun
#35 drupal8.config-data-types.35.patch13.34 KBsun
#28 drupal8.config-data-types.28.patch15.41 KBsun
#27 23-27-interdiff.patch7.31 KBalexpott
#27 1653026-27-config-variable-type.patch7.82 KBalexpott
#23 22-23-interdiff.txt1.53 KBalexpott
#23 1653026-23-yaml-variable-type.patch11.8 KBalexpott
#22 20-22-interdiff.txt862 bytesalexpott
#22 1653026-22-yaml-variable-type.patch12.14 KBalexpott
#20 1653026-20-yaml-variable-type.patch12.87 KBalexpott
#20 18-20-interdiff.txt9.38 KBalexpott
#18 drupal8.config-data-types.18.patch13.49 KBsun
#17 1653026-17-yaml-variable-type.patch15.62 KBalexpott
#17 15-17-interdiff.txt6.61 KBalexpott
#15 10-15-interdiff.txt5.22 KBalexpott
#15 1653026-15-yaml-variable-type.patch14.55 KBalexpott
#13 drupal8.config-data-types.13.patch10.92 KBsun
#11 1653026-10-yaml-variable-type.patch9.33 KBalexpott
#10 4-10-interdiff.txt6.18 KBalexpott
#4 1653026-3-yaml-variable-type.patch2.35 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

After coming accross this whilst starting CMI integration for Views (above issue), this makes alot of sense to me. Config should be as you say, 'dumb', and return exactly what it is given to store IMO. The YAML spec deals with types, so I think we should use them.

sun’s picture

The reason we are force-casting everything into a string is because that is the common denominator for all possible storage engines.

(FYI: There are considerations to change the active store implementation to essentially store each key within the config object as a separate item/record.)

sun’s picture

Issue summary: View changes

Fixed code comment on second array

sun’s picture

Title: Use Yaml's ability to store variable type » All configuration values are stored as strings
Priority: Normal » Major

I'm bumping this to a major bug, since we should better be sure that we'll go with the enforced type-casting, as we're implanting more and more type-casts into runtime code in the current config conversion patches.

Due to that, I've also replaced the issue summary.

alexpott’s picture

Title: All configuration values are stored as strings » Use Yaml's ability to store variable type
Priority: Major » Normal
FileSize
2.35 KB

Couldn't resist rolling a patch to do part 1 of proposed solution.

This additionally solves the issue of config's setData($array) method not being equivalent to set($key, $value) method. A config object created using the setData currently does not have the values cast to strings.

alexpott’s picture

Title: Use Yaml's ability to store variable type » All configuration values are stored as strings
Priority: Normal » Major

Crosspost... :(

sun’s picture

Status: Active » Needs review
sun’s picture

Status: Needs review » Needs work

The important part is that every storage controller needs to be able to support the same features.

But since FileStorage uses YAML and DatabaseStorage uses serialized PHP variables, that requirement would be met (for the implementations we have in core).

To confirm that the system works as intended, this patch needs to update ConfigStorageTestBase accordingly; i.e., assert that integers, floats, Booleans, octal numbers, etc are written and read as expected.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

I think we want to do this.

However, we need rigorous tests which assert that we do _not_ support insane data types (such as a serialized PHP class instance or resource...)

alexpott’s picture

Status: Needs work » Active

Have a look at http://drupal.org/node/1496480#comment-6293186 for what this change will gives us.

One issue at the moment is that many of the config conversion YAML files are being produced by hand. Many of the elements set by FAPI will still have to be strings (eg. image style width and height 'numbers') since if these are changed by the UI they will become strings. Additionally I think a single checkbox returns an integer of 0 or 1 not a 'true' boolean.

So the question could be: should we be type-casting on input? I think this might be a good option as it'll make reads more performant and we shouldn't really care too much about writes since if theses are happening on every page request we/you are abusing the configuration system.

re #8: I'm working on those tests.

alexpott’s picture

Status: Active » Needs review
Issue tags: -Configuration system, -VDC
FileSize
6.18 KB

With some tests - we still have more to write as we need to test setting configuration with types using a config object rather than the one of the Storage implementations.

alexpott’s picture

Missing patch...

What is quite amusing is that we had tests for casting a boolean to a string equaling '0' or '1'.

They don't work :)

From ConfigFileContentTest.php

    // Read false value
    $this->assertEqual($config->get($false_key), '0', t('Boolean FALSE value returned the string \'0\'.'));

    // Read true value
    $this->assertEqual($config->get($true_key), '1', t('Boolean TRUE value returned the string \'1\'.'));

    // Read false that had been nested in an array value
    $this->assertEqual($config->get($casting_array_false_value_key), '0', t('Nested boolean FALSE value returned the string \'0\'.'));

I think $this->assertIdenitcal should have been used - gotta love PHP :)

sun’s picture

I'd prefer to leave ConfigFileContentTest alone... this test was the first that was added and basically exists for legacy reasons only. It has no clear scope and purpose, and we should remove it at some point.

Technically, storage controllers are responsible for encoding and decoding, so this change mainly affects storage controllers only, so the entire expectations and assertions should happen in ConfigStorageTestBase. (It is also the StorageInterface::decode() method that will blow up when the data contains e.g. a serialized PHP object that cannot be re-instantiated.)

However, I'm not sure where we want to implant the data type validation (to disallow PHP objects and resources from getting serialized). It might make sense to do that in Config::set() -- although, as you've pointed out already, Config::setData() would then be able to circumvent that validation. So perhaps the validation might need to happen in StorageInterface::encode()...?

The data type validation I have in mind is:

$value_is_valid = (is_scalar($value) || is_array($value));
sun’s picture

Something along the lines of this. Please consider this a quick hack and prototype only.

Status: Needs review » Needs work

The last submitted patch, drupal8.config-data-types.13.patch, failed testing.

alexpott’s picture

This patch extends @sun's idea by implementing a new method StorageInterface::validateData().

This allows better error messages along the lines of "Drupal\Core\Config\StorageException: Unallowed non-scalar value of type object in key config_key in config config_object". Additionally there's no point in trying to print the value of an object so now the error message just prints the type of the invalid value.

alexpott’s picture

Status: Needs work » Needs review

Lets test :)

alexpott’s picture

Added tests for preserving and validating data types using config object.

sun’s picture

A procedural callback in config.inc doesn't really fly for me, given that we just did #1704196: Remove Config's dependencies on procedural Drupal code in includes/common.inc

So even though the location is still wonky, I'm moving the callback back onto Config.

Also, array_merge_recursive() supports additional user parameters to be supplied to the callback, so the indirection and duplicate exception handling is unnecessary. ;)

Status: Needs review » Needs work

The last submitted patch, drupal8.config-data-types.18.patch, failed testing.

alexpott’s picture

How about bringing back the abstract class StorageBase? There's never a wrong idea... just the wrong time :)

Also I'm a fan of creating a validateData method since we might want to validate data and not write it... eg. maybe on import... you never know.

alexpott’s picture

Status: Needs work » Needs review

Go testbot go...

alexpott’s picture

Remove unnecessary duplicate validateData() method in config object.

alexpott’s picture

Remove unnecessary static - In fact the statics on encode and decode are no longer necessary (will raise an issue later) - our design must be getting better :)

sun’s picture

Hrm.

1) In my mind, a long-term is still to rebase the config storage controllers onto #1202336: Add a key/value store API, as soon as that is in core. Making all storage controllers extend from a base controller seems incompatible with that.

2) While working on an indentation fix for the Yaml\Dumper, I recognized that it in fact has built-in support for serialized objects: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Yam... -- but even with that, I'm not sure whether config system should support that. I don't think so.

3) It might make most sense to move the validation into the Config::save() method. Since that is our high-level layer that contains the business logic for operating on config objects in the end.

gdd’s picture

I agree with sun that Config::save() is probably the best place to be doing this. Are we concerned about there being some inconsistency in the files due to the issues with data coming from FAPI? For instance, booleans being stored as true/false in some places and '0'/'1' in others. That kind of bothers me.

One thing that could make this nicer is if we renamed the keys of boolean values so they are a little more self-describing (aka 'cache_enabled' vs 'cache') but that's probably fodder for another issue.

sun’s picture

The Form API values part is actually the most concerning for me here. Forgot to mention that somehow, sorry.

Problem being, Form API expects string input all over the place. There can be #default_value/#value_callback implementations that reject and straight-out deny non-string values, which makes _form_validate() ultimately blow up with the most awful error message of

An illegal choice has been detected. Please contact the site administrator.

E.g., see #811542: Regression: Required radios throw illegal choice error when none selected

For some reason, this patch doesn't seem to trigger that validation error yet - but it's possible that this is only the case, because we're still converting stuff to config, and the config we currently have might not contain edge-cases yet.

alexpott’s picture

Moved validation logic into config object.

Maybe the reason this patch hasn't trigger the validation error yet is because the config defaults are all strings :)

sun’s picture

This looks much better now.

Ultimately, however, the consequences are still a bit blurry.

I can only suspect that - if the chmod file settings were configurable through the UI and thus Form API - then we'd have to do some data type munging in the form constructor and handlers, so as to convert the original data type into one that is compatible for Form API, and on submit it would have to be converted from whatever input/string back into the config data type.

Status: Needs review » Needs work

The last submitted patch, drupal8.config-data-types.28.patch, failed testing.

gdd’s picture

Yes, sun is right, I spent some time playing around with this. For instance, octal: 0755 in a YAML file will get converted to 493 by the Symfony YAML parser and thus printed as such in a form. You would actually need to somehow know in advance that you were dealing with an octal value at read and save time in order for data to be dealt with properly. I suppose we could add type-specific FAPI textfields that can handle the conversions behind the scenes ('#type' => 'octal' would decoct() default values on read and octdec() them on submit, same for booleans in checkboxes aka false->0->false) but that is going to get super-ugly and is a much bigger change.

I'm not sure there's a good way for us to fix this without going through a lot of additional pain.

sun’s picture

So... the essential assertion that is missing in this patch is that the YAML file actually still contains the data types after writing them, because assertIdentical() only asserts this:

$ php -r "var_dump(1.0, 1, 1.0 === 1);"
float(1)
int(1)
bool(false)

$ php -r "var_dump(0xC, 12, 0xC === 12);"
int(12)
int(12)
bool(true)

$ php -r "var_dump(0755, 493, 0755 === 493);"
int(493)
int(493)
bool(true)

The moment we'd write 493 to the config file, the configuration value would lose its human-readable meaning. And this already happens right at installation time, when the default config files are loaded from the module's config dir and saved to the site's config dir.

The Yaml tests actually clarify that our expectations are incompatible with the YAML specification and/or the PHP interpreter.

There also was a very interesting discussion about this on yaml's mailing list some years ago - somewhat starting with the same symptom, but then continuing to an even brainfsck example of a 06511 zipcode ;)
http://comments.gmane.org/gmane.text.yaml.general/2689

It looks like the only way to resolve this would be via metadata - i.e., whenever a config file is written, run its data against a file-specific schema, so as to convert the data types before they are written.

gdd’s picture

Status: Needs work » Postponed

The zip code example is pretty entertaining. I guess we need to postpone this on #1648930: Introduce configuration schema and use for translation at the very least.

gdd’s picture

Priority: Major » Normal

I'm also de-prioritizing.

sun’s picture

If we go with the simplified plan in #1324618-91: Implement automated config saving for simple settings forms, then we could actually make a proper "form input to config data type" conversion a part of the new tutorial for "final-converting" settings forms into config forms.

I.e., a manual conversion, along the lines of:

function foo_settings_form_submit($form, &$form_state) {
  config('foo.settings')
    ->set('integer', (int) $form_state['values']['enabled'])
    ->save();
}

The remaining edge-cases of #31 would have to be documented.

In other words, I'm still +1 on this and would like to do it. The entire whatever-data-type-to-string futzing turned out to be a major confusion for most people involved in the config conversion issues already, so getting rid of it would be a major help and benefit -- even if it introduces some rare edge-cases at the same time.

sun’s picture

[hackfest] Re-rolled against canonical file storage branch. Will pass when that is in.

sun’s picture

Status: Postponed » Needs review
Issue tags: +YAML
FileSize
6.64 KB

Introducing a new YAML issue tag to track the issues with it.

Merged in HEAD.

Removed the custom validation for data types in Config entirely to allow usage of objects.
Adjusted tests accordingly.
Also removed the config file conversions for now, since there have been too many conflicts.

Status: Needs review » Needs work

The last submitted patch, config.datatypes.36.patch, failed testing.

webchick’s picture

Priority: Normal » Major

Reading patches like #1938570-8: Make views active config save format match the default yml file (order and quotes) make me sad, and I'm told this issue is at fault.

It seems like this should be a major bug, rather than a normal one, since it's quite the WTF for someone to define proper boolean/integer/etc. values in their YAML files like they can in any other framework and then have them unceremoniously dumped out as strings.

alexpott’s picture

This issue can go some way fix the quoting... the order is something that needs to be "right"...

DamienMcKenna’s picture

Issue tags: +Drupalism

Would it make sense to split this into separate issues: the ability to correctly read files valid to the YAML specification so we don't create Yet Another Drupalism, and the ability to correctly wrote/export non-Drupalism YAML?

DamienMcKenna’s picture

Issue tags: -Drupalism

FYI my rationale for requiring D8 to accept valid YAML, rather than Drupal YAML, is that it will make it much easier for other YAML-capable tools to integrate with Drupal without needless "Oh, Drupal doesn't like this perfectly clean YAML" hackery.

webchick’s picture

Hm. I don't think "Drupalism" applies here... this isn't a case of our own special flavour of YAML, it's a case of "What goes in isn't what you get out." It's just a straight-up bug IMO.

YesCT’s picture

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

added duplicate and related issues

vijaycs85’s picture

alexpott’s picture

damiankloip’s picture

Status: Needs work » Needs review
FileSize
6.66 KB

Rerolled @sun's patch, as it's been nearly 6 months and surprisingly, didn't apply cleanly :)

I also removed the tests for object types, as I don't think we are entertaining that idea anymore?

Also if we want the last assertion in COnfigCRUDTest::testDataTypes to pass, I think we need to change this line in FileStorage::encode:

-    return $this->getDumper()->dump($data, PHP_INT_MAX);
+    return $this->getDumper()->dump($data, PHP_INT_MAX, 0, TRUE);

Status: Needs review » Needs work

The last submitted patch, 1653026-46.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.57 KB
7.1 KB

Sorry, forgot the test yaml file, also me, alexpott, and beejeebus discussed a couple of changes in IRC. Removing the foreach for invalid types in the test and add an integer as a string to the tests.

damiankloip’s picture

FileSize
7.07 KB

And the newline...

Status: Needs review » Needs work

The last submitted patch, 1653026-48.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.47 KB
11.54 KB

This should fix up the test failures, now we have proper type values.

Anonymous’s picture

Status: Needs review » Needs work

this is looking good.

i'd like to see us check the strings on disk as well as in-memory data here, because config->save() followed by config->get() on the same object will just give you back the in-memory values, and not refetch from disk.

+    // Re-set each key using Config::set().
+    foreach($data as $key => $value) {
+      $config->set($key, $value);
+    }
+    $config->save();
+    $this->assertIdentical($config->get(), $data);
+    $this->verbose('<pre>' . file_get_contents($storage->getFilePath($name)) . var_export($storage->read($name), TRUE));
+
+    // Set data using config::setData().
+    $config->setData($data)->save();
+    $this->assertIdentical($config->get(), $data);

so, can we test the strings on disk are what we expect as well as the in-memory values? or reload the object then test ->get()? or both?

gdd’s picture

I don't have anything against this patch except that it doesn't help anything at all because we're still going to have Form API dumping crap into our files. Here's an example. I applied this patch and then took system.performance.yml and converted it to use proper boolean and int values:

cache:
  page:
    use_internal: false
    max_age: 0
css:
  preprocess: false
  gzip: true
fast_404:
  enabled: true
  paths: '/\.(?:txt|png|gif|jpe?g|css|js|ico|swf|flv|cgi|bat|pl|dll|exe|asp)$/i'
  exclude_paths: '/\/(?:styles|imagecache)\//'
  html: '<!DOCTYPE html><html><head><title>404 Not Found</title></head><body><h1>Not Found</h1><p>The requested URL "@path" was not found on this server.</p></body></html>'
js:
  preprocess: false
  gzip: true
response:
  gzip: false
stale_file_threshold: 2592000
theme_link: true

Now here is what happens by simply opening the Performance settings form and saving it back with no changes

cache:
  page:
    use_internal: 0
    max_age: '0'
css:
  preprocess: 0
  gzip: true
fast_404:
  enabled: true
  paths: '/\.(?:txt|png|gif|jpe?g|css|js|ico|swf|flv|cgi|bat|pl|dll|exe|asp)$/i'
  exclude_paths: '/\/(?:styles|imagecache)\//'
  html: '<!DOCTYPE html><html><head><title>404 Not Found</title></head><body><h1>Not Found</h1><p>The requested URL "@path" was not found on this server.</p></body></html>'
js:
  preprocess: 0
  gzip: true
response:
  gzip: 0
stale_file_threshold: 2592000
theme_link: true
cache:
  page:
    use_internal: 0
    max_age: '0'
css:
  preprocess: 0
  gzip: true
fast_404:
  enabled: true
  paths: '/\.(?:txt|png|gif|jpe?g|css|js|ico|swf|flv|cgi|bat|pl|dll|exe|asp)$/i'
  exclude_paths: '/\/(?:styles|imagecache)\//'
  html: '<!DOCTYPE html><html><head><title>404 Not Found</title></head><body><h1>Not Found</h1><p>The requested URL "@path" was not found on this server.</p></body></html>'
js:
  preprocess: 0
  gzip: true
response:
  gzip: 0
stale_file_threshold: 2592000
theme_link: true

Note that all the bools have been changed to 0/1, and the int max_age has been quoted.

While I agree that making everything in the YAML files as strings is somewhat of a 'Drupalism', it also means our files are consistent and it is very easy to teach people how to write them. 'Quote everything. The end.' Otherwise we have to figure out what Form API is going to write in every single possible situation and document it to make them match, and even then it won't be 'proper' YAML. See also the discussion above about the difficulty of determining what format something should be for data like octals and zip codes.

Our only hope for a real solution here is to add data types or config-data binding to Form API and its just too late for those types of changes.

Amateescu did have an idea in IRC earlier, when we save config data, we could check against the schema, get the type, and cast accordingly. This could work, although it would require some mapping to base types (for instance, type: email doesn't help unless we know email is a string or alternately just assume anything we don't recognize is a string.) I could potentially get on board with this type of change, however I think the patch as it currently stands is a no-go because its just going to introduce confusion.

gdd’s picture

Also note that schema are totally not going to be required, and I'm sure many contrib won't provide them, so relying on it for file consistency will bring us back to square one in those situations (which we may not care about but its worth bringing up.)

amateescu’s picture

Contrib usually follows core and other (major) contrib, so as long as we set some good examples and documentation, I think we should be good. Or they can be fixed :/

Edit: actually, I got that order wrong, major contrib is first because core can be quite daunting to look into at first.

damiankloip’s picture

I can see the concerns here, but I really think we shouldn't limit the configuration system based on the form api. The configuration system should be about storing the data, imo it shouldn't be making this casting assumption at all. If we want to give people more direction as to how they should create these files, so be it.

I just get the feeling that leaving this in will bite us at some point down the line. There may be certain implementations using config that don't use the form api to save things, and therefore, would not benefit at all by this.

amateescu’s picture

Err.. what? My proposal was to cast based on the config schema, form api is not involved anywhere in this?

damiankloip’s picture

I'm just saying generally, one of the main arguments not for is the form api. Until I see your plan in action....

damiankloip’s picture

Status: Needs work » Needs review
FileSize
937 bytes
11.75 KB

Just rerolling and including feedback from #52.

damiankloip’s picture

FileSize
12.31 KB

Rerolled, didn't apply.

alexpott’s picture

Assigned: Unassigned » gdd
Status: Needs review » Reviewed & tested by the community

So I think that we're breaking an important principle here... that config should return the data in an identical format to what is passed to config->save(). At Barcelona @merlinofchaos said that CMI should be a dumb as possible and I think that here we are breaking that maxim.

Just because FAPI returns everything as strings is not a reason to go ahead here. We can leave all the simple config as strings if we want or we can use the schema (if available) to save typed config. But config entities should be allowed to save their data in the types stored on the object.

This is in my opinion rtbc - assigning to @heyrocker cause we need his buy-in.

Gábor Hojtsy’s picture

How is the removal of mixed_mode_sessions related? (From the end of the patch).

damiankloip’s picture

FileSize
11.76 KB

errr hmmm , sorry. :)

The patch is good, apart from that straggler.

YesCT’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined
@@ -197,4 +198,56 @@ function testNameValidation() {
+      'octal' => 0775,
+      'string' => 'string',
+      'string_int' => '1',

+++ b/core/modules/config/lib/Drupal/config/Tests/Storage/ConfigStorageTestBase.phpundefined
@@ -161,6 +161,30 @@ function testCRUD() {
+      'octal' => 0775,
+      'string' => 'string',

+++ b/core/modules/config/tests/config_test/config/config_test.types.ymlundefined
@@ -0,0 +1,9 @@
+octal: 0775
+string: string
+string_int: '1'

'string_int' is missing from the testDataTypes()

+++ b/core/modules/config/lib/Drupal/config/Tests/Storage/ConfigStorageTestBase.phpundefined
@@ -161,6 +161,30 @@ function testCRUD() {
+  }
+
+
   abstract protected function read($name);

*if* this get a new patch, might as well take out this extra blank line introduced here.

damiankloip’s picture

FileSize
782 bytes
11.78 KB

'string_int' is missing from the testDataTypes()

Good catch. That must have gone un-noticed for a while now :)

YesCT’s picture

using @heyrocker's example re-written system.performance.yml:

cache:
  page:
    use_internal: false
    max_age: 0
css:
  preprocess: false
  gzip: true
fast_404:
  enabled: true
  paths: '/\.(?:txt|png|gif|jpe?g|css|js|ico|swf|flv|cgi|bat|pl|dll|exe|asp)$/i'
  exclude_paths: '/\/(?:styles|imagecache)\//'
  html: '<!DOCTYPE html><html><head><title>404 Not Found</title></head><body><h1>Not Found</h1><p>The requested URL "@path" was not found on this server.</p></body></html>'
js:
  preprocess: false
  gzip: true
response:
  gzip: false
stale_file_threshold: 2592000
theme_link: true

without the patch, a diff of the saved active config:

$ diff core/modules/system/config/system.performance.yml sites/default/files/config_yZESkTnfGmqTNMCn9pGSjcC8MF9UsEFh_Ycj-yWk7c4/active/system.performance.yml 
3,4c3,4
<     use_internal: false
<     max_age: 0
---
>     use_internal: '0'
>     max_age: '0'
6c6
<   preprocess: false
---
>   preprocess: '0'
14c14
<   preprocess: false
---
>   preprocess: '0'
17c17
<   gzip: false
---
>   gzip: '0'

with the patch, a diff the saved active config:

$ diff core/modules/system/config/system.performance.yml sites/default/files/config_BvAWKAWLbeaqyibW0qSZU_SxvVXEsKa1gG0FmEq-6Y0/active/system.performance.yml 
3,4c3,4
<     use_internal: false
<     max_age: 0
---
>     use_internal: 0
>     max_age: '0'
6c6
<   preprocess: false
---
>   preprocess: 0
14c14
<   preprocess: false
---
>   preprocess: 0
17c17
<   gzip: false
---
>   gzip: 0

That's not what I expect. I thought this changed the config save format so that ints were not quoted. and I thought bools would be false or true.

alexpott’s picture

This patch does not fix what FAPI returns... if we want to do that we either have to cast the values in the form submit handler - or use the schema to somehow do this.

YesCT’s picture

+++ b/core/modules/config/tests/config_test/config/config_test.types.ymlundefined
@@ -0,0 +1,9 @@
+array: []

is array a sequence? and noted with - instead of brackets?

like:
#1942110: Make breakpoint active config save format match the default yml file.
or.. #1933548: Book allowed_types settings repetitive and in under certain conditions can change unexpectedly

Maybe it should just be:

array:

#1948284: [policy no patch] config save format and default yml file format and coding standards might have something to say about how to specify an empty array.

gdd’s picture

Status: Reviewed & tested by the community » Needs work

Sorry but I'm still completely against this, because it means our core yaml files are going to be completely inconsistent, and it will be difficult to explain to developers how and why that is true. I agree with what everyone says about being able to save things in the format that it is submitted, but that can't be at the cost of a consistent understandable file. In some cases, if some of the data comes from FAPI and some doesn't, we may even end up with bools as FALSE or as '0' *in the same file* depending on how the data is being saved! This is just ridiculous and its going to confuse the hell out of people.

Sorry I just can't get behind this.

alexpott’s picture

Issue tags: -Configuration system, -VDC, -YAML

Ok @heyrocker fair enough... I agree we have to stamp out the inconsistency...

So could we make the FAPI problem go away by adding the necessary casts to the submit handlers?

OR

using the schema to save config... which might not be a bad idea since then it would make it more likely that contrib will provide schema (a good thing I think). Actually this is the only way to go... because then drush / whatever else... can always save config correctly... and we get basic validation - like if the config is meant to contain an email address... make sure any value it has is a valid email address.

alexpott’s picture

Issue tags: +Configuration system, +VDC, +YAML

Ok @heyrocker fair enough... I agree we have to stamp out the inconsistency...

So could we make the FAPI problem go away by adding the necessary casts to the submit handlers?

OR

using the schema to save config... which might not be a bad idea since then it would make it more likely that contrib will provide schema (a good thing I think). Actually this is the only way to go... because then drush / whatever else... can always save config correctly... and we get basic validation - like if the config is meant to contain an email address... make sure any value it has is a valid email address.

webchick’s picture

Option A sounds like it depends on module developers doing special handling everywhere, which strikes me as "no." ;)

Option B would work, but makes config schemas a hard requirement, rather than a helper for multilingual config. I agree it's the only sensible way to solve this, though.

Anonymous’s picture

i'm not sure i understand the objections here.

variable_set() and variable_get() never mess with your data. also, variable_get() and variable_set() don't provide any help for you. if you put in a string and you should have put in an int, so be it.

so, this is a regression on a couple of fronts:

- if you know what you're doing, you can store types other than strings. gone
- whatever you put in, we give you the same thing back. gone

i don't think we need to require schema for casting. we should use it if it's there, so devs get rewarded for providing it.

and we should treat bogus type changes caused by cycling data through FAPI (or whatever reads then writes CMI data) as bugs.

webchick’s picture

Actually, option B wouldn't necessarily be a hard requirement. If the schema file's present, it could be used for casting; if it's not, just default to string. This would give module developers who knew what they were doing and cared about the nuance between FALSE, 0, NULL, and '0' the ability to do so, without pushing schemas as a requirement everywhere.

I forget also why we don't want schemas a requirement everywhere? What was the Cliff's Notes there?

webchick’s picture

Re #73: The difference is that when datatypes are randomly swapping out from underneath you in D7, because you happened to submit a form that contained a value you may or may not have even changed, the only thing that knows that is the variable table. A site builder never had to be confronted with that fact; it happened totally transparently to them.

In Drupal 8, the difference between 1 and "1" is a diff when doing deployment changes that you don't remember ever doing. That's a big WTF.

I think what beejeebus is arguing though is "Yes, that IS a bug. So fix those on a one-off basis." Which is a fair stance to take, I guess, but given the requirement for module developers to spend time chasing down weird FAPI auto-casting-inspired bugs or just creating a schema file consistently, I'd prefer the latter. Seems much less frustrating.

webchick’s picture

Oh, the other option of course is fixing FAPI so it respects data types. :P I asked chx about this and he said:

"i wanted to release before 2015
the problem is the same as with any other generic fapi patch
there are MANY forms
he last time we did a wholesale conversion it made moshe think we will never release
since then surely did the community grow but also the core grown.
you need to tell fapi what the types and while most of it surely can be handled on a hook_elements level some of it will need to go to per element"

So um. Let's not try and tackle this here. ;)

catch’s picture

I think it's OK to use the schema here. If schema's not defined then a UI might not be either?

Are there real cases where you'd not define a schema and expect everything else to work perfectly?

Gábor Hojtsy’s picture

@catch: many people argued when config schemas were introduced that they should be kept as optional nice to haves that they don't need to think about at least in their contribs. Also, there is a very minimal set of people who actually worked on introducing the actual schema files (huge-huge props to @vijaycs85 especially). It would be great for them to get more real-life use so they are exposed to more than a couple people :D

gdd’s picture

Yes, right now the schemas are completely optional from a functionality perspective. I believe the value they offer is identifying strings for translators, so even all the multilingual functionality will work without them. Gabor can correct me on this if I'm wrong.

I have a really icky feeling about making the schema required. They are not that easy to grok and write, and module developers have so many hurdles this release. I still continue to believe that casting everything to strings is our most sane option. (note i didnt say best)

webchick’s picture

This is all true. However, the advantage is that there are now multiple dozens of schema examples in core to copy/paste/modify from. I also found the documentation at http://drupal.org/node/1905070 quite excellent.

gdd’s picture

When you compare the learning curve of 'Everything is a string, the end' to the learning curve of 'Define this stuff over here in this nested schema file that is in yet another format you have to learn, and define the types you want to have in your main file. Don't forget to take into account the mapping for types like email, path, label, etc.'

I seriously and in my heart don't see what the horrible deal is with having everything be strings. It is simple and straightforward and it *functionally* hurts absolutely nothing. I'm obviously on the wrong side of this fight here so I'll drop it if I can't convince anyone else but man, this one seriously boggles my mind.

webchick’s picture

Well, #73 lays out the cons. It's a DX trade-off either way. We're either forcing people to write schema files with obtuse syntax (which then has the benefit of providing the module with multilingual support) or we're forcing them to deal with frustrating and un-workaroundable typing issues that they don't deal with anywhere else in PHP/Drupal (except Form API, I guess...).

One idea that could be a possible compromise is, if a schema file exists, use it for data typing. If it doesn't, default to strings. Justin made that recommendation on IRC, but not sure if made it to the issue. Could that work?

webchick’s picture

Or, I guess the other option is the last sentence of #73. We remove the string casting, and find places where things aren't typecast properly and cause diffs just due to saving a form, and fix them one by one.

I don't have a good sense of what that would look like for module developers. The hunks in BlockStorageUnitTest.php don't seem all that onerous, and is in fact how I would've written that code (and my YAML file, for that matter) anyway, had I not known that CMI was "special." But if a module developer would need to throw (bool) and (int) casts on every other line that would obviously be extremely annoying.

catch’s picture

Hmm I thought the compromise in #82 was the suggestion here... - use the schema if available, everything's a string if not.

webchick’s picture

Hm. This patch seems to remove the castValue() function in its entirety, so I don't think so. I think this patch is doing #83.

amateescu’s picture

You are both right, that suggestion was made in IRC some while ago and @heyrocker posted it in the last paragraph of #53, but the current patch does *not* implement it.

tkoleary’s picture

@webchick: We're either forcing people to write schema files with obtuse syntax [ ] or we're forcing them to deal with frustrating and un-workaroundable typing issues

As part of my exploration of "big ideas" I have come across a very interesting new animal, the WYSIWYM (what you see is what you mean). It adds RDFa and microdata options to WYSIWYG. Here is the Tiny MCE demo: http://rdface.aksw.org/new/tinymce/examples/rdface.html

My purpose was more centered around content, not configuration but given your comment perhaps we should strive for a unified solution that improves both UX and DX.

I have already reached out to the lead of the project and asked if they had any plans to extend this to CK.

Gábor Hojtsy’s picture

Yeah, the configuration system is using a very simple and easy to type YAML format which is not really RDFa or (attached) microdata compatible. Which is why we introduced the schema system in the first place to put on some level of metadata we needed about the structure and types from the outside. Expressing that data within the YML would break the simplicity which is/was its #1 selling point.

tkoleary’s picture

@webchick, @catch

This is also very interesting, if you are not already aware of it: http://www.easyrdf.org/

I think the WYSIWYM above depends on this

tkoleary’s picture

@Gábor Hojtsy

Ah. I see. Well nevertheless there are some tangential benefits that can be gleaned from continuing exploration of the following idea:

"how can we build interfaces for both users and developers that enable them to write fully machine-readable strings effortlessly using natural language."

Perhaps at a future point we will be able to expand that concept into this area.

YesCT’s picture

So is next step here to implement the approach:
if a schema file exists, use it for data typing. If it doesn't, default to strings.
?

gdd’s picture

I still don't like that approach, as it is going to introduce a great deal of inconsistency in the YAML files, which is going to confuse new developers looking through these files for examples of how to format their own yaml. If we go this route I think we need to make schemas required, which I'm also against.

scor’s picture

#90:

"how can we build interfaces for both users and developers that enable them to write fully machine-readable strings effortlessly using natural language."

I don't think this is achievable given the timeframe for core, and it needs more research, prototypes and user feedback on the usability. Definitely an approach to explore as a contrib initiative à la spark.

tim.plunkett’s picture

I just got burned by this while porting the fullcalendar module.
I very carefully trick FAPI and Views to get of the different options to be cast to the right type, but CMI turns them into strings, and the fullcalendar jQuery library errors out because it is getting '0' instead of false.

if a schema file exists, use it for data typing. If it doesn't, default to strings.

I think that's a fair compromise...

damiankloip’s picture

Thanks for helping confirm that keeping the string casting is not a good idea! I think this is going to be one of many... We can't just think of how this will be in core, but contrib too...

damiankloip’s picture

Assigned: gdd » Unassigned
Status: Needs work » Needs review
FileSize
11.86 KB

Rerolled, it's been a while.

alexpott’s picture

So I think that this sums up what I feel about changing everything to strings nicely...

Quietly changing data is completely unacceptable in most applications (Domain-Driven Design: Tackling Complexity in the Heart of Software - Eric Evans)

http://books.google.co.uk/books?id=xColAAPGubgC&lpg=PA58&ots=q9ZBbeUR6x&...

Status: Needs review » Needs work

The last submitted patch, 1653026-96.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
12.62 KB

This should fix a couple of them, There is something weird going on for the Image field test, doesn't happen in a normal env. Something to do with the file (or not in this case) being loaded...

dawehner’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined
@@ -194,5 +195,57 @@ function testNameValidation() {
+  function testDataTypes() {

should better be public function ....

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined
@@ -194,5 +195,57 @@ function testNameValidation() {
+    $this->container->get('module_handler')->enable(array('config_test'));

It seems to be that this test should maybe better a proper unit test?

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined
@@ -194,5 +195,57 @@ function testNameValidation() {
+    $config = config($name);

config() should be better $this->container ...

Status: Needs review » Needs work

The last submitted patch, 1653026-99.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.35 KB
2.45 KB

The problem is the following.

This test creates an image field without a default image, so the default value is NULL. Previously it got casted to a string, so it seemed to be "0". This then got into entity_load, so it looked like array(0 => "0"). Now it is array(0 => NULL) and flipping does not work anymore.

It seems to be that this test should maybe better a proper unit test?

This review was not really done properly.

damiankloip’s picture

Nice work on the fix, Lets do this.

catch’s picture

Issue tags: +D8 upgrade path

Tagging with D8 upgrade path - it doesn't block it, but we need to fix this before beta, do a beta-beta upgrade path (likely impossible), or move it to 9.x if it's not done by then.

damiankloip’s picture

FileSize
14.41 KB

Just another reroll.

Status: Needs review » Needs work

The last submitted patch, 1653026-105.patch, failed testing.

mtift’s picture

Status: Needs work » Needs review
FileSize
14.44 KB

Rerol

Status: Needs review » Needs work

The last submitted patch, 1653026-107.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.52 KB
18.96 KB

Let's see how this gets on.

Status: Needs review » Needs work

The last submitted patch, 1653026-109.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: -beta blocker
FileSize
2.49 KB
21.42 KB

Let's try this.

As a note, status should be fine in the long run (even with forms) as long as setStatus/enable/disable is called we will always have a bool there.

alexpott’s picture

Issue tags: +beta blocker

If we are going to address this issue we need to do so before beta

alexpott’s picture

Issue tags: +beta blocker

xpost

Status: Needs review » Needs work

The last submitted patch, 1653026-111.patch, failed testing.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs work » Needs review
FileSize
22.06 KB
1.67 KB

Working through this, here's a couple fixes to start

Status: Needs review » Needs work

The last submitted patch, config-1653026-115.patch, failed testing.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
24.92 KB
2.86 KB

We should follow this up with switching all of the default config values like status and locked to be actual bools, and weight to be an int, but if its not needed for the tests, we shouldn't explode this patch with them.

yched’s picture

I assume this relies on the assumption that all forms that save to a config entry somehow have an explicit step that casts the submitted values to their correct type, right ?

Looks like this will require some work for config entities that include 3rd party components (e.g field-type specific settings in field / instance config entities, field / filter / sort handlers in views, image effect settings in image styles...)

tim.plunkett’s picture

Yes. The point is that if you did have that step before, CMI would undo all of your work.

jbrown’s picture

+++ b/core/modules/config/tests/config_test/config/config_test.types.yml
@@ -0,0 +1,9 @@
+array: []
+boolean: true
+exp: 1.2e+34
+float: 3.14159
+hex: 0xC
+int: 99
+octal: 0775
+string: string
+string_int: '1'

Surely strings should always be quoted to avoid being misinterpreted as something else like boolean or octal, e.g. the zip code example.

tim.plunkett’s picture

FileSize
23.95 KB

Surely strings should always be quoted

I don't disagree, but I don't know if that's in scope here. Strings without a space are fine without quotes, and this doesn't really affect that.

Straight reroll, no changes.

Status: Needs review » Needs work

The last submitted patch, config-1653026-122.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
761 bytes
23.93 KB

s/enable/install

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

this looks RTBC to me.

jibran’s picture

RTBC +1

vijaycs85’s picture

Issue tags: +Needs change record
webchick’s picture

Assigned: Unassigned » catch

Since alex wrote a lot of this, and catch had proposed a compromise solution back a few dozen comments that was not implemented here, assigning to catch.

sun’s picture

I'm not sure I understand why this is RTBC.

The discussed compromise back in #94 and before was:

if a schema file exists, use it for data typing. If it doesn't, default to strings.

However, the latest patch just removes the casting to strings entirely.

Unless I'm missing something, it does not appear to introduce any code that would (optionally) cast values according to data type definitions in the schema.

The stated compromise actually asks for EITHER casting to strings OR casting to schema-defined data types, depending on whether a schema is available.

Due to PHP's loose data typing, the compromise made sense to me. The current patch does not.


Lastly, you might want to remove the octal data type from the test, since your test results are misleading. You are not able to support octal numbers. YAML parses them correctly, but PHP interprets an octal number as an integer. When writing back to the file, 0775 becomes 509. assertIdentical() reports success, because the data type and integer value is identical.

tim.plunkett’s picture

Adding schema based casting is a great follow-up! Feel free to open that.
The current system is broken, this removes the brokenness without solving any extra problems.

sun’s picture

I think you're missing something big. The current Config::castValue() code path was added for very good technical reasons. IIRC, I originally disagreed with its addition, but was compelled after studying the (original) problems. As visible in the code, it is an extra (and expensive) operation that is executed. We wouldn't have introduced that operation if there wouldn't have been major problems. I'd have to study older issues and discuss with @heyrocker to recall why.

alexpott’s picture

As far as I recall the current Config::castValue() is a hang up from when we were storing configuration in xml with no schema. Yaml and PHP serialization both support datatypes and I think it is reasonable to demand that whatever config storage system you use it returns data exactly the same as when it was provided. This function breaks that - I stand by my comment #97.

catch’s picture

Status: Reviewed & tested by the community » Needs review

I still think supporting data types via schema would be good, but for now manually casting when necessary is better than not having the option and having certain values be wrong all the time.

Having said that though, I don't see any code here to cast values from configuration form submissions - which is where this will go wrong, don't we need to update those here as well?

Anonymous’s picture

please, no. this issue is about making the config system save what it's given, and send it back out.

fixing consumers of the config system is not for this issue. we don't have any fails, so we need to identify what if anything actually breaks, and fix the broken code.

when we fix that, we may also use schema to make it easier, but again, that is not for this issue.

catch’s picture

Title: All configuration values are stored as strings » Change notice: All configuration values are stored as strings
Category: bug » task
Status: Needs review » Active

Hmm OK. I'm fine with doing that in a follow-up. Committed/pushed to 8.x - this will need a change notice, and we should open any follow-ups before closing this out.

Anonymous’s picture

ok, i'll work on a change notice tomorrow when i'm sober.

as for the rest, i guess we need some tests? because if we broke anything, we can't prove it.

chx’s picture

I would like to see some thought put into whether assertIdentical is correct or we want to go key-by-key. AssertIdentical cares about array order and I think we don't. Or is this testing that the writing preserves the order as well?

webchick’s picture

Anonymous’s picture

sheesh. followed that issue.

chx pointed out we only really care about each value, patch coming that changes the test to do that, rather than comparing the whole lot (so we don't care about order).

andypost’s picture

This needs follow-up to convert all current .yml files to use proper types, for example 'Tags' vocabulary have all values as strings

andypost’s picture

Issue summary: View changes

reordered

vijaycs85’s picture

Create sub-task with files that need to be changed. Few things still not clear:

1. Route name/Machine names are in quotes on some files and other files not.
2. most of the info.ymls have true and other places we have TRUE
3. Version number in info are in quotes and some place no quote.

We have to make some standard around the above and update necessary files to the individual sub-tasks.

webchick’s picture

Honestly, it feels like this was too premature. Everything failed super hard today when I went to demo this functionality in front of people at PNWDS. Did anyone test this patch with a real dev/live deployment scenario?

webchick’s picture

Gábor Hojtsy’s picture

I've been thinking what implications does this have for #1952394: Add configuration translation user interface module in core, which is another big producer of .yml files in config, which instead of updating files, it creates files based on the patterns of other files. I think this does not (yet?) apply there given the translation module works with all strings, no other types. If someone sees an issue there, we are welcome to adapt to the new ways - as always :)

chx’s picture

chx’s picture

Issue summary: View changes

Adding sub-task section.

vijaycs85’s picture

Issue summary: View changes

Warning for sub-tasks

vijaycs85’s picture

Issue summary: View changes

Updated issue summary.

krishnan.n’s picture

FileSize
326.51 KB

Hi,

Will this (brute-force) script work:

#!/bin/bash
# Usage: Save file and run this from drupal root dir. For ex:  ./stripquote.sh
# TODO: replace grep pattern with better find params; -path 's lesser cousin should work.
find . -name '*.yml' | grep "\/modules\/\<[a-z]*\>\/config\/.*yml\>"  | xargs sed -i -e s/\'//g

Is something like the replacement below OK?

- items_per_page_label: 'Items per page'
- items_per_page_options: '5, 10, 20, 40, 60'
...
+ items_per_page_label: Items per page
+ items_per_page_options: 5, 10, 20, 40, 60

Thanks,
krishnan

krishnan.n’s picture

Status: Active » Needs review

Hi,

Changing status to 'needs review'.

Thanks,
krishnan

Status: Needs review » Needs work

The last submitted patch, config-1653026-146.patch, failed testing.

krishnan.n’s picture

Status: Needs work » Needs review
FileSize
69.39 KB

Converting all '0' to false and '1' to true.

#!/bin/sed -f
# convert 0 to false and '1' to true.
s/\'//g
s/\<0\>/false/g
s/\<1\>/true/g;

script to locate all yaml config files and then call the sed script above.

#!/bin/bash
# Usage: Save file and run this from drupal root dir. For ex:  boolconvert.sh
# TODO: include the multi-line sed into this file itself (RTFM running multi-line sed inside shell)
find . -name '*.yml' | grep "\/modules\/\<[a-z]*\>\/config\/.*yml\>"  | xargs sed -i -f ../boolconvert.sed

Notes:
1. need to run script in sequence, call sed script boolconvert.sed.
2. comments in #147 apply.

Status: Needs review » Needs work

The last submitted patch, config-1653026-149.patch, failed testing.

alexpott’s picture

Hi @krishnan.n thanks for your work here but this issue is "active" to get the change notice written (or an existing one updated). #2106459: Core config has everything as string already did most of the work that can be scripted. Unfortunately your scripts are a bit too greedy as well - we can not assume that all 0s and 1s should be true or false. Plus an empty string '' is meaningful and strings with a space should be surrounded by single quotes. If you want to help ensure that all the values are correct please help us work through the sub tasks in the issue summary and discover what still needs to be done. The important thing with default configuration files is that once the file is imported to active store when you diff the two files the only differences seen should be expected.

vijaycs85’s picture

Status: Needs work » Active

Thanks for working on this @krishnan.n, but this issue is to fix the type casting that we do on main process of configs. We have a list of follow-ups (https://drupal.org/node/1653026#sub-tasks) to fix individual modules. For your patch, you can either update them on subtask or raise a new issue (version 2 of #2106459: Core config has everything as string), so that we can leave this issue active for change notice.

krishnan.n’s picture

Alex, Vijay thanks for the inputs. Pl check if my fix 2105905 is OK?

krishnan.n’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

catch’s picture

Assigned: catch » Unassigned

Unassigning me.

yanniboi’s picture

Assigned: Unassigned » yanniboi
Status: Active » Needs review

Here is a draft for the change notice:

Summary

Currently in Drupal Core we're converting all config values to strings for storage because strings are common denominator for all possible storage engines. Since the storage process currently only deals with serialized PHP and YAML files, both of which support primitive data types, we are removing this forced type casting so that config values can be stored and retrieved in the same format without having to be converted into a string in the process.

  • No conversion necessary for storage and retrieving from storage
  • No need to check whether config values are numeric or boolean when retrieving strings from storage

Storage Example:

  // An integer value.
  $items_limit = 10;
  \Drupal::config('system.rss')->set('items.limit', $items_limit)->save();
  var_dump(\Drupal::config('system.rss')->get('items.limit'));

Before this change this would output:

string(2) "10"

After this change it outputs:

int(10)

Examples:

Integers:
Before
weight: '0'
After
weight: 0

Booleans:
Before
status: '1'
After
status: true

Null:
Before
'visibility' => ''
After
'visibility' => NULL

jessebeach’s picture

So we are removing the enforced type-casting and moving all config data strings.

Where are we moving the config data strings to? The last part of that phrase is a touch hard to parse, can you clarify please?

yanniboi’s picture

You are correct, I could hardly make sense of that myself..
I have changed it to read:

So we are removing the enforced type-casting and changing all configuration values to be stored as strings.

alexpott’s picture

In Drupal Core we're dealing with YAML and serialized PHP. Both of them support primitive data types. Strings are common denominator for all possible storage engines. So we are removing the enforced type-casting and changing all configuration values to be stored as strings.

@yanniboi actually this paragraph is a bit misleading. This patch removed casting all values to strings so that we are now using YAML's ability to store scalar values that represent strings, integers, dates, and other atomic data types (see the spec). This means that a Config API stores values exactly as provided and will return them that way too. Perhaps the best way to illustrate this is like this:

  // An integer value.
  $items_limit = 10;
  \Drupal::config('system.rss')->set('items.limit', $items_limit)->save(); 
  var_dump(\Drupal::config('system.rss')->get('items.limit')); 

Before this change this would output:

string(2) "10"

After this change it outputs:

int(10)
yanniboi’s picture

Thanks @alexpott, that is very clear.

I have updated the change notice accordingly.

webchick’s picture

Thanks, yanniboi! Could you go ahead and add this as a change notice, so we can close this out?

I'd much prefer drafts to just go live as change record nodes immediately, rather than in comments, because even if they're not perfect, people can at least find them. :)

yanniboi’s picture

Title: Change notice: All configuration values are stored as strings » [META] All configuration values are stored as strings
Status: Needs review » Active
Issue tags: -Needs change record

@webchick, that's all I needed to hear.
Here is the change record: [#2153567].

This issue can now serve as a meta for the sub-issues I think.

yanniboi’s picture

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

Title: [META] All configuration values are stored as strings » [META] Use properly typed values in module configuration
Issue summary: View changes
Issue tags: -Needs change record

In the meantime I also copied and lightly edited yanniboi's suggestion. That change notice is at https://drupal.org/node/2153569. Eg. instead of saying "Drupal core currently does string casting", which is not true, we should say it used to :) Also the title of "All configuration values are stored as strings" of yanniboi's change notice was exactly what we are moving away from, so not very good to highlight the change IMHO. Sorry, but deleted his change notice and kept my copy, since we had two. Please edit that as needed.

Also since the META title is not anymore true here, adjusting to be more up to date.

xjm’s picture

Priority: Major » Critical

It does not make much sense to postpone these changes to Drupal 9, and this does represent configuration schema changes. Bumping to critical accordingly after discussion with @webchick and @effulgentsia.

xjm’s picture

Hmm, actually, we should have a separate critical for the meta. The original change here has been resolved.

vijaycs85’s picture

Issue summary: View changes
alexpott’s picture

yanniboi’s picture

Assigned: yanniboi » Unassigned

Unassigning myself.

vijaycs85’s picture

All in-progress sub-tasks are closed as they all moved to #2167623: Add test for all default configuration to ensure schema exists and is correct. So setting this meta fixed.

Gábor Hojtsy’s picture

Status: Fixed » Closed (duplicate)

Yay, thanks! Duplicate would be more appropriate.