Motivation
- Typed has the concept of lists and complex data, however there is general notion of general "data containers". That makes navigating in a tree of typed data unnecessarily complex as there is no fitting "data container" interface for the parent object of properties or list items.
- We have quite some methods that both interfaces (List + Complex data) share, e.g. get(), set(), isEmpty() - having this in a common container interface allows us to easily make use of those when working with data parent objects (e.g. the result of getParent()).
- Meanwhile, there is a TraversableTypedDataInterface which was aided as band-aid but fails to deliver the semantic meaning or
- When mapping data structures to typed data which neither are a fit for list interface or complex data interface, it's possible to stick to DataContainerInterface then. This is the case for sequences of config schema and therefore would help solving https://www.drupal.org/node/1928868.
Proposed resolution
Extract a common interface: DataContainerInterface. According to wikipedia, a container can either be a list or complex data.
As complex data and lists use different terminology for their contained data it probably makes sense to keep shared methods overridden in those interface for better documentation.
Then, as there no use left for TraversableTypedDataInterface we should deprecate that in favour of DataContainerInterface.
Remaining tasks
Do it.
User interface changes
-
API changes
None - we could decide to add some type-hint to DataContainerInterface somewhere if we want to though.
Comment | File | Size | Author |
---|---|---|---|
#58 | 2350597-58.patch | 9.8 KB | amateescu |
#44 | 2350597-44-interdiff.txt | 5.16 KB | Berdir |
#44 | 2350597-44.patch | 10.12 KB | Berdir |
#41 | interdiff-2350597.txt | 579 bytes | dawehner |
#41 | 2350597-41.patch | 7.13 KB | dawehner |
Comments
Comment #1
XanoComment #2
XanoComment #4
XanoComment #5
fagono complex data here.
TypedDataInferface:
* @return \Drupal\Core\TypedData\ComplexDataInterface|\Drupal\Core\TypedData\ListInterface
Should be fixed.
We could improve type hints on them, what means a small API change for inheriting classes. Most extend TypedData though.
Comment #6
XanoComment #8
XanoComment #9
jhedstromComment #12
cafuego CreditAttribution: cafuego commentedComment #13
cafuego CreditAttribution: cafuego commentedSorry, no idea where to even start with this reroll.
Comment #14
XanoRe-roll/remake.
Comment #15
XanoComment #17
cilefen CreditAttribution: cilefen commentedComment #18
fagoAs highlighted by #2420067: Promote usage of TraversableTypedDataInterface and #1928868: Typed config incorrectly implements Typed Data interfaces a common, parent interface is really missing. While meanwhile TraversableTypedDataInterface was added, I strongly dislike it as it has no semantic meaning, but highlights a PHP implementation detail of being traversable.
I think we should get the ball rolling on this one again, add it and deprecate TraversableTypedDataInterface by it.
Edit: Please also see my comment at https://www.drupal.org/node/2420067#comment-11405779. As noted there Collections as a term is maybe not a perfect fit for the DataContainer as it does not really meet the following requirement:
That said, associate arrays are considered collections also - so usage and exact definition varies / is a bit blurry.
Comment #20
fagoThis is a task.
Comment #21
fagoComment #22
fagoAdding more clarification on why this is important for Typed data: introducing the data container cleans up the glitches around TraversableTypedDataInterface
Comment #23
fagook, attached is a patch which introduces the data container interface and deprecates the TypedDataTraversableInterface. DataContainerInterface contains the common methods, however I decided to keep get() and set() defined in the list + complex data interfaces as they contain documentation in the data-structure specific wording.
Note there is toArrray() missing from DataContainerInterface as this is right now not on ListInterface. I'd love to add that but that deserves its own issue/discussion.
Comment #24
fagoAnd here more work that deprecates the usage of the traversable typed data interface.
Comment #25
dawehnerIt 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.
Comment #26
tstoecklerSorry to be a party pooper, but I'd like to take a look at this first before this goes in.
Comment #27
dawehnerOH yeah, no worries. I'm a bit sleep deprived, so don't take me too serious.
Comment #28
tstoecklerSo this issue is a bit frustrating because the newly
DataContainerInterface
doesn't really give us anything practical over the currentTraversableTypedDataInterface
. 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.
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()
andset()
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()
, andisEmpty()
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: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.
Comment #29
tstoecklerComment #30
dawehner@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.
Comment #31
tstoecklerThank 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
andTraversableTypedDataInterface
but the latter already extends the former, so that seems unnecessary?EDIT: Forgot some words.
Comment #32
dawehnerThat's a good point. I wonder whether there are tools to automatically detect that kind of stuff.
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.
Comment #33
tstoecklerYes, that's a very good point. Probably a commit here should be blocked on that.
Comment #34
Wim LeersThis 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.
Comment #35
dawehnerI asked fago for feedback on twitter. Maybe this helps.
Nice catch! Too bad phpstorm doesn't tell you that automatically.
Comment #37
dawehner@tstoeckler @fago
Is there any way to get some feedback from you here?
Comment #38
tstoecklerLooked at this for a bit, and I like it a lot. I think it is a step in the right direction.
Some thoughts:
Should this not return
TypedConfigInterface
?I think this makes sense and is a big win of this patch in terms of understability.
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?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.Comment #39
BerdirHaven'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?
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.
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.
Comment #40
dawehnerThat is a bit tricky. What happens if you want to typehint that?
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?
Comment #41
dawehnerNote: This just addressses @tstoeckler
Here is a patch with addressing the first point for now.
Comment #42
BerdirI 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.
Comment #43
tstoecklerYeah, that's a really good summary, thank you. I agree with everything, but for:
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...
Comment #44
BerdirMade some progress towords my summary. Very untested.
Comment #58
amateescu CreditAttribution: amateescu for Tag1 Consulting commentedRe-rolled #44 to maybe get some eyes back on this issue :)
Comment #59
Wim LeersOhhhhh 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? 🙏