This is a sub-issue of #1910624: [META] Introduce and complete configuration schemas in all of core.

Problem/motivation

#1866610: Introduce Kwalify-inspired schema format for configuration introduced the idea of config schema. The changelog leads to (hopefully extensive) documentation on the format at http://drupal.org/node/1905070. While there are little cleanups planned for the format overall, the current format is a result of months of back and forths, so it should be perfectly fine to apply it more widely to core.

Proposed solution

Create a configuration schema for block and custom block modules. Block configuration entities should be covered as well as custom block type entities.

Schema in place

None.

Schema not yet in place

Custom block module has custom block types (config entity at core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlockType.php), example config .yml at core/modules/block/custom_block/config/custom_block.type.basic.yml

Block module itself has the block config entity (config entity at core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php, config yml at core/profiles/standard/config/block.block.bartik.login.yml and many others there).

Issues for concerns noticed while working on this issue

These are kind of follow-ups, but not directly related to schema stuff.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
3.23 KB

Updating patch...

Config inspector:
Only local images are allowed.

Only local images are allowed.

Gábor Hojtsy’s picture

- Why did you add the block type with system module? Not clear to me.
- Also, why did you make visibility its own type?
- It is strange that label is missing on the screenshot from the block editing.
- My hunch is that the visibility__active_tab is more like a formapi artifact and not really intended to be saved in config? (Not sure).

Status: Needs review » Needs work

The last submitted patch, 1948884-block-config-schema-1.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

- Why did you add the block type with system module? Not clear to me.

thought is it common site wide and we got filter, mail related data types there already

- Also, why did you make visibility its own type?

thought of re-use it somewhere? and keep schema bit clear?

- It is strange that label is missing on the screenshot from the block editing.

Seems we don't have label in config_inspector's config_inspector_build_form(). Adding label case solving issue :)

      switch ($type) {
        case 'boolean':
          $type = 'checkbox';
          break;
        case 'string':
        case 'path':
          $type = 'textfield';
          break;
        case 'text':
          $type = 'textarea';
          break;
        case 'integer':
          $type = 'number';
          break;
      }
- My hunch is that the visibility__active_tab is more like a formapi artifact and not really intended to be saved in config? (Not sure).

it is part of config. Should we go for a new issue?

Gábor Hojtsy’s picture

Status: Needs review » Needs work

We include things with system module which are defined by system module. Blocks are an optional module. If there are other things in system.schema.yml that are not defined by system module, that would be an issue. I also don't think we want to reuse block visibility as its own type, no need to crowd the top level namespace IMHO.

Fixed the label problem in http://drupalcode.org/project/config_inspector.git/commitdiff/703327efee... :)

On visibility__active_tab, I'm fine with making it defined in the schema, but we ned to see if this is supposed to be saved. Looks like a formapi artifact to me.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
30.47 KB
2.79 KB

Moved block to block.schema.yml and merged visibility.

block-schema-2.png

Status: Needs review » Needs work
Issue tags: -Configuration system, -D8MI, -language-config, -Configuration schema

The last submitted patch, 1948884-block-config-schema-6.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +D8MI, +language-config, +Configuration schema

#6: 1948884-block-config-schema-6.patch queued for re-testing.

vijaycs85’s picture

Updating label...

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/block/config/schema/block.schema.ymlundefined
@@ -0,0 +1,83 @@
+
+block.block.*.*:
+  type: block
+  label: 'Block settings'
+
+
+# Block settings.

I think we should remove this indirection. 'Block settings' is the type of thing we use for regular settings yml files also, so does not sound correct.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.72 KB

Updated...

Status: Needs review » Needs work

The last submitted patch, 1948884-block-config-schema-11.patch, failed testing.

YesCT’s picture

+++ b/core/modules/block/config/schema/block.schema.ymlundefined
@@ -0,0 +1,77 @@
+              sequence:
+               - type: string
+                 label: 'Node type'

this indentation looks off by one. [edited: the code snippet to show the indentation better] I know indentation is important, but I dont know exactly what the repercussions are.

I'll make a patch for that later.

while testing (I wondered why the admin_label was not a type: Label. So I made a custom block. It seemed like a block should have a label, and that we would want that to be translatable.

I noted several strange things. If these are not issues already, I'll open them.

The title is not used in the block list UI.
The description is used.
The title of a (non custom) block is used in the UI: on the block shown to the users.
I can set the title of a custom block, but then I never see it again.

I think that custom blocks should have a setting: show title, so then when shown to the users the title and body might be there.

I think that the titles should be used in the block ui listing (with the descriptions in smaller type).
I think when a block is edited, the title and body should be editable
I think when a block is configured, those should be the "settings" which would be the admin description and other stuff. (not the title)

1.
description.png

2.
no_edit_description.png

3.
edit_and_configure.png

4.
edit-description-body.png

5.
search_title.png

6.
search_in_block_ui.png

----
I'll take another look and actually try the patch in about an hour.

YesCT’s picture

1. uuid is missing from the schema

2.
label:
type: label
label: 'Title'
is missing from the (active) config yml but *is* in the UI form

3.
admin_label:
type: string
label: 'Admin title'
is missing from the (active) config yml
this could be ok for the Search block. admin_label might be for custom blocks

3a. coming back to think about this again, the admin_label is not just for custom blocks because it's in the block schema.
If it's just for custom blocks, we should move it to the custom block schema.
If it's for default blocks also though, then it should be showing in the active config and also in the config inspector

4.
langcode is out of order (similar to the views issue where langcode was added)

5.
I wonder why the order in the configure form for Search has Content Types and Roles out of order compared to both the schema and the active config

6.
Search_overview.png

7.
I dont think this is a schema problem, and should have a new issue:
In the active config:
Why for roles, when none are checked, is there a {}
But for content types, each is listed as
whatever: '0'
or
whatever: whatever
?

7a, so I think roles and content types should use the same pattern
7b. I think content types has a similar trouble as we saw with book: book

See #1928082: Make usage of book.settings:allowed_types consistent
and #1933548: Book allowed_types settings repetitive and in under certain conditions can change unexpectedly

8.
with no roles the config inspector does not list anything
roles_no_roles.png

9.
but with roles... it still does not list any roles
role_with_roles.png

10.
roles.png

---
ok. now for a custom block

related to #13 (in that it's definitely not this issue)
11. there is no revision tab on a block, even when there are two revisions.
12. there is no edit action in the dropbutton from the block ui for custom blocks

13. missing from the config inspector
label:
type: label
label: 'Title'

but that's probably because this is a custom block and they do not have label Titles. nope. admin_title is the description!
So, Title is missing from the config inspector.

14. langcode is saved as und
what should it be saved as?
there is no way to select a language when creating the block.
the d8mi matrix from sprint weekend lists language select for custom block as not needed because it does not make sense.
I think we should reconsider that.

15. I think admin_title is actually Block description on the Block edit page, block/1/edit (and the block admin ui listing)
So
admin_label:
type: string
label: 'Admin title'
should be:
admin_label:
type: string
label: 'Block description'

16. also, why did we start calling it admin_label anyway?
maybe because it is used for the block admin ui listing.
then I think the search block should have had that data too.

block.module says
admin/structure/block
is built with block_admin_display
I think block.admin.inc

    $variables['block_listing'][$region][$i]->block_title = drupal_render($block['info']);

says to get the info part to use as the title/label/description on the block list admin page.
which matches what I'm seeing for the custom block

17. so why was there no info in the active config for the Search block?

18. missing from the config inspector and the schema are:
under settings:

  status: '1'
  info: 'a test block so I can figure out config'
  view_mode: full

19. and what is up with all the different orderings?
schema files are written my hand. ok. so we can make any order we want, but the active config order seems like it might be alphabetical, for example, under settings: admin_label, cache (where as in the schema, it's cache, admin_label)
but then also under settings: is status, followed by: info. and that's not alphabetical.
So, I think we are changing schema order to match the active config save?
there is a different issue to make the default config yml (which we do not have for blocks) to match the order and format of the active config yml: #1938580: [META] Make active config save format match the default yml file (order and quotes)

20.
custom_block_schema.png

--------

I know this is disorganized and not limited to this issue. So I'm going to save it, and then come back and sort out what should be done in this issue, and open other issue for the other things.

YesCT’s picture

I have on old version of the config inspector.
Getting that now.

Gábor Hojtsy’s picture

YesCT: on #13, most of your review points relate to how blocks and custom blocks relate. So think of blocks as a placement container for *something*. The placement container has a title, a region, visibility, etc. It does not define what is actually contained. So you can place the search block, login block, views blocks, etc. with a block. The contained thing is either module provided (eg. search, user login, etc.) or edited elsewhere. In case of a views block, you'd edit the contents on the views UI. Now take the same to custom blocks. Custom block is one possible contained data. So the placement container holds the title for the block, but the content holds the body. So when you go edit the custom block (Edit contextual link), you get to edit the body of the block. When you go to configure the block, you get to edit the placement container, including the display title. The custom block also has a description which serves the same role as the admin labels provided by modules for built-in blocks (search, user login, etc.). So even though you can edit the display title, the admin label is displayed on the blocks backend. These are particularities of how the blocks system works now and it makes editing harder sometimes vs. Drupal 7 and translation not less so. That is how it works though, its not really a bug as much as usability problem.

Re #14:

- I had the same feeling that admin_title would only be for custom blocks? Others have this coming from code and might not be saved back to config. I'm not sure if it is not saved back or is, so needs to be checked.

- As for langcode, etc. not saving in the proper order, that would be a separate issue for this module I think. The shipped config is not in the right order. Much like for views. Same for the content types and roles inconsistency :)

- The missing label in config inspector was an outdated config inspector, it was just fixes yesterday :)

- Langcode saved as und is at #1947814: New config entities often save as langcode: und incorrectly :)

- The custom block itself is already a content entity, and as such should have a language selector (and bundle level config on the block type for language setup, as well as show up on the ET UI). If that is not already supported, that is a missing feature, should be a separate issue (and put on the matrix :) Block config entities I'm not sure should have a language selector. You already picked a language for your custom block, then you could pick for your block config entity and then you also have language based visibility. That sounds like a bit too much flexibility and would make it hard to understand how these relate/interoperate :D

- As for odering the keys in the schema the same as in the data, that is not needed, since its a mapping AKA associative array, so the key names define the relations, not the order of them. It should work either way.

alexpott’s picture

The issue with the failing tests in Drupal\system\Tests\Module\EnableDisableTest is that we're expecting config if the module has an empty directory due to the ModuleTestBase::assertModuleConfig() function... adding the following condition resolves this.

diff --git a/core/modules/system/lib/Drupal/system/Tests/Module/ModuleTestBase.php b/core/modules/system/lib/Drupal/system/Tes
index 7804b0a..aed76b3 100644
--- a/core/modules/system/lib/Drupal/system/Tests/Module/ModuleTestBase.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleTestBase.php
@@ -104,6 +104,11 @@ function assertModuleConfig($module) {
     // Verify that the module's default config directory is not empty and
     // contains default configuration files (instead of something else).
     $all_names = $module_file_storage->listAll();
+    if (empty($all_names)) {
+      // Module has an empty config directory. For example it might contain a
+      // schema directory.
+      return;
+    }
     $this->assertTrue($all_names);

     // Look up each default configuration object name in the active
vijaycs85’s picture

Status: Needs work » Postponed
vijaycs85’s picture

Status: Postponed » Needs review
FileSize
863 bytes
3.56 KB

Adding test failure fix suggested by @alexpott in #17

Gábor Hojtsy’s picture

Still needs to resolve the feedback from @YesCT and myself from above on the actual schema.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.3 KB
4.08 KB

Addressing issues mentioned in #14

#14.1 - Fixed.
#14.2 - Available for custom block
#14.3 - Available for custom block
#14.4 - Fixed
#14.5 - Need to check
#14.6 - Fixed
#14.7 - New issued and related to #1928082: Make usage of book.settings:allowed_types consistent as mentioned in #14.7b
#14.8 - Fixed
#14.9 - Fixed
#14.10 - related to #14.7
#14.11 - Revision is in custom block type definition. e.g. custom_block.type.basic.yml and the URL for UI is /admin/structure/custom-blocks/manage/basic
#14.12 - Not an issue, as per #14.11
#14.13 - Not an issue, as per #14.11
#14.14 - Fixed as part of #1947814: New config entities often save as langcode: und incorrectly
#14.15 - Fixed
#14.16 - Not an issue, as per #14.11
#14.17 - Not an issue
#14.18 - Fixed
#14.19 - Separate issue
#14.20 - Not an issue, Part of them from of custom_block type.

YesCT’s picture

I agree that is what we went through in irc.

#14.13 was fixed, (not related to #14.11 though) :) It was a fix to config_inspector
#14.16 is not a problem here, but I think will be a new issue. vijay fixed it though, with a better label noting that it is the description used on the block admin list page. There is still some thing related to info and that will need a new issue.

YesCT’s picture

+++ b/core/modules/block/config/schema/block.schema.ymlundefined
@@ -44,7 +47,7 @@ block.block.*.*:
-              type: seqeunce
+              type: sequence

oh! :)

+++ b/core/modules/block/config/schema/block.schema.ymlundefined
@@ -69,9 +72,21 @@ block.block.*.*:
+        admin_label:
+          type: label
+          label: 'Description'

yes. this is type: label now since this value shows in the ui in the block admin list page. So it needs to be translatable.

--
note: core/modules/block/custom_block/config/schema/custom_block.schama.yml
is about the block types
admin/structure/custom-blocks
editing a type there,
or adding a custom type
admin/structure/custom-blocks/add

YesCT’s picture

Status: Needs review » Needs work

ok, reviewing:
git stash
git checkout 8.x
sudo rm -r sites; git checkout sites; git status;
git pull --rebase
drush am 1948884
git status
git add core*
git commit -m "22"
drush -y si --account-pass=admin --db-url="mysql://root:root@localhost/d8-patch" --site-name=patch

extend -> enable config_inspector

open core/modules/block/config/schema/block.schema.yml

those are the files reviewing here.
the next files are useful for comparing to make sure that schema matches config.

open sites/default/files/config_A49bj39gKJKohrjlzgIXsPg8g9keHOg6DYR82nxrEJE/active/block.block.bartik.search.yml
active config for the search block that come enabled by default with core

add a block: administration, then open it's config
admin/structure/block/list/block_plugin_ui%3Abartik/add
click configure block
admin/structure/block/add/system_menu_block%3Amenu-admin/bartik
added show only for some of the roles (so that we can see if the config inspector sees that)
this form can be compared to the file
core/modules/block/config/schema/block.schema.yml
and the active config:
sites/default/files/config_A49bj39gKJKohrjlzgIXsPg8g9keHOg6DYR82nxrEJE/active/block.block.bartik.administration.yml

add a custom block (not a custom block type, not yet)
block/add
there is a bit of config there, not just content
config there is Block description
I filled out the form with: my description, my body. then save
Next form filled out with: my title, my_title_machine_name, left the rest defaults. save. configure block.
this form can be compared to the file
core/modules/block/config/schema/block.schema.yml
and the active config:
sites/default/files/config_A49bj39gKJKohrjlzgIXsPg8g9keHOg6DYR82nxrEJE/active/block.block.bartik.my_title_machine_name.yml

I moved the blocks to the header.

I looked at each relative to it's form in the config_inspector report, the form in the ui (configure the block), the active config, and the schema.
Looks really good.

Now the custom block types.
open core/modules/block/custom_block/config/schema/custom_block.schama.yml

open core/modules/block/custom_block/config/custom_block.type.basic.yml
default config for the basic block type that comes with core

edit the basic block type at admin/structure/custom-blocks
admin/structure/custom-blocks/manage/basic/edit
check the box to 'Create new revision'
open sites/default/files/config_A49bj39gKJKohrjlzgIXsPg8g9keHOg6DYR82nxrEJE/active/custom_block.type.basic.yml

similar to:

    label_display:
      type: string
      label: 'Display title'

which is a check box 'Display title'
here we should use the label: 'Create new revision'
comparing also to the config inspector form form custom_block.type.basic

patch coming for that.
Also changed the label to custom block *type* settings
since this is for block types, not custom blocks. That also matches what is in the form: admin/structure/custom-blocks/add

YesCT’s picture

Status: Needs work » Needs review
FileSize
730 bytes
4.09 KB

ok. this is all good from my perspective.
If vijay or someone else ok's it and it's green, then we should be rtbc.

I'll open up follow-ups for the blocks concerns. (I'll check for existing issues first.)

YesCT’s picture

from #13
this might cover my confusion about not being able to change the title when editing the block:
#1875058-5: Provide separate interface for editing custom blocks

about not seeing the title of a custom block... #1875260-91: Make the block title required and allow it to be hidden

about blocks in general:
#1535868: Convert all blocks into plugins
and
http://drupal.org/project/issues/search/drupal?status%5B%5D=Open&issue_t...

I'm still looking for existing issues/opening follow-ups, so more info to come.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looked through the schema again. Cross-checked that custom block types have a description as textfield indeed. I think this looks good to go.

Gábor Hojtsy’s picture

Issue tags: +Quick fix

Mark as quick fix.

Gábor Hojtsy’s picture

(Note that the included small test fix is for #1954376: Configuration schema without any shipped configuration fails tests, which this patch provides a fail case for, so it was simplest to include here).

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow, ARROWPOCALYPSE. :D

Thank you for your thorough testing!

Committed and pushed to 8.x. Thanks!

swentel’s picture

So the filename is custom_block.schama.yml - shouldn't that be schema.yml ?

swentel’s picture

Status: Fixed » Needs review
FileSize
897 bytes

Quick patch - will probably not fail on head, feel free to set back to fixed in case this is valid of course.

swentel’s picture

FileSize
1.76 KB

Bah wrong patch

swentel’s picture

FileSize
339 bytes

Even better patch, thanks to YesCT

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

good catch. and nice patch.

thanks @Cottser (and @tstoeckler) for the tip in irc: `git diff -C -M`

vijaycs85’s picture

my bad.. sorry for that file name and thanks for the fix. Good to know that we are checking just folder config/schema no name validation the file name under it i.e. *.schema.yml

SchemaStorage.php
    return drupal_get_path($type, $name) . '/config/schema';
YesCT’s picture

#1875252-60: [META] Make the block plugin UI shippable puts my general blocks confusion in a better place, a blocks place.

xjm’s picture

Haha, schama.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oh dear. :) That was bad of me not to catch. Thanks!

Committed and pushed to 8.x.

YesCT’s picture

we broke simplytest.me with this. See: #1958680: Drupal 8 does not install with safe_mode = On

I wonder if there were other directories with empty config that this effected.

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

Anonymous’s picture

Issue summary: View changes

added a section for issues opened while noticing stuff working on this issue