Ensure page_manager config schema is valid

Can you provide an example config snippet that has this page_manager.block_plugin.* in it?

Very interesting, just removing this property removed fatal and made PageManagerAdminTest green locally. just noticed I got $strictConfigSchema = FALSE. Still investigating locally.

Without running the code here is my hyphotesis:

1. Mapping::getPropertyDefinitions() finds page_manager.block_plugin.[id], so it calls its buildDataDefinition which is delegated to TypedDataManager::buildDataDefinition().

2. In TypedDataManager::buildDataDefinition() if that definition has a dynamic component, like in this case page_manager.block_plugin.[id], it is resolved to the value, such as page_manager.block_plugin.system_menu_block:main.

3. Then still from TypedDataManager::buildDataDefinition() TypedDataManager::getDefinition() is called on that again to resolve to any wildcard matches if there was no direct match for this one (and there is no direct match for that one), so it resolves to page_manager.block_plugin.* I believe. We are still inside getDefinition() right after getFallbackName() has its chance.

4. Then this code kicks in:

    $definition = $definitions[$type];
    // Check whether this type is an extension of another one and compile it.
    if (isset($definition['type'])) {
      $merge = $this->getDefinition($definition['type'], $exception_on_invalid);
      $definition = NestedArray::mergeDeep($merge, $definition);
      // Unset type so we try the merge only once per type.
      unset($definition['type']);
      $this->definitions[$type] = $definition;
    }

So now we have $type as page_manager.block_plugin.* which itself has a type of block.settings.[id], so and this code throws that value back into getDefinition(). That only allows for wildcard fallback, no value replacement, so it goes with block.settings.* here.

There is not a chance that the id value is replaced again because once a value was replaced there is only getDefinition() called merging their results upwards. So this way block.settings.* goes one more level down to block_settings which goes to mapping which does not have a type anymore, so that is bubbled up, adding up the following as the definition:

- the definition of mapping
- the definition of block_settings
- the definition of block.settings.*
- the definition of page_manager.block_plugin.*

And that's it. The merge of these upwards ends up being the definition of page_manager.block_plugin.system_menu_block:main.

To resolve this I think we should fix this in core with tests and somehow let control back to buildDataDefinition() where we still have the data if the resolved type is still dynamic. Because getDefinition() is too late, that is not data dependent anymore. (And cannot be since this is how typed data defines the interfaces I believe). It seems like multi-level dynamic types are not really supported ATM. (Unless of course you put a concrete type inbetween like embed the further layer of dynamic types in a mapping which are the kind of things we encountered in core so far). But it does not seem like a stretch to be able to resolve this one in core.

Quoting a revelation I had this morning. Sorry I did not have it last night :/

[12/12/14 08:04:31] Gábor Hojtsy: so I had a bad feeling about your approach to extending block settings but could not point out why
[12/12/14 08:04:56] Gábor Hojtsy: how do you ensure that the array with arbitrary keys that you are extending does not contain the keys you want to add?
[12/12/14 08:07:37] Gábor Hojtsy: I think this is why I had this feeling right away that this use case is not supported
[12/12/14 08:08:20] Gábor Hojtsy: What others do like Views is they box the dynamic thing under a key, like in your case maybe "block"
[12/12/14 08:08:32] Gábor Hojtsy: And then add their settings alongside
[12/12/14 08:09:05] Gábor Hojtsy: So they box the unpredictable data structure

I mean how can we be sure that no block plugin that page_manager would work with will have settings keys like 'region', 'weight' or 'uuid'? (Or for that matter any other key that you may want to introduce on this level in your config later). Imagine a block that connects to a third party service that returns news for your region. It may have a region setting to set the machine name of the geographic area to take the news for. Same for weight, etc. I don't think its possible to ensure an arbitrary dynamic array does not contain the keys you want to add on to it?

If I were you I would use a structure like block.block.* where the settings are boxed well into a settings child-key (edited snippet from block.schema.yml):

block.block.*:
  mapping:
    region:
      type: string
      label: 'Region'
    weight:
      type: integer
      label: 'Weight'
    settings:
      type: block.settings.[%parent.plugin]

block.settings.*:
  type: block_settings

This allows you to add any keys that may exist in a random block's setting and also allows random blocks to add settings that may exist in plugin maanager. They don't clash.

So instead of

      e0258a7a-fbd1-4f67-bf00-cfad5041d106:
        id: 'views_block:who_s_online-who_s_online_block'
        label: ''
        provider: views
        label_display: visible
        cache:
          max_age: 0
          contexts: {  }
        views_label: ''
        items_per_page: none
        region: bottom
        uuid: e0258a7a-fbd1-4f67-bf00-cfad5041d106
        context_mapping: {  }

You would have:

      e0258a7a-fbd1-4f67-bf00-cfad5041d106:
        block:
          id: 'views_block:who_s_online-who_s_online_block'
          label: ''
          provider: views
          label_display: visible
          cache:
            max_age: 0
            contexts: {  }
          views_label: ''
          items_per_page: none
        region: bottom
        uuid: e0258a7a-fbd1-4f67-bf00-cfad5041d106
        context_mapping: {  }

And the schema would be (just relevant snippets):

display_variant.plugin.block_display:
  type: display_variant.plugin
  mapping:
    blocks:
      type: sequence
      label: 'Blocks'
      sequence:
        - type: page_manager.block_plugin

page_manager.block_plugin:
  type: mapping
  mapping:
    region:
      type: string
      label: 'Region'
    weight:
      type: integer
      label: 'Weight'
    uuid:
      type: string
      label: 'UUID'
    context_mapping:
      type: sequence
      label: 'Context assignments'
      sequence:
        - type: string
    block:
      type: block.settings.[id]

I have been working in this issue, probably a lot must be changed.
I am providing an initial patch where I applied recommendations #12 and #13 with more necessary fields.

Functions are overridden because of the change in the schema (this is tested and works well in PageConfigSchemTest). For the next step some structures in the form have to change to achieve what we want to do here.

Maybe is not the best way but a feedback is needed to continue this work.

I think #2392057: Config schema fails to expand dynamic top-level types will make the tests green here, it worked for an installation profile I was working on.

However, I ran into an additional schema problem to do with views_block, this seems to be an issue with how derivatives match schema in TypedConfigManager::getFallbackName().

EDIT: I think the root of the issue is how we tie the custom settings from views_block into the custom settings for page manager blocks?

page_manager.block_plugin.views_block:*:
  type: views_block
  mapping:
    region:
      type: string
      label: 'Region'
    weight:
      type: integer
      label: 'Weight'
    uuid:
      type: string
      label: 'UUID'
    context_mapping:
      type: sequence
      label: 'Context assignments'
      sequence:
        - type: string

Just leaving the patch here now for reference, will investigate core a little further.

I have the same problem for system menu blocks.

@tim.plunkett: So long as the value of [whatever] appears in the data, you can use dynamic config schema typing to refer to that piece of data and automate it.

I actually think we should break out the config schema derivative issues into a new issue and add a dedicated test for that. Then keep this issue for the config expansion here: #2392057: Config schema fails to expand dynamic top-level types ?

@Gabor, I briefly looked at this and the problem was that [whatever] looked like "type.some_thing:big_long_id" and it needs to get the definition of "type.some_thing:*" but it was actually trying to get the definition of simply "type.*"

Not so fast :)

Great that is in. Variants didn't really change anything though, we just moved it.

We still have strict config schema check disabled in the tests. Lets enable that to see if it really works.. there might be other problems too that were hidden so far.

Oh, sorry. All fine then :)

Yay, finally :) I am glad config schema stood the test of this complex case (with a bugfix :). Thanks for your contribution! :)

Page manager definitely needs more test coverage. See #2663410: TypedConfigManager::_getDefinitionWithReplacements() incorrectly replaces generic definition, the follow-up that I created.

Apparently our tests only create blocks of a single type at the moment. We need a test with at least two blocks of different types.

Not sure if we want to do this here or in a new issue.