Problem/Motivation
Configuration schema for common properties of configuration entities are defined(or duplicated) in each configuration which makes it very hard to maintain and add new item to it. For example, #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall introduced new element 'dependencies' in ConfigEntityBase which need be added in all config entity schemas.
Proposed resolution
Introduce new data type config_entity. From current implementation in can hold below elements:
config_entity:
type: mapping
label: 'Block'
mapping:
id:
type: string
label: 'ID'
uuid:
type: string
label: 'UUID'
weight:
type: integer
label: 'Weight'
status:
type: boolean
label: 'Status'
langcode:
type: string
label: 'Default language'
dependencies:
type: config_dependencies
label: 'Dependencies'
Remaining tasks
Issue patch, test, review, commit
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#17 | 2273631-config_entity-type-17.patch | 26.26 KB | vijaycs85 |
#9 | 2273631-config_entity-type-9.patch | 24.22 KB | vijaycs85 |
#7 | 2273631-diff-1-7.txt | 22.92 KB | vijaycs85 |
#7 | 2273631-config_entity-type-7.patch | 24.17 KB | vijaycs85 |
Comments
Comment #1
vijaycs85Adding the type and updating just shortcut. If agreed, can update all config entity schemas.
Comment #2
Gábor HojtsyI think this makes a ton of sense, good I questioned if status and label really common/required properties? Looking at Entity and ConfigEntityBase, it looks like status is there and with the current "public properties are exported" scheme, it would become a property of all config entities. Label is not in either base class though. So at best that would be best practice, no?
Comment #3
vijaycs85Thanks for the review @Gábor Hojtsy. I agree that some of the property (like label) is not common for all config entity types, however they are used in almost all types. So adding in data type doesn't hurt until we define an entity with label property for some other purpose/type(say for example, use label as boolean to flag label available or not). I don't think we will have that problem in core as well as it is not best DX to use the same property for different purpose. So we should be fine?
Comment #4
Gábor HojtsyYeah right, well, that you can define the label even though it may not be used is thanks to the schema being applied to the data and not the data to the schema :D That means elements defined in the schema are optional in the data in practice. I don't think we have that documented or relied on it so much knowingly at least. It sounds like it makes sense in this context and someone who need more strict definitions would still be able to define their own mapping. The config entity with label is more like an 80% use case.
Comment #5
Gábor HojtsyMoving this onto D8MI sprint. Let's get the rest of the types covered then. :)
Comment #6
Gábor HojtsyComment #7
vijaycs85Here is an update...
Comment #8
Gábor HojtsyLooks like the name == machine name is not a good unification, some use name == label and type == machine name :/
Looks like those properties can only be unified with some API changes. :/ We should keep those out of the type but then not sure how it looks that some common ones are not in there... It may still be better than what we have now.
I really like we would get rid of ugly stuff like this one on node types. This label is silly....
Comment #9
vijaycs85Thanks for the quick review @Gábor Hojtsy.
#8.1 - Fair point, let's get this fixed separately #2276379: Unify name property of configuration entities.. For this issue, removed the 'name' related changes.
#8.2 - yeah, sure.
Comment #12
vijaycs85Fixing test fail...
Comment #13
vijaycs85It is green. Ready for review.
Comment #14
Gábor HojtsyYay, now looks good, except:
Erm, this type name looks copy pasted :D Not appropriate for a generic type.
Comment #15
vijaycs85good catch, In fact, we don't need label there.
Comment #17
vijaycs85Rerolling...
Comment #18
Gábor HojtsyLooks good to me now, yay!
Comment #19
Jose Reyero CreditAttribution: Jose Reyero commentedI think this is looking great.
About label, status and other maybe optional properties, this may be an issue if we end up doing some validation. According to @Gábor, and I agree, a schema property that is not in the data should produce a warning, and I don't think validating the default configuration deployed with Drupal core should produce any warning at all. Comment here, https://drupal.org/node/1928868#comment-8828687
However, if we make progress with merging TypedConfig and TypedData, in the DataDefinition base type (which will be built from the schema definition) there's already a 'required' property that will look like this:
See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21TypedData...
Comment #20
Jose Reyero CreditAttribution: Jose Reyero commentedWell, the morale of my previous comment, just in case is: go ahead with this one, if missing properties are an issue for validation later (that is to be implemented), we can fix it on the schema itself.
Comment #22
alexpottNice - way less duplicated schema!
Committed 7a98471 and pushed to 8.x. Thanks!
Comment #23
Gábor HojtsyWoot, superb, thanks.