Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
ComplexDataDefinitionBase declares abstract public function getPropertyDefinitions() which made me think that this method is not in the interface, so if my own class does not extend the Base class but just uses the interface, then I don't need to care about implementing that method. That was a false assumption, because the same method is also declared in ComplexDataDefinitionInterface. Bad DX.
Proposed resolution
Remove the declaration from ComplexDataDefinitionBase.
Remaining tasks
User interface changes
API changes
None
Beta phase evaluation
| Issue category | Task because the behaviour is completely unchanged |
|---|---|
| Issue priority | Minor because DX is slightly improved |
| Unfrozen changes | Not unfrozen according to the written requirements, but the change is trivial to a similar extent. |
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 2493367-4.patch | 581 bytes | joshi.rohit100 |
Comments
Comment #1
joshi.rohit100But if a class implements an interface, then it must implement all the interface methods. If we remove the method from the class, it will throw error. Also as per my understanding, if a class doesn't have interface method implementation, then that method and class must declare as abstract.
Comment #2
ArlaYes, but the thing is that ComplexDataDefinitionBase is already abstract. It has to, because otherwise it couldn't declare the method as abstract.
Comment #3
joshi.rohit100@Arla - thats my point. ComplexDataDefinitionBase is abstract due to the method getPropertyDefinitions() (as it has no defination). Also as it implements the interface, it must decalre (at least signature) that method in class.
Comment #4
joshi.rohit100Please review this :)
Comment #5
ArlaLooks good.
Comment #6
cilefen CreditAttribution: cilefen commentedAll issues need beta evaluations to be committed in this phase. Please consider the evaluation components carefully to make things easier on the committers.
In order that we can get more people doing reviews, please say a little about what you did to determine this is RTBC. Here is an example of what I did on a similar simple issue.
Comment #7
ArlaInserted beta evaluation.
Comment #9
Mile23ComplexDataDefinitionBaseimplementsComplexDataDefinitionInterface, which declarespublic function getPropertyDefinitions().Because the base class wants subclasses to fill in the blanks on
getPropertyDefinitions()and thus fulfill the promise of the interface, it declaresgetPropertyDefinitions()as abstract, so you can't new one into existence without implementing that method.There might be a reason it doesn't simply follow the lead of
getPropertyDefinition()(singular) and make a generic implementation, but I don't know what that reason is.But as far as the scope of this issue:
ComplexDataDefinitionBaseseems to be doing the right thing. If it were declaring the method as protected or something it might be wrong, but it's not.Marking as closed won't fix, but feel free to re-open if my analysis is wrong.
A good separate follow-up issue might be to ask whether
getPropertyDefinitions()should have boilerplate code like it's singular pal.Comment #10
ArlaIt is already abstract as it is declared by ComplexDataDefinitionInterface. This is not breaking coding standards, it's just bad DX. AFAIK, nowhere else do we re-declare a method in an abstract class identically to how it's declared in an interface.
I'm fine with closing the issue. If someone else agrees with me then we can discuss it again.
As I see it, there's no reason to assume that several complex data definitions would declare similar properties, or generate them in a similar way. Core has only two implementations (EntityDataDefinition and MapDataDefinition) and they have very different implementations.