Sequence and Mapping implement interfaces incorrectly, make them honest about what they support

@chx,
Yes, good points, this still needs some more clean up and documentation.

Mostly we are trying to keep up with TypedDataInterface / ListInterface, which is quite a moving target, so I was thinking of doing the clean up maybe later but anyway now is a good moment for starting too.

1. The class doxygen is useless. Is it a list of items? Can the keys be strings? What are some examples? When should I use something else for an array

Intended to map any yaml sequence, now it will only work for sequences without indexes because of ListInterface (these ones will become numeric indexes at some point in the parsing).

For string keys, check out mapping (Mapping), will it work for you?

To do: Add some docs for both Mapping and Sequence.

2. getItemDefinition contains a fatal error. No, seriously, look at it: buildDataDefinition is called with two arguments, it requires three. What does this say about test coverage? Do we need this method if it's not called anywhere, code or test?

True. Method implemented because the interface requires it, not actually used.

To do: Implement properly, add test coverage, so at least tests use it :-)

4. All the method doxygen are the old style implements instead of inheritdoc.

Right, though I would leave this for a final cleanup, just in case they change their mind again...

4. onChange is looking for parent which is not a defined property nor is it clear how would it be set.

It is defined, 'parent' is inherited from TypedData which is the root class for all this.

But you are right it is badly defined, implementation is plain wrong in \Drupal\Core\TypedData\Plugin\DataType\ItemList which is from where I guess someone copied and pasted that from.

(Note $parent is type TypedDataInterface and we, and ItemList, and Map, and everyone else, are calling a method on 'parent' that is not part of the interface)

If we can ever fix TypedData and related interfaces and objects we can copy whatever ends up there.

<whinning> The worst of it is every time someone adds some 'feature' to TypedDataInterface & Family they add here whatever implementation so php parser just won't complain :-( </whinning>

Asigning it to me, I'll be working on a patch to address all these issues in typed configuration, but still, we should fix the "root issue" which is TypedDataInterface / ComplexInterface.

Ok, let's get this fixed.

Really, there is no way Sequence implements properly ListInterface (because we cannot have a single itemdefinition). Also the related Mapping doesn't properly implement ComplexDataInterface. We've been for ages playing catch up with those interfaces and it is becoming worse as they evolve...

This patch:
- Adds a new ListElementInterface that is the one to use for typed config replacing ListInterface and ComplexDataInterface. We don't need the overhead of ComplexDataInterface.
- Simplifies everything, we can get rid of a dozen methods or so, half of them were not properly sticking to the interface anyway.
- Adds some documentation for Sequence and Mapping.
- Replaces occurrences of 'instanceof ArrayElement' with a proper interface, getting rid of some ugly parts (like the test that cannot mock an ArrayElement....)

- **** Allows Sequences to have any type or number of keys. This should work for your configuration data I guess (?)

@chk,
Not sure what you mean, $this->value should be either an array or null, it returns the plain config value.

  public function toArray() {
    return isset($this->value) ? $this->value : array();
  }

Fixed minor issues, updated tests for the new interface.
(Needed to add onChange() because some contained elements may need it)

@chx,
Do you think this will work for your sequence or array elements?

Still wondering whether to rename ArrayElement to ListElement or ElementListInterface to ArrayElementInterface, but I think name should be the same... any idea about this?

Looking at the failed test...
Fixed test mock builder, now using interface instead of class.

Retitled, adding config tag. I think moving the common code one level up makes sense, less maintenance, the differences clearly show.

1. +++ b/core/lib/Drupal/Core/Config/Schema/ListElementInterface.php
   @@ -0,0 +1,86 @@
   + * Contains \Drupal\Core\Config\ListElementInterface.
   ...
   +   * Gets a element object.
   

   I think the name is strange. This is not for an element of a list but an element that IS a list.

   This combined with the doc on get() that says gets an element is confusing :D

2. +++ b/core/lib/Drupal/Core/Config/Schema/ListElementInterface.php
   @@ -0,0 +1,86 @@
   + * When implementing this interface which extends Traversable, make sure to list
   + * IteratorAggregate or Iterator before this interface in the implements clause.
   

   Why?

Also needs issue summary update.

New revision of the patch:

- Trying to fix the failing test, ConfigMapperManagerTest, maybe better not changing that part (which was changed by this patch before)

- Fixed issue 1. above, better comment: "Gets a contained typed configuration element."

- About 2. That's a php generic reminder for interfaces that extend Traversable, just copied it from ComplexDataInterface.
Not sure whether you ask 'why the comment', or why the interface behaves like that, which is explained here, http://php.net/manual/en/class.traversable.php

Also I'm thinking, for next patch, that we don't really need ArrayAccess here (we have get/set), I'll be giving a try to removing it. It makes everything confusing by allowing two different ways of accessing contained properties.

Thanks! The ListElement class naming still sounds iffy to me. It is not an element OF a list but an element that IS a list. Don't know what better name to suggest though :/ Why not just make it just List? After all other types are not "Element"s either.

Yet another twist to these ugly interfaces:
- Renamed ListElementInterface to TypedConfigListInterface
- Renamed ArrayElement to TypedConfigListBase
- Created a TypedConfig wrapper that extends ConfigBase *************
- Moved SchemaCheckTrait into this new wrapper ********** (No need for trait anymore)
- LocaleTypedConfig extends TypedConfig (and fits nicely into the wrapper structure).

Some notes:
- TypedConfig is not typed data, but a wrapper for typed config items, similar to Config objects (which are wrappers for config data),
- No need for dotted notation in TypedConfigListInterface::get() anymore, so our typed data objects look more like other typed data objects.

In short what we are doing is using still typed data, but also proper config wrapper that doesn't need to implement TypedDataInterface. There are other issues about similar moves for entitites, etc...

Then all the rest fits nicely (included SchemaCheckTrait that doesn't need to be a trait anymore), typed config wrappers just get the ability to test their own schema.

To do:
- Minor clean up, some methods may not be needed anymore, review docs, etc..
- Rename SchemaCheckTraitTest accordingly (just kept for now to point out this works just the same).
- Further simplifications are possible for config translation, locale typed config, etc...

I don't think there's too much point in an interdiff, as this renames about everything...

Just fixed some docs.

Are all these things still in scope? On a first look this looks carried away a bit honestly :/

Retaking this one, to be re-targetted and simplified. Assigning it to me.

This is a fresh restart from #13, re-rolling for current head.

I think that was the last version that was more or less in scope, quick summary:
- Add own interface for config maps / sequences (forget about ComplexDataInterface, ListInterface)
- Moved common methods from Mapping and Sequence to ArrayElement.

To do:
- Better name, maybe ConfigListInterface / ConfigArrayInterface.. ?
(Really, all the *Element names here are not the best ones, but we better don't get into renaming too much for the patch to be manageable, so good names only for new things, keep old names as they are)
- Simplify the interface, it doesn't need that many methods (most of them inherited from previous interfaces)
- See about reusing this one, #2346129: Introduce a TraversableTypedDataInterface and use that for typehinting instead of ArrayElement

("Needs review" is mostly for testbot to run)

So this makes a lot of sense, but I think we should postpone it on #2346129: Introduce a TraversableTypedDataInterface and use that for typehinting instead of ArrayElement. Do you agree?

@tstoeckler, yes, agreed.

Awesome.

Well, that was quick.

Ok, still assigned to me, so this will be my homework for this week.

Re-rolled for latest head, specially taking these changes into account #2346129: Introduce a TraversableTypedDataInterface and use that for typehinting instead of ArrayElement

Only for the tests, I'll post next iteration asap.

And this is the patch with a few improvements (more old code being removed):
- Renamed ListElementInterface to TypedConfigInterface, I think better name, since this will be the outermost element of all typed config objects.
- Removed ArrayAccess from the interface. Not needed anywhere, since we have already get/set, and also it was only being used in tests.
- Updated docs, for some return types.
Example TypedConfigManager::get() returns always TypedConfigInterface

Updated title and issue summary.

Added parent issue

Needs re-roll after #2359449: TypedData calls onChange() parent's method that is not part of the interface

rerolling...

awesome. Thanks @rpayanm. This is ready for review.

Discussed at last week's D8MI meeting that due to the patch size, it would be great to have a visual to help review before/after API structure, that would help reviewing immensely.

Re-rolled the patch with a few changes from #38:
- Removed onChange() method from TypedConfigInterface (it is already defined on the parent interface)
- Fixed docs for isEmpty(), copied from ComplexDataInterface instead of ListInterface which didn't really match
- Renamed get($name) parameter for consistency with the other interfaces.
Further simplifications for TypedConfigInterface:
- Not implementing Countable anymore, not really needed, just updated test
- Removed method getElementDefinition(), again not needed -nor used- for anything, can be a protected method in the class

To do next: Update issue summary with some diagrams.

I think I now finally understand what this patch is trying to do and I like it a lot. It's the next step in making typed config actual typed data.

I still have to spend some time with it to be able to completely grok it, but so far I have one question:

+++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
@@ -47,59 +36,101 @@ protected function getAllKeys() {
+        if ($element instanceof TypedConfigInterface) {

Shouldn't this be TraversableTypedDataInterface? Since both Mapping and Sequence (the only implementations) implement both it shouldn't matter code-wise, but when traversing into objects, we never get further typed config objects inside other typed config objects, but we get sequences or mappings (i.e. traversable typed data) inside of config object.

That might mean that we have to move get() onto TraversableTypedDataInterface but I think that would make sense anyway.

@tstoecker,

Basically yes, though as you say:

That might mean that we have to move get() onto TraversableTypedDataInterface but I think that would make sense anyway.

And same for many other methods, if you can reconcile method signatures and exceptions in ComplexDataInterface and ListInterface, which if you look at them are not exactly the same.

However I am trying to keep this patch from touching anything in TypedData and change only the typed config part and for that, the safest bet atm is using 'TypedConfigInterface', which is the one implemented by all config structures and also defining the get() method.

Updated issues summary with some diagrams.

Thanks for the diagrams, they are very, very helpful!

I'm reviewing the old and new code in context now, so this is me basically thinking out loud:

• \Countable is removed from ArrayElement (as can be seen in the diagrams). This makes sense as being countable is nothing at all specific to configuration schema. If TraversableTypedDataInterface at some point wants to implement \Countable then it's free to do so, but it makes total sense to remove it here.
• ArrayElement::validate() is removed. According to my IDE it is unused within the configuration schema system. Also the parent class TypedData implements it already. It implements it in a different way - by directly deferring to the constraint validator - but AFAIK the constraint validator should handle nested objects fine, so I don't think the explicit loop in the current implementation should be necessary.
• onChange() and isEmpty() are currently completely identical between Mapping and Sequence so they are moved ArrayElement. Nice!
• It's great to remove \ArrayAccess. offsetSet() is currently broken anyway, as TypedDataInterface is not used. Also because Mapping currently inherits offsetSet() there is a very strange mismatch between that and set(). set() and get() are moved into ArrayElement with this patch. This means that sequence gains support for nested keys. I.e. you can do $sequence->get('0.some.sub.key') with this patch which strangely did not work before even though $mapping->get('key.some.sub.key') worked before already (and still works with the patch)
• Currently ArrayElement declares an abstract function parse() which both Mapping and Sequence implement almost identically except that they get their information from the mapping and sequence keys, respectively. This patch consolidates the actual parsing logic (because it is identical) in ArrayElement and instead delcares an abstract function getElementDefinition() in which Mapping reads the mapping key and Sequence reads the sequence key. Btw, the end result of this patch is that those methods are the only thing that's left in Mapping and Sequence and that's a good thing. Even though they are conceptually different their native data structures are both arrays in PHP and thus the processing logic can be consolidated for both classes. The only difference is the key they get their information from.
• Element::parseElement() and Element::buildDataDefinition() are not used in any of the other subclasses of Element so they are moved to ArrayElement. This makes the protected $value the only thing left in Element. Also, ArrayElement no longer extends from Element but that is wrong because it still used $this->value which is now referencing an undeclared variable. The fix to this is moving the declaration of $value up into TypedData. That already references it (in getValue() but does not declare it. That way we could get rid of Element entirely.
• Some code that explicitly unsets a value (vs. setting it to NULL) in Mapping::set() is removed. I'm fairly sure that's not needed anyway, and in any case that code is not present in Sequence so I think it's safe to remove.
• toArray() is moved from Mapping to ArrayElement (so that Sequence has it as well now. Also it is being fixed to always return an array, even if no value exists.
• Mapping no longer implements ComplexDataInterface and as part of that getProperties() is removed (although) toArray() is kept. In the same light Sequence no longer implements ListInterface and first() and getItemDefinition() are removed. It might be convered above, but I'm not sure why that is and this feels very wrong. Mapping and Sequence are exactly the config-schema equivalents of Map and List (the fact that we need to have config-schema specific versions of those is unfortunate but that's a separate problem)
• A bunch of methods are introduced on a new TypedConfigInterface which extends TraversableTypedDataInterface. I think this is wrong and at least some of them should be moved directly to TraversableTypedDataInterface. Note that both ComplexDataInterface and ListInterface have get(), set(), and isEmpty(). So with this patch all three child interfaces of TraversableTypedDataInterface provide three identical methods. Let's not do that. toArray() is only on ComplexDataInterface, not on ListInterface but I think that's an oversight, so IMO it should be added to TraversableTypedDataInterface as well. getElements() is sort of specific. Although I do see how we could implement that generically for Map and List in the future, it currently has config-specific semantics, so that should stay on TypedConfigInterface for now, IMO.

Some minor remarks on the patch:

• +++ b/core/modules/locale/src/LocaleTypedConfig.php
  @@ -10,6 +10,7 @@
  +use Drupal\Core\Config\Schema\TypedConfigInterface;
  

  Unused

• I don't much like the getElements() usage in the tests, I would prefer to use explicit get() calls on the actually needed keys, but that's an implementation detail and definitely something I can live with.

So in summary this patch is a lot of awesome, but the missing $value in ArrayElement is certainly a needs work and IMO the TypedConfigInterface and the lack of ComplexDataInterface and ListInterface need work as well.

@tstoeckler,
Thanks for great review.

Re-rolled for latest 8.0.x
- Fixed both remarks in #45 (Removed unused TypedConfigInterface, added ArrayElement's $value)
- Removed leftover Sequence::filter() method, introduced in https://www.drupal.org/node/2365585

About the rest, not sure if there are any other actionables in there.

TypedConfigInterface and the lack of ComplexDataInterface and ListInterface need work as well.

??

Sorry for my absence on this issue. I have an updated patch which makes clear my current reservations against the patch and how I would envision this to work. Since it's not meant as an iteration on the previous patch but "my take on this" which we might ditch or adopt or meet somewhere in the middle I didn't want to spam this issue with a bunch of patches, so I've been "hiding" in a testing issue thus far. It's not quite green yet, as soon as it is, I will post it here together with some thoughts.

@tstoeckler, @reyero: The window for API changes is gradually closing. It would be great to come to some agreement ASAP so the perfect will not be the enemy of progress on this one :/

I had a big summary written up about this but lost it. So here is the patch for now. I rerolled the previous one to provide an interdiff. Will hopefully get around to explaining this later today.

This *should* pass but I did make one last-minute change so crossing fingers...

Hehe, it passes on PHP 5.5...

Ok, so here's my second attempt at explaining what my patch does.

In writing the below I noticed two oversights in my last patch, which the attached one fixes.

Make Mapping implement ComplexDataInterface again

In typed data, we have two types of traversables: ItemList and Map, which implement ListInterface and ComplexDataInterface respectively. In the typed config system we have the same distinction, but for technical reasons we unfortunately cannot use those classes directly which is why we have Sequence and Mapping. They are semantically the same thing though (respectively), so I feel strongly that they should implement the respective interface. This does not have the implication of any added methods.

Make Sequence implement ListInterface

See the previous point for the reasoning. Concretely, this means that getItemDefinition(), filter() and first() are reintroduced to <code>Sequence. Because the latter method is identical between Sequence and ItemList (and in general rather generic) I introduced a ListTrait for the two to share. I'm not a big fan of having to implement these methods, but to my mind the argument given above wins over that. I think we can explore removing those methods from ListInterface and e.g. into FieldItemListInterface as I suspect that is where they are actually being used. If you feel strongly about this we can also explore that here, of course.

Remove TypedConfigInterface

TypedConfigInterface was an abstraction over Sequence and Mapping because we need to interact generically with those objects without knowing which of the two it will be. TraversableTypedDataInterface, however, exists to fill the exact same need. It just doesn't fully fulfill the requirements of the typed config system at the moment. I don't think it's a good idea, though, at this point in the release cycle to introduce a new interface as a shim with the perspective to remove it when TraversableTypedDataInterface is up to speed. Therefore I've enhanced TraversableTypedDataInterface to meet the needs of the typed config system in this patch. I've also accounted for existing code in the Field/Typed data system which works generically with TraversableTypedData and currently still has to check explicitly whether a given object is a ComplexDataInterface or a ListInterface. All those places can no simply use TraversableTypedDataInterface directly. I want to stress again that this is not an artificial (over-)abstraction that is driven by some attempt at theoretical purity: both the Field/Typed data system and the Typed config system already have the need for this generic interface, we are just working around it currently. Concretely, this means adding the following methods to TraversableTypedDataInterface:

get($key)

All three of ListInterface, ComplexDataInterface and TypedConfigInterface already had this before, so it seems like a no-brainer.

set($key, $value, $notify)

Again all three interfaces have this method already. Consolidating them means we have to reintroduce the $notify parameter, which I am not really excited about. Again, though, I think the consistency we gain with this is more important. We can always look into removing $notify from the interface in the future.

getElements($include_computed)

This was on TypedConfigInterface and ComplexDataInterface had the equivalent getProperties() which I simply renamed. Again, we inherit the $include_computed parameter from ComplexDataInterface, which is not great, but not terrible in my opinion. The consolidation also means that we have to implement this method in ItemList, which is rather trivial, though.

getElementDefinition($name)

This is in fact not needed for the typed config system, but the Typed data system has this need in TypedDataManager::getPropertyInstance(). It is a consolidation of $item_list->getItemDefinition() and $map->getDataDefinition()->getPropertyDefinition($name). We could look into removing that function alltogether in the future, maybe. As calling ListInterface::getItemDefinition() (i.e. without a specific argument) is useful, I kept that method around.

isEmpty()

Again, all three interfaces have this already, so it's a no-brainer.

toArray()

TypedConfigInterface already shares this with ComplexDataInterface. Also there's #1854782: Add to/setArray() methods to TypedData ListInterface so this seems like a no-brainer as well. The actual implementation of ItemList is trivial due to iterator_to_array().

Note that in aggregate this makes ComplexDataInterface empty, which is strange, but perhaps also something we can explore improving in a follow-up.

Removes Element

The only thing it did in the previous patch was to declare $value as a member variable. $this->value is already used in its parent, TypedData, though, so it makes more sense to simply document it there. That lead to an interesting problem, which is also the reason it took me ages to get this patch green. See FieldItemBase::__construct() if you feel like headdesking.

Promotes TraversableTypedDataInterface further

In addition to the consolidation of the actual code that interacts with traversables generically I've also updated documenation of the type "ComplexDataInterface or ListInterface" accordingly. Furthermore I've added typehints to things like $parent in a few places.

Moves \ArrayAccess and \Countable from ListInterface to FieldItemListInterface

They are only ever used in the Field system so it seems pointless to require them. Especially \ArrayAccess I feel very strongly about not imposing on implementors of ListInterface. \Countable I could live with, as it sort of makes sense for a list, but as it's not used, I went for moving it as well. I also added the two to ItemList as it currently has it and, thus, would be an additional API change to remove. I do not feel strongly for keeping it, though.

@tstoecker,

While I think it is a good idea to promote TraversableTypedDataInterface and move some methods there, and moving some part of ListInterface to FieldItemListInterface is a smart move, I don't think this really fixes the issue with typed config which was it not implementing these interfaces cleanly -just because it was not possible.

If you look at the list in the issue summary (Problem/Motivation), this latest patch doesn't really fix all those issues and we are still left without proper implementations. However the patch makes a lot of progress about cleaning up TypedData interfaces.

Some examples of this:

1. Sequence::getItemDefinition(). A Sequence doesn't really match ListInterface which:

• "contains only items of the same type", our (config) lists don't and that's why we cannot really implement getItemDefinition().
• Also Sequence::getItemDefinition() makes the assumption that keys are numeric, configuration keys may or may not be numeric.

2. TraversableTypedDataInterface::set()
* @throws \InvalidArgumentException
* If the specified property does not exist. ??? This was not previously consistent behavior for lists and maps. How are we supposed to add elements to a list then?

3. TraversableTypedDataInterface::get()
As per the interface it doesn't support dot-notation but config elements do support it.
ComplexData get() throws exception when the key doesn't exist
Itemlist::get() just creates a new item

Those are some of the reasons why I think we'd be better off with a proper typed config interface. Even when you try to fix these typed data interfaces they don't really match our case and that is why I'd rather consider having typed config as an extension of typed data.

Maybe, as all of the classes really lack documentation, it is some help trying to clarify the ideas behind all of these objects. This is some summary:

ComplexDataInterface (typed data)

Map (named keys). Keys must have some DataDefinition. Trying to get/set keys that don't throws exception.

ListInterface (typed data)

List (orderable, numeric keys) I understand keys don't really matter, just order...
Keys don't need to be defined...

Mapping (typed config)

Map (named keys)
Unlike complex data, we don't have / cannot create elements when we don't have the value, even if the key is defined.

Sequence (typed config)

Map with a generic definition that is used for all items (but unlike ListInterface, the definition needs to be computed with actual configuration values)
Also unlike ListInterface, keys are meaningful (maybe uuid, plugin id, etc...)
We should be able to add new elements on the fly, but only with set(), not with get()...

So, quick summary: I like the patch, though it doesn't really fix this issue, but it fixes many other things.

FYI: #2414953-2: Element uses \Drupal::service() as a service locator and prevents injecting a custom typed config manager in config.

Thanks for #55, I agree on all points.

So I basically moved all changes from my interdiff out into the following issues:
#2420055: Document $value in TypedData
#2420049: Move \ArrayAccess from ListInterface to FieldItemListInterface
#2420067: Promote usage of TraversableTypedDataInterface

That allows us to go back to your patch, which I rerolled. With an eye towards #2420067: Promote usage of TraversableTypedDataInterface I'm also uploading a patch with makes ArrayElement::getElementDefinition() public, as that one introduces such a (public) method on TraversableTypedDataInterface.

Marking RTBC regardless. It's @Jose Reyero's patch, I just re-rolled it (twice), and the change from protected to public doesn't count as a significant contribution IMO. And if the change to public is not deemed appropriate for some reason, then the other one is RTBC.

Ahh, was about to open a bug against 8.0.x but realized there is in fact a problem introduced with this patch.

Also attaching the interdiff now, I meant to upload in the previous comment.

Yeah, the re-roll after #2414953: Element uses \Drupal::service() as a service locator and prevents injecting a custom typed config manager in config was not quite correct yet.

Also fixed come incorrect docs that were introduced with this patch.

Conflict on #2164601: Stop auto-creating FieldItems on mere reading of $entity->field[N]. Since that change was only added because Sequence implements ListInterface in 8.0.x (but not with this patch) there's nothing we need to do here.

Back to RTBC per #57.

As per the issue summary Both implementations are wrong, have a lot of bugs/issues and still worse, none of the interfaces fits 100% these classes intended behavior so should be bug.

We should go with the non-public patch since making it public can easily be done in #2420067: Promote usage of TraversableTypedDataInterface

This is great clean up. Thank you. This issue is a normal bug fix, it's disruptive changes are disruptive to anything extending config schema internals - highly unlikely at this point - and the cleanup reduces complexity and fragility, so it is allowed per https://www.drupal.org/core/beta-changes.

Some things before I can commit:

1. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
   @@ -47,59 +52,101 @@ protected function getAllKeys() {
        if ($value instanceof TypedDataInterface) {
          $value = $value->getValue();
        }
   

   This probably has been broken for sometime. There is no TypedDataInterface available in this scope to be able to make this comparison.

2. +++ b/core/lib/Drupal/Core/Config/Schema/ArrayElement.php
   @@ -109,4 +156,52 @@ public function getIterator() {
   +   * @param string $name
   ...
   +  protected function createElement($definition, $value, $key) {
   

   That should be @param string $key

Fixing as per #69. The changes are against the non-public patch in #66, i.e. 2298687-66.patch

@hussainweb ah my point with #69.2 is that this must be dead code.

And untested :)

@alexpott: I think you meant #69.1. I see what you mean about dead code. I quite agree, but what do you propose? Adding tests? I don't think you implied removing the code itself.

Yes, I've seen that quite a few times before and was never quite sure what to do with it. That's why I opened #2420383: Missing typehint in ArrayElement last week. I think that's a non-trivial issue including test coverage and everything and since it's not introduced here (and this patch is already quite complex) I think it's fair to handle that in a separate issue.

Re-RTBC-ing because of that, but I hope that's not received as status ping pong. Feel free to disagree, as always, and un-RTBC.

Ahh wait. The last patch already adds the use statement, so that contradicts solving it in a different issue. (... which still would make sense IMO.) That means the patch is not RTBC as it is.

yeah @tstoeckler I think we should remove it as it cannot be being used.

Also looking at it

  /**
   * Replaces the item at the specified position in this list.
   *
   * @param int|string $key
   *   Property name or index of the item to replace.
   * @param mixed $value
   *   Value to be stored at the specified position. It can be a raw
   *   configuration value or \Drupal\Core\TypedData\TypedDataInterface
   *
   * @return $this
   */

Is also wrong because in the patch in #66 it can not be a TypedDataInterface and it can be in the patch in #70 but it is completely untested (and apparently not needed).

This is largely over my head atm, but sounds like an awesome cleanup from the outside. Sequence kept getting in my way when I was working on ItemListInterface :-)

Just - noob remark of the day :
core/lib/Drupal/Core/Config/Schema/Element.php has a stale "use" on TypedConfigManagerInterface
core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php has a stale "use" on TypedDataInterface

Side note : it's a bit sad that core/lib/Drupal/Core/Config/Schema is a unsorted bag of very different animals (some TypedData classes, two exceptions, a discovery, a trait...). Not introduced here, but... anything we can do here or in a followup ?
(then again, the parent directory is much scarier - ah well...)

So I wanted to check every usage of TypedConfigInterface::set() with this patch. Now note that object creation in Drupal is very "multi-faceted" to say the least, so I certainly might have missed something, but I did give it my best shot and spent quite some time on this. I only found a single instance of this usage in all of core and that is in ConfigSchemaTest. A test patch I uploaded confirms this finding.

So I will open a follow-up to discuss removing that function alltogether. But for the scope of this issue it most certainly is fine to remove the "ability" to pass in typed data objects (which doesn't work anyway, as stated above).

That's what this patch does. As this is my only code contribution to this patch and it has been specifically requested and signed off by @alexpott, I am being bold and setting this to RTBC directly. As always, feel free to be bold and disagree.

Edit: And that one usage in ConfigSchemaTest sets a simple value directly and not a typed data object.

That's #2429157: Remove TypedConfigInterface::set().

This is an almighty cleanup. Thanks. Committed 4337033 and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/Config/Schema/Element.php b/core/lib/Drupal/Core/Config/Schema/Element.php
index 78f6ae9..2993f8c 100644
--- a/core/lib/Drupal/Core/Config/Schema/Element.php
+++ b/core/lib/Drupal/Core/Config/Schema/Element.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\Core\Config\Schema;
 
-use Drupal\Core\Config\TypedConfigManagerInterface;
 use Drupal\Core\TypedData\TypedData;
 
 /**
diff --git a/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php b/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php
index 3aac975..b35596a 100644
--- a/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php
+++ b/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\Core\Config\Schema;
 
-use Drupal\Core\TypedData\TypedDataInterface;
 use Drupal\Core\TypedData\TraversableTypedDataInterface;
 
 /**

Thanks @yched - fixed on commit.

Reverted and committed 9026147 and pushed to 8.0.x to fix the docs:

diff --git a/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php b/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php
index 3aac975..0847df0 100644
--- a/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php
+++ b/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php
@@ -60,8 +59,7 @@ public function get($name);
    * @param int|string $key
    *   Property name or index of the item to replace.
    * @param mixed $value
-   *   Value to be stored at the specified position. It can be a raw
-   *   configuration value or \Drupal\Core\TypedData\TypedDataInterface
+   *   Value to be stored at the specified position.
    *
    * @return $this
    */

Amazing, unbelievable! Yay! Thanks @reyero and @tstoeckler for sticking to this for so long. It was worth it :)

Awesome cleanup indeed! Congrat folks :-)

Ahh, sorry forgot about the docs. Thanks @alexpott, also for bringing this forward. Awesome!