Problem/Motivation

In configuration schema we have an 'undefined' type, that is meant to be used for configuration values whose type is not defined.

As it reads in core.data_types.schema.yml:

# Undefined type used by the system to assign to elements at any level where
# configuration schema is not defined. Using explicitly has the same effect as
# not defining schema, so there is no point in doing that.
undefined:
  label: 'Undefined'
  class: '\Drupal\Core\Config\Schema\Undefined'

So it is supposed not to be actually used in schemas, it doesn't make too much sense to define something as 'undefined' (either it is defined or it is not), still it is used in two current schemas, namely: user.schema.yml and views.cache.schema.yml

The use case for those sequences with 'undefined' elements (all of them are in sequences) is the need for some arrays that do need to be empty, ie. they are standard plugin settings arrays that will not have elements in these types. The explicit undefined typing was added because the schema code did not deal well with no item type definition. Since #2277945: Typed configuration implements TypedDataInterface improperly the missing type definition will result in assumed undefined and behave the same way.

Proposed resolution

Remove those 'undefined' from configuration and instead let the runtime figure out that they are not defined. If there is any data in there, validation will fail because they will be undefined types. So this fulfils the need to define the empty arrays and gets rid of explicit uses of the undefined type.

These schemas already have test coverage even several times over with #2295737: Not all shipped configuration passes validation even with all modules enabled.

API changes

None.

Files: 
CommentFileSizeAuthor
#13 2285189-no-explicit-undefined.patch1.09 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,444 pass(es). View
#9 5189-undefined-ignore.patch1.09 KBJose Reyero
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,636 pass(es). View
#3 2285189-3.patch2.99 KBJose Reyero
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2285189-3.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

Gábor Hojtsy’s picture

I think using the ignore type would be more useful at these places? Why would we add an 'any' type how would that be different from ignore? Did not look at which context they are used in though.

Gábor Hojtsy’s picture

Title: Config schema: 'undefined' means 'not defined', » Undefined config schema type should not be explicitly used in config schemas

Retitled.

Jose Reyero’s picture

Status: Active » Needs work
FileSize
2.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2285189-3.patch. Unable to apply patch. See the log in the details link for more information. View

First patch, it is a short one, will add proper tests later.

@Gábor,
Reply is in the patch itself, and in any case if we don't have any meaning for ignore we should drop it (including its class) to use the typed data one, which would be a duplication.

# Explicit type to use when no data typing is possible. Instead of using this
# type, we strongly suggest you use configuration structures that can be
# described with other structural elements of schema, and describe your schema
# with those elements.
any:
  label: 'Any'
  class: '\Drupal\Core\TypedData\Plugin\DataType\Any'
  
# Explicit type to use when on top of having no explicit data typing like the
# previous one, we want the schema parser to just ignore the element, not doing
# any further type casting, validation, etc..
ignore:
  label: 'Ignore'
  class: '\Drupal\Core\Config\Schema\Ignore'

Does it make sense for you? Because otherwise the 'ignore' name doesn't make any sense for me either.

('Needs review' for tests to run)

Gábor Hojtsy’s picture

I don't see what would be the difference between the any and the ignore type especially since you modify the casting to have the same behaviour for the two? Is it just so the validation still goes on but casting will not? I don't see the use case.

The places we use the undefined type seem to be for empty sequences, so its not sequences with whatever types of values but rather empty sequences (which have no way to be specified now in core).

Jose Reyero’s picture

I'm just thinking of future validation.....

Think validation 'constraints':

thing:
  type: any
  constraints:
     color: blue

Will take anything as long as it is blue: a blue car, a blue dog.... Constraints are pluggable so we can very well add a CheckColorConstraint somewhere....

While on the other side:

thing:
  type: ignore
  constraints:
     whatever: not for our validator, someone else may use them or not
  other property: You can add anything, our parser will just ignore it.

Also I really find 'ignore' makes no sense, which kind of data type is it? But I've promised myself time ago I'm not discusing names anymore, so I'm not, ok?

Anyway -about to give up- just replacing 'undefined' by 'ignore' will do it. I may try in the future replacing 'ignore' by 'any' for consistency with Typed Data, but that may be a different issue.

Gábor Hojtsy’s picture

First of all the places you found 'undefined' neither 'undefined' neither 'any' nor 'ignore' would be correct. If we want to *actually* explain what is there then we need a way to be able to say *empty* sequence so we can describe a sequence/array that should not have any elements. All other possible definitions including the current one would pass if there would be something in the array which is not correct.

So the actual fix for these undefineds is introducing a syntax to describe an empty array.

As for why 'ignore' is ignore, its named after what the system will do with it, true, not what is there. I personally don't like we have an ignore type and the sole reason was for migrations because they did not want to change the migration config entity structure to a format that can be described by config schema neither to abandon using config entities. See lengthy discussion in #2183957: Provide configuration schema for Migration module. I don't think the use of 'ignore' and especially not 'any' is a good idea in general based on what we use config schema for. We want people to not skip swaths of their data structure because they are lazy documenting it or don't want to make it conform to structures that can be described by schema because then we cannot ensure type consistency for deployment, we cannot support translation, etc. These uncertain types are bad precedents IMHO. I pressed as hard as I could to NOT introduce the ignore type and deliberately named it like so people feel at least a bit bad to use it. Does that make sense?

Jose Reyero’s picture

@Gábor,
After our conversation on IRC I think what really needs to be fixed is storage of empty arrays, that should be at the very least consistent, and that can be done when type casting the configuration prior to being saved.

Or if we do need an array that needs to be empty -as opposed to 'can be empty', not sure I got the whole story-, a) we can extend the schema with 'empty_array' or b) we can add in constraints like 'max_items' => 0,

About the name for the unknown thing, whatever ignore or any will do for now, but I disagree on it not being really needed. There are valid use cases for it, I can think of:

- Mutable stuff, that is the case when it may be of different types, still be valid and will be handled at runtime.
- Lists of mixed types of items, that would be a sequence with an item of type 'any/ignore' (Or maybe we should fix Sequence's handling for it not to require an item type and leave it as 'undefined' (I think this should be an easy one)
- Well, lazy developers -though other people may call them 'busy' too-, that don't want / don't need to add a new element type in case they need it, but still better having a partial schema than none (aka undefined).

PS: Could we rename 'ignore' to 'stupid' then?, which at least is a type kind of word... ;-)

Gábor Hojtsy’s picture

@Jose: whether its exported as null or empty array we still cannot describe it with current schema, because its a list of nothing, we cannot really say what it is a list of and be valid :) so we definitely need change in the schema structure to describe it, not just how we export it.

As for your three examples of ignore, I think all are at their core equal the third example. Ie. a type that can be anything is currently totally possible with a container that tells which type it will be (ie. where the data tells us what type to expect at another place). That is the only way to validate so we have something to crosscheck against.

Jose Reyero’s picture

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

Ok, we've talked enough here, I'm giving up on the 'any' type, and this is a fresh restart.

This patch just replaces occurrences of 'undefined' in schemas by 'ignore'.

Note: Only tests that explicitly check for 'undefined' expected to fail, will fix them if any on the next version.

Jose Reyero’s picture

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

My point of view that this need schema API changes did not change. The latest proposed patch is not explaining the data structure well either, these arrays should not contain elements. The proposed patch says they are valid whatever they contain.

Jose Reyero’s picture

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

Issue tags: +Configuration schema
FileSize
1.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,444 pass(es). View

Let's see what fails when we remove the undefineds and fix those. The type should be autodiscovered undefined.

Gábor Hojtsy’s picture

Issue summary: View changes

#2277945: Typed configuration implements TypedDataInterface improperly fixed the notices so if there is no definition it will start off with an empty one which is our intention (and work the same way as if it was defined to be undefined). So IMHO this fixes the problem.

Updated issue summary.

Gábor Hojtsy’s picture

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

Bring in D8MI sprint for the sake of tracking since the D8MI team is working on this one even if not directly language related.

Jose Reyero’s picture

Status: Needs review » Reviewed & tested by the community

Yes, this makes a lot of sense and the patch (#13) kind of proves we don't really need to use explicit 'undefined' (Tests just pass)

I think this is ready.

The last submitted patch, 3: 2285189-3.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f64568d and pushed to 8.x. Thanks!

  • alexpott committed f64568d on 8.x
    Issue #2285189 by Jose Reyero, Gábor Hojtsy: Fixed Undefined config...
Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks!

Status: Fixed » Closed (fixed)

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