Typed configuration implements TypedDataInterface improperly

This first patch takes care of the objects in Drupal/Core/Config but not the rest of the system (locale?) nor updates tests that may rely on type definitions being arrays so it is not expected to pass tests yet.

Just to let tests run

@fago,
Thanks for looking at this.

whether it make sense to use composition instead of inheritance

I think that may work too but it would be a conceptual rework and would need some more code. I'm not for major reworks atm, just into consolidating existing APIs.

TypedConfigManager is the same kind of animal as TypedDataManager, only it uses different discovery and needs some more definition pre-processing so extending it is OK IMO. But both use type definitions and produce TypedData objects. Ideally we would use a single class for both, just passing them different discovery but we are still far from that because of implementation details.

Could you please take a look at how DataDefinition interfaces are introduced through the patch and let me know whether they are bing 'used right', I mean if they are used how they are intended to be used?

Working on re-rolling the patch, some replies in the meanwhile:

1. MapDataDefinition / ListDataDefinition This is not very clear at all. What's the difference between the two classes? This should have docs somewhere.

Basically, these interfaces have API extension to return property definitions or list item definitions. Anyway, I don't think it's the place, that is part of the TypedData API.

2. Sequence->getItemDefinition().

Right, it's not true in our case as the data definition depends on the data itself. But as that method must be implemented because it's part of the interface so we just return what is possible, which is a 'generic item definition'.
Anyway that is not used for the items, item definitions are processed internally, see the parse() method, so we should be all right.

3. Requiring name for buildDataDefinition(array $definition, $value = NULL, $name = NULL, $parent = NULL)

Not sure. Depending on the definition itself, it may need value and name (if there are tokens to replace) or not. But anyway, at runtime, name is always passed. It doesn't make sense for the root element, as name is the name of the file itself (configuration name), not the name of a property and I don't think it would be good allowing the option of different schema types depending on the configuration name, unless there's a case for it. I mean in that case, the configuration object would be processed differently if it has a different name...?

Maybe yes for the value, so I am removing that $value = NULL

4. create() not implemented.
Nope, but some code that gets passed a TypedDataManagerInterface does use it, so it must be somewhere in the interface.
Ideally, there would be a TypedDataManagerInterface to extend, some day...

5. Right

New version of the patch:
- Addresses issues in #7
- Updates some more functions (config translation, locale)
- Updates some tests adding new schema properties (definition_class, type)

Uh, tests won't run :-(

Obviously I need to learn what to do with these 'Mock' objects,...

Help!

class ConfigMapperManagerTest extends UnitTestCase {
  /**
   * Returns a mocked schema element.
   *
   * @param array $definition
   *   The definition of the schema element.
   *
   * @return \Drupal\Core\Config\Schema\Element
   *   The mocked schema element.
   */
  protected function getElement(array $definition) {
    $data_definition = $this->typedConfigManager->buildDataDefinition($definition, NULL);
    $element = $this->getMock('Drupal\Core\TypedData\TypedDataInterface');
    $element->expects($this->any())
      ->method('getDataDefinition')
      ->will($this->returnValue($data_definition));
    return $element;
  }
  ....
}

This should fix the issue with the tests, let's try

Yeah, test passes, attaching interdiff for last patch

@Gábor,
Good morning -if not at Drupalcon :-)

Yes, but: the name is not in the data definition.

If you want that then we need to add some 'root element' data type, so it handles its own name, and that one could be the parent for all trees

Anyway, I think that is out of scope for this issue.

I think we are talking about different things here. This is how I see it:

Since all this TypedData stuff is used for entities too, maybe we can think of the configuration name as the entity id, which is a different concept from a property/field name, which is what getName() is used for as per the interface definition (TypedDataInterface) or that I believe.
The difference here is that these TypedData elements exist by themselves instead of being contained in something else, maybe because it's some limited reuse, and in our case, the 'containing object' would be the Config object or the ConfigEntity object, not the TypedData parent object itself.
Maybe we shouldn't get the Typed config form the TypedDataManager but from the Config object itself, and then it would be the root element? And yes, that one actually has a configuration name (which is a property of the Config object, not of the data contained in it).

So yes, there's some 'conceptual gap' here that I don't really know how to fix. But it doesn't mean there's anything wrong with typed config, maybe you can think of it as a kind of TypedData that doesn't have a Root/Container entity. Also the TypedData implementation works fine as long as the topmost element in the tree is a Mapping or a Sequence object. For a 'perfect match' though I'd say it should be the Config/ConfigEntity object but that would require some important extension of these objects.

interface TypedDataInterface { 
  ...
  /**
   * Returns the root of the typed data tree.
   *
   * Returns the root data for a tree of typed data objects; e.g. for an entity
   * field item the root of the tree is its parent entity object.
   *
   * @return \Drupal\Core\TypedData\ComplexDataInterface|\Drupal\Core\TypedData\ListInterface
   *   The root data structure, either complex data or a list.
   */
  public function getRoot();
  ...
}

And anyway about that method (buildDataDefinition), passing the name there will only use it for variable replacement, that's not the name of the element as in the one retrieved by getName(). And that's why using the configuration name for that replacement may be wrong, since the data types of something shouln't depend on the name of that something.

Whatever, for validation we should be ok as these objects are (suppossed to be) run through validation one at a time so whatever code is handling that will 'know' the name of the configuration object that is validating to be able to print validation errors in context, if I got the issue right.

Maybe @fago could throw some more light at this.

Re-rolled patch for latest Drupal core updates.

Fixing issues pointed out by @Gábor #19:

1. Fixed
2. Good point. Fixed providing 'undefined' type for data elements that don't have a definition.
* For the future we can still validate schema completeness checking for 'undefined' elements in the data.

3. Fixed, now it reads:

    // Creates a new data definition object for each item from the generic type
    // definition array and actual configuration data for that item. Type
    // definitions may contain variables to be replaced and those depend on
    // each item's data.

4. We call it also from /Drupal/Core/Config/Schema/Element::parseElement(). That is used to build nested elements while get() is used only for the root element.

5. No, it is always a data definition object. Note we are not doing the implementation but reusing the one
in TypedDataManager. Wondering whether we could leave this out of the interface...

In addition to that, fixed implementation of ComplexDataInterface::get()
It now returns the right exception, and only when there's no data for it. Otherwise, in case a missing mapping it returns an 'Undefined' property.

I don't expect tests to pass, just need to know which ones to fix..

Fixed a problem with Mapping:get() implementation
Replaced SchemaIncompleteException by InvalidArgumentException

Another run for the tests.

We're almost there, just one test failing and considering whether that one makes any sense....

The problem is not tests but this quickly getting out of scope so I'm thinking getting it back to its original form (#17 + minor fixes asked for in #19)

However there are things in the schema that need fixing, and I've just bumped into one of those: 'undefined' data type, which 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, still it is used in two current schemas, namely: user.scyema.yml, views.cache.schema.yml

Funny paradox: Something that is defined as 'undefined'.... is it defined, or is it not?

So I think we should replace that with 'ignore' which is our current placeholder for 'any data'. But also I think our 'Ignore' should actually be 'Any' (Drupal\Core\TypedData\Plugin\DataType\Any)

Also I've found some trouble with StorableConfigBase::castValue() whose declaration says 'Casts the value to correct data type using the configuration schema' but actually it goes a bit futher than that (and it is the one test failing btw). Currently it does also some schema validation, throwing exceptions for mapping elements that are not defined in the schema. That is not casting, that is validating, and I'd say the method is kind of lying about its purpose... (And btw if the undefined* element's value is an array I think it will just break)

* Using 'undefined' as 'not defined' in this context, not as "defined as 'undefined' "... funny eh?

All these issues I had planned for a follow up patch, but bumped into them with this one.

So wondering.... Should we try and fix them all here? (Which is really little code but it may open up some never ending discussion about the real meaning of 'undefined' or 'type casting'... or life..).... or should we get back to the original intent, trimming down the patch to its original form and just fixing that definition arrays?

Ok, restarting from patch #17 (and fixing issues from #19):

1. Fixed
2. Good point. Fixed providing 'undefined' type for data elements that don't have a definition.
* For the future we can still validate schema completeness checking for 'undefined' elements in the data.

3. Fixed, now it reads:

    // Creates a new data definition object for each item from the generic type
    // definition array and actual configuration data for that item. Type
    // definitions may contain variables to be replaced and those depend on
    // each item's data.

I may create some new case for the issues mentioned in the last 3 comments.

New patch and inter-diff from #17

Fixed @Gábor #28 (.2) removing all unneeded 'use' (all those and some other use Language in config_translation/ElementInterface)

About the first point (reuse of MapDataDefinition and ListDataDefinition) I think it is a fair point, but I don't know what to do, unless you want me to reimplement all these definition classes for typed config (and moving a good amount of schema logic here)... and then this will become pretty bigger. Well, the other option may be just using DataDefinition for all of them.. ?

However I don't think the MapDataDefinition::create() should be an issue, because that is the default but there's alreay a parameter in there to produce other types so my understanding is it is intended to work also with other types... Note it doesn't create a Map, it creates a MapDataDefinition...

public static function create($type = 'map') {
  $definition['type'] = $type;
  return new static($definition);
}

So admitting those definition classes should be ideally overridden and extended, I'd rather do it in a follow up patch because doing it properly would take moving part of mapping and sequence processing into them, which is beyond the intent of this one which just fixes the TypedDataInterface::getDataDefinition() method. And maybe as a part of that one we can also get Mapping and Sequence to extend or become Map and List.

Re-rolled for latest d8, tests may fail again because of the new schema validation, let's see...