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
Comment | File | Size | Author |
---|---|---|---|
#18 | block-schema-2274175-18.patch | 7.98 KB | tim.plunkett |
#16 | block-2274175-16.patch | 8.13 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettComment #2
tim.plunkettOkay, here it is with test coverage, and fixing the modules with missing block schemas.
Comment #3
Gábor HojtsyVery good find! Looks good except:
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.
Comment #4
vijaycs85Agree 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.
Comment #6
vijaycs854: 2274175-block_plugin_settings-4.patch queued for re-testing.
Comment #7
tim.plunkettWorks for me.
Comment #8
vijaycs85Thanks for the quick review and RTBC @tim.plunkett. Adding take to make sure it is in D8MI focus board.
Comment #9
Gábor HojtsyWait 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.
What do I not understand right?
Comment #10
vijaycs85Thanks 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:
Comment #11
tim.plunkettWe don't have this anywhere *yet*, except in Views.
Because we have barely any plugin schemas at all, except in Views.
Comment #12
Gábor HojtsyLet 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.
Comment #13
tim.plunkettI 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:
And that would be it.
So I like the wildcard definition as a safety net for those.
Comment #14
Gábor HojtsyI 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.
Comment #15
Gábor HojtsyAdd D8MI topic tag.
Comment #16
tim.plunkettPSR-4 reroll. No changes.
Comment #18
tim.plunkett#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.
Comment #19
alexpottCommitted 0ad74d1 and pushed to 8.x. Thanks!
Comment #20
vijaycs85Thanks @alexpott.
Comment #22
Wim LeersFollow-up to correct the config schema changes that landed here: #3379725-5: Make Block config entities fully validatable.