I am foreseeing more and more problems with the way we are currently injecting and merging the regions of all our layouts into the info array of the theme through hook_system_info_alter(). We will have to find a better solution... sadly...

Again, this is especially about how we treat layout regions when they are different than those provided by the theme.info file.

The problem with those regions is that we have to inject them through hook_system_info_alter() to make them appear on the Block Configuration page. Integrating the layouts with Panels and Context is a simple and non-problematic thing to do.

CommentFileSizeAuthor
#3 rething-layouts-1902464-3.patch5.09 KBfubhy

Comments

fubhy’s picture

fubhy commented
Issue summary: View changes

Updated issue summary.

jorgensean’s picture

jorgensean commented

I think only allowing regions to come from the themes info file is the way to go. I don't see a lot of use cases where you need to bring in a region that another layout in the same theme explicitly couldn't use (i.e. not.. rendering the region conditionally per layout is an unacceptable solution). My vote goes to layouts being able to use any region available to the theme.

The way I see it, if I want to place a search box in 3 different locations, it would be easiest to make a search region, place the search block in it, and control it's location rendered in my layout file(s). If people are thinking they want to use layouts to get the search box block to render in the sidebar_first region on layout 1, sidebar_second region on layout 2, and footer region on layout 3, it is beyond the scope of the layout system. To accomplish this, you would have to create a per-layout block configuration, which seems unnecessary. You're better off making a new sub-theme?

What happens when layouts .inc region string declarations conflict? Does the block configuration page pull its region names from these files (it doesn't look like it)? Should it (I'm not suggesting it should)?

fubhy’s picture

fubhy commented

Okay, so let me explain the way it currently works first:

On hook_system_info_alter() we merge the layout regions into the theme regions. At that point we also create a backup copy of the original regions so we know what the theme originally came with. Additionally we store the regions of each layout in the theme registry (under the hook entry for the theme hook suggestion that represents the layout). We put them there during hook_theme(). Okay, so now we know the regions of every layout, the theme and all merged together. So far so good.

Now, on hook_block_list_alter() we look up the current layout (if any) and adjust the list of blocks accordingly. So that we really only process those that the current layout (or native page.tpl.php) has regions declared for. So, if the current page is a layout, we match the list of blocks against the available regions of the layout (we fetch that from the theme registry at this point @see above for an explanation how it got there). If it's not a layout then we simply match it with the backup copy that we set up in hook_block_list_alter().

What happens when layouts .inc region string declarations conflict? Does the block configuration page pull its region names from these files (it doesn't look like it)? Should it (I'm not suggesting it should)?

Yes, the block configuration page pulls the regions from what we injected during hook_system_info_alter(). That means, it's the merged array of all of the regions (including layouts). If there are two conflicting declarations the first declaration wins (the very first is the original theme region).

Okay... So here is the plan then I guess:
We keep the ability for layouts to declare regions. However, a layout is only valid/compatible with the theme if the theme declares the regions. That means that the array of regions declared in a layout would more or less end up as a requirements check (which I think is fine). That way we would get rid of the stupid Omega Tools dependency too. Oh and we could delete quite a bit of code too.

We can't ditch the declaration of layout region entirely though because of two reasons:

  1. At some point I want to implement hook_panels_layout_info() (or whatever that hook's name is) and forward layout declarations to Panels so they can be used with Panels everywhere for example (or Panels in general really).
  2. Also, we don't want to show ugly error messages when the regions of the theme do not suffice for a given layout. Instead, we wan't to signal that the layout is incompatible beforehand and offer a solution to the problem in a descriptive info message: "Layout is currently not compatible with your theme. You need to declare region x,y,z in your theme.info file for the layout to be compatible." Or something else that someone with better english comes up :P.
fubhy’s picture

fubhy commented
StatusFileSize
new rething-layouts-1902464-3.patch5.09 KB

This is how that would look...

Note: I would want to add a little message for every layout that requires regions that are not defined in the theme's .info file. That message should appear next to the layout on the theme settings page.

However, I added a page preprocess hook that initializes the region array anyways. So even if you can't place any blocks in it and hence will never render it it won't throw an illegal offset notice when trying to access it in the layout.page.tpl.php file.

fubhy’s picture

fubhy commented
Status: Active » Needs review
fubhy’s picture

fubhy commented
Status: Needs review » Fixed

Fixed. Follow-up: #1906928: Show a warning for layouts that are not fully supported by a theme

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.