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.
In the general spirit of D8, we should move data types from their current, awkward callback function-based structure to proper OO plugins.
Comment | File | Size | Author |
---|---|---|---|
#17 | 2080791-17--data_type_plugins--interdiff.txt | 663 bytes | mollux |
#17 | 2080791-17--data_type_plugins.patch | 31.39 KB | mollux |
Comments
Comment #1
drunken monkeyComment #2
drunken monkeyAlso possibly worth a thought: introduce a proper method for checking for data type support in a service class, instead of using a "magic name"
supportsFeature()
call.Comment #3
mollux CreditAttribution: mollux commentedComment #4
mollux CreditAttribution: mollux commentedThe attached patch provides the datatype plugin functionality and cleans up the getdatatype() logic to be more useful.
Things to do:
- write tests
- add documentation page with all the datatypes.
Comment #5
drunken monkeyGreat work, Mattias, thanks!
I didn't try it out yet, but I looked over the code and saw only a few minor issues:
Unless I'm mistaken, services should generally be sorted alphabetically, so this should go further up.
In my eyes, this looks weird, I'd prefer
DataType
for both namespace and class names. It's the same in method names already, after all.Of course, we use "datasource" as one word, but I think that's a special case.
Incomplete doc comment.
@see
should come after@return
. See the documentation standards.These should be separated by an empty line.
Missing trailing newline.
Overlong comment line.
I think just using the
t()
function is fine here, too. You should only use the OOP way if you can inject the translation manager.Incomplete doc comment.
Comment #6
mollux CreditAttribution: mollux commentedComment #7
mollux CreditAttribution: mollux commentedFixed the issues above + wrote the basic tests to check the functionality. UI tests should be added.
Comment #8
mollux CreditAttribution: mollux commentedComment #10
mollux CreditAttribution: mollux commentedAaargh, the tests moved. Re-roll will be for tomorrow
Comment #11
mollux CreditAttribution: mollux commentedre-roll
Comment #13
mollux CreditAttribution: mollux commentedfixed the tests
Comment #14
mollux CreditAttribution: mollux commentedAdded webtest to check that the custom data type can be selected. This is normally RTBC :)
The UI stuff (overview page of (custom) data types ant their description + making it visible if the data type of a field isn't supported by the backend) will be tackled in a separate issue.
Comment #15
drunken monkeyThanks, great work, looks very good already!
Just had a few code style fixes, in the attached patch. Please reviw to see if you agree with them. (The empty line before the end of a class is my personal taste, but currently (I think) uniform within the module.)
Also, it seems this is still missing the page listing all data types? Or is there already an issue for that?
Comment #17
mollux CreditAttribution: mollux commentedAs discussed, I changed the test code to use the magic method again. For me the other changes are ok!
Comment #18
drunken monkeyGreat, committed.
Thanks again!