Provide a way to add theme-specific config

I'd see a config action for this, but how do we deal with region names? Even if there are some unwritten conventions, you cannot ensure the default theme will have the region you expect.

Good point. Maybe it could accept either a string or an array for the region. If it's an array, it will iterate through and place the block into the first matching region it finds. If a match can't be found, it should skip the config creation.

A place block config action seems a great place to start.

Yes I was going to say that it could take region as an argument...

In parallel, we're lacking a "place a block in [region-list]" action in ECA, but I'm not sure if they could share any implementation details?

Updated the proposed resolution based on some investigation into the existing config actions.

There is a first draft of what the code might look like in MR !142, though it needs a little work before it will be ready for review, including tests.

I just realized that the code so far doesn't provide any flexibility around the region name, which could be different between themes. Maybe there could be an optional key in $value like "region_backup", that could be a string or array. If the main region (e.g. $value['region']) doesn't exist for the specified theme, then it would look for a value in $value['region_backup']. If that exists, it can normalize to an array, then iterate through to find a matching region in the theme. If none are found, it could throw an exception.

A first pass at this is done and the feedback should be resolved. I need to add some tests and then it should be good to review.

This really needs tests so we can validate any changes made to the config action.

Agreed about the tests. My plan is to add to the existing ConfigActionTest class.

Tests have been added, and pass.

Can you define what is possible in the action? The comments that @alexpott added above were great, but I want to make sure those things can be done or not.

block.block.admin_help
  placeBlock:
    theme: admin
    # A list of regions in order of preference,
    regions: 
      - content_top
      - content
    # You could allow words like first / last here.
    weight: -10
    plugin: help_block
    # You could allow this to be omitted to use the defaults
    settings:
      id: help_block
      label: Help
      provider: help
      label_display: false
    # You could allow this to be omitted to use the defaults
    visibility: {  }

Also, brilliant allowing selection of either the default or admin themes (can it be both?). That way you don't need to know the theme name.

block.block.admin_help
  placeBlock:
    theme: admin
    # A list of regions in order of preference,
    regions: 
      - content_top
      - content

If a region isn't specified but a regions array is, it will go through them in order and use the first one available in the theme.

    # You could allow words like first / last here.
    weight: -10

This has not yet been implemented. I would prefer to to address this in a child issue.

    plugin: help_block
    # You could allow this to be omitted to use the defaults
    settings:
      id: help_block
      label: Help
      provider: help
      label_display: false

The settings available / needed vary based on the type of block, so it isn't easy to provide defaults. If the id is omitted in the settings, it will populate it from the plugin value of the main array.

    # You could allow this to be omitted to use the defaults
    visibility: {  }

An empty array means that the block will be shown everywhere. If the key is omitted then an empty array will be set. This matches the default behavior when adding a block through the UI.

I have added the code to replace 'first' and 'last' keywords in the weight value with appropriate numbers.

This is looking great now that there is some test coverage. On this project we have code coverage reporting and it is showing that the test is missing coverage of the regions logic and the weights logic - see the red lines next to https://git.drupalcode.org/project/distributions_recipes/-/merge_request...

I added tests for the regions and weights logic, and also added a test for using the 'default' theme keyword. The MR now indicates that it will increase the test coverage percentage. :)

Going forward, the process for validating these config actions will need to be sorted out. It's not clear (to me at least) how strict they should be, or if a recipe should be applied when the config action refers to things that don't (yet?) exist. But this issue is probably not the place to have that conversation.

This plugin works to do something like this (with the minimal profile installed):

name: 'Place a block'
config:
actions:
block.block.whatever:
place_block:
id: minimal_test
theme: default
region: sidebar_second
weight: -10
plugin: system_powered_by_block
visibility: { }

The tests cover other situations, and the PlaceBlock class does a lot of checking already.

This looks great to me. Nice work, @madclu!

Thanks @mtift! I have also created #3455955: Update documentation to include new place_block config action → to make sure the relevant documentation gets updated once this issue is merged. I would be happy to take that on, and welcome any thoughts on other documentation that should be updated.

@mandclu. For a better developer experience, we've decided to unify the config actions to use camelCase. Could you update this action to placeBlock?

For reference, see:
https://www.drupal.org/project/drupal/issues/3455113 ✨ Rename ensure_exists to createIfNotExists, and camel-case simpleConfigUpdate for consistency RTBC

Since this is also a single issue, I think this should go against core. I am going to update this issue, but can you update the MR to be against core?

Here is a new MR against core, with placeBlock as camel case.

Looks even better in camelCase!

mandclu → changed the visibility of the branch 3448131-provide-a-way to hidden.

Yes! Thanks so much for all your hard work @mandclu, and @mtift for reviewing!

I think we still need to error if the theme and plugin ID are different. And maybe need options about what do to wrt to visibility & settings.

I've added checks (and test coverage) to catch if the theme or plugin is different.

It's an interesting question about how to handle visibility differences. My first reaction is to say that the ideal approach would be to use the union of the existing block and any incoming conditions. That could get very complex, however, since different kinds of conditions would need different code to actually accomplish this. Also, what if the two conditions are actually the opposite of each other? Does that mean it should show always, or never? Or throw an error instead? I suspect that anything even remotely nuanced here could quickly become very complex. I'm inclined to say that if we do anything related to visibility, we should check if the key exists within $value, and if so set the visibility of the existing block to match. In other words override the existing block's visibility with whatever the recipe author intended.

The settings is a little different, I would argue, since the Block class doesn't actually provide an explicit getter or setter for settings. Sure, we could use the generic get and set methods, but even then, there is so much variability between kinds of blocks, I'm not sure we can be sophisticated in comparing different settings arrays. The best solution I can think of would be iterating through the settings values passed into the config action, and throwing an error if the existing setting doesn't match.

I'd appreciate some direction on how to handle the visibility and settings, and happy to move this forward again once the preferred approaches are clear.

The MR now also contains code to set the visibility and any settings passed to the config action, if there is an existing block. There's test coverage for these additions too.

@mandclu nice work on adding the functionality to kinda merge the visibility rules and settings. It is really hard to work out the best approach here. I think ideally recipes would only use these when 100% necessary.

It would be great if the test was less monolithic but split out into separate methods testing different bits of logic within the action. Also I think the action class could potentially be a bit less one function with many ifs. And many a new vs existing block could have separate protected methods.

Thanks for sticking with this one. It's going to be really good when we get it right.

Setting back to needs work for #37

I refactored the PlaceBlock class a little to split out updating an existing block, as well as setting the weight based on keyword. There didn't seem to be much else that warranted being moved out to a helper method.

For the tests, I tried to split them out, but most of them require at least one theme to be installed, so I kept them in a single function, to avoid the performance hit of installing a theme over and over.

Hopefully that addresses the most recent feedback, but please let me know if there's more that needs to be done.

I don't think this is ready to go, sadly. It's doing a lot more work than it should be, which makes it brittle. I'd like to take a stab at a more simplified version of this that basically is a nicer wrapper around createIfNotExists.

The thing I'm unclear on is: what exactly are we trying to accomplish with this, as opposed to just using createIfNotExists?

The thing I'm unclear on is: what exactly are we trying to accomplish with this, as opposed to just using createIfNotExists?

In a word, flexibility. The createIfNotExists config action solves for the problem of being able to reapply recipes, but is still brittle in the sense of only being able to work as intended (specifically for the use case of placing blocks) based on the recipe author's knowledge of what theme(s) are installed which exact regions are available, and the weights of blocks placed into those regions. One of the stated goals for recipes for them to be able to be applied to sites well after the initial install of the site, and for placing placing blocks a recipe author simply can't know all of these details.

I speak from my own experience maintaining recipe-like modules (and now recipes) like Tasks → , Quick Links → , Alerts → , and Event Platform → where I had to either place a block in a submodule that would only work with Olivero, or put the block placement into optional config that would leave the block not placed at all if the Olivero theme isn't installed.

The goal of this config action is to make recipes that will still do something useful a lot of the time. Will they always work as intended? Of course not. Particularly when we anticipate that a recipe could be applied to a site that was created weeks, months, or even years ago, there will always be edge cases that we can't foresee. But the goal of this config action to add flexibility for the specific use case of placing blocks, where we know there are specific kinds of intent a recipe author may need to present in order to provide useful output for the site builder.

That rationale makes sense to me, and I agree about the somewhat limited nature of createIfNotExists. But, by that very same token, I think it also means we need to radically rethink the approach here, and make block placement more flexible in an incremental way, over multiple issues.

As an example, I see that we want to be able to place a block in the default theme or the admin theme, regardless of which themes those actually are. I think that small scope of work should be tackled on its own (and, in fact, it could already be done with the mechanism I've implemented in ✨ Make it possible for recipes to prompt for input values Fixed , so we might want to pursue getting that issue done).

If we need more flexibility from there, then we should do that in further, separate, tightly scoped issues. Trying to do too much at once -- adding too much flexibility at once -- is a greased slide to ill-defined APIs and unpredictable, buggy behavior. Which are things we are strongly trying to avoid in the recipe system, and in the config actions API.

We can talk about it later, but I'd love to get a sense of what kind of flexibility you'd like to see when placing blocks. That is, specific aspects of block placement that should be more flexible, and in what way.

As an example, what if we implemented the action like this?

block.block.some_block_entity_id_that_must_already_exist_and_could_be_created_using_createIfNotExists:
  placeBlockInDefaultTheme:
    region: name_of_region_that_must_exist_in_the_default_theme
    at: end # can also be 'start', or an absolute integer position
    replace: false # if true, and 'at' is an absolute position, then the block already at that position in the region is replaced

There could just as easily be a placeBlockInAdminTheme action that behaves exactly the same way.

mandclu → changed the visibility of the branch 3448131-placeblock-block to hidden.

As a part of this config action, we would also need to support all the properties that are provided by core block like:

1. Pages
2. Response status
3. Roles
4. Content type
5. Vocabulary

In addition to this, there could be scenarios where we want to place the same block in multiple regions as well.

Here's a new MR that moves the new config action into the block module, and has a first pass at the new feedback. Happy to continuing working on this, but some of the feedback could use additional discussion.

As for the flexibility provided by the new config action, it currently includes:

• specify a theme, or use the 'default' or 'admin' keywords, the proper machine name will be determine, and other elements (such as the config name) will be update appropriately
• the ability to include a "regions" array. If this is included instead of a single "region" string, the action will iterate through the array and use the first region that can be found in the determined theme
• In addition to accepting a numerical weight, a recipe author can use the "first" or "last" keywords, and the action will determine the necessary number and use that

There are also some attempts to provide sensible defaults for omitted keys, but happy to remove those if that's deemed a better approach.

In general, I think it makes for a better developer experience if the placeBlock config action can be as close as possible to the output that would be created by placing a block through the admin UI and then exporting the result, but with the support for the keywords mentioned above.

specify a theme, or use the 'default' or 'admin' keywords, the proper machine name will be determined, and other elements (such as the config name) will be updated appropriately

I think this should be represented as two different config actions, rather than keywords. That would make it impossible for there to be collisions with themes named "default" or "admin" (however unlikely that may be), and it means that it's impossible to screw up, because if you get the name of the action wrong, you'll get an exception.

the ability to include a "regions" array. If this is included instead of a single "region" string, the action will iterate through the array and use the first region that can be found in the determined theme

I don't think this is the best approach, because it is not predictable. Which region will it be? That depends on the theme. I think we can still create flexibility here, but have it be more predictable, by doing something like this:

region_map:
  olivero: some_region
  gin: some_region
  claro: some_region
  ...

If the current default or admin theme isn't in that list, you get a ConfigActionException. I like this because it lets the recipe be broadly compatible with any number of themes, but also guarantees that the recipe will always behave the same way in various contexts. We want recipes to be very, very predictable. IMHO this design would strike the right balance between flexibility and predictability.

In addition to accepting a numerical weight, a recipe author can use the "first" or "last" keywords, and the action will determine the necessary number and use that

I agree with this, although I think we should use numerical positions, not weights. Weights are an internal thing that might not be known ahead of time. Numerical positions are, again, predictable: if I say put this block at position 2, it will be at position 2, regardless of what the actual weight is.

There are also some attempts to provide sensible defaults for omitted keys, but happy to remove those if that's deemed a better approach.

For now, to keep things simple, I would suggest removing the extra key support for now. There would be nothing preventing a developer from creating the block entity with whatever keys they want, then using one of the placeBlock variants to adjust it.

I think this should be represented as two different config actions (placeBlockInAdminTheme and placeBlockInDefaultTheme), rather than keywords.

It's worth pointing out that with the current approach you could still target a specific theme, but take advantage of the flexibility elsewhere, for example in setting the weight.

region_map:
olivero: some_region
gin: some_region
claro: some_region
...

The current approach is predictable, in the sense that the action will iterate through the array and use the first match. So a recipe author can order the array to have the most ideal but potentially unavailable regions first, with the most likely to be available but less ideal regions (for example, content) at the end of the array. Having to use an array keyed by the theme name makes the recipe unable to place a block in an unknown theme, which is one of the goals I want to achieve with this action. Quite often an existing site will have a custom default theme.

There would be nothing preventing a developer from creating the block entity with whatever keys they want, then using one of the placeBlock variants to adjust it.

The code for this was specifically written so that anything provided by the recipe would override the defaults, and a minimal set of defaults was chosen, using what seem to be always present for blocks placed in the UI and then exported.

Having to use an array keyed by the theme name makes the recipe unable to place a block in an unknown theme, which is one of the goals I want to achieve with this action.

I'm not sure we really want this, and I feel quite strongly that it's not appropriate.

"Place this block in a region named such-and-such, without knowing if it will actually make visual sense, or be desirable from a UX perspective" doesn't feel like a good idea to me. Truth is, recipe authors have no way of knowing what region names mean, and that's just a fact. I'm not sure it's appropriate for us to dance around it.

Also, iterating through an array and using the first match just feels complicated to me, from a recipe authoring perspective. I'd rather

I propose this as a compromise: how about an optional key, called default_region, which is the region it will use if region_map doesn't list the theme being targeted? If a default region isn't defined, then you get a ConfigActionException.

I spoke with @mandclu on Zoom and I think we arrived at an understanding on how to carry this issue forward.

In essence, I agree with the usefulness of a flexible block placement solution, but this MR is trying to do too much. We need to reduce its scope, and add more functionality in follow-up issues.

So why don't we start here: in this issue, let's define two new config actions: placeBlockInAdminTheme and placeBlockInDefaultTheme. If we want, they can be based on the same class, split by a deriver. This plugin should be a wrapper around the EntityCreate plugin class, and create a block entity. But the block's theme key should be automatically determined by the plugin. Beyond that, it should accept an array of entity values, just as EntityCreate would.

Then, before we commit it, let's create follow-ups for the following additional things:

• Flexible choice of region -- the block can go in a region specific to particular themes, or use a fallback region
• Flexible choice of position -- rather than specify a weight, the block can either go first in the region, last in the region, or at a specific absolute position.

I wonder if we should consider blocking this on ✨ Make it possible for recipes to prompt for input values Fixed , since that would mean we could do stuff like this:

inputs:
  default_theme:
    from: config
    config: 'system.theme:default'
config:
  actions:
    block.block.foo:
      createIfNotExists:
        theme: %default_theme

Prashant.c → changed the visibility of the branch 3448131-provide-a-way to active.

Prashant.c → changed the visibility of the branch 3448131-provide-a-way to hidden.

Can someone please let me know which fork and which branch to use for all the latest code for this issue?
Currently, there are multiple (forks and branches).

mandclu → changed the visibility of the branch 3448131-placeblock-block-core to hidden.

@Prashant.c it probably depends on what you want, If your intent is to contribute here, I would work on MR !8806. That said, one of the earlier MRs would represent a more feature complete version of the vision for this config action, but won't work in exactly the way the final version is likely to land.

I wonder if we should consider blocking this on #3303126: Allow recipes to reuse existing config values

Personally I would say no, since the appropriate version of the action should be able to determine without extra effort on the part of the recipe author. That said, I'm going to go ahead and create child issues to capture the separate discussions (and code changes) for allowing flexibility on the region order of the block placement. Also updating the IS to reflect the latest approach.

I added some keyword-based permission handling; it turned out not to be too tricky.

The main differences are that !8806 is a lot less "busy", and doesn't try to validate anything. We can rely on config validation for that, which is being added in 📌 Make Block config entities fully validatable Fixed . So that's all gone.

I did some refactoring to make it a lot less verbose, and hopefully easier to follow. I also think it doesn't benefit us to try and handle every possible key of a block entity; we can just pass those to the EntityCreate action.

Things for follow-ups:

• Make this capable of updating an existing block. (This might necessitate moving the "change region", and "change position" parts to a new config action, maybe called moveBlock.)
• Give it the ability to handle absolute integer positioning, rather than merely "first" and "last".

From #60:

Personally I would say no, since the appropriate version of the action should be able to determine without extra effort on the part of the recipe author.

After sleeping on it, I think I agree with you here. It costs us very little to provide two actions with very clear names; I think that'll be good for recipe authors. Having to rely on a config input, while technically equivalent, is more complicated.

I see the follow-ups are already done, so this is ready IMHO.

I tested the new config actions by applying a recipes with this in the recipe.yml file:

name: 'Place blocks'
config:
  actions:
    block.block.powered_first:
      placeBlockInDefaultTheme:
        id: powered_first
        region:
          olivero: content
        weight: first
        plugin: system_powered_by_block
    block.block.powered_first_admin:
      placeBlockInAdminTheme:
        id: powered_last
        theme: admin
        region:
          claro: content
        weight: first
        plugin: system_powered_by_block

Both blocks were placed, but ended up with zero as the weight, so were not placed before existing blocks.

After looking at the code some more, I realized that the above was a problem with the recipe using the weight key instead of the intended position key. With the updated recipe the blocks were placed as expected and the weight assigned the expected numerical values, for both 'first' and 'last'.

mandclu → changed the visibility of the branch 3448131-new-config-action to hidden.

This is looking great - love the work @mandclu and @phenaproxima are doing.

Posted some comments on the MR - most of which are are tiny nits.

The updated code works as intended.

Committed 89d18d3 and pushed to 11.x. Thanks!

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

Cherry-picked back to 10.4.x so recipes on 10.4 and 11.1 are mostly compatible,