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

CommentFileSizeAuthor
#17 2273631-config_entity-type-17.patch26.26 KBvijaycs85
#15 2273631-diff-12-15.txt388 bytesvijaycs85
#15 2273631-config_entity-type-15.patch26.47 KBvijaycs85
#12 2273631-diff-9-12.txt2.29 KBvijaycs85
#12 2273631-config_entity-type-12.patch26.5 KBvijaycs85
#9 2273631-diff-7-9.txt3.48 KBvijaycs85
#9 2273631-config_entity-type-9.patch24.22 KBvijaycs85
#7 2273631-diff-1-7.txt22.92 KBvijaycs85
#7 2273631-config_entity-type-7.patch24.17 KBvijaycs85
#1 2273631-config_entity-type-1.patch1.63 KBvijaycs85
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Active » Needs review
FileSize
2273631-config_entity-type-1.patch1.63 KB

Adding the type and updating just shortcut. If agreed, can update all config entity schemas.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

I 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?

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented

Thanks 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?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Yeah 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.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Introduce new config schema data type 'config_entity' » Unify config entity schemas with a base schema type
Component: configuration entity system » configuration system
Issue tags: +sprint, +language-config

Moving this onto D8MI sprint. Let's get the rest of the types covered then. :)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Assigned: Unassigned » vijaycs85
vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
FileSize
2273631-config_entity-type-7.patch24.17 KB
2273631-diff-1-7.txt22.92 KB

Here is an update...

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work 
  1. +++ b/core/config/schema/core.data_types.schema.yml
@@ -231,3 +231,29 @@ config_dependencies:
+    name:
+      type: string
+      label: 'Machine name'

+++ b/core/modules/field/config/schema/field.schema.yml
@@ -9,24 +9,9 @@ field.settings:
-    name:
-      type: string
-      label: 'Name'

+++ b/core/modules/filter/config/schema/filter.schema.yml
@@ -12,21 +12,12 @@ filter.settings:
     format:
       type: string
       label: 'Machine name'
-    name:
-      type: label
-      label: 'Name'

+++ b/core/modules/node/config/schema/node.schema.yml
@@ -12,18 +12,12 @@ node.settings:
     type:
       type: string
       label: 'Machine-readable name'
-    uuid:
-      type: string
-      label: 'UUID'
-    name:
-      type: label
-      label: 'Name'

    Looks 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. 

  2. +++ b/core/modules/node/config/schema/node.schema.yml
@@ -42,12 +36,6 @@ node.type.*:
-    status:
-      type: boolean
-      label: 'Enabled status of the configuration entity'

    I really like we would get rid of ugly stuff like this one on node types. This label is silly....

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
2273631-config_entity-type-9.patch24.22 KB
2273631-diff-7-9.txt3.48 KB

Thanks 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.

The last submitted patch, 7: 2273631-config_entity-type-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2273631-config_entity-type-9.patch, failed testing.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
2273631-config_entity-type-12.patch26.5 KB
2273631-diff-9-12.txt2.29 KB

Fixing test fail...

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Assigned: vijaycs85 » Unassigned

It is green. Ready for review.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work

Yay, now looks good, except:

+++ b/core/config/schema/core.data_types.schema.yml
@@ -231,3 +231,26 @@ config_dependencies:
+  label: 'Shortcut settings'

Erm, this type name looks copy pasted :D Not appropriate for a generic type.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
2273631-config_entity-type-15.patch26.47 KB
2273631-diff-12-15.txt388 bytes

good catch, In fact, we don't need label there.

Status: Needs review » Needs work

The last submitted patch, 15: 2273631-config_entity-type-15.patch, failed testing.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
2273631-config_entity-type-17.patch26.26 KB

Rerolling...

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Reviewed & tested by the community

Looks good to me now, yay!

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented

I 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:

propertyname:
  type: ....
  required: true

See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21TypedData...

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented

Well, 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.

  • Commit 7a98471 on 8.x by alexpott: 
    Issue #2273631 by vijaycs85: Unify config entity schemas with a base...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Nice - way less duplicated schema!

Committed 7a98471 and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -sprint

Woot, superb, thanks.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.