Problem/Motivation
ComplexDataDefinitionInterface documents getPropertyDefinitions() as "Gets an array of property definitions of contained properties." ComplexDataDefinitionBase however additionally expects the method to set the result to $this->propertyDefinitions. It leaves getPropertyDefinitions() abstract with {@inheritdoc}, so the method documentation is insufficient to anyone wanting to implement it.
Proposed resolution
Set $this->propertyDefinitions = $this->getPropertyDefinitions()
in ComplexDataDefinitionBase::getPropertyDefinition(). Or maybe change the documentation to include something like "set the result to $this->propertyDefinitions".
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#12 | revised-complexDataDefinition-2412363-12.patch | 734 bytes | Arla |
| |||
#11 | revised-complexDataDefinition-2412363-3.patch | 641 bytes | mgifford |
#3 | revised-complexDataDefinition-2412363-3.patch | 641 bytes | zealfire |
Comments
Comment #1
ArlaAs long as someone who is already well-informed in the typed data system can confirm this and decide on the appropriate resolution, it should be a Novice task to provide a patch.
Comment #2
zealfire CreditAttribution: zealfire commentedI would love to work on this and this is what i feel that since the function ComplexDataDefinitionBase::getPropertyDefinition() expects to set $propertyDefinitions when it does not exists so we should go we this: $this->propertyDefinitions = $this->getPropertyDefinitions().
If there is a yes then i may submit a patch.
Comment #3
zealfire CreditAttribution: zealfire commentedPlease see mine comment:2 and submitting this patch to see if its passes test.Please review.
Thanks.
Comment #4
ArlaLooks good to me!
Comment #5
amateescu CreditAttribution: amateescu commentedLet's wait for a review from a typed data maintainer first :)
Comment #6
cmanalansan CreditAttribution: cmanalansan commentedTriaged as Novice.
Comment #7
fagoYep, that makes sense - we should not rely on the implementation internal.
Comment #8
xjmSo, from the summary:
That's a dash of code smell for sure. Thanks for finding and patching this. And thanks @fago for the signoff, and @amateescu for making sure we got it. ;)
I find it a little weird for the getter for one property to be setting a different property, and find myself wondering what happens if the results of the getter have changed since the property was set.
Wouldn't the following make more sense?
Setting NR for feedback on that. (Also removing the novice tag since there are no novice tags at present.)
Comment #9
xjmForgot to mention -- if the goal is to statically cache the results of
getPropertyDefinitions()
to the property, then that method should be responsible for its own internal static cache and return early with the property. Having another method responsible for it seems a bit dubious. :)Comment #10
ArlaI agree that it's weird that "singular" getter modifies the "plural" property. But I think the snipped suggested in #8 has the disadvantage that it pushes down the responsibility of static-caching the property to the implementor. How about implementing getPropertyDefinitions in the base class with caching, and declare abstract loadPropertyDefinitions?
Comment #11
mgiffordRe-uploading patch for the bots.
Comment #12
Arla#3 duplicates the assignment to $this->propertyDefinitions, and #10 breaks BC. @xjm's #8 + #9 seems like the most reasonable approach.
But does it make sense to keep the declaration of $propertyDefinitions in Base? Pushing it down will break BC.
Comment #13
BerdirMakes sense to me.
Comment #14
catchComment #15
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!
Comment #19
BerdirUh, weird status change. Setting back to fixed.