Problem/Motivation

If there was no schema type specified, TypedConfigManager will introspect the data type and assign a 'string' type if there was no type specified and the value was a string. This is dangerous for various reasons.

The following file would pass validation if only 'key' is defined but 'foo' is not: (A)

key: 'aa'
foo: 'bb'

The following files would not pass schema validation under the same conditions: (B)

key: 'aa'
foo: 123

or (C)

key: 'aa'
foo:
  - 'bb'

This is IMHO *extremely* confusing. While having a default type for string values sounds good, all other values get 'undefined' as their type and not introspected. What makes it worse is even if the data structure in C is what the module expects, A would still pass validation, so it would appear to the config schema system the file is correct. So if certain things are not defined, it somehow they get string values, they will always pass validation even if they should never be strings but an int checkbox value for example.

Proposed resolution

1. Remove the data introspection based default type assignment for strings.
2. Fix schemas to say type: string for the ones where it was missing. Probably plenty.

Remaining tasks

Do it.

User interface changes

None.

API changes

Type is now required even for strings. Defining a key without a type will be still undefined type.

Files: 
CommentFileSizeAuthor
#6 interdiff.txt4.84 KBGábor Hojtsy
#6 no-assume-type-config-schema-6.patch5.7 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,243 pass(es). View
no-assume-type-config-schema.patch883 bytesGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,242 pass(es), 333 fail(s), and 0 exception(s). View

Comments

Gábor Hojtsy’s picture

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

Status: Active » Needs review
Gábor Hojtsy’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, no-assume-type-config-schema.patch, failed testing.

alexpott’s picture

+1 makes sense - fallback to string made sense when the Config object casted everything to string - it no longer does and it should never should.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,243 pass(es). View
4.84 KB

Looks like the actual fails are all in views schemas. Here are some fixing for those. Possibly not all. Also fixed the asserts in the schema test now that we are NOT magically detecting strings. Note that even in the views schema some items got the explicit string definition already and those were confusingly intermixed with ones that did not have it.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Didn't know this default exist and used in these many places. Looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed bfb8fdf and pushed to 8.x. Thanks!

  • Commit bfb8fdf on 8.x by alexpott:
    Issue #2268439 by Gábor Hojtsy: String introspection backwards data...

Status: Fixed » Closed (fixed)

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