Problem/Motivation

The block config entity schema contains all of the properties of itself but also the block plugin schema itself.
However, any other module referencing a block plugin in a config entity has to redeclare the schema

Proposed resolution

Define the block plugin schema as its own type, and reference it from the block entity

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
2.1 KB
tim.plunkett’s picture

FileSize
7.76 KB

Okay, here it is with test coverage, and fixing the modules with missing block schemas.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Configuration schema

Very good find! Looks good except:

+++ b/core/modules/block/config/schema/block.schema.yml
@@ -63,43 +63,49 @@ block.block.*:
+block.plugin.*:
+  type: mapping

+++ b/core/modules/book/config/schema/book.schema.yml
@@ -24,3 +24,11 @@ book.settings:
+block.plugin.book_navigation:
+  type: block.plugin.*

Defining the base type like this does not allow to detect when a plugin is missing their definition, since it would automatically fall back on the base type. So no automated detection is possible then that all these items you added were missing.

We usually define dot-less top level types eg. in this case block_plugin to inherit from, which does not apply automatically then to all future plugins.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
8.17 KB
5.53 KB

Agree with @Gábor Hojtsy at #3. What do we feel about this patch which has:

- new data type block_settings in core.data_types.
- All plugin settings extends block_settings with prefix block.settings.
- Added block.settings.* with block_settings type for new/undefined plugin types.

Status: Needs review » Needs work

The last submitted patch, 4: 2274175-block_plugin_settings-4.patch, failed testing.

vijaycs85’s picture

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

Works for me.

vijaycs85’s picture

Issue tags: +D8MI, +sprint

Thanks for the quick review and RTBC @tim.plunkett. Adding take to make sure it is in D8MI focus board.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Wait a minute, I questioned this part above and it was not resolved at all. There is now an additional layer of type definition but it does not resolve the core of my argument.

1. From what I can see here the 80% use case is NOT that the default block settings would be enough, so defaulting to that is actually a disservice to developers and would make it appear they defined their schema (incorrectly) while they did not.
2. I also don't think we do this defaulting for arbitrary plugin ids (ie. that you would have a schema defined even if you did not define one). I grepped for ".*:" in core and did not find one that stood out.

+++ b/core/modules/block/config/schema/block.schema.yml
@@ -71,41 +71,5 @@ block.block.*:
+block.settings.*:
+  type: block_settings

What do I not understand right?

vijaycs85’s picture

Thanks for the review @Gábor Hojtsy. Here is an update on your questions

#9.1 - good point, if it is not going to help or not common, let's not add block.settings.*.

#9.2 - Well, we have used few places in views:

 core/modules/views/config/schema  (10 usages found)
                views.area.schema.yml  (1 usage found)
                    (3: 11) views.area.*:
                views.argument.schema.yml  (1 usage found)
                    (3: 15) views.argument.*:
                views.field.schema.yml  (1 usage found)
                    (3: 12) views.field.*:
                views.filter.schema.yml  (2 usages found)
                    (3: 13) views.filter.*:
                    (124: 25) views.filter.group_items.*:
                views.pager.schema.yml  (1 usage found)
                    (3: 12) views.pager.*:
                views.relationship.schema.yml  (1 usage found)
                    (3: 19) views.relationship.*:
                views.schema.yml  (1 usage found)
                    (71: 11) views.view.*:
                views.sort.schema.yml  (1 usage found)
                    (3: 11) views.sort.*:
                views.style.schema.yml  (1 usage found)
                    (3: 12) views.style.*:
tim.plunkett’s picture

We don't have this anywhere *yet*, except in Views.
Because we have barely any plugin schemas at all, except in Views.

Gábor Hojtsy’s picture

Let me repeat the other question/statement then. Do we expect it to be the 80% use case for contrib plugins to not need to implement this schema because they add no settings whatsoever? If yeah then this default seems to be fine. If not, then why make misleading defaults? I'd rather not set defaults that will not apply for the majority use case or we risk people/tools assume there is a schema but it would not fit.

tim.plunkett’s picture

I can't be sure what contrib will need here, but every block that extends BlockBase (which is all of them) and doesn't override the configuration methods (which is many of them) will *only* need this:

block.plugin.mymodule_plugin_id:
  type: block_settings

And that would be it.

So I like the wildcard definition as a safety net for those.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

I think this will make debugging schemas for those harder, but I'll not be the one wasting my time on that debugging, so if you believe this is better, then let's be it.

Gábor Hojtsy’s picture

Priority: Normal » Major
Issue tags: +language-config

Add D8MI topic tag.

tim.plunkett’s picture

FileSize
8.13 KB

PSR-4 reroll. No changes.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: block-2274175-16.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.98 KB

#2273631: Unify config entity schemas with a base schema type and this issue both tried to change the end of some files, just a reroll.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0ad74d1 and pushed to 8.x. Thanks!

vijaycs85’s picture

Issue tags: -sprint

Thanks @alexpott.

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Follow-up to correct the config schema changes that landed here: #3379725-5: Make Block config entities fully validatable.