Extract a common DataContainerInterface for lists and complex data

Sorry, no idea where to even start with this reroll.

Re-roll/remake.

It makes sense for me to name this common interface DataContainer. It semantically makes it kind of clear.

I also agree that overriding some of the methods is a great idea to not have too generic methods. This adds better DX
All in all, I couldn't find anything really bad as part of the patch. The only small and potential worrying are BC problems with changing of existing signatures, but well, typed data is seriously some quite internal stuff.

Sorry to be a party pooper, but I'd like to take a look at this first before this goes in.

OH yeah, no worries. I'm a bit sleep deprived, so don't take me too serious.

So this issue is a bit frustrating because the newly DataContainerInterface doesn't really give us anything practical over the current TraversableTypedDataInterface. When I started pushing for the latter a couple years ago there was not much interest from anyone in typed data land, and now basically we are doing the same thing a second time.

And especially annoying is if not even proper justification is given.

there is a TraversableTypedDataInterface which was aided as band-aid but fails to deliver the semantic meaning

Is just pretty weak tea, I'm sorry. The issue summary doesn't at all mention that TraversableTypedDataInterface is 90% of what we want, just by a different name.

So that is a process argument, and the reason why I am a bit annoyed about this.

So the actual issue (which you would not get from the issue summary, though) is naming: I agree that "traversable" is a pretty limited concept and slapping things like get() and set() on something that is called a "traversable" is not very semantic. I like the "container" word better. So on a code level I agree with the general direction of the patch.

I see absolutely no point in deprecating TraversableTypedDataInterface, though. I agree that it is currently overloaded, but the current patch does the same thing just the other way around: it completely overloads the "container" semantic. get(), set(), and isEmpty() make total sense for something that is called a "container", but traversing is a completely different - and separate - semantic. The following hunk in the patch is completely bogus, I don't even know how to explain why, it's just so apparent from reading the code:

-    if ($element instanceof TraversableTypedDataInterface) {
+    if ($element instanceof DataContainerInterface) {
       foreach ($element as $child_element) {

That should be reverted and TraversableTypedDataInterface should be kept for usages of traversing (typed) data.

Going back to a process argument, this last part also makes it rather clear to me that actual semantics are not necessarily at the heart of this patch.

@tstoeckler
Thank you for the great review. I agree its painful. I honestly agree 100% with you, we are post release, and we cannot change those fundamentals any longer.
After talking with @alexpott, here is a try to remove the potential risk as much as possible which makes is just about adding the new interface and converting existing usages, but a) don't deprecate the other ones b) Allow to still use it completely.

Thank you @dawehner, that patch looks really great. I would still have to actually apply it and look at it in context to RTBC but this does not need to hold anyone else from pulling the trigger.

One nit-pick:
DataContainerInterface extends \Traversable and TraversableTypedDataInterface but the latter already extends the former, so that seems unnecessary?

EDIT: Forgot some words.

DataContainerInterface extends \Traversable and TraversableTypedDataInterface but the latter already extends the former, so that seems unnecessary?

That's a good point. I wonder whether there are tools to automatically detect that kind of stuff.

Thank you @dawehner, that patch looks really great. I would still have to actually apply it and look at it in context to RTBC but this does not need to hold anyone else from pulling the trigger.

Sure, please take your time, seriously.

I think we should also try to somehow get fago involved or at least approve the smaller scope we have now.

I think we should also try to somehow get fago involved or at least approve the smaller scope we have now.

Yes, that's a very good point. Probably a commit here should be blocked on that.

This is blocking #1928868: Typed config incorrectly implements Typed Data interfaces which is blocking #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs … which is blocking #2300677: JSON:API POST/PATCH support for fully validatable config entities.

I asked fago for feedback on twitter. Maybe this helps.

One nit-pick:
DataContainerInterface extends \Traversable and TraversableTypedDataInterface but the latter already extends the former, so that seems unnecessary?

Nice catch! Too bad phpstorm doesn't tell you that automatically.

@tstoeckler @fago
Is there any way to get some feedback from you here?

Looked at this for a bit, and I like it a lot. I think it is a step in the right direction.

Some thoughts:

1. +++ b/core/lib/Drupal/Core/Config/TypedConfigManagerInterface.php
   @@ -20,7 +20,7 @@
   -   * @return \Drupal\Core\TypedData\TraversableTypedDataInterface
   +   * @return \Drupal\Core\TypedData\DataContainerInterface
   

   Should this not return TypedConfigInterface?

2. +++ b/core/lib/Drupal/Core/TypedData/ComplexDataInterface.php
   @@ -18,7 +18,7 @@
   -interface ComplexDataInterface extends TraversableTypedDataInterface {
   +interface ComplexDataInterface extends DataContainerInterface {
   

   I think this makes sense and is a big win of this patch in terms of understability.

3. +++ b/core/lib/Drupal/Core/TypedData/DataContainerInterface.php
   @@ -0,0 +1,76 @@
   +interface DataContainerInterface extends TypedDataInterface, TraversableTypedDataInterface {
   

   Why do we extend TraversableTypedDataInterface here? I guess our implementations will always (or at least mostly?) implement both semantics, i.e. they are both containers in that you can access child items directly, but you can also traverse them - but in terms of the interface the semantics are different, no?

4. +++ b/core/lib/Drupal/Core/TypedData/DataContainerInterface.php
   @@ -0,0 +1,76 @@
   +  /**
   +   * React to changes to a child object.
   +   *
   +   * Note that this is invoked after any changes have been applied.
   +   *
   +   * @param string|int $key
   +   *   The key of the child to get; e.g., a property name like 'title' or
   +   *   'name', or the index of a list item.
   +   */
   +  public function onChange($key);
   

   I think it is more semantic to have this here than on TraversableTypedDatataInterface, but that means we should remove it from that one. I sincerely hope that no one would consider that an API change.

1. +++ b/core/lib/Drupal/Core/Config/Schema/TypedConfigInterface.php
   @@ -14,7 +14,7 @@
     */
   -interface TypedConfigInterface extends TraversableTypedDataInterface {
   +interface TypedConfigInterface extends DataContainerInterface {
    
   

   Haven't really followed the issue closely so far, but this is a class that doesn't have a default base class/trait, so adding new methods (by extending a different interface) is problematic.

   I'm not sure what the best way to address that is, one option might be to not touch TypedConfigInterface and instead explicitly add this interface to specific implementations that we provide?

   And in the other direction as well, code might type hint on that interface and would break if we wouldn't do that anymore. I understand the new interface extends from TraversableTypedDataInterface, so that solves that, we might want to also document that we do it (partially?) because of that?

2. +++ b/core/lib/Drupal/Core/TypedData/DataContainerInterface.php
   @@ -0,0 +1,76 @@
   + * Interface for data containers; i.e., data containing other typed data.
   + *
   + * This typed data might be either other containers, like a list, or some
   + * primitive values like a string or an integer typed data.
   

   Using i.e. in the first line description is fairly uncommon, I had to actually look up the rules again to be sure this is correct (it is). Wondering if we can rewrite it so it is easier to understand for non-native speakers.

   Similar for the second sentence.. we just defined what data containers are, so maybe we can say This (That?) typed data can either be other data containers or primitive values like strings or integers.

   "either be other" sounds better in my mind than "be either other", not sure if that is really better or not. And we start the sentence with typed data, so I think we don't need to repeat that for the primitive values. Maybe just primitives instead of primitive values.

3. +++ b/core/lib/Drupal/Core/TypedData/DataContainerInterface.php
   @@ -0,0 +1,76 @@
   + * This is a common interface for complex data and lists, which both contain
   + * other typed data items. If neither complex data or lists are a good fit for
   + * a certain data structure, this generic interface can be implemented directly.
   

   This is also a bit repetitive IMHO.

   We explained that data containers contain other typed data, so it is IMHO clear that list and complex data do that by implementing this interface, we don't need to repeat that.

   What about:

   This interface allows to work with any type of data container, including lists and complex data, the two common implementations for data containers and specializations of this interface.

Why do we extend TraversableTypedDataInterface here? I guess our implementations will always (or at least mostly?) implement both semantics, i.e. they are both containers in that you can access child items directly, but you can also traverse them - but in terms of the interface the semantics are different, no?

That is a bit tricky. What happens if you want to typehint that?

I think it is more semantic to have this here than on TraversableTypedDatataInterface, but that means we should remove it from that one. I sincerely hope that no one would consider that an API change.

So this would a bit contradict your point with moving the traversable out of data containerinterface? For me traversable and container are indeed two things and should be separated. Given that onChange should be on both?

Note: This just addressses @tstoeckler

Here is a patch with addressing the first point for now.

I discussed this in person with @dawehner and @tstoeckler and I think (correct me if I got something wrong, some details are also not 100% clear yet), we came to the following conclusions:

* Don't block #1928868: Typed config incorrectly implements Typed Data interfaces on this, go with a workaround there, that we might change again here. That issue enables a lot of blocked issues around config entities.
* While discussing BC and so on, we noticed that this is basically a more generic replacement for TypedConfigInterface, we the plan is to deprecate that interface in favor of this: It has two additional methods, getElements() (only used internally, so can be protected in 9.x) and toArray() (apparently not used at all), we were discussing to have some sort of replacement for this on DataContainerInterface, possibly a getKeys() or similar method. Argument against that is that there could be very large data sets that have to possibly even load their data, my personal argument is that a getKeys() method could possibly be optimihzed in many cases as opposed to the only alternative of traversing it completely.
* (not 100% sure about this) Also deprecate TraversableTypedDataInterface, move onChange to DataContainerInterface and type hint on \Traversable directly if that's what we want from an object.
* Implement \Traversable in DataContainerInterface (not 100% agreement on this I think, as it's not 100% correct but correct enough for the PHP world)

So we deprecate 2 strange interfaces and replace it with a saner version of it, yay :) Explicitly deprecating but keeping the old interfaces also nicely avoids my BC concerns.

Also not sure yet if we want to do all of that here, but it might be easier, especially since adding methods to an interface once it exists is again hard except when we make it internal.

Yeah, that's a really good summary, thank you. I agree with everything, but for:

So we deprecate 2 strange interfaces and replace it with a saner version of it, yay :)

Not sure the interface that we have then come up with is very "sane", and the reason we are deprecating TraversableTypedDataInterface is not because it is not sane, but to my mind because we have given up on clear data semantics wholesale and are just trying to find something that is generic enough to properly marry config with field API through the power vested in typed data. And as stated above already, since that was the original goal of TTDI in the first place - on a conceptual level at least - this is really more a rename that anything else. And I guess "container" is a less obtuse word than "traversable", so maybe at least on that front we are gaining some "sanity", but, yeah...

So I agree with the path forward, I just personally don't see any reason to be excited about typed data becoming less confusing any time soon. But I guess that's up to everyone's own opinion...

Made some progress towords my summary. Very untested.

Re-rolled #44 to maybe get some eyes back on this issue :)

Ohhhhh very interesting!

I wondered how this is different from \Drupal\Core\TypedData\TraversableTypedDataInterface, but the issue summary explains that.

Could you convert this to an MR? 🙏