Updated: Comment #0

Problem/Motivation

in #1653026: [META] Use properly typed values in module configuration we removed the automatic casting of all config scalar values to strings.

Proposed resolution

Use typed config during Config::save() ensure the values are cast to the correct type.

Remaining tasks

Review

User interface changes

N/a

API changes

Adds get($key) method to Drupal\Core\Config\Schema\Sequence so that schema wrapper objects for a particular key can be returned.

All changes to Drupal\Core\Config are internal - if we need to open this up in the future we can change the protected methods added to public.

Files: 
CommentFileSizeAuthor
#123 d8_typed_inheritdoc.patch2.4 KBfago
PASSED: [[SimpleTest]]: [MySQL] 59,910 pass(es). View
#119 2130811.119.patch43.45 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,912 pass(es). View
#119 115-119-interdiff.txt4.04 KBalexpott
#115 2130811.115.patch43.81 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,768 pass(es). View
#115 111-115-interdiff.txt5.07 KBalexpott
#114 2130811-save-schema-114.patch42.57 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 59,888 pass(es). View
#114 2130811-diff-111-114.txt1.54 KBvijaycs85
#111 2130811.111.patch42.25 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,821 pass(es). View
#111 107-111-interdiff.txt1.28 KBalexpott
#107 2130811.107.patch42.24 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,738 pass(es). View
#107 104-107-interdiff.txt2.66 KBalexpott
#104 2130811.104.patch42.05 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,887 pass(es). View
#104 101-104-interdiff.txt5.14 KBalexpott
#101 2130811.101.patch38.9 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,504 pass(es), 0 fail(s), and 1 exception(s). View
#101 interdiff-100-101.txt1.35 KBalexpott
#100 2130811.100.patch38.85 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,723 pass(es). View
#100 interdiff-93-100.txt569 bytesalexpott
#93 config.typed-values.93.patch39.58 KBsun
FAILED: [[SimpleTest]]: [MySQL] 59,726 pass(es), 9 fail(s), and 0 exception(s). View
#91 interdiff.txt2.45 KBsun
#85 2130811.85.patch40 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 59,414 pass(es), 2 fail(s), and 0 exception(s). View
#85 interdiff-string.txt2.54 KBGábor Hojtsy
#84 interdiff-installer.txt2.51 KBGábor Hojtsy
#84 2130811.84.patch39.79 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 59,353 pass(es), 1 fail(s), and 0 exception(s). View
#79 2130811.79.patch38.78 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#79 interdiff-null.txt4.31 KBGábor Hojtsy
#74 2130811.74.patch38.63 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#74 interdiff-injection.txt15.45 KBGábor Hojtsy
#65 2130811.65.patch26.42 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 59,354 pass(es). View
#54 2130811.53.patch26.42 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 59,485 pass(es). View
#54  interdiff.txt1003 bytesGábor Hojtsy
#51 interdiff.txt4.62 KBGábor Hojtsy
#51 2130811.51.patch26.55 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 59,475 pass(es). View
#47 interdiff-44-46.txt3.14 KBalexpott
#46 2130811.45.patch26.4 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,483 pass(es), 2 fail(s), and 1 exception(s). View
#44 2130811.44.patch26.38 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,461 pass(es). View
#43 8258949-config-save-string-43.patch26.32 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#43 8258949-diff-37-43.txt969 bytesvijaycs85
#37 2130811.37.patch25.37 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 59,420 pass(es), 2 fail(s), and 0 exception(s). View
#35 34-35-diff.txt3.68 KBalexpott
#35 2130811.35.patch25.38 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2130811.35.patch. Unable to apply patch. See the log in the details link for more information. View
#34 2130811-diff-26-34.txt770 bytesvijaycs85
#34 2130811-save-schema-34.patch27.78 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 59,026 pass(es). View
#26 2130811.26.patch27.03 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,138 pass(es), 2 fail(s), and 0 exception(s). View
#21 interdiff.txt6.4 KBWim Leers
#21 config_form_enforce_schema-2130811-21.patch28.52 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#15 2130811.15.patch24.88 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 61,016 pass(es). View
#15 12-15-interdiff.txt968 bytesalexpott
#12 2130811.12.patch24.82 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 61,125 pass(es), 0 fail(s), and 2 exception(s). View
#12 11-12-interdiff.txt1.69 KBalexpott
#9 2130811.11.patch23.45 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 61,162 pass(es), 0 fail(s), and 2 exception(s). View
#9 8-11-interdiff.txt2.24 KBalexpott
#8 2130811.8.patch21.17 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 61,064 pass(es), 0 fail(s), and 2 exception(s). View
#8 4-8-interdiff.txt1.56 KBalexpott
#4 2130811.4.patch19.49 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#4 2-4-interdiff.txt5.69 KBalexpott
#2 2130811.2.patch16.81 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 60,808 pass(es), 23 fail(s), and 448 exception(s). View

Comments

alexpott’s picture

The default config for system.performance.yml is

cache:
  page:
    use_internal: '0'
    max_age: 0
css:
  preprocess: false
  gzip: true
fast_404:
  enabled: '1'
  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

After changing some value on the form (admin/config/development/performance) and pressing save it is

cache:
  page:
    use_internal: 1
    max_age: '1800'
css:
  preprocess: 1
  gzip: true
fast_404:
  enabled: '1'
  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: 1
  gzip: true
response:
  gzip: 0
stale_file_threshold: 2592000
theme_link: true

Note that some of the key/values stored in this file are not editable on through this form.

alexpott’s picture

Status: Active » Needs review
Issue tags: +Needs issue summary update
FileSize
16.81 KB
FAILED: [[SimpleTest]]: [MySQL] 60,808 pass(es), 23 fail(s), and 448 exception(s). View

Okay here is a patch that enforces the data type from a schema on save.

will update issue summary later reflect what this patch is doing and what's been discovered. As the patch shows the current schemas are far from complete and it's even discovered unnecessary configuration (system.maintenance should not have a enabled key!)

alexpott’s picture

FileSize
5.69 KB
19.49 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Let's see where this gets us

alexpott’s picture

Status: Needs work » Needs review
Related issues: +#2132551: Picture module uses config keys with a dot
FileSize
1.56 KB
21.17 KB
FAILED: [[SimpleTest]]: [MySQL] 61,064 pass(es), 0 fail(s), and 2 exception(s). View

So yay #2132551: Picture module uses config keys with a dot is causing us problems! Since this means the current schema for picture mapping entity the patch attached to this comment just comments out the schema.

alexpott’s picture

FileSize
2.24 KB
23.45 KB
FAILED: [[SimpleTest]]: [MySQL] 61,162 pass(es), 0 fail(s), and 2 exception(s). View

Adding tests

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
24.82 KB
FAILED: [[SimpleTest]]: [MySQL] 61,125 pass(es), 0 fail(s), and 2 exception(s). View

The field instance schema for the options lists are complete incorrect saying that they are a mapping and then declaring a sequence. In fact these field instances have no settings afaics so rather than saying anything about what the sequences contain I think we should just leave this empty.

alexpott’s picture

Explaining some of the changes.

  1. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -459,4 +483,125 @@ public function merge(array $data_to_merge) {
    +      try {
    +        $class = get_class($this->getSchemaForKey($key));
    +      }
    +      catch(\Exception $e) {
    +        // @todo throw an exception due to an incomplete schema
    +        $class = FALSE;
    +      }
    

    If we remove this try catch block then we will error on config files that have schema but do not conform to it. At the moment filter and views config entity do not conform.

  2. +++ b/core/lib/Drupal/Core/Config/Schema/Mapping.php
    @@ -27,7 +27,7 @@ class Mapping extends ArrayElement implements ComplexDataInterface {
    -      if (isset($this->value[$key])) {
    +      if (array_key_exists($key, $this->value)) {
    

    This allows values to be null

  3. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockStorageUnitTest.php
    @@ -92,11 +92,11 @@ protected function createTests() {
    -      'region' => -1,
    +      'region' => '-1',
    

    Regions are strings the -1 value is being used to denote no region.

  4. +++ b/core/modules/number/config/schema/number.schema.yml
    @@ -12,10 +12,10 @@ field.number_integer.instance_settings:
    -      type: integer
    +      type: string
    ...
    -      type: integer
    +      type: string
    
    @@ -51,10 +51,10 @@ field.number_decimal.instance_settings:
    -      type: integer
    +      type: string
    ...
    -      type: integer
    +      type: string
    
    @@ -71,7 +71,7 @@ field.number_decimal.value:
    -           type: integer
    +           type: string
    
    @@ -86,10 +86,10 @@ field.number_float.instance_settings:
    -      type: integer
    +      type: string
    ...
    -      type: integer
    +      type: string
    
    @@ -106,5 +106,5 @@ field.number_float.value:
    -          type: integer
    +          type: string
    

    The changes are are because floats and decimals are not integers and the max and min have to be able to be nulls or empty strings. Casting a null or empty string to integer results in a max and min of 0 :D

  5. +++ b/core/modules/system/config/system.maintenance.yml
    @@ -1,3 +1,2 @@
    -enabled: '0'
    

    This was moved to state in #2084339: The configuration value system.maintenance.enabled should be moved to the state system - I forgot to remove this value - if we enforce the schema to match what we have in config then this would have produced an error.

alexpott’s picture

FileSize
968 bytes
24.88 KB
PASSED: [[SimpleTest]]: [MySQL] 61,016 pass(es). View

Ok the field instance schema for option's fields are causing a problem - because they are just empty - ie. have no schema.

alexpott’s picture

Status: Needs work » Needs review
alexpott’s picture

So the exceptions in the module install uninstall test in patches attached to #8, #9 and #12 are caused by having the content_translation module enabled before installing the options module. Content translation injects a setting into each and every field instance's settings - translation_sync - so there is no way the schema can reflect this. What can we do here? I'm not sure that we should allow dumping grounds in other module's config entities.

Also the statement in #15

Ok the field instance schema for option's fields are causing a problem - because they are just empty - ie. have no schema.

is not true once the content_translation module is enabled.

xjm’s picture

Issue tags: +beta blocker
damiankloip’s picture

Great, content_translation modules strikes again...

vijaycs85’s picture

Thanks for the issue & patch @alexpott. Overall, it is great improvement & utilisation of config schemas. Here is some reviews comments:

  1. Reg #14.5 - if we enforce the schema to match what we have in config then this would have produced an error. - Not sure we really want to do as most of the time schema holds all possible config data whereas the config file would have sub-set of it.
  2. +++ b/core/modules/picture/config/schema/picture.schema.yml
    @@ -1,33 +1,33 @@
    -picture.mappings.*:
    -  type: mapping
    -  label: 'Picture mapping'
    -  mapping:
    -    id:
    -      type: string
    -      label: 'Machine-readable name'
    -    uuid:
    -      type: string
    -      label: 'UUID'
    -    label:
    -      type: label
    -      label: 'Label'
    -    mappings:
    -      type: sequence
    -      label: 'Mappings'
    -      sequence:
    -        - type: sequence
    -          label: 'Mapping'
    -          sequence:
    -            - type: string
    -              label: 'Image style'
    -    breakpointGroup:
    -      type: string
    -      label: 'Breakpoint group'
    -    status:
    -      type: boolean
    -      label: 'Status'
    -    langcode:
    -      type: string
    -      label: 'Default language'
    +#picture.mappings.*:
    +#  type: mapping
    +#  label: 'Picture mapping'
    +#  mapping:
    +#    id:
    +#      type: string
    +#      label: 'Machine-readable name'
    +#    uuid:
    +#      type: string
    +#      label: 'UUID'
    +#    label:
    +#      type: label
    +#      label: 'Label'
    +#    mappings:
    +#      type: sequence
    +#      label: 'Mappings'
    +#      sequence:
    +#        - type: sequence
    +#          label: 'Mapping'
    +#          sequence:
    +#            - type: string
    +#              label: 'Image style'
    +#    breakpointGroup:
    +#      type: string
    +#      label: 'Breakpoint group'
    +#    status:
    +#      type: boolean
    +#      label: 'Status'
    +#    langcode:
    +#      type: string
    +#      label: 'Default language'
    

    Intentional?

  3. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -459,4 +483,125 @@ public function merge(array $data_to_merge) {
    +   * Sets the schema wrapper property using the current configuration data
    

    Minor: Missing '.' at the end. Surely not a blocker :)

Wim Leers’s picture

Title: Use typed config in saving and validating configuration to ensure data is consistent and correct » Use config schema in saving and validating configuration form to ensure data is consistent and correct
Related issues: +#2105939: Make sure all YML files in Editor module has no type-casting to string + config system changes broke image uploading in editors, +#2129399: Use configuration schema for configuration form type casting
FileSize
28.52 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
6.4 KB

#20.2: yes, that's intentional, see #8.


Despite this being opened later, I have marked #2129399: Use configuration schema for configuration form type casting as a duplicate.


In #2105939: Make sure all YML files in Editor module has no type-casting to string + config system changes broke image uploading in editors, I introduced manual typecasting to ensure correct .yml files for editor.module (which is what spawned #2129399). I'm ecstatic to be able to delete that code here :)

In this reroll, I fixed a bunch of nitpicks (including #20.3) and removed the code I introduced in #2105939. I don't know enough about the config system to do a solid review.

Wim Leers’s picture

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -506,14 +508,13 @@ public function getSchemaWrapper() {
-    $typed_config_manager = $this->getTypedConfigManager();
-    $definition = $typed_config_manager->getDefinition($this->name);
+    $definition = $this->getTypedConfigManager()->getDefinition($this->name);

This apparently makes the installer fail. But why? How is this different than what was there before?

alexpott’s picture

Status: Needs work » Needs review
FileSize
27.03 KB
FAILED: [[SimpleTest]]: [MySQL] 59,138 pass(es), 2 fail(s), and 0 exception(s). View

@Wim Leers thanks for the fixing all the nits and removing the unnecessary code in editor! Yep this patch should mean that all modules need to do is declare the correct config schema and everything will just work (tm) :D

There was a slight error in the refactoring to Config::setSchemaWrapper()...

Interdiff was weird... here is the relevant bit:

    *   The configuration object.
    */
   public function setSchemaWrapper() {
-    $definition = $this->getTypedConfigManager()->getDefinition($this->name);
+    $typed_config_manager = $this->getTypedConfigManager();
+    $definition = $typed_config_manager->getDefinition($this->name);
     $this->schemaWrapper = $typed_config_manager->create($definition, $this->data);
     return $this;
   }
yched’s picture

alexpott’s picture

26: 2130811.26.patch queued for re-testing.

xjm’s picture

Fail is:

Drupal\config_translation\Tests\ConfigTranslationListUiTest
Raw "<th>Language</th>" found	Other	ConfigTranslationListUiTest.php	380	Drupal\config_translation\Tests\ConfigTranslationListUiTest->doPictureListTest()	

Not sure if the last fail was the same or not, so triggering a retest just to be sure.

xjm’s picture

Status: Needs work » Needs review

26: 2130811.26.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 26: 2130811.26.patch, failed testing.

xjm’s picture

Same fail again, plus we picked up another. Fab. Both also failed when I ran the test on simplytest.me under minimal. The verbose result immediately preceding the config whatever list failed assertion shows "access denied" so that one at least should be easy to solve. :)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
27.78 KB
PASSED: [[SimpleTest]]: [MySQL] 59,026 pass(es). View
770 bytes

As per #8, we don't have picture schema anymore, so need to comment out picture test?

alexpott’s picture

FileSize
25.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2130811.35.patch. Unable to apply patch. See the log in the details link for more information. View
3.68 KB

Now that #2132551: Picture module uses config keys with a dot has landed we can re-instate the picture test and not comment out the schema.

This was a reroll too so interdiff not possible so attaching diff of 34 and 35.

The last submitted patch, 35: 2130811.35.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
25.37 KB
FAILED: [[SimpleTest]]: [MySQL] 59,420 pass(es), 2 fail(s), and 0 exception(s). View

Rerolled. It was conflicting in edit module due to getFieldSettings() => getSettings() change. No change.

Gábor Hojtsy’s picture

Reviewed the changes in the patch too. The schema changes are beautiful :) The only thing that stepped out is:

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -468,4 +494,125 @@ public function merge(array $data_to_merge) {
+        // @todo throw an exception due to an incomplete schema

Should we open an issue for this or do this right away here?

Otherwise I'd say this look as good as it can :)

Status: Needs review » Needs work

The last submitted patch, 37: 2130811.37.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

37: 2130811.37.patch queued for re-testing.

Gábor Hojtsy’s picture

"Setup environment: failed to create checkout database." was the fail. No wonder it took so long to come back :)

Status: Needs review » Needs work

The last submitted patch, 37: 2130811.37.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
969 bytes
26.32 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Fixing test fails...

alexpott’s picture

FileSize
26.38 KB
PASSED: [[SimpleTest]]: [MySQL] 59,461 pass(es). View

And improving the @todo :) mentioned by @Gabor

        // @todo throw an exception due to an incomplete schema once
        //   https://drupal.org/node/1910624 is complete.

The last submitted patch, 43: 8258949-config-save-string-43.patch, failed testing.

alexpott’s picture

Issue summary: View changes
FileSize
26.4 KB
FAILED: [[SimpleTest]]: [MySQL] 59,483 pass(es), 2 fail(s), and 1 exception(s). View

Ho hum I really can not write English sentences! Fixed the @todo to be

        // @todo throw an exception due to an incomplete schema. Only possible
        //   once https://drupal.org/node/1910624 is complete.

Whilst reviewing the patch and the issue summary I realised that the methods added to Config were mostly public although we currently have no use case. This was because I originally planned to implement this functionality in ConfigFormBase.

I've updated the issue summary as well to reflect the API addition.

alexpott’s picture

FileSize
3.14 KB

Forgot to add an interdiff :(

The last submitted patch, 46: 2130811.45.patch, failed testing.

sun’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -468,4 +494,125 @@ public function merge(array $data_to_merge) {
    +  public function getSchemaWrapper() {
    ...
    +  public function setSchemaWrapper() {
    ...
    +  protected function getTypedConfigManager() {
    

    There's an inconsistency between getSchemaWrapper() + setSchemWrapper() and getTypedConfigManager() here; the latter lacks a setter method.

    I don't know whether we have an architectural standard for this in D8, but if there is any point in having a separate setter method (tests? or what is it for?), then we should consistently apply it - even more so, within the scope of a single class.

    If there is no technical reason for the separate setter, then I wonder why the getter for the schema wrapper doesn't also use the on-the-fly inline instantiation as in the getter for the typed config manager?

    I don't have a preference, all I'm pointing out and asking for is consistency. :)

  2. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -468,4 +494,125 @@ public function merge(array $data_to_merge) {
    +    if (empty($this->schemaWrapper)) {
    ...
    +    if (empty($this->typedConfigManager)) {
    

    Can we use !isset() here?

    empty() only makes sense if you expect a value of FALSE to have a meaning when compared to NULL.

    Wherever services are conditionally injected, isset() should be sufficient (and faster).

  3. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -468,4 +494,125 @@ public function merge(array $data_to_merge) {
    +        case 'Drupal\Core\TypedData\Plugin\DataType\Integer':
    +          $value = (int) $value;
    +          break;
    

    Do we want to add Floats?

    TypedData has a type for floats, and at least YAML supports floats, too.

  4. +++ b/core/lib/Drupal/Core/Config/Schema/Mapping.php
    @@ -27,7 +27,7 @@ class Mapping extends ArrayElement implements ComplexDataInterface {
    -      if (isset($this->value[$key])) {
    +      if (array_key_exists($key, $this->value)) {
    

    Do we want to use the performance-optimized combined isset() || array_key_exists() pattern here?

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy

I'll take a crack at those :)

Gábor Hojtsy’s picture

FileSize
26.55 KB
PASSED: [[SimpleTest]]: [MySQL] 59,475 pass(es). View
4.62 KB

@sun:

1. Looked into this and discussed with @alexpott on IRC. Basically the schema wrapper depends on the config name *and* the whole data array. So if either the name or anything in the data array changes, the schema wrapper is not valid anymore. The patch had/has no invalidations sprinkled all around because this was introduced as protected internal functionality. So we can control it when we use it, and we only use it on save() which is also when we reset it. So the updated patch includes setting that value to NULL and integrates the set method into the get method. Logically doing the same thing either way. Added some more doc on the getter method about how the wrapper becomes stale quick.

2. Done.

3. Not only typeddata but also config schema has float types. I checked the system types and that was indeed the only one missed. Added coverage for casting and tests.

4. Done.

I also ran the image field display tests that failed above, but those did not fail for me locally. Will see if it fails again on testbot :)

Gábor Hojtsy’s picture

All right, passed :) Any other concerns? :) AFAIS this should be ready to go in. And very timely :)

amateescu’s picture

  1. +++ b/core/includes/config.inc
    @@ -212,9 +212,9 @@ function config_get_entity_type_by_name($name) {
    - * @see \Drupal\Core\TypedData\TypedDataManager::create()
    

    There is no \Drupal\Core\Config\TypedDataManager ?

  2. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -468,4 +494,122 @@ public function merge(array $data_to_merge) {
    +          $value = (bool) $value;
    

    Casting to boolean is not so simple, I'd recommend filter_var($value, FILTER_VALIDATE_BOOLEAN).

  3. +++ b/core/lib/Drupal/Core/Config/Schema/Sequence.php
    @@ -49,4 +49,19 @@ public function onChange($delta) {
    +   * @param $key
    

    Missing 'string'.

Gábor Hojtsy’s picture

FileSize
1003 bytes
26.42 KB
PASSED: [[SimpleTest]]: [MySQL] 59,485 pass(es). View

Fixed 1 and 3. The TypedDataManager is indeed not in config, although the TypedConfigManager obviously is. Can you elaborate on 2? Provide a few examples as to why this is not good enough? Why is it good for ints or floats then?

amateescu’s picture

Gábor Hojtsy’s picture

Sounds to me casting 'yes' and 'on' to TRUE while casting 'off' and 'no' (and especially 'false') to FALSE is a feature request? The task in this issue is to ensure the type of the values matches the expected types by the schema, not to support magic values for them that cast to values in proper types.

penyaskito’s picture

#55 I thought we wanted to make configuration idempotent. Allowing those can make tools like diff for comparing configs could behave weird because of the differences, while the underlying value could be the same.

amateescu’s picture

Cool, carry on then.

Gábor Hojtsy’s picture

@penyaskito: no the proposal of #55 was that if you do

\Drupal::config('somekey')->set('key', 'off');

And you have a schema defining 'key' as a boolean, it would save as false (whereas with the current patch it would save as true). So basically extend the scope of what PHP considers false/true values to more magic values. I think this is definitely out of scope for this issue and should be a followup.

Gábor Hojtsy’s picture

@amateescu: any other concerns or do you think this is good to go then? :)

sun’s picture

I agree with @Gábor. Intelligent filtering of magic (INI) string values into Booleans could lead to unexpected behavior. While configuration files may be edited by humans, humans need to respect the data type representations of the serialization format being used.

amateescu’s picture

Sure, it's a one line change/improvement, it definitely needs to be a followup.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

No other concerns :)

Wim Leers’s picture

I agree with Gábor and sun too.

I reviewed the patch again, could find only one nitpick. But I don't feel I could RTBC this, because I don't know CMI well enough — I may very well be missing things. All I can say is that I strongly support this patch and that it feels like it's getting close.

Ideally, somebody with comprehensive CMI knowledge would review this patch to get it to RTBC.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTest.php
@@ -242,4 +246,47 @@ function testSchemaData() {
+  public function testConfigSaveWithSchema () {

The space between the function name and the parentheses shouldn't be there.

Gábor Hojtsy’s picture

FileSize
26.42 KB
PASSED: [[SimpleTest]]: [MySQL] 59,354 pass(es). View

@WimLeers: thanks, fixed (by hand-editing, no interdiff attached).

@amateescu: It may be a one line change if we discount any documentation and test coverage. Also looks like a maybe innocent looking but controversial change to me.

sun’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -468,4 +494,122 @@ public function merge(array $data_to_merge) {
    +  protected function castValue($key, $value) {
    ...
    +        case 'Drupal\Core\TypedData\Plugin\DataType\Integer':
    +          $value = (int) $value;
    +          break;
    

    There's one data type that originally triggered the entire discussion on typed data for configuration values: Octals.

    system.file.yml contains two chmod file permission masks:

    chmod:
      directory: '0775'
      file: '0664'
    

    I wondered whether this patch would help to resolve that original problem now (through a new DataType\Octal plugin):

    $ php -r "var_dump($i = 0775, 0 . decoct($i));"
    int(509)
    string(4) "0775"
    

    But come to think of it, the schema-based data type conversions are happening way before the Yaml dumper is actually invoked, so even if we would attempt to properly convert an integer back to an octal representation, it would still be dumped as an integer, since the decoct() conversion would have to happen directly in the Yaml dumper, when the raw PHP value is serialized and written out into the Yaml document.

    In turn, no improvement regarding Octals, bummer.

    Nothing we can do about that I guess, so please ignore this part of the comment, just wanted to document considerations.

  2. +++ b/core/modules/number/config/schema/number.schema.yml
    @@ -12,10 +12,10 @@ field.number_integer.instance_settings:
       label: 'Integer'
       mapping:
         min:
    -      type: integer
    +      type: string
           label: 'Minimum'
         max:
    -      type: integer
    +      type: string
           label: 'Maximum'
    

    Why are the min/max settings of integer fields converted from integer to string?

    Unless that isn't a mistake or temporary, that could use a comment.

    The same applies to the min/max settings of decimal and float number fields (but of course, using float as data type instead of integer).

amateescu’s picture

@Gábor Hojtsy, ok, ok, I stand corrected :)

Wim Leers’s picture

#66.2: from #14.4:

The changes are are because floats and decimals are not integers and the max and min have to be able to be nulls or empty strings. Casting a null or empty string to integer results in a max and min of 0 :D

vijaycs85’s picture

sun’s picture

the max and min have to be able to be nulls or empty strings. Casting a null or empty string to integer results in a max and min of 0

Hm. I see.

However, changing the data type to a string in order to avoid that issue completely defeats the point of having data types in the first place.

With the proposed patch, we're essentially declaring the configuration values to be arbitrary strings, whereas they may be numbers, but that is nowhere enforced, even though the consuming code exclusively interprets them as numeric values.

We have type casting now, but what we're missing is a conditional, type-specific handling of optional values, no?

In essence:

case Integer:
  if (!isset($value) || $value === '') {
    $value = NULL;
  }
  else {
    $value = (int) $value;
  }
  break;

Expected result
(continuing Integer example)

  1. Saving value "1":
      min: 1
    
  2. Saving empty value "":
      min: ~
    

    Note: ~ is null in YAML.

alexpott’s picture

Assigned: Gábor Hojtsy » Unassigned
Status: Reviewed & tested by the community » Needs work

Let's address Sun's concerns about the null values here. It's be great if max and min could be integers imo.

Also lets inject the typed config manager in the constructor - discussed with @crell, @timplunkett and @sun on IRC and we agreed that we should

inject all the things until we have hard evidence that this is not a good idea

Wim Leers’s picture

Can't we just use -1 to mean "null" in this case?

Gábor Hojtsy’s picture

Re: the integer empty question (#70/#71), I'm not sure we want to always cast an empty string as an int to a null. That sounds strange. If I set a typed value to integer and the user did not enter a value how is Drupal sure I wanted it to be saved as something different from zero? The "integer with a special don't care value" sounds like a "type" in itself to me. Even if we special case this in the casting system in config, the code will still need to take care of the default value maybe being null to not result in 0 in the output again. It is true \Drupal\number\Plugin\Field\FieldWidget\NumberWidget does not do anything special with the value and in itself defaults the value to NULL if none was provided. So I guess the question is maybe only if we want to have this magic apply to any integer value wholesale and if this is what people would expect from a general integer type (to not cast to integer all the time).

Re: the typed config manager injection: yeah. Will do.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
15.45 KB
38.63 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Here is all the fun you get for injection.

To constructor-inject Config, we'll need to obviously pass this in whenever it is instantiated. It is of course instantiated in ConfigFactory, which is a service, so it goes all the way through that to its service definition. But it is also instantiated from config.inc as well as ConfigImporter, which in turn has all its dependencies injected and instantiated from ConfigSync and various tests. I'm not even 100% sure I caught all the instances of where this needs to reach, but I think I caught most.

This is the type of change core committers usually ask to not do in a patch doing something else so it is still possible to review what the patch does otherwise.

Note I did not do anything about the integer problem yet, this probably in itself can do with a little round of review.

Status: Needs review » Needs work

The last submitted patch, 74: 2130811.74.patch, failed testing.

yched’s picture

Can't we just use -1 to mean "null" in this case?

We can't, -1 (or any other actual number) is a totally valid value for "min" / "max"...

If I set a typed value to integer and the user did not enter a value how is Drupal sure I wanted it to be saved as something different from zero? The "integer with a special don't care value" sounds like a "type" in itself to me

Well, in PHP anything can be NULL ? I don't think we want to duplicate all our types with a *_or_null variant ? It looks like we're facing the same issues than in a db layer. A db column is typed, but can still be "NULL".
Storing such a "not set" value when an integer value comes as '' sounds reasonable to me. If there is specific cases where "no input" should be interpreted as 0, then it's a job of the specific form / domain logic at hand ?

Gábor Hojtsy’s picture

Ok this fails with:

Original

Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "config.factory", path: "theme.registry -> config.factory -> theme_handler" in Symfony\Component\DependencyInjection\Container->get() (line 283 of /Users/gabor/Web/d8mi/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Container.php).

Additional

Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "theme_handler", path: "theme_handler -> config.factory". in Symfony\Component\DependencyInjection\Container->get() (line 283 of /Users/gabor/Web/d8mi/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Container.php).

Which is puzzling for me. I don't see these circular dependencies based on the service definitions. We only change the factory dependencies and config typed is not dependent on anything with themes or vice versa. It has some shared dependencies with config factory, but that should not be an issue...

Wim Leers’s picture

We can't, -1 (or any other actual number) is a totally valid value for "min" / "max"...

In this particular case, you're right, but in general there are many places where negative values are nonsensical (e.g. "max upload size"), so there you can employ this solution.
It feels like in this case, we simply need a boolean min_is_set, and only when that's set to true, the min value is used, otherwise it's ignored (and should probably be 0, though it doesn't matter).

#74: wow.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
4.31 KB
38.78 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

@yched: well, SQL DDL has a [NULL | NOT NULL] constraint to attach to columns too for this reason. Anyway, let's assume ints may be null also. We need to apply this to floats as well then, since they have the same min/max/default setting where 0 and "none" are both meaningful values. So floats need the same code / test coverage.

That said this rolls back part of the original schema "fixes" however fixes the decimal and float field types to have min/max and default values typed as floats. Makes sense? :)

Note: still don't know how the circular service dependency arose, please help fix.

Status: Needs review » Needs work

The last submitted patch, 79: 2130811.79.patch, failed testing.

The last submitted patch, 79: 2130811.79.patch, failed testing.

yched’s picture

@Wim #78:

It feels like in this case, we simply need a boolean min_is_set, and only
when that's set to true, the min value is used, otherwise it's ignored (and
should probably be 0, though it doesn't matter

Ew :-( Split the info to an additional helper *_is_null property to be able to correctly interpret the way ConfigStorage stored the primary property ? Frankly, that sounds like a *very* kludgy and DX-painful abstraction leak...

In our case here, would mean NumberItem field type has to expose and manage two seemingly redundant *_is_set additional field settings, leading to: min, min_is_set, max, max_is_set - and keep them consistently set.

- This pollutes the DX of defining base fields as well, because you have to provide the right _is_null settings even if your definition is never going to be stored in YML
- This pollutes the DX of writing a field type. Field types are currently developped for base fields and configurable fields indifferently, but now you need to wonder about what will work for config storage.
- Frankly I'm scared of all the other places where we'd have to apply similar splitting.

This would boil down to "the moment there's any chance someone stores you in CMI, you cannot rely on PHP's way of handling NULLs". As I said, that's a very unfortunate and far-reaching abstraction leak IMO.

yched’s picture

@Gabor #79

well, SQL DDL has a [NULL | NOT NULL] constraint to attach to columns too for this reason

Right, and as you say, there's a reason for that :-). So maybe similarly we need a is_null boolean flag at the config DDL layer, i.e in config schema entries ?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
39.79 KB
FAILED: [[SimpleTest]]: [MySQL] 59,353 pass(es), 1 fail(s), and 0 exception(s). View
2.51 KB

Now comes with installer service updates as well (based on suggestion from @alexpott) which was indeed the reason for the "circular dependencies" (not very helpful error, heh). Also fixed the property name references in config factory. This now installs for me at least :)

@yched: I'd rather not expand the scope of this issue even more. (Re: more schema attributes to do more specific typing).

Gábor Hojtsy’s picture

FileSize
2.54 KB
40 KB
FAILED: [[SimpleTest]]: [MySQL] 59,414 pass(es), 2 fail(s), and 0 exception(s). View

So @alexpott says we should preserve NULL on the string type as well (but not cast '' to NULL). Here you go. Any other concerns?

yched’s picture

@Gabor: sure, the approach in the current patch, dealing with "empty number" internally in the storage layer, is good for me.

What I mean is, if we find more tricky cases that needs different logic, we should rather deal with them at the same place with additional DDL metadata, like SQL does, rather than forcing each upstream model logic to handle it themselves.

sun’s picture

I think a new "nullable" definition property would be an alternative option, but like others, I'm scared of the declaration/definition overhead (since a lot of configuration values are optional), and I also wonder whether that definition wouldn't live in the wrong spot, since the functionality here is just about saving values and shouldn't involve any more sophisticated business logic.

If we would declare a nullable/emptiness flag, then I'd think we would actually have to invoke the type-casting algo in the setter method already (as opposed to the final/belated save operation). In my mind, a declarative flag in the definition would act like the 'required' flag; i.e., a field value validator (which should bail out during validation already).

But regardless of whether we will add an explicit definition property in the future, we'll need the proposed code logic here anyway.

As far as I can see, the proposed logic does not involve actual business logic, just a means of figuring out whether the input value represents a "cast-able" value — if not, then the typed value must be empty/null.

A "suspicious" gut feeling is understandable here.

I think it would be ok, since (1) all "arbitrary" values typically went through entity/form validation already, and (2) the scope is explicitly limited to values in configuration objects (a manageable scope).

The only possible conditions for all data types are null or empty string. The only exception is a data type of string, for which an empty string means an empty string, only null means null.

As a result, the check for a null value could actually be moved apart and simplified, because whenever a value is null, the saved value will also be null, regardless of data type.


Perhaps a larger question might be whether this kind of logic shouldn't be part of the data type plugins in the first place (as opposed to custom logic in a one-off function), but that's a topic for a follow-up. — I'm a bit surprised to see that our data type plugins are only able to handle values and do not actually care for the data type of the values being set, have no notion of empty/null values, and also no means for handling values of a different data type (which happens often in a loosely typed language like PHP).

The last submitted patch, 84: 2130811.84.patch, failed testing.

The last submitted patch, 84: 2130811.84.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 85: 2130811.85.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
39.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config.typed-values.91.patch. Unable to apply patch. See the log in the details link for more information. View
2.45 KB

The last submitted patch, 91: config.typed-values.91.patch, failed testing.

sun’s picture

FileSize
39.58 KB
FAILED: [[SimpleTest]]: [MySQL] 59,726 pass(es), 9 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 93: config.typed-values.93.patch, failed testing.

alexpott’s picture

fago’s picture

Not sure how far validation should go here, but given config schema is sort of typed data and typed data has built in the symfony validator for validation, I'm wondering whether it would make sense to make sure typed config is proper typed data (see #1905230: Improve the typed data API usage of configuration schema) and leverage that.

alexpott’s picture

alexpott’s picture

93: config.typed-values.93.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 93: config.typed-values.93.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
569 bytes
38.85 KB
PASSED: [[SimpleTest]]: [MySQL] 59,723 pass(es). View

Fixing the ConfigEntityTest

alexpott’s picture

FileSize
1.35 KB
38.9 KB
FAILED: [[SimpleTest]]: [MySQL] 59,504 pass(es), 0 fail(s), and 1 exception(s). View

Lets improve the test coverage

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -466,4 +496,108 @@ public function merge(array $data_to_merge) {
    +        case 'Drupal\Core\TypedData\Plugin\DataType\String':
    +        case 'Drupal\Core\TypedData\Plugin\DataType\Uri':
    +        case 'Drupal\Core\TypedData\Plugin\DataType\Email':
    +        case 'Drupal\Core\Config\Schema\Property':
    +          $value = (string) $value;
    +          break;
    +
    +        case 'Drupal\Core\TypedData\Plugin\DataType\Integer':
    +          $value = ($value !== '' ? (int) $value : NULL);
    +          break;
    +
    +        case 'Drupal\Core\TypedData\Plugin\DataType\Boolean':
    +          $value = ($value !== '' ? (bool) $value : NULL);
    +          break;
    +
    +        case 'Drupal\Core\TypedData\Plugin\DataType\Float':
    +          $value = ($value !== '' ? (float) $value : NULL);
    +          break;
    

    I don't know TypedData very well, but this doesn't seem very extensible.

    Couldn't these typed data classes have a

    public static function castValue($value) {
    return (bool) $value; } 

    or whatnot? And then each can specify their cast.

  2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTest.php
    @@ -242,4 +246,56 @@ function testSchemaData() {
    +    \Drupal::config('config_test.no_schema_data_types')
    +           ->setData($untyped_values)
    +           ->save();
    

    Extra big indent here

Status: Needs review » Needs work

The last submitted patch, 101: 2130811.101.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.14 KB
42.05 KB
PASSED: [[SimpleTest]]: [MySQL] 59,887 pass(es). View

@timplunkett actually @sun and I talked about adding a just such a method - it looks pretty nice.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -466,4 +497,97 @@ public function merge(array $data_to_merge) {
+          // Config only supports primitive data types. If the config schema
+          // does define a type $element will be an instance of
+          // \Drupal\Core\Config\Schema\Property. Convert it to string since it
+          // is the safest possible type.
+          $value = $element->getString();

This delegation certainly looks nicer than before. Good stuff!

If we want to bring this full circle though, we may want to resolve the schema defined type to a base type to ensure it gets the right base cast.

Eg. if I have

test_type:
  type: integer
  label: Test integer value

my_config_key:
  type: mapping
  mapping:
    keyname: test_type

It would ideally come down to an integer cast, not a string cast.

It is true that all core non-primitive simple custom types are strings (text, label, path, etc), but that may not necessarily be true for contrib.

In fact since we have this, we need test coverage for the custom type anyway :) I think based on your code comment the above would cast the value to a string even though it resolves to an int, but based on the actual class resolving process, I think it may actually already resolve it to an integer?!

AFAIS needs more tests for those custom types then.

fago’s picture

  1. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Boolean.php
    @@ -25,4 +25,10 @@
    +    return ($this->value !== '' ? (bool) $this->value : NULL);
    

    I don't think there should be exception for '' - at least not at the typed data level. That would mean we interpret the boolean as string, but why would we? It's a boolean, so it should be interpreted as boolean.

  2. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Uri.php
    @@ -23,6 +23,6 @@
    +class Uri extends String implements UriInterface {
    

    Right now primitive classes are exclusive, i.e. implement just one primitive interface. This would make the uri class implement both.

    I don't think this would be problematic, but I think it's safer code-path wise to keep not doing that. The number of primitives is low enough, such that one can easily make sure each primitive is handled properly. If that means handling URIs just as strings, that should be simple enough anyway.

  3. +++ b/core/lib/Drupal/Core/TypedData/PrimitiveInterface.php
    @@ -28,4 +28,10 @@ public function getValue();
    +   * Gets the primitive data value casted to the correct type.
    

    Let's better clarify this casts to the correct PHP type. (not to the declared data type)

It would ideally come down to an integer cast, not a string cast.

That should be fine, as the integer class casts using (int).

It is true that all core non-primitive simple custom types are strings (text, label, path, etc), but that may not necessarily be true for contrib.

Yeah, contrib data types would just implement one of the primitive interfaces and would thus have to add the right casting method as well. So that should work as well?

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.66 KB
42.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,738 pass(es). View
sun’s picture

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Float.php
@@ -25,4 +25,10 @@
+    return ($this->value !== '' ? (float) $this->value : NULL);

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Integer.php
@@ -25,4 +25,10 @@
+    return ($this->value !== '' ? (int) $this->value : NULL);

The assumption of empty string → empty value is specific to the configuration system.

I absolutely +1 adding getCastedValue() methods on the data type plugins directly, but since those affect much more than just configuration, we'll need more accurate conditions there.

For example, both Integer and Float can implement:

return is_numeric($this->value) ? (int) $this->value : NULL;

That is because:

var_dump(is_numeric(NULL));  // bool(false)
var_dump(is_numeric(''));    // bool(false)
var_dump(is_numeric(TRUE));  // bool(false)
var_dump(is_numeric('1'));   // bool(true)
var_dump(is_numeric('1.2')); // bool(true)

Since this is explicitly about type-casting, the following would not work:

var_dump(is_float('1.2'));   // bool(false)

Lastly, wouldn't the following method name be more accurate?

getTypedValue()

Or would that be confusing?


Slightly OT, but I still wonder whether this operation shouldn't be moved into the setValue() methods later on; e.g., like this:

class Integer {
  public function setValue($raw_value) {
    if (is_int($raw_value)) {
      $this->typedValue = $raw_value;
    }
    elseif (is_numeric($raw_value)) {
      $this->typedValue = (int) $raw_value;
    }
    $this->value = $raw_value;
  }

  // Gets the raw value.
  public function getValue();

  // Gets the typed value (if type-valid).
  public function getTypedValue();
}
fago’s picture

Slightly OT, but I still wonder whether this operation shouldn't be moved into the setValue() methods later on; e.g., like this:

We had that initially, but I don't think it makes sense as it's the job of validation to determine whether the value is correct, thus we shouldn't alter/massage the value beforehand. If we'd do the cast suddenly every float would become a valid integer, but it isn't. I think it's good to have the casting possibility, but it shouldn't be automatic.

Instead, we could name the method "castValue()" and just cast the set value + return it. I'm not sure which variant is more convenient here.

ad getTypedValue():
It's not clear to me what the "typed value" should be exactly, so yeah I think this is confusing. I'd prefer staying with casts as we all know what those do.

fago’s picture

Improvements in #107 look good to me.

While checking the interfaces, I noticed some dumb copy&paste mistakes :/ -> opened #2157787: Durations are not primitive.

alexpott’s picture

FileSize
1.28 KB
42.25 KB
PASSED: [[SimpleTest]]: [MySQL] 59,821 pass(es). View

@sun nice idea using is_numeric

@fago re Uri extending String - this is exactly what Email does.

fago’s picture

I'm not sure about the isNumeric(), it's not really the casted value any more.

>@fago re Uri extending String - this is exactly what Email does.
Yes, but Email does not implement two different primitive interfaces now, but URI would do so.

Gábor Hojtsy’s picture

@fago: re #106:

It is true that all core non-primitive simple custom types are strings (text, label, path, etc), but that may not necessarily be true for contrib.

Yeah, contrib data types would just implement one of the primitive interfaces and would thus have to add the right casting method as well. So that should work as well?

I was talking of the *schema defined* types which cannot define their own PHP classes per say (using schema only at least, without writing PHP code :D) and @alexpott already added test coverage for that in #107, so looks good to me :)

So looks good to me, no outstanding issues for me.

vijaycs85’s picture

FileSize
1.54 KB
42.57 KB
PASSED: [[SimpleTest]]: [MySQL] 59,888 pass(es). View

Adding test case to check custom data type in sequence

alexpott’s picture

FileSize
5.07 KB
43.81 KB
PASSED: [[SimpleTest]]: [MySQL] 59,768 pass(es). View

@fago I think that the comment in the URI class implies it extends the primitive string type :)

/**
 * The URI data type.
 *
 * The plain value of a URI is an absolute URI represented as PHP string.
 *
 * @DataType(
 *   id = "uri",
 *   label = @Translation("URI")
 * )
 */
class Uri extends PrimitiveBase implements UriInterface {

}

That said if you truly think this is a bad idea I'm happy to add a getCastedValue() method to the Drupal\Core\TypedData\Plugin\DataType\Uri. I just can't work out why it is so bad to make it extend from String.

Patch attached removes the use of is_numeric and introduces another function on the primitive interface isNumericType()

@vijaycs85: I don't think we need another custom data type test - we have that covered. Using the parent stuff here just adds complexity and it is tested elsewhere in the same test file! An explicit sequence test is worth it though so thanks :).

Gábor Hojtsy’s picture

@alexpott: I think there was value in testing that dynamic named types also properly resolve to their parent type, but that may be implicitly covered elsewhere.

fago’s picture

I just can't work out why it is so bad to make it extend from String.

I don't think it's necessarily bad, but it's the way we've chosen to implement the primitives. Float does not extend from integer either, or Integer from string.

sun’s picture

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -466,4 +497,107 @@ public function merge(array $data_to_merge) {
+        if ($element instanceof PrimitiveInterface) {
+          // Special handling for integers and floats since the configuration
+          // system is primarily concerned with saving values from the Form API
+          // we have to special case the meaning of an empty string for numeric
+          // types. In PHP this would be casted to a 0 but for the purposes of
+          // configuration we need to treat this as a NULL.
+          if ($element->isNumericType() && $value === '') {
+            $value = NULL;
+          }

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Integer.php
@@ -25,4 +25,17 @@
+  public function isNumericType() {
+    return TRUE;
+  }

Do we really need to pollute the public API with a new method for this one-off use-case?

From my perspective, the typed data plugins are [too] complex enough already now, and I cannot really see a use-case beyond this config/schema system case for isNumericType()?

Perhaps, as a different approach: We appear to have a base String primitive that other types can extend from, but not a Number/Numeric primitive?

Or just a NumericInterface?

Thoughts?

alexpott’s picture

FileSize
4.04 KB
43.45 KB
PASSED: [[SimpleTest]]: [MySQL] 59,912 pass(es). View

How about just checking the existing interfaces :)

-          if ($element->isNumericType() && $value === '') {
+          if ($value === '' && ($element instanceof IntegerInterface || $element instanceof FloatInterface)) {
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me, resolving the above concerns. I think if we can make this even better later on, it is still a hugely valuable fix that will help us catch all kinds of config issues as time goes on :) Yay!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. HEAD. Data consistency FTW. :)

Gábor Hojtsy’s picture

Issue tags: +Spark

Amazing, thanks! Wrote a change notice for this at https://drupal.org/node/2160639 :) Retroactively tagging Spark since I worked on this as part of my Spark dayjob.

fago’s picture

Title: Use config schema in saving and validating configuration form to ensure data is consistent and correct » Follow-up: Use config schema in saving and validating configuration form to ensure data is consistent and correct
Status: Fixed » Needs review
FileSize
2.4 KB
PASSED: [[SimpleTest]]: [MySQL] 59,910 pass(es). View

Looking at the typed data changes again, I've noted the docblocks use the wrong syntax - attached is a small follow-up patch for correcting these.

Gábor Hojtsy’s picture

Component: configuration system » documentation
Priority: Major » Minor
Status: Needs review » Reviewed & tested by the community

Looks good. Moving to docs, so jhodgdon can commit it :)

beejeebus’s picture

Component: documentation » configuration system
Status: Reviewed & tested by the community » Needs work

i came across this code because i had to update #2098119: Replace config context system with baked-in locale support and single-event based overrides to work with it.

there may be implementation details i'm missing, but i thought we had agreement with 'what you put in to the configuration system is what you get out'? because this patch breaks that expectation completely.

can someone explain to me how that's a good idea? and yes, i know, http and forms and strings and typed data don't automagically match up and stuff, but i thought we decided that despite that, we were still going to make config give you back what was put in?

i guess i thought this patch was about validation? as such, what is it doing silently changing data?

beejeebus’s picture

wat wat wat from Config.php:

  protected function castValue($key, $value) {
    if ($value === NULL) {
       $value = NULL;
    }

is that required because of some typed data magic? if so, can we please have a comment? if not, wtf?

webchick’s picture

Priority: Minor » Major

Committed and pushed the doc follow-up in #123. Restoring priority. Bejeebus's questions still need answering, so leaving needs work.

sun’s picture

Unfortunately, this caused a critical performance regression in the Drupal installer:
#2162013: Critical performance regression due to config schema + TypedData lookups in installer

Please do not roll back this patch. Let's fix the regression instead.

beejeebus’s picture

discussed this with alexpott in IRC.

apparently silently changing the data that is handed in is a feature, not a bug, so there's nothing else to explain about that.

i think this is completely the wrong choice, but i'm not going to fight about it, given that 'you don't get out what you put in' was intentional here.

so, only leaving this open for the NULL bit in #126.

alexpott’s picture

@beejeebus so fine let's add a comment - we just preserve NULL values.

It's worth looking at the sample with a bit more context.

    if ($value === NULL) {
      $value = NULL;
    }
    elseif (is_scalar($value)) {
      // cast the value if we have a schema
    else {
      // Recursively handle arrays.
    }
    return $value;

But lets do this is a separate issue rather that leaving a beta-blocking major open.

xjm’s picture

Status: Needs work » Fixed

Agreed. Please file separate followups for each of the above (including the docs fix).

xjm’s picture

Title: Follow-up: Use config schema in saving and validating configuration form to ensure data is consistent and correct » Use config schema in saving and validating configuration form to ensure data is consistent and correct
Issue tags: -Needs issue summary update

Status: Fixed » Closed (fixed)

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

Sutharsan’s picture

Created a followup issue to remove the 'system.fast_404' schema which was not removed in this issue. See #2197873: Clean-up system.schema.yml to remove deprecated system.fast_404.