Problem/Motivation
Configuration files must be either lists or maps, i.e. a configuration file that contains just a scalar value is invalid. In particular, the configuration schema of config objects is either a Sequence
or a Mapping
. SchemaCheckTrait
validates this by checking whether the typed config objects are instances of ArrayElement
- the common parent class of the two implementations. This is a fairly arbitrary check, though.
Proposed resolution
What we really are validating is that the schema definition object is traversable. Therefore we should have an interface for typed data objects that are traversable: TraversableTypedDataInterface
.
ComplexDataInterface
and ListInterface
should extend TraversableTypedDataInterface
and ArrayElement
should implement it.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#17 | 2346129-15-array-element-typed-data.patch | 15.53 KB | tstoeckler |
Comments
Comment #1
tstoecklerSo in fact, I was wrong.
ComplexDataInterface
maps toMapping
, butListInterface
(which does not extendComplexDataInterface
) maps toSequence
. So this patch instead introduces aTraversableTypedDataInterface
which is justTraversableTypedDataInterface extends TypedDataInterface, \Traversable
which can be used for type-hinting, and which bothComplexDataInterface
andListInterface
extend.Let's see how badly this breaks.
Comment #2
tstoecklerComment #3
Jose Reyero CreditAttribution: Jose Reyero commentedI agree that we shouldn't be using ArrayElement for that, but not sure we need to invent a new interface.
Could we just check whether it implements \Traversable ? Or it implements \Traversable && \TypedDataInterface ?
Comment #4
Jose Reyero CreditAttribution: Jose Reyero commentedForget my previous question
@tstoeckler exlplained it to me in person to me and the reason is function getArrayTranslation(TraversableTypedDataInterface $element...)
So I'm all for this one, it is a simple patch and makes a lot of sense...
... and also we could be using it in some more places in typed data like
All the typed data's parent parameters could/should (?) be this type, I think.
Comment #5
tstoecklerI just realized that I totally forgot to follow-up on this, sorry @Jose Reyero.
Nice catch on the TypedDataInterface::getParent(). Updated that and ::getRoot() as well.
Btw, a note on some of the seemingly unrelated changes: I went through all usages of
ArrayElement
to find whether they were still legit and therefore included a few easy clean-ups even though their not necessary strictly speaking.Comment #6
Jose Reyero CreditAttribution: Jose Reyero commentedIt looks great to me, just two minor things.
Not sure whether this is on purpose (using \Traversable), it looks ok anyway.
Should we change this one too, not really important, but maybe yes just for consistency since getParent() returns TraversableTypedDataInterface...?
Whatever I think the patch is ready to go, so we get rid of 'instanceof ArrayElement', whether we use it for all other things in TypedDataInterface, is not that important IMO..
Comment #7
Jose Reyero CreditAttribution: Jose Reyero commentedRelated issue, that could very well benefit from this new interface, #2298687: Sequence and Mapping implement interfaces incorrectly, make them honest about what they support
That is one more reason to push this one forward.
Comment #8
vijaycs85Comment #9
tstoecklerHere we go.
CanIHazRTBC? :-P
Comment #11
tstoecklerMaybe this time?
Comment #13
tstoeckler...
Comment #15
Gábor HojtsyComment #16
vijaycs85may just need this:
Comment #17
tstoecklerMaybe this one?
Comment #18
andypostGreen! +1 to RTBC
Comment #19
vijaycs85Looks like irrelevant to this issue. In fact the old format (i.e. use) looks like the right way.
Comment #20
tstoeckler@vijaycs85 you are correct that this is theoretically not necessary but I was changing the implements line anyway, so...
And our coding standards dictate the leading backslash for classes in the global name space.
RTBC per #18
Comment #21
alexpottCommitted 221a48c and pushed to 8.0.x. Thanks!
Minor code doc fix on commit.
Comment #23
tstoecklerYay, thanks!
Comment #24
Gábor Hojtsy