Installing default blocks

[rupertj]

Adds a feature that installs default blocks for other LGD modules.

Implements the ideas discussed in #211.

[rupertj]

There's a few bits of configuration that I want to add to this, but it works. Please take a look and tell me what you think!

I'd recommend starting with the tests: They include a module called localgov_core_default_blocks_test which does nothing but provide a custom block plugin and some configuration to place it using the new mechanism.

[andybroomfield]

Tested by installing a BHCC custom module with a block using the new config/localgov block tecnique

Installs on localgov_scarfolk

and on gov.uk theme

I also tested this using the footer region and the content_bottom region, the later missing from govuk theme and verified it is not placed anywhere and didn't see any error messages. (Maybe it is worth adding a message saying the block still needs to be placed). Perhaps a future enchancement could be that the region key is replaced with a regions array of acceptable regions in order?

I'm happy with this approach, though I have a few questions.

I originally tried to test with Olivero, but that theme uses slightly different names for it's regions (content_below instead of content_bottom). This rasises the issue of wanting to provide default blocks for themes but in different regions, which we could do using the existing method of placing it in config/optional.

I have a question on what to do about Alert banner, which currently does not depend on core currently. Switching to this method would mean that if users did want to use localgov_base and localgov_alert_banner they would also need core to get the default block placement or could I continue to provide one in config/optional? Would that mean two blocks end up getting placed if localgov_core is present? I'm thinking the case that is I provided default blocks under /optional for Olivero and that happens to be the active theme. Maybe a way of either detecting or a module declaring that it will handle installing for that theme itself so this service can skip it.

I also notice that it only works with the active theme, so doesn't effect the admin theme (which is good!). Some site (BHCC) have multiple front end themes where default block placement could be useful, or could also be a hinderance depending on perspective.

Noting there are @todo statements on both those issues above, so it's something we can come back to if we need to move this forward. I'm tempted to actully say it should only target the active theme and so module continue as now with providing default blocks and we skip over the localgov_base / scarfolk.

[rupertj]

@andybroomfield I'd like it if people didn't have to do both the existing method of using optional config and the new one. We could support your use case of install into olivero (or another theme with different regions) by using your idea about an array of themes rather than a single value. So if you wanted a block in content_below on olivero, or content_bottom on a localgov theme, you'l list both, and it'd check the regions in order and install into all the localgov themes, plus olivero if it's set as the active theme.

For alert banner, I'd suggest that you add the yml files for this new mechanism and remove the existing config. There's currently only config there for base and scarfolk, and I have a strong suspicion that anyone who's using one of those themes will have localgov_core installed anyway. Anyone who's got a subtheme of base probably won't get the blocks anyway using the current methodology, so nothing's been lost, and at least if they have a subtheme of base and localgov_core installed, they will get them.

[finnlewis]

Discussing this today in Merge Tuesday, general enthusiasm for the approach.

@Adnan-cds also mentioned the possibility of listing multiple potential regions a block might want to be in.

It'd be good to test this more widely and get this in soon.

[finnlewis]

@rupertj I'm testing this, finally!

[finnlewis]

I've tested the functionality, and confirmed that it works.

block config placed in /config/localgov does get installed (as long as it's extension is .yml not .yaml 😄 )

Could do with another review of the code if possible, but I've stepped through it with xdebug and it all makes sense to me.

@andybroomfield you happy with the code?

[stephen-cox]

I may have found an issue with this. To test I moved the base block config for LGD services from the optional dir into a localgov dir, removed the theme settings from the YAML and deleted the block config for the scarfolk theme.

Site installs fail with the error The theme core does not exist.

It looks like when the install hook is running neither the LGD base or Scarfolk themes are enabled and the DeffaultBlockInstaller::targetThemes() method is returning 'core' as a theme.

Am I missing something in the testing here?

[ekes]

I think you're concern @stephen-cox is also fixed now?

[stephen-cox]

Unfortunately, I've found another couple of problems with this.

The block installer doesn't check dependencies so it can try to install blocks before the code defining them has been enabled, which displays a warning. This is not a serious issue though, it just means we have to be sure all dependencies are in place in a module that contains the block config.

More of a problem is how Drupal handles enabling blocks in a theme before the theme is installed. It looks like blocks are added to the first theme region and disabled. So the blocks don't show without some manually intervention.

For a site install it would be better to have the block installer triggered after the themes are installed, but that's going to mean overhauling how this is triggered.

It would be nice if there was a hook for when site installs are finished, but this doesn't look to exist (https://www.drupal.org/project/drupal/issues/2924549).

[rupertj]

Ah, that's annoying. Thanks for the further testing though.

I could add an install step to the end of the localgov profile to set up the blocks?

[stephen-cox]

I could add an install step to the end of the localgov profile to set up the blocks?

Assuming this is going to be used as with site installs (which I assume it will be if we include Publications) then I think this sounds sensible.

Maybe only work for themes that are installed (ignoring core) and then provide a way for projects and themes to rerun the block installer - or something like that.