Currently, system_default_region() simply returns the first region in the list of regions. This isn't great, because the first displayed region is usually something like 'header', which in general is not a good default.
Proposed resolution
Fall back to the content region if there is one, and only after that fall back to the first region found.
Potentially improve this logic more generally, by allowing themes to specify roles/hints for their regions, in #3278410: block_theme_initialize() sledgehammers square blocks into round regions, allow themes to provide a mapping of regions to 'roles'.
A previous solution was to allow themes to specify their default region, but this is likely to be 'content' in most cases, so doesn't necessarily offer a lot more than improving the fallback logic without configuration.
Comment | File | Size | Author |
---|---|---|---|
#48 | 2635166-47.patch | 3.67 KB | alexpott |
Comments
Comment #2
dcrocks CreditAttribution: dcrocks as a volunteer commentedHere is a patch that adds a parameter('block_default_region') to the theme's info.yml file. If it is set in a theme, when that theme goes thru install any blocks merged from the current theme that don't have matching regions to the new theme will go into the region specified in 'block_default_region' if it exists. Otherwise it falls back to the old rule.
For example, I changed stark.info.yml to:
This moved several blocks that were previously placed in 'sidebar_first' to 'content' when the stark theme was installed. If you have a better suggestion for the parameter name, please say so.
Comment #4
swentel CreditAttribution: swentel commentedName sounds fine to me.
needs indentation
same here
Comment #5
dcrocks CreditAttribution: dcrocks as a volunteer commentedHere is another patch with a small improvement and formatting fixes.
Comment #7
Jeff Burnz CreditAttribution: Jeff Burnz commenteddon't want bike shed this but underscores feel a bit out of place here, how about just...
default region: content
orregion default: content
Comment #8
dcrocks CreditAttribution: dcrocks as a volunteer commentedTrying to get logic right.
Comment #10
dcrocks CreditAttribution: dcrocks as a volunteer commentedsystem_get_info() returns an empty array in some of the tests so the way the code was formtted generated errors. Hopefully this fixes it.
note that since I test that the region provided by block_default_region is a valid region if an invalid one is specified the fail is silent. It just shows the old behavior.
Comment #11
tim.plunkettShouldn't this logic be located in system_default_region? That function should respect this setting, not just block.module's invocation of it.
Comment #12
dcrocks CreditAttribution: dcrocks as a volunteer commentedThe only call to this function in core is from the block module. I guess I just thought of this as a block thing and not a theme thing. Could move it easily but any other use cases for system_default_region?
Comment #13
tim.plunkettYou're right that it's only called by block module in core.
But it's in system.module, and anyone in contrib could call it.
Anyone calling it would likely want the same thing block module gets...
Comment #14
joachim CreditAttribution: joachim commentedNothing other than block module uses it in core, but we've no idea whether anything else might use it.
So I agree with #11: this logic should be somewhere where system_default_region() can find it.
So either in system_default_region(), or baked into the stored theme info in ThemeHandler::rebuildThemeData()?
Comment #15
joachim CreditAttribution: joachim commentedHmm although I think it makes more sense in ThemeHandler::rebuildThemeData(), there's a problem with doing that:
The default for themes that don't set a default region is the first region in the list, *out of the visible ones*.
The problem is that in ThemeHandler::rebuildThemeData(), we don't have the list of hidden regions, because that's something system module adds in the alter hook:
So we need to add the 'default region' key to the theme info before we invoke the alter hook, but we don't know which regions are definitely visible until the alter hook has run and has altered the data :(
Comment #16
dcrocks CreditAttribution: dcrocks as a volunteer commentedThis patch moves the code to system_default_region in system.module.
Comment #18
dcrocks CreditAttribution: dcrocks as a volunteer commentedSorry, wrong patch. This is the real #14.
Comment #19
dcrocks CreditAttribution: dcrocks as a volunteer commentedShouldn't system_default_region() be returning BlockInterface::BLOCK_REGION_NONE instead of '' when a theme has no regions?
Comment #20
joachim CreditAttribution: joachim commentedThe indentation is wrong here.
The existing return needs to be indented too now that it's in an else {} block.
> Shouldn't system_default_region() be returning BlockInterface::BLOCK_REGION_NONE instead of '' when a theme has no regions?
Probably not, because a) that constant is part of block module, which system module doesn't depend on, and b) that would be an API change rather than an addition, which we can't do in 8.x.x.
We should maybe take this opportunity to add some docs to this that explain how the default region is figured out.
Comment #21
dcrocks CreditAttribution: dcrocks as a volunteer commentedShouldn't this need a change record as well?
Comment #22
dcrocks CreditAttribution: dcrocks as a volunteer commentedSmall changes
Comment #23
dcrocks CreditAttribution: dcrocks as a volunteer commentedI added a draft change record Themes can specify default region for block placement. Please review.
Comment #29
markhalliwellWas about to RTBC this, but realized there is no test for this. Patch still applies though.
Comment #30
joachim CreditAttribution: joachim commentedAlso:
First line of a docblock should be a single line paragraph.
Comment #36
shivam kaushal CreditAttribution: shivam kaushal at OpenSense Labs commentedComment #37
shivam kaushal CreditAttribution: shivam kaushal at OpenSense Labs commentedComment #39
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedsystem_get_info() is deprecated has been removed in 9.x.
https://www.drupal.org/node/2709919
Comment #42
alexpottThe original issue summary mentioned choosing the content region as the default region if it exists as a better choice than header. After discussing with @catch and @spokje it was felt that this is a better approach than having yet another thing to configure. This makes the behaviour of block_theme_initialise a bit better because stuffing everything into the header region by default is not a great look.
Added test coverage of the change too.
No interdiff because this is a new approach based on the existing issue summary.
Comment #43
alexpottThe reason this came up today is due to #3278342: Theming blocks bonanza and how themes are supposed to look after being switched to.
Attached to this comment are some before and after screenshots for install 9.4.x standard and then installing bartik and setting to the default theme. Not going to embed the screenshots because they are a full browser window.
Comment #44
alexpottImproved the useless comment to be more useful.
Comment #47
joachim CreditAttribution: joachim at Factorial GmbH commented> The original issue summary mentioned choosing the content region as the default region if it exists as a better choice than header. After discussing with @catch and @spokje it was felt that this is a better approach than having yet another thing to configure
Ok, but then that's no longer what this issue is about.
And what if a theme doesn't have a 'content' region?
I'd say the new proposal is maybe a quicker fix to the problem, and could get in as a separate issue which is a much quicker fix.
Comment #48
alexpottFixing the tests.
Comment #49
alexpottThe left_sidebar region does not exist in stark so this was relying (unintentionally) on system_default_region()
Because a block has moved from the header to the content region the row in the block UI that this block is in has changed.
Comment #50
catchWe often have multiple approaches to fix the same problem on one issue, sometimes it's easier to spin different approaches out, sometimes it's easier to keep discussion in one place. Don't have a strong preference with this one.
Having said that, on providing more of an API for this, I think rather than a single default region, what would help with the UX of switching default themes and blocks landing in more appropriate places, is the ability to provide a mapping of regions to 'roles' - 'header', 'primary navigation', 'secondary navigation', 'highlighted', 'primary sidebar', 'secondary sidebar', 'breadcrumbs', 'content', 'footer'. Then if both the old theme and new theme map a region to one of those roles, the blocks could be moved 1-1 even if the regions have different names.
Comment #51
joachim CreditAttribution: joachim at Factorial GmbH commentedTrue, but the issue summary and even the issue title don't reflect the latest approach at all.
And agreed on the region roles approach.
Comment #52
catchOpened #3278410: block_theme_initialize() sledgehammers square blocks into round regions, allow themes to provide a mapping of regions to 'roles' for the roles/hints idea. I've updated the issue summary and title here too.
Comment #53
joachim CreditAttribution: joachim as a volunteer commentedThanks.
So the roadmap is a quick fix in this issue, and a more comprehensive follow-up in #3278410: block_theme_initialize() sledgehammers square blocks into round regions, allow themes to provide a mapping of regions to 'roles'? That sounds good to me.
I think a comment to explain the order of fallback here would be good.
Also, do we need the ?? at the end? Would there ever be a case where a theme has no regions at all?
Comment #57
andypostComment #58
andypost