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
Currently a workflows state is a fairly limited because it's just an arbitrary object, which is pretty loose and doesn't offer us many benefits.
Proposed resolution
Convert State to a DataType using the TypedData API. This allows us to make use of all TypedData API features as well as related APIs such as Context.
Remaining tasks
- Review
- Tidy
- Test
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 2846335-3.patch | 26.78 KB | timmillwood |
Comments
Comment #2
timmillwoodComment #3
timmillwoodThink I'm missing some files.
Comment #6
timmillwoodNot sure why this broke the translation test.
Comment #7
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedCan the deleted lines in test be ported instead of deleted? Is there a reason they are no longer relevant?
Comment #8
timmillwood@Sam152 - I expect they can be ported, the issue is they depend on TypedDataManager, which makes unit tests a bit more complex.
Comment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI wonder if there is a concrete use case for this change that can be included with the patch to demonstrate why it's valuable.
Also, do transitions need the same treatment? Maybe a real world "this will allow us to achieve..." would help answer that question.
Comment #11
timmillwoodI guess transitions could have it in the future.
I came to wanting states to be data types to be able to do
new Context(new ContextDefinition('state'), $state);and use state as a context. Then the more I thought about it, it just seemed to make sense. Drupal has this inbuilt thing for defining types of stuff, but we just create an arbitrary State class.Comment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedMaybe @alexpott should weigh in on the design patterns, being the author. Value objects would seem to have a place here but seems reasonable to use a Drupal abstraction if the shoe fits.
What was this for? Filtering a view by some state? Passing it into some other context?
Comment #13
timmillwoodI was thinking one use for context would be within the access handler when deleting states, we'll know the state from the context.
Comment #14
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedCould we add the state context to the access checks as part of the patch to demonstrate the usefulness? Could be a useful DX addition regardless of the core use of it.
Comment #15
tacituseu CreditAttribution: tacituseu commentedComment #16
bojanz CreditAttribution: bojanz at Centarro commented-1.
Value objects are simple, and can be instantiated directly through code.
DataTypes can't be instantiated directly, and are harder to understand.
This patch adds a big DX regression just to simplify context passing.
Comment #17
timmillwoodComment #18
geek-merlinI support bojan's point. And no need to give this up then. Look into entities, they *are* no typed data, but can be wrapped to typed date via EntityAdapter class.