Problem/motivation

It is already possible to have different page layouts/templates for pages with an existing discovery mechanism for templates. As can be seen in template_proprocess_page() where page template file suggestions such as page__node, page__node__%, page__node__1, and page__node__edit are produced (for a page like node/1/edit. As well as theme(), where the template is chosen based on these suggestions.

It is not currently possible to switch page layouts by any other means such as making it configurable as to which page layouts are used for which paths / path patterns or other conditions, unless there is a module altering ($variables['theme_hook_suggestions']) overriding what the theme provided entirely or sidestepping page themeing altogether.

If we don't want to sidestep theme page templates, making it possible to switch between (potentially exclusively theme provided) layouts requires the theme to provide a list of the layouts included within and Drupal to provide a user interface to switch between them. This issue is about defining the list of layouts provided.

Proposed solution

Themes should explicitly declare their page layouts, so instead page.tpl.php (page-node.tpl.php, etc) type of files among other templates, themes would have a layouts subdirectory, which would contain directories for each layout defined. For one layout, the corresponding CSS styles, tpl.php template and possibly an icon for the layout would be contained in this directory as well as a simple .yml file to describe the layout.

As an example with the Bartik theme

  • Instead of bartik/templates/page.tpl.php, the template would be at bartik/layout/page/bartik--page.tpl.php (where the layout subdirectory name "page" is arbitrarily picked by the theme and the tpl.php name consists of the theme name and the filename for the .yml, this bartik--page.tpl.php (see the .yml file below)
  • Instead of bartik/css/layout.css, the layout style becomes bartik/layout/page/layout.css (or bartik/layout/page/page.css), again the file name is declared in the .yml file below, the important piece is that it is co-located with the layout definition)
  • There would optionally be an icon for the layout in the same directory.
  • A file named page.yml (again the name is arbitrary for the developer, but this defines the layout as bartik__page - the first part being the theme name, the second being the layout name, and the template file being looked for is derived from the .yml) would be located in the same directory. Example file contents:
    title: Bartik page
    category: Other
    icon: bartik.png
    css: page.css
    regions:
      header: 'Header'
      help: 'Help'
      highlighted: 'Highlighted'
      featured: 'Featured'
      content: 'Content'
      sidebar_first: 'Sidebar first'
      sidebar_second: 'Sidebar second'
      triptych_first: 'Triptych first'
      triptych_middle: 'Triptych middle'
      triptych_last: 'Triptych last'
      footer_firstcolumn: 'Footer first column'
      footer_secondcolumn: 'Footer second column'
      footer_thirdcolumn: 'Footer third column'
      footer_fourthcolumn: 'Footer fourth column'
      footer: 'Footer'
    

    This file defines the layout, its name, regions, etc. The region definition in theme .info files becomes obsolete.

The same changes in a graphical way:

Which lets us make modules and install profiles even provide layouts either as static pre-built layouts (using the same file structure as themes) or dynamic layouts by implementing the plugin interface provided by the layout system. The patch includes a couple static layouts for demonstration as well.

Related issues

#1787634: [META] Decouple layouts from themes

CommentFileSizeAuthor
#144 layout-144.patch21.64 KBGábor Hojtsy
#141 layout-141.patch25.8 KBGábor Hojtsy
#132 interdiff.txt5.51 KBGábor Hojtsy
#132 layout-132.patch25.76 KBGábor Hojtsy
#113 layout-113.patch25.97 KBGábor Hojtsy
#113 interdiff.txt1.3 KBGábor Hojtsy
#112 layout-112.patch26.02 KBfubhy
#111 layout-111.patch26.05 KBfubhy
#110 layout-110.patch27.11 KBEclipseGc
#102 layout-102.patch28.7 KBGábor Hojtsy
#102 interdiff.txt2.84 KBGábor Hojtsy
#101 interdiff.txt1.04 KBGábor Hojtsy
#101 layout-101.patch28.76 KBGábor Hojtsy
#100 layout-100.patch28.24 KBGábor Hojtsy
#100 interdiff.txt6.1 KBGábor Hojtsy
#97 layout-96.patch27.53 KBGábor Hojtsy
#97 interdiff.txt5.75 KBGábor Hojtsy
#94 layout-94.patch27.05 KBGábor Hojtsy
#94 interdiff.txt967 bytesGábor Hojtsy
#92 interdiff.txt5.44 KBGábor Hojtsy
#92 layout-91.patch27.02 KBGábor Hojtsy
#85 interdiff.txt1.7 KBGábor Hojtsy
#85 layout-85.patch22.77 KBGábor Hojtsy
#84 layout-84.patch22.64 KBeffulgentsia
#84 interdiff.txt2.31 KBeffulgentsia
#74 layout-74.patch21.69 KBGábor Hojtsy
#73 layout-73.patch23.27 KBGábor Hojtsy
#73 interdiff.txt8.63 KBGábor Hojtsy
#72 layout-72.patch22.03 KBGábor Hojtsy
#71 layout-71.patch21.17 KBGábor Hojtsy
#70 layout-70.patch32.57 KBGábor Hojtsy
#61 layout.diff31.14 KBEclipseGc
#59 layout.diff30.58 KBEclipseGc
#37 Screen Shot 2012-09-26 at 23.46.07.png43.03 KBswentel
#16 layouts-16.patch19.69 KBGábor Hojtsy
#16 interdiff.txt10.96 KBGábor Hojtsy
#15 Drupal8LayoutNew.jpg30.51 KBGábor Hojtsy
#15 Drupal8Layout.jpg58.92 KBGábor Hojtsy
#12 layouts-11.patch18.44 KBeffulgentsia
#10 layouts.diff14.66 KBEclipseGc
#9 layouts-9.diff17.48 KBeffulgentsia
#9 interdiff.txt9.64 KBeffulgentsia
#3 layouts.diff13.15 KBEclipseGc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Had a good discussion about this with EclipseGC:

[5:18pm] GaborHojtsy: EclipseGc: I've been reading up on http://drupal.org/book/export/html/1637614
[5:19pm] EclipseGc: GaborHojtsy: what I was thinking is a discovery class that looks in [theme]/templates/[dir-names-per-layout-plugin]
[5:19pm] EclipseGc: GaborHojtsy: so a discovery class like so, and processDefinition() in the LayoutManager class that adds a default class to any metadata we return from that discovery
[5:20pm] EclipseGc: GaborHojtsy: so we have a default class in like… the "layout" module and then if we end up with a complex layout system (like say responsive) then the class just needs to be included in a proper PSR-0 namespace and documented in the metadata for it
[5:22pm] EclipseGc: GaborHojtsy: I'd be happy to write beginnings of a layout module that had this basic approach in it taken care of
[5:22pm] GaborHojtsy: EclipseGc: I don't think we want a layout module that is required
[5:23pm] EclipseGc: GaborHojtsy: err… then we have very different goals here
[5:23pm] GaborHojtsy: EclipseGc: I mean we can have a layout module that is required and does this, then the actual dynamic layouts module would have a different name, that works too
[5:23pm] EclipseGc: GaborHojtsy: oh, like "Responsive" or something
[5:24pm] GaborHojtsy: EclipseGc: I think those who use a theme that has 3 layouts and they want to configure pages for those but never create any layouts outside of a .tpl.php should not need to use a module that has this baggage of a layout builder
[5:24pm] GaborHojtsy: EclipseGc: we can have a layout module and all it does is look up layouts in themes and provides a UI for people to assign layouts to pages
[5:24pm] GaborHojtsy: EclipseGc: and make it required
[5:25pm] EclipseGc: GaborHojtsy: I agree, but, and this is a point we need to clarify with kevin, I think the layout builder should be a property of the Responsive Layout Plugin Class
[5:25pm] GaborHojtsy: EclipseGc: if its better then this being in a more generic upper level module
[5:25pm] GaborHojtsy: EclipseGc: I know system is passé
[5:25pm] GaborHojtsy: EclipseGc: "the layout builder should be a property of the Responsive Layout Plugin Class" ? what does that mean?
[5:25pm] EclipseGc: GaborHojtsy: that the layout module should expect that SOME layouts may want to provide additional administrative procedures (like responsive and flexible do today in panels) and allot for that
[5:26pm] EclipseGc: GaborHojtsy: but that the basic Layout Class wouldn't need any of that, and so it wouldn't happen for them
[5:28pm] GaborHojtsy: EclipseGc: yes, which is why I assumed it would be just part of "the general system" and not a specific module, in my vocabulary at least layout module was the dynamic UI layout builder thing
[5:28pm] GaborHojtsy: EclipseGc: we can adjust my vocabulary
[5:28pm] EclipseGc: GaborHojtsy: oh sure, yeah when I say "layout module" I just mean a thing which allows us to assign layouts to pages… basically "Page Manager"
[5:28pm] EclipseGc: but I'm just conceptualizing the layout component of that for the moment
[5:29pm] GaborHojtsy: EclipseGc: what I expect to achieve first is that themes declare their layouts and we have a layout plugin system that modules could then later inject their layouts too (like responsive layout builder if we want to build the base system in a module and name it layout, that is totally fine
[5:30pm] EclipseGc: GaborHojtsy: yeah, I'm not horribly concerned with what we name it, just trying to align your vocabulary and my own. I've not actively discussed this part of things with many people so I think we're going to have to discovery the vocabulary we like best as we go
[5:30pm] GaborHojtsy: EclipseGc: ok, I can also get started on a tiny layout module that defines a plugin and discovers .yml files with metadata about theme layouts
[5:31pm] GaborHojtsy: EclipseGc: my 1st question after reading the plugin doc was essentially: ok, so I write a plugin but then were do I put it given there is no place yet for this
[5:31pm] EclipseGc: GaborHojtsy: actually, I'm arguing with myself on that topic right now… you familiar with derivatives yet?
[5:31pm] GaborHojtsy: EclipseGc: we can make it a new module instead of using any upper level moduels
[5:31pm] GaborHojtsy: EclipseGc: yup
[5:33pm] EclipseGc: GaborHojtsy: so I was thinking that since we're likely to only have 2 or 3 layout classes in total… we might could not make a new discovery, but instead just make a more complex derivative solution for each of them
[5:33pm] EclipseGc: GaborHojtsy: then the classes can still be module provided, but the derivative system for each of those could vary as much as necessary
[5:36pm] EclipseGc: GaborHojtsy: ok, so what were you thinking of doing with regard to the yml files?
[5:36pm] EclipseGc: GaborHojtsy: like, what sort of organization were you thinking?
[5:36pm] GaborHojtsy: EclipseGc: well, so there are two existing declarative systems, regions use .info files, breakpoints is RTBC and uses .yml files (but does nothing with them yet)
[5:36pm] GaborHojtsy: these are two things themes declare
[5:37pm] GaborHojtsy: so for layouts I think the Drupal 8-eque way would be to have .yml files instead of trying to shoe-horn them into info files too
[5:37pm] EclipseGc: GaborHojtsy: can you link me?
[5:37pm] GaborHojtsy: http://drupal.org/node/1775774
[5:37pm] Druplicon: http://drupal.org/node/1775774 => Allow themes to identify their breakpoints to Drupal => Drupal core, theme system, normal, reviewed & tested by the community, 32 comments, 1 IRC mention
[5:37pm] • EclipseGc will read up after this
[5:37pm] EclipseGc: GaborHojtsy: so I agree on not using .info
[5:37pm] GaborHojtsy: that was totally independent of Spark stuff btw
[5:38pm] GaborHojtsy: so they have simple .yml files to define breakpoint metadata
[5:38pm] GaborHojtsy: +mobile: '(min-width: 0px)'
[5:38pm] GaborHojtsy: +narrow: 'all and (min-width: 560px) and (max-width:850px)'
[5:38pm] GaborHojtsy: +wide: 'all and (min-width: 851px)'
[5:38pm] GaborHojtsy: like this
[5:38pm] EclipseGc: I'm not sure I want to use yml though since that feels sort of like stepping on CMI's toes
[5:38pm] EclipseGc: hmmm
[5:38pm] EclipseGc: interesting
[5:38pm] GaborHojtsy: core/themes/bartik/config/bartik.breakpoints.yml
[5:38pm] GaborHojtsy: core/themes/seven/config/seven.breakpoints.yml
[5:38pm] GaborHojtsy: etc.
[5:38pm] EclipseGc: right, so these really are CMI files in this case
[5:39pm] EclipseGc: who/what uses the knowledge of them?
[5:39pm] GaborHojtsy: so I think we need some format that defines the names and template files for page layouts basically, with possibility for an icon let's say
[5:39pm] EclipseGc: are the media queries being generated from them?
[5:39pm] GaborHojtsy: EclipseGc: there is a breakpoints module proposed to core that uses that
[5:39pm] GaborHojtsy: and uses the info for responsive images, etc.
[5:39pm] EclipseGc: interesting
[5:39pm] GaborHojtsy: http://drupal.org/project/breakpoints
[5:39pm] GaborHojtsy: again, that was built independent of spark
[5:40pm] GaborHojtsy: it has a stable D8 release
[5:40pm] EclipseGc: lol, sure
[5:40pm] EclipseGc: "stable D8 release"
[5:40pm] GaborHojtsy: so anyway
[5:40pm] GaborHojtsy: we can make up some metadata format, but why make up yet another thing when .yml files are simple and easy
[5:41pm] GaborHojtsy: and we have a system to read them, etc.
[5:41pm] GaborHojtsy: which I guess is what the breakpoints thinking was
[5:41pm] EclipseGc: GaborHojtsy: yeah, they'll just by yml files not in config
[5:41pm] GaborHojtsy: (I was not part of the discussion, just trying to ride a wave instead of making up yet another new thing)
[5:42pm] EclipseGc: GaborHojtsy: that's my only misgiving here, I'm 100% cool we reusing code, I just want to make sure we cover our bases with regard to why it's yml that's not CMI
[5:42pm] EclipseGc: GaborHojtsy: it makes me want to check twig a little harder to see if it's commenting is introspectable
[5:43pm] GaborHojtsy: EclipseGc: http://drupal.org/node/1775774#comment-6445824
[5:43pm] Druplicon: http://drupal.org/node/1775774 => Allow themes to identify their breakpoints to Drupal => Drupal core, theme system, normal, reviewed & tested by the community, 32 comments, 2 IRC mentions
[5:43pm] GaborHojtsy: EclipseGc: Overall, the config file approach makes much more sense to me for this use-case.
[5:43pm] GaborHojtsy: In #1712250: Convert theme settings to configuration system and some related issues, we're considering to move some of the theme .info stuff into a [theme].settings configuration object. A [theme].breakpoints would nicely complement that.
[5:43pm] GaborHojtsy: So +1 for using config.
[5:43pm] GaborHojtsy: from @sun
[5:44pm] EclipseGc: yeah, I hear that, but I've been down the "CMI as metadata for plugins" road before
[5:45pm] EclipseGc: it's sort of a mess, so I just want to double check that step before we take it again
[5:45pm] EclipseGc:
[5:46pm] GaborHojtsy: ok, tried to get some of the people with attention there to our stuff then http://drupal.org/node/1775774#comment-6491872
[5:46pm] Druplicon: http://drupal.org/node/1775774 => Allow themes to identify their breakpoints to Drupal => Drupal core, theme system, normal, reviewed & tested by the community, 33 comments, 3 IRC mentions
[5:46pm] EclipseGc: GaborHojtsy: because the fundamental problem here is that doing that separates the metadata from the plugin
[5:46pm] EclipseGc: which is really problematic from a thought-cohesion standpoint
[5:46pm] EclipseGc: GaborHojtsy: k
[5:47pm] GaborHojtsy: EclipseGc: yeah, well, "the plugin" would be core running the .tpl.php with variables right? so which .tpl.php to run is really config for that plugin which comes from the layout configuration and this data is metadata to be able to produce that config
[5:47pm] GaborHojtsy: EclipseGc: so its two levels apart from the plugin in my mind
[5:47pm] GaborHojtsy: EclipseGc: layout declation => layout config => plugin execution
[5:47pm] EclipseGc: GaborHojtsy: right, and it would be the only plugin (currently) to separate metadata from plugin
[5:48pm] EclipseGc: GaborHojtsy: which is why I'd rather search through directories and then cache the return
[5:48pm] GaborHojtsy: EclipseGc: sounds like it needs to since the themes need to be swappable while the execution engine needs to be unified
[5:49pm] GaborHojtsy: EclipseGc: well, you just encode metadata in the dir structure then instead of a file
[5:49pm] GaborHojtsy: EclipseGc: the source of the metadata is the same just encoded differently
[5:49pm] EclipseGc: GaborHojtsy: that includes things like "what regions and what breakpoints" when I say metadata
[5:49pm] GaborHojtsy: EclipseGc: and we want to allow layouts to have a label and possibly an image, which i guess then you'd introspect from a file in some other format
[5:49pm] GaborHojtsy: EclipseGc: which again would be external metadata
[5:50pm] GaborHojtsy: I don't see how to avoid this, it is just intrinsically part of the system
[5:50pm] EclipseGc: when I say metadata, I mean essentially this: http://drupalcode.org/project/panels.git/blob/refs/heads/7.x-3.x:/plugin...
[5:51pm] EclipseGc: GaborHojtsy: it's not intrinsically part of the system though, we could just put the yml file right next to the rest of the components and discovery and parse it manually as part of the plugin discovery, instead of relying on CMI for that
[5:51pm] EclipseGc: it's only a discovery issue
[5:51pm] EclipseGc: i.e. exactly like this: http://drupalcode.org/project/panels.git/tree/refs/heads/7.x-3.x:/plugin... only it'd say onecol.yml instead of onecol.inc
[5:52pm] GaborHojtsy: well, using .yml files it not necessarily relying on CMI
[5:52pm] EclipseGc: GaborHojtsy: right, that's sort of my point
[5:53pm] GaborHojtsy: I don't know how the breakpoints stuff is planned to be leveraged, it is definitely not included in the patch, its just .yml files for now
[5:54pm] EclipseGc: GaborHojtsy: so are we ok with putting the yml file in the directory with the css, js, png, tpl?
[5:56pm] GaborHojtsy: EclipseGc: I think we can work that out on the issue, I don't care
[5:56pm] EclipseGc: k
[5:56pm] EclipseGc: GaborHojtsy: so… obviously you seem to be getting involved here, what can I do to support you?
[5:57pm] GaborHojtsy: EclipseGc: looks like we agree we would use .yml for metadata about layouts and have a Plugin that scans for those and can "execute them"
[5:57pm] EclipseGc: GaborHojtsy: I'm happy to write up and document a little bit of what I think the layout plugin code should look like
[5:57pm] GaborHojtsy: EclipseGc: that would be useful on the issue indeed
[5:57pm] GaborHojtsy: I was planning to pate in our discussion
[5:57pm] GaborHojtsy: EclipseGc: for good measure
[5:57pm] EclipseGc: GaborHojtsy: I'm not sure I should call that module layouts
[5:58pm] EclipseGc: GaborHojtsy: got a better suggestion?
[5:58pm] GaborHojtsy: well, it would be singular for core conventions
[5:58pm] GaborHojtsy: EclipseGc: it can be layout I think, I don't really have a better name
[5:58pm] EclipseGc: hmmm ok
[5:58pm] EclipseGc: GaborHojtsy: I'm off to write an interface
[5:58pm] EclipseGc: GaborHojtsy: ping me if you have any questions
[5:59pm] GaborHojtsy: EclipseGc: ok, thanks!

attiks’s picture

"I don't know how the breakpoints stuff is planned to be leveraged, it is definitely not included in the patch, its just .yml files for now"

Breakpoints can be declared by the theme, but it is the site builder decision if he wants to use them, and/or overwrite them. Think of the theme.breakpoints.yml as a suggestion.

Also keep in mind the breakpoints is just going to be an API module, that's isn't going to do anything without another module (like picture, layout, ...).

I think the layout story is different, but AFAIK using yml for the (meta)data makes sense.

EclipseGc’s picture

FileSize
13.15 KB

So... I spent a lot of time working on this today, it's not actually working yet, but one day is an awfully short timeframe to reproduce panels in, so I don't feel too bad ;-)

This utilizes a layout plugin type defined by the layout module. There is a DefaultLayout plugin class that defines a derivative class that goes looking through all modules and all themes for a particular directory structure (i.e. we should cache these results). The definitions that return are used to define available theme functions to drupal. The system needs to be complicated just slightly because I'd like for the various layout plugins (responsive, flexible, default) to have a specific directory that's unique to them, and this code doesn't do that yet, but it should be fairly trivial to do.

As I said this was just a first pass. I went ahead and converted panels onecol and twocol layouts. These look like good utility and good test fodder.

Future Steps:

I tried to dig into the ViewSubscriber to start doing what would need doing there. The onHtml() method needs to be checking the available access rulesets that a panels everywhere sort of approach would entail and deliver a relevant CMI object to represent it. There may be a better place higher up the chain to do this, but as a first pass onHtml() looks very tempting just to prove the basics of how this works... Unfortunately, we can't override that class right now because the mechanism that would support that is still in flux and the subscriber that is registered for that class can't be overridden (at least, as I understand the situation). So instead of being self contained, this patch would have had to start hacking that, and it's pretty late at night to start that nonsense, so... maybe tomorrow or something.

Hopefully this code begins to communicate the basics of how we can do this. I'm very open to changing stuff, but I wanted to at least get what I was thinking "out there".

Thanks for the feedback.

Eclipse

EclipseGc’s picture

oh, and the interface and class for Layout Plugins are still 100% in flux, and we need to create a Builder Interface that Layout Plugins can implement if they have "builder" components (like flexible and responsive). I'm sure there's more I've forgotten here, but this should be most of the highlights.

Eclipse

Gábor Hojtsy’s picture

Status: Active » Needs review

First, very good stuff! I intentionally ignored missing docs or whitespace issues, that is just a nature of a first patch :)

+++ b/core/modules/layout/layout.infoundefined
@@ -0,0 +1,5 @@
+name = Layout
+description = Provide abstract layout handling for html delivering routes.

Minor: This is pretty early, but we'll need to wordsmith a more user friendly description for this module, especially once it allows actual layout switching.

+++ b/core/modules/layout/layout.moduleundefined
@@ -0,0 +1,27 @@
+function layout_theme($existing, $type, $theme, $path) {
+  foreach (layout_manager()->getDefinitions() as $name => $layout) {
+    $variables = array('css_id' => NULL, 'renderer' => NULL);
+    foreach (array_keys($layout['regions']) as $key) {
+      $variables[$key] = NULL;
+    }
+    $theme[$layout['theme']] = array(
+      'variables' => $variables,
+      'path' => $layout['path'],
+      'template' => str_replace('_', '-', $layout['theme']),
+    );
+  }
+  return $theme;
+}

You are essentially proposing that all layouts would have a unique name and rendered via the theme system, so what happens now for theme('page') becomes a broker that decides which layout to have and render that with theme().

This sounds like a reasonable approach to me, although we'll need to figure out what happens to all the preprocess logic (all kinds of global varibales, class names, etc) that happen in http://api.drupal.org/api/drupal/core%21includes%21theme.inc/function/te...

+++ b/core/modules/layout/layout/onecol/layout--onecol.tpl.phpundefined
@@ -0,0 +1,19 @@
+<?php
+/**
+ * @file
+ * Template for a 3 column panel layout.
+ *
+ * This template provides a very simple "one column" panel display layout.
+ *
+ * Variables:
+ * - $id: An optional CSS id to use for the layout.
+ * - $content: An array of content, each item in the array is keyed to one
+ *   panel of the layout. This layout supports the following sections:
+ *   $content['middle']: The only panel in the layout.
+ */
+?>
+<div class="panel-display panel-1col clearfix" <?php if (!empty($css_id)) { print "id=\"$css_id\""; } ?>>
+  <div class="panel-panel panel-col">
+    <div><?php print $content['middle']; ?></div>
+  </div>
+</div>

I think the use of terminology here for "panel" is misleading. Would they just be better off just named layout or static layout?

Also I think these example layouts are great for now, but once/if a dynamic layout editor gets into core which makes these markup/CSS pairs reproducible with an editable interface (so you can actually adjust layouts to your needs instead of just picking from a fixed set), we'd be better off if we just ship we pre-built configs for the layout builder.

I think of this akin to having a node listing built in SQL vs. shipping a preconfigured View once Views is in core.

Of course that only makes sense *once/if* a layout builder UI is in core and until then we can only work with some prebuilt layouts to show the potential.

+++ b/core/modules/layout/layout/onecol/onecol.cssundefined
--- /dev/null
+++ b/core/modules/layout/layout/onecol/onecol.ymlundefined

+++ b/core/modules/layout/layout/onecol/onecol.ymlundefined
+++ b/core/modules/layout/layout/onecol/onecol.ymlundefined
@@ -0,0 +1,6 @@

@@ -0,0 +1,6 @@
+title: Single column
+category: Columns: 1
+icon: onecol.png
+css: onecol.css
+regions:
+  middle: 'Middle column'

The patch does not yet show how a theme would be affected. Eg. how would Bartik define its current single layout with the same system. Sounds like it would need a layout directory with one more directory where the template, CSS and .yml would be placed.

The way I envisioned this issue is that we'd add the metadata to the themes and introduce the plugin system. Until themes are converted, I don't see a way to swap out their layouts and it would be really powerful / important for themes to define layouts too the same way modules can, so they are swappable.

I know this likely just have not gotten to your attention yet, but it would really help get some themer attention in "hey, this is how this change affects you, now review".

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/LayoutInterface.phpundefined
@@ -0,0 +1,11 @@
+use Drupal\layout\Plugin\RendererInterface;
+
+interface LayoutInterface {
+  public function getRegions();
+
+  public function renderLayout(RendererInterface $renderer);

I have not found RendererInterface in my Drupal 8 checkout (or in this patch?). Did I miss something or is it being developed somewhere (which would be a dependency for this patch?).

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/layout/layout/DefaultLayout.phpundefined
@@ -0,0 +1,62 @@
+  public function getCss() {
+    $definition = $this->getDefinition();
+    drupal_add_css($definition['path'] . '/' . $definition['css']);
+  }
+
+  public function getAdminCss() {
+    $definition = $this->getDefinition();
+    $css = isset($definition['admin css']) ? $definition['admin css'] : $definition['css'];
+    drupal_add_css($definition['path'] . '/' . $css);
+  }
+
+  public function getJs() {
+    $definition = $this->getDefinition();
+    if (isset($definition['js']))  {
+      drupal_add_js($definition['path'] . '/' . $definition['js']);
+    }
+  }
+

Minor: it is strange these are named getXX() while they actually don't return anything, they affect Drupal at other places.

Status: Needs review » Needs work

The last submitted patch, layouts.diff, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

On a second look for renderer, you seem to take a RendererInterface in $renderer but then not using it at all, just resorting to theme() - which I think would be a great first stop with this patch to keep the scope under check :)

EclipseGc’s picture

yeah, so the layout plugins I actually stole from panels, and didn't update them at all, so my bad. This should already essentially work the same for themes. Just add a layout/my_layout dir with a yml file in it that looks similar to what I did in the module and it should get picked up, so converting theme's page.tpl.php files to this should be pretty easy. Granted, it still won't work, but for purposes of picking up the actual plugin, that part should be working.

The renderer stuff still needs to be expanded, and I have a pretty big comment in here in the actual rendering process of the layout that says we should abstract how blocks are rendered. I'm pretty sure that's actually something the renderer should do because it will need to do any sort of special casing for building ESI/Varnish urls instead of the actual block output, etc etc. Also things like the panels in place editor need the ability to provide a separate renderer so that they can render additional html/js/css to allow for different UI use cases.

In any event, I'll try to clean this up some in the next few days, but I have to address the issues with wscci that I previously mentioned.

Eclipse

effulgentsia’s picture

FileSize
9.64 KB
17.48 KB

Per #1787942-1: Allow assigning layouts to pages, this is what I have in mind as a way to:

- Integrate temporarily with drupal_render_page(), even though that will eventually go away once WSCCI/SCOTCH routing/subrequests are figured out properly.
- Decouple renderLayout() from theme().

This actually works (enable Layout module and all Bartik pages use the layout template and not page.tpl.php), though per bartik--bartik.tpl.php's @todo, logo, primary/secondary links, and everything else that isn't a block is not being rendered: per #1696302: [meta] Blocks/Layouts everywhere, those all need to be converted to blocks.

EclipseGc’s picture

FileSize
14.66 KB

So, I've taken Effulgentsia's patch and gone a bit further with it. We're not rendering what's in the page regions anymore, instead there's a hardcoded array that represents a CMI object and it has a list of regions and the relevant CMI names of blocks that should appear there. (This data in in the layout_get_layout() method and you should create some blocks by the same names if you'd like to see this working, which will require you to apply the newest patch from: #1535868: Convert all blocks into plugins and create appropriately named blocks).

Similarly, layout_get_layout() is just a stand in for the moment, and a getInstance() method on the layout_manager makes a lot of sense to me (effulgentsia's suggestion). The logic in that method should ultimately solve the negotiation of which CMI object represents a page and should satisfy (to some extent) #1787942: Allow assigning layouts to pages. I've not read that issue extensively yet, but they definitely overlap here.

Custom blocks theme function is not being called, I need to figure that out, but it's sort of a separate problem, so I've left it alone. Likely I'll fix it and put it in #1535868: Convert all blocks into plugins.

Thanks for the help on this people. Greatly appreciated.

Eclipse

PS: Much of this is intended to get the basic logic existing and working, not saying that the hooks we've invoked to do it are the right way to do this.

Status: Needs review » Needs work

The last submitted patch, layouts.diff, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
18.44 KB

#10 generates an error due to missing the Bartik layout, so here's a patch with that included (just copied from #9). No other changes.

This still generates a SQL error since it's now expecting blocks to be plugins, which as #10 mentions, requires #1535868: Convert all blocks into plugins.

Also, we should now be able to remove the $regions argument in renderLayout(), which per the todo was just a temporary stopgap.

Status: Needs review » Needs work

The last submitted patch, layouts-11.patch, failed testing.

Gábor Hojtsy’s picture

So with the latest patches this introduces two dependencies for the patch to be committed:

- convert all the blocks to plugins #1535868: Convert all blocks into plugins
- convert all the non-blocks to blocks (no issues even started yet)

I see hardly any chance these would be done in a couple weeks (out of 10 remaining until Drupal 8 feature freeze), and this foundational patch is *very* important to get going on editable layouts (which have all kinds of other foundations as well at least in breakpoints and regions). Can we please not target the best possible implementation for first with all the ideal dependencies done but instead just plug into what we already have in the page preprocess system, etc. and fed those things to the page layout? Obviously the bartik page layout would use it, but the rest would not use it yet since they are not blocks, but we can work on those things possibly even after Dec 1st, its just applying conversion to pieces to a technology we introduced, not a new feature. However this patch in itself is a pretty big feature, so I'd not make it depend on things we can easily do later.

I'm going to update the summary with the overview of the proposed solution from the patch (for layout declaration that is) and include a figure I made for this :) Then see if I can clean up some minor stuff at least with the patch.

Gábor Hojtsy’s picture

FileSize
58.92 KB
30.51 KB

Images for issue summary.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

FileSize
10.96 KB
19.69 KB

Rerolled patch with some minor changes:

- docs additions, several places lacked @file comments, method/function comments, etc.
- renamed getXYZ to addXYZ where all it does it calls drupal_add_xyz() - as suggested above
- moved the bartik layout; honestly bartik/layout/bartik/bartik--bartik.tpl.php was not too classy :) at least made it bartik/layout/page/bartik--page.tpl.php now, it makes the components cleaner
- did not include any function changes

Here are some review notes from an implementer perspective:

  1. It is very strange that css, js, etc. are defined in the .yml file but the template IS NOT. IMHO we should make the processor let the .yml provide the template and only fall back on the default pattern if not provided.
  2. Which leads to the next thinking that we can apply the same magic to the rest of the CSS/JS files (definable in .yml but if not, do some magic). Alternatively we can just skip the magic and require them to be always defined.
  3. The template name also has a weird syntax, repeating both the theme/module name and the layout name, eg. themes/X/layout/Y/X--Y.tpl.php; I wanted to change this to simple Y.tpl.php, but I understand you might be going for this for overridability in subthemes(?) - although this does not really work at all with subtheming since there is no logic built in for that yet (I think that could be a followup btw). Otherwise it just looks unnecessarily complex, it is already way deep in a directory structure.
  4. It is possible to define an admin CSS but not an admin JS (although I think that one is more likely needed). Is an admin CSS so common with static layouts or is more a property of dynamic layouts (which will have their own plugins anyway?). I'd go for parity or more likely remove admin CSS support unless demonstrably needed by static layouts.
  5. It is only possible to define one layout CSS/JS file for each layout. What if I want to componentize my CSS/JS? Should we make it support a list of files instead of one file only?
Gábor Hojtsy’s picture

Status: Needs work » Needs review
mherchel’s picture

Some [requested] general feedback

1. I love the logical separation of the of the different layouts into their own directories. I know similar things can be done currently, but the way this is looking makes sense to me.

2. I agree 100% with this quote below. In my general use cases, we'll have 2-3 layouts and our editors won't generally be creating more. I'd rather Drupal core be 'lean and mean' then having the overhead of unneeded modules.

[5:24pm] GaborHojtsy: EclipseGc: I think those who use a theme that has 3 layouts and they want to configure pages for those but never create any layouts outside of a .tpl.php should not need to use a module that has this baggage of a layout builder

Thanks,

ipwa’s picture

What would happen when you change layouts to a layout that doesn't have al the regions you have assigned blocks to? Would these blocks get disabled or assigned to a default region?

EclipseGc’s picture

That's under debate. I would prefer a mapping screen to pop up that let you move all blocks in one regions to another region in the new layout.

Gábor Hojtsy’s picture

Also, a layout switching Ui is *not* among the goals of this issue, see it is part of an issue treee of 3 issues with a parent metaissue. This one is about the layout definitions that modules and themes would use, and their integration in Drupal. Then we can build whatever Ui in another issue :)

EclipseGc’s picture

Agreed!

moshe weitzman’s picture

The title here says 'themes should declare their layouts'. I'll just pose a question that perhaps layouts are a top level "object" and don't belong to a given theme.

Lets please do this cleanly, even if it has dependencies. This is way too important to rush in half assed. As for Dec 1, Dries typically makes exceptions. I'd guess that this patch qualifies. Deep breaths.

Gábor Hojtsy’s picture

Moshe: as per the summary images, description and the code, we are making up a declaration mechanism for layouts that both themes and modules could use. I intended to have this issue to deal with themes implementing those only, but EclipseGC already has implementation for module provided ones too in the patch. So not sure if this was a comment on the issue title or the actual summary / patch :)

ipwa’s picture

Moving my question about block assignment to the appropriate issue: #1787956: Make blocks relate to layout instances instead of themes

I think this patch makes sense, as a themer I like the idea of declaring my layouts and have layouts in a separate directory. Having easier ways to set layouts on a path or context based way sounds great.

klausi’s picture

Side note: looks like Omega 4.x is going for a similar layout solution in Drupal 7. http://drupal.org/project/omega

Gábor Hojtsy’s picture

@klausi: any comments on this one then? Looking good?

attiks’s picture

@gabor, I started using this today, there's no UI (yet) but it's pretty easy to define layouts:

  1. it has an 'info' file mylayout.layout that defines the regions, css and js
  2. it has a mylayout.tpl.php that replaces the page.tpl.php

As far as I can see you cannot nest layouts inside layouts, but if I'll learn more I'll let you know.

Related issue #1744592: [META] Omega 4.x Documentation

dasjo’s picture

Gábor Hojtsy’s picture

I'd love if you guys would comment on how this compares and whether you like this solution here or see any missing pieces. We have essentially 66 days left until December 1st, and this is a foundational issue to get any layout building tools in core. If we don't want any layout building / swappable layout UI in core, we can debate this until the end. If we want a UI to build layouts, we need to crank through this *much* faster. Guys, tell us if you hate this and want theme-baked in layouts and no user facing features at all or you want this to go ahead or anything inbetween.

fubhy’s picture

I love this issue and will comment more on on it as soon as I find the time.

How would nesting of layouts work?

One thing that I am currently unsure about are layout-specific sub-template overrides. E.g. a custom block template for a specific layout (something like bartik--mylayout--block.tpl.php). This is something that I didn't put into Omega 4.x yet either. I can think of quite a few use-cases where that would make sense though.

I always separate my theme CSS into two major parts: Structural "layout" CSS (The CSS that builds and describes the page structure) and, on the other hand, the "Painting" of that layout. So optimally, we would end up with the "painting" CSS and assets in the theme 'root' and the structural / layout CSS in each separate layout.

Regarding a Layout-Builder-UI:
I am very open for the idea of having an (optional) layout builder in Core. It would have to be in a separate module, of course. However, I am not going to say that something like that should be #1 priority. This is something that could be done in contrib, too. So I am absolutely neutral when it comes to this decision.

EclipseGc’s picture

I'm neck deep in making block visibility rules work for #1535868: Convert all blocks into plugins right now, but I'm getting close to having that working at which point I'll come back to do this and show how layouts can be nested. I think it will be pretty trivial (I hope).

Eclipse

adubovskoy’s picture

Status: Needs work » Needs review
Issue tags: -Blocks-Layouts, -Spark

[deleted]

Stalski’s picture

I personally like the approach very much. It's a good thing themes and modules can provide layouts.

Two remarks/questions:
- will the notion of "panels" be replaced with "layout" for CSS classes?
- The DefaultLayout executes the "block_load" function. Shouldn't this be a class extending on DefaultLayout, like BlocksLayout or DefaultBlocksLayout? I am thinking of for instance fields that are rendered in a layout as well after this gets in ... and then they are not blocks but fields to populate the regions. It might be easier to have the content loaded just before that so it only needs to be assigned to the regions.

I know the last remark is a bit of topic, but on the other hand we don't want to close doors for something so important as this to go in.

In general: awesome!

swentel’s picture

Status: Needs review » Needs work
+++ b/core/modules/layout/layout.infoundefined
@@ -0,0 +1,5 @@
+description = Makes it possible to swap different page layouts.

A node rendered as a teaser has a template/layout file too, default to one region, but can easily have a more complex structure. Is this only going to focus on page level layouts in core ? That seems to be a missing piece for me - a fairly critical one imo. Other than that, I like the patch already.

This would also be great for #1510532: [META] Implement the new create content page design.

swentel’s picture

Status: Needs work » Needs review

Woops, didn't mean to change the status

swentel’s picture

To give you some more ideas and also the power of node template switching (in the end entity layouts in general), a simple screenshot from Display Suite which adds a template selector on Field UI (in 7.x-2.x and 8.x branch). That's something we can easily expose on there, even in Field UI's current state which will probably be impossible to rewrite to a more visual interface. I don't see a way

The same layout/template selector could also be applied to forms as well and would benefit the content create page issue.

We have some issues re: field UI to make gradual small changes, including #1792600: Refactor field_ui so common behavior for fields and display overview screens are extracted (making both manage fields and display consistent) and also #1786198: Make consistent regions in code for fields UI overview screens. The latter is currently on hold because entity objects don't have the notion of 'regions', but with this patch in we could do that very easily. While Field UI in core would still be not the best interface, we would have the option though to have swappable layouts easily with this.

Screen Shot 2012-09-26 at 23.46.07.png
Note: ignore the custom wrappers/classes/fields, it's not my intention to have those there in core of course :)

I'll leave it to this. In case you feel this is hijacking the issue, feel free to uncomment though, but I feel there's many possibilities here if we get the API right.

Gábor Hojtsy’s picture

@Stalski, @swentel: indeed, the current patch focuses on page layouts only. I've heard EclipseGC has ideas to make it nest, so then it can be applied to others but pages. That would need to come with a whole reimagining of how things are applied to pieces of layouts, with blocks possibly being only one piece (unless fields, etc. are wrapped as "blocks" too). I don't necessarily see how we would get there and build any useful thing on top of this in the remaining time, but I'm ready for surprises / ideas :) Otherwise are you saying this does not make sense with this scope and you'd rather not have even replaceable page layouts, so we should just keep it as in D7 and modules will hack around it?!

swentel’s picture

Otherwise are you saying this does not make sense with this scope and you'd rather not have even replaceable page layouts, so we should just keep it as in D7 and modules will hack around it?!

Oh no, absolutely not! Even if core only has the ability to swap page layouts, that would be a fantastic feature. It just makes sense in our heads to apply it to everything - well, I guess everyone wants that, but it's just that we have a lot of experience on the entity level.

We have an old issue in Field API thinking about having an 'display object' - see #367498: Introduce 'display' as a way to group and reuse instance and formatter settings. - besides storing formatter settings, it can also store a layout/template file which would make a lot of sense. I guess in the end we should try and see if can come up with a proof of concept replacing theme('node') using renderLayout() in let's say an EntityLayout class. I'll think a bit with stalski and zuuperman the next couple of days at work to see if we're ready for that :)

webchick’s picture

Issue tags: +Spark

Tagging with "Spark."

Stalski’s picture

@Gábor Hojtsy: No on the contrary. I think this patch has lots and lots of potential. I am totally interested in this going in very fast, same as you. It's exactly the swappable layout we need. As swentel said, the UI's can have their implementation later or in contrib. The most essential thing is that there are layouts defined by modules and themes, and making it possible to have a nesting layout. This could not be very hard imo.

I have a litlle theming experience and I think we would need to make it possible to have default classes for one type of layout rendering. Meaning this issue probably should make use a kind of prefix "page-layout" while child layouts can use layout of whatever (in stead of the current "panels-" classes. This is just an idea. I did not see something like that in this patch yet, or am I wrong?

As you already said in this issue, we really need some people with theming experience to review this, especially html, css, js. I'll have some theming people of our company look at this. Today :)

So imo, the only thing that would need to change in this issue, is making it possible to assign $content to regions and not need to load the particular piece of content. Then if this gets in, we'll have much more backup on the issues #1792600: Refactor field_ui so common behavior for fields and display overview screens are extracted and #1786198: Make consistent regions in code for fields UI overview screens. Especially the last one.
Short explanation on those two issues:
1) #1792600: Refactor field_ui so common behavior for fields and display overview screens are extracted: just add a little consistency through OOP inheritance
2) #1786198: Make consistent regions in code for fields UI overview screens: work this consistency further so forms and view mode follow the same pattern, and make it possible to assign $content to regions.

I can help on this issue if my propositions are valid. Let me know :)

seutje’s picture

Status: Needs review » Needs work
+++ b/core/modules/layout/layout.moduleundefined
@@ -0,0 +1,87 @@
+/**
+ * Temporary code to make Bartik work.
+ *
+ * @todo
+ *   Make this work generally.
+ */

so this bit is suppose to be lifted from the yml, right?

+++ b/core/modules/layout/layout/onecol/onecol.cssundefined
@@ -0,0 +1,22 @@
+
+#panels-edit-display .panel-pane,
+#panels-edit-display .helperclass {
+  margin: .5em;

I'm a bit confused as to why this is in there, is it leftover copy/paste stuff or am I missing something?

seutje’s picture

Status: Needs work » Needs review

silly Dreditor...

Gábor Hojtsy’s picture

@seutje: Yes, the panels classing is temporary, its a D7 copy-paste to prove the patch works. See @Stalski.

@Stalski: Indeed, looks like if we can let people namespace their class names (so if I nest a 2 column layout in a 1 column layout and then nest another 2 column layout in one of the columns, then I don't get repeated wrapper names) in the layouts and provide arbitrary content for the regions in rendering, we might have achieved nesting as a theoretic capability. I'm not sure it is easy to do on a UI in some way for the D8 release though. I don't know how this compares to EclipseGC's ideas, but it sounds pretty reasonable to me. I raised some concerns from a themer perspective in #16, it would be great to validate those with real themers and act on them if they are real. It would be great to have some consistency here.

Stalski’s picture

@Gábor Hojtsy: Totally hear you, great. I am a big fan of this issue since it finally opens doors.

I'm not sure it is easy to do on a UI in some way for the D8 release though

Indeed, I always imagined this wasn't even possible (without overlays or in separate pages). This a bit like the holy grail imo, problems to solve later I persume.

I'll also quote what you said, so it's easier to answer the remaining questions:

Here are some review notes from an implementer perspective:

  1. It is very strange that css, js, etc. are defined in the .yml file but the template IS NOT. IMHO we should make the processor let the .yml provide the template and only fall back on the default pattern if not provided.
  2. Which leads to the next thinking that we can apply the same magic to the rest of the CSS/JS files (definable in .yml but if not, do some magic). Alternatively we can just skip the magic and require them to be always defined.
  3. The template name also has a weird syntax, repeating both the theme/module name and the layout name, eg. themes/X/layout/Y/X--Y.tpl.php; I wanted to change this to simple Y.tpl.php, but I understand you might be going for this for overridability in subthemes(?) - although this does not really work at all with subtheming since there is no logic built in for that yet (I think that could be a followup btw). Otherwise it just looks unnecessarily complex, it is already way deep in a directory structure.
  4. It is possible to define an admin CSS but not an admin JS (although I think that one is more likely needed). Is an admin CSS so common with static layouts or is more a property of dynamic layouts (which will have their own plugins anyway?). I'd go for parity or more likely remove admin CSS support unless demonstrably needed by static layouts.
  5. It is only possible to define one layout CSS/JS file for each layout. What if I want to componentize my CSS/JS? Should we make it support a list of files instead of one file only?
catch’s picture

I haven't reviewed the patch properly yet, so not commenting on the new design.

A few things perplexed me reading through though, note I've not read the whole issue either:

1. Why this is a new module? It looks like it'll be absolutely required to do any rendering of layouts, so why not baking it into the render API/theme seystem? And apart from the hooks it implements it doesn't seem to be using anything of the actual module system (no router items, no schema etc.).

2. The drupal_add_css() calls make we wince, we need to mark that function deprecated in Drupal 8. Seems like it ought to be possible to collect the information then use #attached instead.

3.There's no code removed here, at all. What about regions[] in .info files and the supporting code?

Gábor Hojtsy’s picture

@catch: I was indeed thinking hard at the start about whether this should be a new module or just in system module altogether :) It will have a UI soon, but not as part of this patch. Once themes provide multiple layouts, we have sister issues (look at the parent META issue linked above in the summary) that would introduce a UI to switch between them. We want to move step by step instead of trying to do everything at once and failing at it. Is the best thinking that we should dump new UIs and plugins in system module then? I think we are moving away from that instead with establishing new modules like actions, ban, etc.

I agree #attached would be much better, again we assumed there are parallel efforts in the rendering/twig area that would stump on us anyway and we want to move faster. If drupal_add_css is destined to be removed anyway, we can figure this one out later if needed.

As for code removed, indeed the page.tpl.php of bartik and the regions in .info files, etc. should be removed. Only bartik is barely converted, and there are holes in the rendering implementation so if we'd just remove the page.tpl.php and .info file regions, it would collapse and fail various tests (eg. block assignment stuff). We DO NOT want to do everything in this patch, eg. also make blocks relate to layouts, so it looks like we need to accept this is an interim step and we can remove other code later once more steps are complete.

I think the scope of this issue is to argue the format of the layout declaration itself (with prime questions for themers especially) and the plugin setup for layouts. Those two in themselves are pretty big and if we'd want to lump in redoing block assignment to relate to layouts instead of theme-global-regions and providing a whole UI, this would not be just way too big in scope. We are trying to parallelize work here, eg. blocks and plugins is redoing how we refer to blocks, etc., so trying to not go there here makes ton of sense.

What do you think?

frankbaele’s picture

Hi, i support this patch, it makes a lot of sense to me as a themer.

Here are my thoughts on what I have read so far.

  1. For the template naming it makes more sense to me if it was just themes/X/layout/Y/Y.tpl.php. Looking at the layouts as a more modular entities that are reused in multiple themes. The ideal situation would be as themer that i have a library with layouts where i can copy from for every new theme without renaming any of them.
  2. For the name convention it would be easier if you had something like Y.css Y.js Y.tpl, again just keeping it simple. It would not be interesting if you had the ability to defined extra files in the yml file because the need for that would be marginal. From my perspective if you have multiple structural css files, there is something wrong with how you define structural. The layout should not contain too much css or js because they should only deliver structural functionality.
  3. There is no problem with componentization of css because you can always include other css files in your Y.css and build a componentized structure beneath that. Or if you use sass/less too compile your css you can limit your output file to 1
sannejn’s picture

  1. I agree with the template naming: i think it should be the same name as the folder. The ability to be able to rename this file will only make things more complicated.
  2. In case of CSS/JS files, i think there should be some more flexibility in case you want to split up your layout-styling in different files.
  3. About the template naming: the themes/X/layouts/Y/Y.tpl.php will do just fine i think. I don’t see any reason why you should create longer filenames if you can just as well use the short ones.
_Cedric_’s picture

As a front-end developer, I fully agree with sannejanssen:

@frankbaele I see no advantage in allowing only 1 css file

ipwa’s picture

@catch: This is a bit off-topic but why remove drupal_add_css, is there an issue already about this? I'm just curious because I use it in some of my modules.

I agree with @sannejanssen about naming conventions.

@Gábor Hojtsy: I would also like to see this move quickly, what can I do to help besides giving my opinion here and trying to get more community members to see and comment on the issue?

EclipseGc’s picture

Understand, with regard to css, there is no intention of any styling being provided by layouts, layout should provide layout css, themes should provide styling. Layouts can be provided by themes or modules, any sort of block styling would be handled in other ways. A layout's css is explicitly for the purposes of laying out the divs in the associated tpl on screen, and nothing more.

Eclipse

sannejn’s picture

A layout's css is explicitly for the purposes of laying out the divs in the associated tpl on screen, and nothing more.

@EclipseGc, I totally agree about this. But how do we handle this in case you want to split up, for example, your responsive behaviour in separate files?
Like @frankbaele mentioned, if you are using SASS or some kind, that might not be a problem, but what if you're not?

EclipseGc’s picture

It's probably worth asking Gabor how the existing Responsive layout for panels handels this.

dcrocks’s picture

I already maintain the separation between layout and content styling in my theme except I have a 'regions.css' file and besides layout I include region specific styling in it(backgrounds, defaults, etc). I'm sure this is not an unusual practice. It makes it easier for me to organize. But what is the specific advantage to drupal by adopting this? It does add additional complexity to give me yet another tool to manage presentation dynamically though the benefit to simple themes is not apparent. So does it provide some benefit to the way drupal itself delivers web sites.

Wim Leers’s picture

#51: #attached is better because it'll ensure that the CSS is only loaded if the HTML it is necessary for also ends up on the page. It allows for easier altering. Etc. Furthermore, drupal_add_css() will be pretty much deprecated in favor of drupal_add_library() + hook_library_info(), so that it is possible to define e.g. a dependency on a certain library (which may consist of a single CSS file, or multiple ones, or also JS, or …).

catch’s picture

@Gabor:
I didn't say put new UIs and plugins in system module. I said why not put the render/theme stuff into the render/theme system. If we add a layout UI than that should be a separate module, but I don't see why we'd have a required module just to render a static layout when we have an entire core subsystem dedicated to that task already.

There's no such effort for #attached with twig, or if there is it's a secret, so let's not regress things here. A recent issue for drupal_add_* pitfalls is #1737148: Explicitly declare all JS dependencies, don't use drupal_add_js.

As for code removed, indeed the page.tpl.php of bartik and the regions in .info files, etc. should be removed. Only bartik is barely converted, and there are holes in the rendering implementation so if we'd just remove the page.tpl.php and .info file regions, it would collapse and fail various tests (eg. block assignment stuff). We DO NOT want to do everything in this patch, eg. also make blocks relate to layouts, so it looks like we need to accept this is an interim step and we can remove other code later once more steps are complete.

If this is going to be so partial, then we should not convert anything in Bartik then, rather than doing a half-conversion that won't actually work. There's no sign of the blocks as plugins patch being anywhere near committable yet and that's presumably working with the old region system at the moment rather than built on this. So since we're extremely close to feature freeze I'd like to know what the follow-up steps are and have issues open discussing those before this gets in - since there's no real documentation of any of these in the patch or this issue which makes it impossible to tell how much work we're saving up for ourselves later.

Gábor Hojtsy’s picture

But what is the specific advantage to drupal by adopting this? It does add additional complexity to give me yet another tool to manage presentation dynamically though the benefit to simple themes is not apparent. So does it provide some benefit to the way drupal itself delivers web sites.

The specific we are looking at here is that instead of the hard-wired theme definitions of which page templates applies where (page-node.tpl.php, page-node-1.tpl.php, page-contact.tpl.php, etc), we would optionally let the user assign pages based on conditions. Depending on how far we get, the conditions might be as versatile as we have for block visibility now or only limited to path patterns. So we'd provide more user benefit by making layout assignment more flexible, not requiring fiddling with obscure template file naming. Of course if you want to lock down a setup on your site, that would all be possible.

That is the immediate benefit. Then the next one is not only themes but modules could provide layouts given the decoupled format, which would let modules like Panels be implemented cleanly and still integrate with your theme. Of course if you *don't want* this flexibility, again, you will be able to do a theme that does not integrate with Panels or layout replacement tools. I think the generic themes would adapt, project specific themes would go as required by the project in question.

The scope of this issue is just to let the layouts be defined and we'd build the UIs and page assignment mechanism later. This is just a technique to get things done vs. arguing the whole picture altogether and not getting anything done. The issue summary had a META issue linked from the start that includes other related issues and should give a better idea of the bigger picture: #1787634: [META] Decouple layouts from themes

EclipseGc’s picture

FileSize
30.58 KB

OK, spent some time on this since my last patch and wanted to provide what I had going on here.

Cleaned up the architecture a bit, removed the layout_get_layout() function in favor of a Plugin Manager Mapper getInstance method. The method provides a hook for any module to respond to the current request, and then if it gets no responses it falls back to the current theme's block configuration as setup in the typical blocks UI. (this is a bit of a blending of approaches for the sake of having a UI that drives layouts today. We're not finding an actual CMI file that represents the layout but are rather generating an array that could be used as such from the CMI files that represent the various block configurations).

In addition to all of this, I've converted most page elements (except for page title and tabs) to blocks in the layout module, and in addition to all of this, provided a layout block (no UI, so you can't use it through the UI yet, but programmatically it works).

Hopefully this is a good start for demoing how we can get where we want to go. A lot of stuff to do still, but I wanted to get this patch up. Will detail specifics later.

Eclipse

EclipseGc’s picture

I need to produce a corresponding blocks patch over in #1535868: Convert all blocks into plugins and will probably spend the bulk of my day getting that working.

Eclipse

EclipseGc’s picture

FileSize
31.14 KB

missed the hook_block_alter in template.php

Eclipse

Wim Leers’s picture

Any chance you could post a #16 -> #61 interdiff? That'd help tremendously in reviewing this :)

dcrocks’s picture

@hotjsy I had interpreted this as in addition to the existing capabilities of page tpl's but are you saying this is instead of that functionality?

effulgentsia’s picture

Re #63, yes, the eventual goal of #1696302: [meta] Blocks/Layouts everywhere is to completely remove the concept of theme('page'). Instead we want to have a concept of layouts, which modules can provide, themes can provide, or a UI tool (like Panel's flexible layout builder or Spark's responsive layout builder) can generate, and consistent configuration (CMI) objects driving which layout to use on which page, and which blocks appear in which regions on those pages, regardless of whether the layout comes from a module, a theme, or is UI generated. Doing this will help fix the current difficulties that site developers have with getting modules like Panels and Context to mesh well with each other and with Drupal core. It will also make it easier to build Drupal distributions that can work more easily with different themes.

However, there are still other big issues that need resolving before we can reach that complete goal. Because we're not there yet, this issue doesn't remove theme('page') from core entirely, it just bypasses it when layout.module is in use.

EclipseGc’s picture

@dcrocks

Yes this is instead of page.tpl.php. The idea being that we want this stuff to be reusable from the start, and not be hardcoded with irrelevant HTML that others will have to eventually undo.

@Wim Leers

I think I missed 16, I'll try to look at it tomorrow or tonight and make heads or tails of the differences.

Eclipse

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/layout/layout/onecol/layout--onecol.tpl.phpundefined
@@ -0,0 +1,19 @@
+/**
+ * @file
+ * Template for a 3 column panel layout.
+ *
+ * This template provides a very simple "one column" panel display layout.
+ *
+ * Variables:
+ * - $id: An optional CSS id to use for the layout.
+ * - $content: An array of content, each item in the array is keyed to one
+ *   panel of the layout. This layout supports the following sections:
+ *   $content['middle']: The only panel in the layout.
+ */

+++ b/core/modules/layout/layout/twocol/twocol.cssundefined
@@ -0,0 +1,37 @@
+.panel-2col { ¶
+/*  overflow: hidden;  */
+}
+
+.panel-2col .panel-col-first { ¶
+  float: left; ¶
+  width: 50%; ¶
+}
+* html .panel-2col .panel-col-first {
+  width: 49.9%;
+}
+
+.panel-2col .panel-col-first .inside { ¶
+  margin: 0 .5em 1em 0;
+}
+
+.panel-2col .panel-col-last { ¶
+  float: left; ¶

You clearly did not take changes I made here and elsewhere fixing docs, whitespace, etc. :/ Oversight? I'm not comfortable in posting updated patches if I'm seeing my changes are not taken in the following patches. :/

+++ b/core/modules/layout/lib/Drupal/layout/LayoutBundle.phpundefined
@@ -0,0 +1,24 @@
+
+/**
+ * @file
+ * Definition of Drupal\layout\LayoutBundle.
+ */
+
+namespace Drupal\Layout;

namespace Drupal\layout (lowercase), no?

+++ b/core/modules/layout/lib/Drupal/layout/LayoutBundle.phpundefined
@@ -0,0 +1,24 @@
+  }
+}
\ No newline at end of file

I think I also fixed the missing newline?!

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/LayoutMapper.phpundefined
@@ -0,0 +1,60 @@
+<?php
+
+namespace Drupal\layout\Plugin;
+
+use Drupal\Component\Plugin\Mapper\MapperInterface;
+use Symfony\Component\HttpFoundation\Request;
+
+class LayoutMapper implements MapperInterface {
+  protected $manager;
+
+  public function __construct($manager) {
+    $this->manager = $manager;
+  }
+
+  /**
+   * Implements MapperInterface::getInstance().
+   */
+  public function getInstance(array $options) {
...
+
+  function sort($a, $b) {

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Type/LayoutManager.phpundefined
@@ -0,0 +1,21 @@
+class LayoutManager extends PluginManagerBase {
+  protected $defaults = array(
+    'class' => 'Drupal\layout\Plugin\layout\layout\DefaultLayout'
+  );
+
+  public function __construct() {

Lacks docs on what is this all for :)

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/LayoutMapper.phpundefined
@@ -0,0 +1,60 @@
+}
\ No newline at end of file

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Type/LayoutManager.phpundefined
@@ -0,0 +1,21 @@
+}
\ No newline at end of file

Minor: file ends should have newlines. I fixed such issues above to not bother you but these are new things.

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/block/block/LayoutBlock.phpundefined
@@ -0,0 +1,31 @@
+    $layout = layout_manager()->createInstance('default_layout:layout__twocol', $this->configuration['layout']);

Uhm, is hardwiring the two column layout a suggestion for this patch pass for the committable version or just an interim step. Hard to see here.

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/block/block/PageSiteName.phpundefined
@@ -0,0 +1,30 @@
+   * Implements BlockInterface::build().
+   */
+  public function build() {
+    return array(
+      '#children' => filter_xss_admin(config('system.site')->get('name')),
+    );

While it is great seeing these converted to blocks, this adds yet more tasks to the list to complete: put init code in so those blocks are placed proper in themes, write upgrade path, and so on. I still fail to see how is this the easy way to go compared to just using blocks.

You also don't actually remove any code so it does not demonstrate how this affects themes or modules at all.

+++ b/core/themes/bartik/lib/Drupal/bartik/blocks/SecondaryLinks.phpundefined
@@ -0,0 +1,30 @@
+<?php
+
+namespace Drupal\bartik\blocks;
+
+use Drupal\layout\Plugin\block\block\PageSecondaryLinks;
+
+class SecondaryLinks extends PageSecondaryLinks {
+
+  /**
+   * Implements BlockInterface::build().
+   */
+  public function build() {
+    return array(
+      '#prefix' => '<div id="secondary-menu" class="navigation">',
+      '#suffix' => '</div>',
+      '#children' => theme('links__system_secondary_menu', array(
+        'links' => menu_secondary_menu(),
+        'attributes' => array(
+          'id' => 'secondary-menu-links',
+          'class' => array('links', 'inline', 'clearfix'),
+        ),
+        'heading' => array(
+          'text' => t('Secondary menu'),
+          'level' => 'h2',
+          'class' => array('element-invisible'),
+        ),
+      )),
+    );
+  }
+}

This is a big "OMG, now this is the way I'd override my secondary links in a theme" moment, and I can see only this one being debated to hell, despite being a nice-to-have in the patch AFAIS, right?

EclipseGc’s picture

You clearly did not take changes I made here and elsewhere fixing docs, whitespace, etc. :/ Oversight? I'm not comfortable in posting updated patches if I'm seeing my changes are not taken in the following patches. :/

Yes, apologies, I missed your patch, my next one will comb through everything you did and attempt to pull it all in. Again, I totally apologize.

Uhm, is hardwiring the two column layout a suggestion for this patch pass for the committable version or just an interim step. Hard to see here.

Since no UI exists for block placement in layouts, I hardcoded so that people could see how this works. I also think I removed the example of how it works, so shame on me.

While it is great seeing these converted to blocks, this adds yet more tasks to the list to complete: put init code in so those blocks are placed proper in themes, write upgrade path, and so on. I still fail to see how is this the easy way to go compared to just using blocks.

You also don't actually remove any code so it does not demonstrate how this affects themes or modules at all.

Ok, I'll see if I can fix that in my next patch.

This is a big "OMG, now this is the way I'd override my secondary links in a theme" moment, and I can see only this one being debated to hell, despite being a nice-to-have in the patch AFAIS, right?

If a layout plugin is nothing but pure layout html, then we'll need a way to customize block output, especially for things like navigation. This doesn't prevent theme function overrides or anything else we're used to, you just can't (shouldn't) add things like menu code inline in the layout's output, so another method was required. This is also ridiculously nice since you could switch off of a links based theme function completely, and supply your own variables, etc. It's a lot more effective and straight forward than a preprocess function imo and a whole lot cleaner.

I can understand some amount of initial recoil here, but I think the approach is sound and simple enough for anyone to ultimately grasp, and should that prove incorrect I could totally see a module like ctools bulk export that supplies this code for you and you just fill in the build function appropriately. (in contrib obviously)

Eclipse

Gábor Hojtsy’s picture

This is also ridiculously nice since you could switch off of a links based theme function completely, and supply your own variables, etc. It's a lot more effective and straight forward than a preprocess function imo and a whole lot cleaner.

I can understand some amount of initial recoil here, but I think the approach is sound and simple enough for anyone to ultimately grasp, and should that prove incorrect I could totally see a module like ctools bulk export that supplies this code for you and you just fill in the build function appropriately. (in contrib obviously)

Honestly I care less about this being nice or not vs. trying to keep this issue on focus instead of dwindling into all new areas of everything. I think the layout introduction in itself is a big enough leap for themes, introducing PSR-0 based block plugin overrides at the same time looks like explodes the scope of this patch way father than what I see would be a feasible focus.

EclipseGc’s picture

Actually, I think the PSR-0 block overrides will ultimately end up in the #1535868: Convert all blocks into plugins issue, so consider it disconnected from this.

Eclipse

Gábor Hojtsy’s picture

FileSize
32.57 KB

Reinstantiated my changes from #16 that were not taken and renamed the classes in the "panels" layouts to "layout-" etc. as it made sense. Did not cut out the excessive unrelated changes yet.

Gábor Hojtsy’s picture

FileSize
21.17 KB

Removed unrelated block plugins (all of them) as well as the PSR-0 class override from bartik. These are heavy duty changes that (a) have no relation to intoducing layouts and (b) have their own issue at #1535868: Convert all blocks into plugins to be discussed.

Gábor Hojtsy’s picture

FileSize
22.03 KB

- Removed the magic file naming thing and added template as an explicit .yml key in the layout definition. This resolves complaints from above that the template file names look odd. I think the fully qualified name in itself makes sense for overriding in themes (although I don't think we want to promote this practice with template files?), but it should be consistent with the rest of the assets listed in the .yml.

- Made it possible to add admin JS much like admin CSS, to mirror the capabilities. Currently falls back on frontend JS if that was provided. Might be up to debate.

- Made it possible to provide multiple CSS/JS files if wanted/needed instead of one and only merged file. This resolves complaints from above too.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
8.63 KB
23.27 KB

Yet another round of docs updates, fixes, etc. Removing some dead code and comments around $regions in renderLayout().

Gábor Hojtsy’s picture

FileSize
21.69 KB

Cut back the layout mapper and the block-region rendering code since we don't have blocks as plugins and even if we'd have that, we don't have layouts to blocks relations anyway. So this is no-op code for now anyway (or would need to be stubbed). Given that we cannot propose this as a right-away complete replacement, we need it to land as a new parallel system and then port blocks to relate to layouts.

Moved the theme page and yml to a test theme and created a test module to be used to do test coverage.

This now restores the patch to its original goal of defining a layout format for themes/modules and being able to grab one of those layouts and render it. Their relation to blocks is not possible to solve here given it needs excessive changes in other subsystems (and blocks are being reworked in the meantime), so no existing theme or module is touched.

I think we should proceed with test coverage for this subsystem, and the test module and theme is a small start for that. We can move the classes around to other modules and/or base components as needed in the meantime as people prefer.

I think/hope this helps focus the work on this issue and bring it to completion much faster barring any cross-dependency with anything else. Its a huge step in itself, so let's get this right and focus on the task.

Now stopping for actual reviews for a while.

Status: Needs review » Needs work

The last submitted patch, layout-74.patch, failed testing.

sdboyer’s picture

Status: Needs work » Needs review

sorry i'm getting in on this so late. i am now working on the controllers that tie this to the new routing system (read: that make this REAL), and am shooting for a first patch by the end of the weekend.

i'm glad to see this moving forward, and it does seem to have the basic elements in place that we really need. we need to have a little more discussion around the layout swapping and initial selection, though, as well as the possibility of things like defaults/suggested layouts to use when no more specific layout is provided. it might also benefit us to define some common scenarios for which people build specific layouts - e.g., landing pages, the admin area, etc., as that will help define the scenarios for which to build layouts and how failover logic should maybe work. in that vein, once we're further along, it would be good to discuss the recommended approaches for building layouts in different types of scenarios - for module devs, for theme devs, for site builders.

some notes on what i've seen in reading through the issue + comments.

from #35

A node rendered as a teaser has a template/layout file too, default to one region, but can easily have a more complex structure. Is this only going to focus on page level layouts in core ? That seems to be a missing piece for me - a fairly critical one imo. Other than that, I like the patch already.

the discussions we had in munich suggested that while we could probably reuse the 'layouts' system for doing things at the sub-page level, the first and foremost orientation of the system is towards page-level operations. my gut says the best approach is to let layout plugins self-identify as being page-level or not, so that we can reuse the same basic plugin architecture and many of the visual config tools for both, but sub-page-level layouts aren't burdened with doing things like providing hidden/system regions. and if we introduce such a flag now, it'll make it easy for contrib to overlay such sub-page stuff if we don't get to it in core.

I'm not sure it is easy to do on a UI in some way for the D8 release though.

With respect to UIs for this whole system, in my mind, our best approach is to have a simple, pluggable core implementation, with the full expectation that contrib will play out better ones. there are lots of different things that comprise "the UI", though - at some point we should break that down and prioritize.

I see hardly any chance these would be done in a couple weeks (out of 10 remaining until Drupal 8 feature freeze), and this foundational patch is *very* important to get going on editable layouts (which have all kinds of other foundations as well at least in breakpoints and regions).

My understanding of the "exceptions" Dries outlined is that they extend to things like the block conversions you're describing, so long as we get the best possible base implementations in place by Dec 1.

Gábor Hojtsy’s picture

@sdboyer: as for layout swapping and default/fallback layouts, etc. the actual layout selection config would belong to other issues in the META family: #1787634: [META] Decouple layouts from themes, not this one. This one is about defining the format and making it possible to render a layout with some dummy content. Then the actual content and the layouts to pages relations happen in other places. This in itself has enough discussion about what should be in the .yml, what should be the file name patterns, whether we should use #attached, and so on. Let's focus on this one. Your help is needed on resolving the rendering part (tying in to the routing system), which is indeed implemented as somewhat of a hack now. Just assume we specifically ask for layouts through an API at this point and we'll do layout assignment to conditions/paths/pages later.

Gábor Hojtsy’s picture

Interestingly the enable/disable module tests (that fail on the latest patch) pass all properly on my machine. Hm. Going to send for retest.

Gábor Hojtsy’s picture

Issue tags: -Dynamic layouts, -Spark

#74: layout-74.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Dynamic layouts, +Spark

The last submitted patch, layout-74.patch, failed testing.

Gábor Hojtsy’s picture

The module enable/disable tests still fail definitely at the point where it reaches layout module (and I cannot reproduce this on my machine), so some help here would be appreciated. Anybody can reproduce this?

andypost’s picture

@Gabor this test fails because it's broken:
Fatal error: Call to a member function getInstance() on a non-object in /var/www/core8/core/lib/Drupal/Component/Plugin/PluginManagerBase.php on line 79

function layout_page_build(&$page) {
  $request = drupal_container()->get('request');
  $options = array(
    'request' => $request,
  );
  $layout = layout_manager()->getInstance($options);

// PluginManagerBase
  public function getInstance(array $options) {
    return $this->mapper->getInstance($options);
  }

but the mapper is NULL

EDIT: suppose you need to call createInstance('some_layout_plugin', $options)
Maybe you missed to add some files in patch

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.31 KB
22.64 KB

Per #83, this brings back the mapper that was removed from #74, but a stripped down one.

Gábor Hojtsy’s picture

FileSize
22.77 KB
1.7 KB

Thanks! I agree that it makes sense to have a simple mapper for now for testing.

Continuing on that, adding a little bit of docs to LayoutMapper and fixing file names in the other two .yml files.

Gábor Hojtsy’s picture

What I'm looking at next is adding tests:

Derivatives:
- whether derivatives are found (in the test theme and 2 in the module) - if/when we'll move the stuff around to the core components, we can move the one column and two column layouts to the test module
- whether those derivatives have the defined parameters (eg. regions)
- whether displaying those derivatives works out as intended

Layout plugins:
- whether it is possible to provide one's own plugin with its own regions and render information as a PHP class (not as a derivative)
- whether that works too

Page render stuff:
- whether using a layout on a page actually produces the layout output expected (with the mock regions)

I think this rounds out this patch and then we only need to take care of concerns from above about lack of #attached use, simplified file naming for tpl files, etc. I don't think the patch lacks functionality per say that would be needed for a first commit. @catch, etc. agree? :)

andypost’s picture

@Gabor Is there any reason ship layout module with 2 layouts that useless (can't be applied)?
I think this patch/module should have visual impact or just a layout_test should be able to demonstrate a ability to change a page layout. There's a lot of things are commited to core that does not actually usable...
For example this module could have contextual link or toolbar integration to allow change a layout

EclipseGc’s picture

Gabor,

The derivative logic needs an extra level of directory structure expectations that I never added. Specifically this line:

$dir = $provider['dir'] . DIRECTORY_SEPARATOR . 'layout';

I was thinking that we should have a protected variable in the class called $type that in this case reads 'default'. so this line should read:

$dir = $provider['dir'] . DIRECTORY_SEPARATOR . 'layout' . DIRECTORY_SEPARATOR . $this->type;

Then a layout plugin like responsive can use the exact same derivative logic but it would just extend this class and update the type variable. So we'll have
layout/default
layout/responsive
layout/flexible

for the 3 different types of layout plugins we know are likely to exist, and as others come into being, they need only update that variable to extend the derivative discovery and provide a whole new subset of directories to find layouts in.

Hopefully this seems sane and within the scope of what we think this patch should do.

Eclipse

sun’s picture

Issue tags: +Blocks-Layouts

re: DIRECTORY_SEPARATOR: Can we use regular forward-slashes, plz?

I hope that themers won't have to deal with ./lib/Drupal/mytheme/Plugin/Layout/... - I think they'd kill us. ;)

Gábor Hojtsy’s picture

@sun: as per EclipseGC's suggestion, themers would need to do themes/$themename/layout/default/$layoutname/$layoutname.yml (and assorted files) to define a layout vs. themes/$themename/page.tpl.php before.

EclipseGc’s picture

@sun

Themers definitely don't have to deal with any PSR-0 structure, the Directory separator is just there to be cross OS compliant without me having to think about it. If you have a different way you want to approach that, I'm completely ok with it. The derivative class that you're looking at this is complicated in order to simplify what we expect from themers, which is just a very specific directory structure.

In this case, a theme or module can implement a 'layout' directory directly in their own dir, and then within that we should have sub directories that specify what type of plugin will handle the theme. "Default" is the only one we're likely to ship with core, so 'layout/default'. Within that we can have as many directories as we want (onecol, twocol, threecol, whatever) and then within those directories would be all the resources that a given layouts needs (icon, css, js, metadata in the form of yml).

The only thing complicated here should be the directory structure, and even that should be relatively simple for themers. I think this likely to be super straight forward.

@Gabor,

Could we rename the DefaultLayout class to StaticLayout and that directory to static? or fixed? or something. I think default has some connotations here I didn't intend. Or if people have better suggestions ++ to that.

Eclipse

Gábor Hojtsy’s picture

FileSize
27.02 KB
5.44 KB

As said, worked on tests! (did not rename directories or the default layout stuff yet, although the suggestions look good, will do later).

The added tests now test the derivative discovery in the module and theme, it tests that the module and theme discovered layouts have (some of) the expected regions, that the can be rendered and the dummy region names and some sample raw markup is found. It also tests for the page callback layout application (the temporary mapper and the layout module page render altering), ensuring that the proper layout is found. The page callback is obviously a "little bit" hacky, better suggestions welcome!

The only piece not tested yet is a custom layout plugin class. I still need to figure out how that would work, and that would be for next week, so if someone knows better right away or EclipseGC if you want to apply the directory change and the rename of the default layout, feel free to do so in the meantime :)

Status: Needs review » Needs work

The last submitted patch, layout-91.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Blocks-Layouts, +Spark
FileSize
967 bytes
27.05 KB

Tests fail on

Fatal error: Call to undefined method DirectoryIterator::getExtension() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/layout/lib/Drupal/layout/Plugin/Derivative/Layout.php on line 88
FATAL Drupal\layout\Tests\LayoutDerivativesTest: test runner returned a non-zero error code (255).

Looking at the docs at http://hu.php.net/manual/en/directoryiterator.getextension.php, getExtension() should only be available from PHP 5.3.6, and Drupal 8 requires 5.3.5 currently. So we cannot rely on that being available. Using the code suggestion from the PHP documentation to replace it with backwards compatible (albeit not as nice) operations.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, layout-94.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.75 KB
27.53 KB

Implemented @EclipseGC's suggestions from #91 (renamed DefaultLayout to StaticLayout) and #88 (introduced yet another level of subdirectory structure, so layout/static contains the layouts in modules and themes defined in core's static layout format).

Reviewers, please speak up as soon as possible as any of the changes made don't make sense or go in a different direction than what you think would be healthy. While I don't necessarily agree that the nested directory structure is best, nobody spoke up and I don't really have a strong opinion either way, so its what it is now :)

Gábor Hojtsy’s picture

Also looked at the potential to use #attached. What we do in our current iteration is we call out to theme_render_template($template, array('content' => $regions)); directly. This is done so we don't need to implement each layout page in a hook_theme() as well as describing it via the layout yml metadata. Sounds like exposing all static templates through hook_theme() might be bit of an overkill. However, this way, we really directly invoke the layout templates, so the arguments above for their naming to be so explicit (provider name + layout name) does not stand, since we don't support themes overriding them anyway. If we want themes to support overriding them, we'd need to use the regular theme() flow. Then the question of registering the layout templates one-by-one via hook_theme() is a possibility, or we can introduce another theme wrapper that would run the template from another argument.

I think this is independent of how we end up tying this into the render pipeline, since its a question about how themes could or could not override the layout templates. Expecting they could override is natural from how the rest of the templates are overridable (and how explicitly we named the files to allow for that to begin with). However, given we want these layouts to be bare-bones focusing on layout only, sounds like themes should provide alternate layouts instead if they so want to, instead of overriding the template or css only (especially given we did not name the CSS specific enough to be safely overridable).

What do people think?

effulgentsia’s picture

I take back what I said in #9. For this issue, let's revert back to calling theme(). After this lands, I'll open a follow up for discussing whether to decouple from theme(), but since this would be the first time in Drupal that we output markup without calling theme(), it really deserves its own issue, not getting lumped into this one.

Gábor Hojtsy’s picture

FileSize
6.1 KB
28.24 KB

Returning to theme() for this inaugural #100 edition :D It does not work fully unfortunately because whatever I put in the variable defaults or the actual variable values for the render array, it seems to only take the first character of the variable and nothing else. So with the current patch, it would take < from the <h3>Footer</h3> code. This equally works for variable defaults from layout_theme() or the render values via renderLayout().

I wanted to use drupal_render() instead of theme() as in EclipseGC's original patch because this would allow us to use #attached as well for rendering.

Any help in figuring this out would be appreciated.

Gábor Hojtsy’s picture

FileSize
28.76 KB
1.04 KB

Added an actual page.css (not just the .yml reference to it as before) to the layout test theme and testing that it is actually included in the page (ensuring #attached CSS worked). That part of the test passes. So looks like we should be figuring out why region variables are not working and then should be in a much better shape!

Gábor Hojtsy’s picture

FileSize
2.84 KB
28.7 KB

Looking back, it is not that big of a mystery. EclipseGC pointed out that the templates indeed use $content['middle'] to refer to the middle region for example, and not directly $middle. So packaging the variables like that properly obviously solved the problem :)

Also moved the CSS assert to the page check, that is the only place where we can actually test CSS being added.

Any other concerns / complaints?

Gábor Hojtsy’s picture

Ok now, any other concerns? :) I have been thinking about this problem of this being in layout module which would then later become a required module to handle layouts, but this is where the layouts get registered with the theme system. If we want to move that to (ehm) system.module let's say, that would not adhere to the separation of concerns that Drupal 8 so much attempts to achieve. We can always remove the layout module and move around pieces to core components if the theme system changes so that we don't need to register these with a hook for example. As a prime example, entities were moved to a module and then out of modules to the core component system in the past months.

Once we introduce a UI for this the question will come up of whether that would go into layout module (making the UI required as well) or pushing the theme stuff to system module and the UI an optional module or making a layout_ui module available. (The UI would be responsible to map layouts to paths).

So any other concerns, advice to do this better? I think the functionality and test coverage for this is pretty complete, so it is only a question of what is left and if we want to move in this direction and give time to implement the remaining pieces in the short time left (I hope so :).

fubhy’s picture

+++ b/core/modules/layout/layout/static/onecol/layout--onecol.tpl.phpundefined
@@ -0,0 +1,19 @@
+/**
+ * @file
+ * Template for a one column layout.
+ *
+ * This template provides a very simple "one column" display layout.
+ *
+ * Variables:
+ * - $id: An optional CSS id to use for the layout.
+ * - $content: An array of content, each item in the array is keyed to one
+ *   region of the layout. This layout supports the following sections:
+ *   $content['middle']: The only region in the layout.
+ */
+?>
+<div class="layout-display layout-1col clearfix" <?php if (!empty($css_id)) { print "id=\"$css_id\""; } ?>>
+  <div class="layout-region layout-col">
+    <div><?php print $content['middle']; ?></div>
+  </div>

+++ b/core/modules/layout/layout/static/onecol/onecol.cssundefined
@@ -0,0 +1,22 @@
+
+.layout-1col {
+/*  overflow: hidden;  */
+}
+
+.layout-2col .layout-col-first .inside {
+  margin: 0;
+}
+
+
+.layout-1col .layout-col {
+  width: 100%;
+}
+
+#layout-edit-display .layout-region,
+#layout-edit-display .helperclass {
+  margin: .5em;
+}
+
+.layout-2col .layout-separator {
+  margin: 0 0 1em 0;

We should maybe clean those templates and CSS files up (a little bit) before committing. Currently it's a 1:1 copy from panels. I mean, I am not talking about super awesome and fancy example layouts. But parts of that CSS don't even belong to that layout obviously :P.

+++ b/core/modules/layout/layout/static/onecol/onecol.ymlundefined
@@ -0,0 +1,7 @@
+template: layout--onecol

Pretty much unrelated: Our template files are getting more and more complex with all the suggestion separators. Eventually we should figure out if some sort of namespacing for layouts would make sense for us. I have no clue why we are not putting suggestions into sub-folders and then build the suggestion from the path.

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Derivative/Layout.phpundefined
@@ -0,0 +1,110 @@
+   * List of derivatives.
+   *
+   * @type array
+   *   Associative array keyed by 'provider__layoutname' where provider is the
+   *   module or theme name and layoutname is the .yml filename, such as
+   *   'bartik__page' or 'layout__onecol'. The values of the array are associative
+   *   arrays themselves with metadata about the layout such as 'template', 'css',
+   *   'admin css' and so on.
+   */
+  protected $derivatives = array();

AFAIK we use @var here and the description is not part of that but should live above that.

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Derivative/Layout.phpundefined
@@ -0,0 +1,110 @@
+
+  /**
+   * Layout derivative type.
+   *
+   * @†ype string
+   *   Defines the subdirectory under ./layout where layout metadata is loooked
+   *   for. Overriding implementations should change this to look for other
+   *   types in a different subdirectory.
+   */
+  protected $type = 'static';

Same here.

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/LayoutMapper.phpundefined
@@ -0,0 +1,32 @@
+  protected $manager;
+
+  public function __construct($manager) {
+    $this->manager = $manager;
+  }

That could use some documentation.

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Type/LayoutManager.phpundefined
@@ -0,0 +1,33 @@
+  protected $defaults = array(
+    'class' => 'Drupal\layout\Plugin\layout\layout\StaticLayout'
+  );

Could also use some documentation even if it is clear what it is.

effulgentsia’s picture

I'll do a more thorough review later today, but in the meantime, can we rename the "layout" folder where modules and themes place their layouts to "layouts"? We're plural elsewhere for non-psr-0 folders (e.g., "images", "templates", "modules", "themes"). Layout.module should remain singular, so we'll have core/modules/layout/layouts, which I think is ok for now, unless someone flags that as a problem we need to worry about.

webchick’s picture

Priority: Normal » Major

Since this lays the foundations for the "Layouts" parts of the Blocks and Layouts initiative, this should probably be at least major.

Gábor Hojtsy’s picture

@effulgentsia: layouts/layout fine either way for me :) Sure we can rename it.

EclipseGc’s picture

I'd prefer layouts, I made it singular cause I thought that was convention, if people prefer the plural (I sure do) then +1000000.

I do think I have an objection to how the template naming is happening. I'll take a pass on this code today come hell or high water to demonstrate the alternative.

Eclipse

effulgentsia’s picture

I looked over #102 again, and don't see anything that couldn't be refined in a follow up. Once fubhy and EclipseGc are happy, let's RTBC this and keep moving on the remaining stuff in #1787634: [META] Decouple layouts from themes.

EclipseGc’s picture

FileSize
27.11 KB

OK, did a few things here.

1.) there's no reason to complicate the actual template name, so I made those just onecol and twocol again.
2.) Added a sane default that if it's not set by the yml, will set it to whatever the yml files was named (in the derivative handler)
3.) removed hook_page_build(). As far as I know, it's going away in the long term anyway. Larry's patch is in and the writing is on the wall.
4.) removed the Mapper, it was totally bogus anyway, and ultimately unnecessary.
5.) Updated the layout to layouts directory names.
6.) Moved all the layouts appropriately.
7.) Fixed the broken tests.

7 also included the layout_test module's layout-test path. This was the only code path executing the mapper at all, and a 2 line change fixed that. I'm as happy as I'm likely to get with this code.

Eclipse

fubhy’s picture

FileSize
26.05 KB

Did some additional cleanup + coding standards work. No logic changes.

I am happy with the code too. EclipseGc++ GaborHojtsy++

fubhy’s picture

FileSize
26.02 KB

Fail.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
FileSize
1.3 KB
25.97 KB

Thanks all for the cleanup! I'm fine with all the changes made :)

One more thing IMHO: let's remove references to non-existent icon images or make the images existent :) I think its easier to skip the icons now, we can bikeshed how they look, what is their visual size, etc. in another issue :D Otherwise I think this is RTBC but given that I worked on it so much, not feeling like I'd be the right person to RTBC it :)

Gábor Hojtsy’s picture

Status: Needs work » Needs review
fubhy’s picture

+1 for RTBC

webchick’s picture

Issue tags: -Dynamic layouts

Tagging. Also? Exciting! :D

webchick’s picture

Issue tags: +Dynamic layouts

What the freaking freak?

fubhy’s picture

Status: Needs review » Reviewed & tested by the community

It's green, it's working, all "complaints" have been fixed and we want to move on fast.

moshe weitzman’s picture

Looks good. A few nits ...

+++ b/core/modules/layout/layout.module
@@ -0,0 +1,33 @@
diff --git a/core/modules/layout/layouts/static/onecol/onecol.tpl.php b/core/modules/layout/layouts/static/onecol/onecol.tpl.php

IMO, the 'static' subdirectory does not add much value. Of course dynamic layouts won't have their own directory. Could we get rid of this subdir? Any new plugin type can put its templates in a subdir if it wants.

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Derivative/Layout.php
@@ -0,0 +1,117 @@
+  /**
+   * Implements DerivativeInterface::getDerivativeDefinitions().
+   */
+  public function getDerivativeDefinitions(array $base_plugin_definition) {
+    $available_layout_providers = array();
+
+    // Add all modules as possible layout providers.
+    foreach (module_list() as $module) {
+      $available_layout_providers[$module] = array(
+        'type' => 'module',
+        'provider' => $module,
+        'dir' => drupal_get_path('module', $module),
+      );
+    }
+
+    // Add all themes as possible layout providers.
+    foreach (list_themes() as $theme_id => $theme) {
+      $available_layout_providers[$theme_id] = array(
+        'type' => 'theme',
+        'provider' => $theme->name,
+        'dir' => drupal_get_path('theme', $theme->name),
+      );
+    }
+
+    foreach ($available_layout_providers as $provider) {
+      // Looks for layouts in the 'layout' directory under the module/theme.
+      // There could be subdirectories under there with one layout defined
+      // in each.
+      $dir = $provider['dir'] . DIRECTORY_SEPARATOR . 'layouts' . DIRECTORY_SEPARATOR . $this->type;
+      if (file_exists($dir)) {
+        $this->iterateDirectories($dir, $provider);
+      }
+    }
+    return $this->derivatives;
+  }

This pattern more or less what we abstracted as a 'group' in hook_hook_info(). It would be a shame if we have to duplicate this in many plugin types. Perhaps Views has duplicated this as well. Not a big deal for this issue

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/layout/layout/StaticLayout.php
@@ -0,0 +1,147 @@
+  /**
+   * Add the CSS files associated with this layout to the page.
+   */
+  public function getCssFiles() {

This doesn't actually add anything to $page. Lets edit down the Doxygen here and similar methods.

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/layout/layout/StaticLayout.php
@@ -0,0 +1,147 @@
+  /**
+   * Implements Drupal\layout\Plugin\LayoutInterface::renderLayout().
+   */
+  public function renderLayout($admin = FALSE) {
+    $definition = $this->getDefinition();
+
+    // Assemble a render array with the regions and attached CSS/JS.
+    $render = array(
+      '#theme' => $definition['theme'],
+      '#content' => array(),
+    );
+
+    // Render all regions needed for this layout.
+    foreach ($this->getRegions() as $region => $title) {
+      // @todo This is just stub code to fill in regions with stuff for now.
+      // When blocks are related to layouts and not themes, we can make this
+      // really be filled in with blocks.
+      $render['#content'][$region] = '<h3>' . $title . '</h3>';
+    }
+
+    // Fill in attached CSS and JS files based on metadata.
+    if (!$admin) {
+      $render['#attached'] = array(
+        'css' => $this->getCssFiles(),
+        'js' => $this->getJsFiles(),
+      );
+    }
+    else {
+      $render['#attached'] = array(
+        'css' => $this->getAdminCssFiles(),
+        'js' => $this->getAdminJsFiles(),
+      );
+    }
+
+    // Include the path of the definition in all CSS and JS files.
+    foreach (array('css', 'js') as $type) {
+      foreach ($render['#attached'][$type] as &$filename) {
+        $filename = $definition['path'] . '/' . $filename;
+      }
+    }
+
+    return drupal_render($render);
+  }

We have omitted libraries - they are a common thing found in #attached. drupal_process_attached() can also operate on arbitrary things besides library/js/css and thats lost now. I guess thats OK for now.

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/layout/layout/StaticLayout.php
@@ -0,0 +1,147 @@
+    $render = array(

The convention when building a render array is to use the variable $build.

catch’s picture

Status: Reviewed & tested by the community » Needs review

After #1737148: Explicitly declare all JS dependencies, don't use drupal_add_js all JavaScript has to be registered as a library, however js added for layouts won't be with the current patch. Can layout module register it on their behalf somehow?

Gábor Hojtsy’s picture

@catch, @Moshe: re libraries; I don't think the current information (JS file name only) is enough to register it as a library in a meaningful way. Eg. we don't have a meaningful name for it and we don't have version numbers and dependencies. We can either rely on themes/modules registering these as libraries themselves and drop js as an option and add libraries in place or add a lot more details in the .yml file, which sounds like would be too much of an indirection to me.

@Moshe; re static dir, are you proposing the static layouts would be in direct subdirs while other types of layouts would be in indirect subdirs (one more level down?)

Wim Leers’s picture

@Moshe I also don't see how we could meaningfully register these as libs. Themes are also not yet registering themselves as libs, right? This is akin to that IMHO.

effulgentsia’s picture

In THEME.info, we have stylesheets and scripts as still supported keys, with each being an array, and we do not yet support a libraries key. How about we update this patch to match (i.e., change the css and js yaml keys to stylesheets and scripts, and make them use the same indexes as what THEME.info uses for them), and then have a separate issue for replacing scripts with libraries for both layout yaml and theme .info?

Gábor Hojtsy’s picture

@effulgentsia: sounds good to me.

tim.plunkett’s picture

There is only one call to drupal_add_js in core themes, and it's to add a setting. No files are added that way.

EclipseGc’s picture

@moshe,

The static dir is because we're going to iterate through every subdir of wherever we end up so that themers can organize their layouts in whatever makes the most sense to them. So flexible/responsive layouts would be in layouts/responsive and layouts/flexible accordingly. I'm ok with changing the name of the directory here, but we actually do need the directory I think.

Eclipse

Gábor Hojtsy’s picture

@EclipseGC, @Moshe: my understanding is that Moshe suggests we do not prepare for other layout types to be available and only assume static in core, so others in contrib would define other subdirectories (either on the same level as layout or under layout). Given that the static layout discovery sucks up all .yml files as static layout definitions, any contrib shipped layout would then need to use something other than .yml in this model, which is I'm not sure a good idea. (Or we need to make some other metainformation available, eg. name the file static.page.yml to identify the static type or include a type = static reference in the file or something like that). Moshe? EclipseGC?

Gábor Hojtsy’s picture

@tim.plunkett: @effulgentsia was referring to this code in theme.inc, proposing that we rename the css/js keys in the .yml to stylesheets/scripts as in the theme .info file and then consider the librarification of this as the same as the libarification of the theme JS files.

  // Do basically the same as the above for scripts
  $final_scripts = array();

  // Grab scripts from base theme
  foreach ($base_theme as $base) {
    if (!empty($base->scripts)) {
      foreach ($base->scripts as $name => $script) {
        $final_scripts[$name] = $script;
      }
    }
  }

  // Add scripts used by this theme.
  if (!empty($theme->scripts)) {
    foreach ($theme->scripts as $name => $script) {
      $final_scripts[$name] = $script;
    }
  }

  // Add scripts used by this theme.
  foreach ($final_scripts as $script) {
    drupal_add_js($script, array('group' => JS_THEME, 'every_page' => TRUE));
  }
fubhy’s picture

The 'static = TRUE" reference in the file doesn't sound right considering that in that case we would have to read out all the files first to identify those that actually match the layout type that we are currently digging through the layout folder for. That said I think it doesn't really matter if we have 'static.layout.yml' files or /static/layout.yml ... or does it?

Regarding the stylesheets/scripts syntax. Do we want to also add the syntax for media queries for stylesheets?

stylesheet[all][] = foo.css

I am not sure if that makes sense for layouts considering that layouts should normally really just serve structural CSS which would be media="all" normally. Right?

Gábor Hojtsy’s picture

@fubhy: as for media queries, I don't think we can replicate this format in the .yml file, we'd need to use nested lists which sounds like would be to the inconvenience of the themer, so unless we see a real reason to introduce this, we probably should not(?).

fubhy’s picture

Yes! Plus this:

I am not sure if that makes sense for layouts considering that layouts should normally really just serve structural CSS which would be media="all" normally. Right?

Gábor Hojtsy’s picture

FileSize
25.76 KB
5.51 KB

Worked on @Moshe's and @effulgentsia's feedback:

- updated the get* function descriptions
- changes css to stylesheets and js to scripts in the .yml file (same for admin *)
- as a consequence, since this is a plural name, now forced to be a list of items at all times (like the .info file)
- renamed $render to $build

I *did not* add libraries support, .info files don't allow for those either.

EclipseGc’s picture

I didn't perceive Moshe's statements that way, I thought he was just asking to remove that directory, but since we're iterating over all directories, we have to have a separate directory in which to start iterating. I'd prefer that directory exist within a common structure so that all layouts are in the layouts directory. I'm definitely against not using yml for all definitions, I think that will end up as a total WTF.

sdboyer’s picture

Priority: Major » Normal

i've integrated this patch in as part of the work done for #1812720: Implement the new panels-ish controller [it's a good purple]. i haven't yet gotten to making some of the changes that need to happen, but a quick summary would be to say that maybe half of the logic related to doing the rendering itself should move out of the layout plugins (as they're defined here) and into the controller proper.

however, i don't think that should block this patch going in. this represents a more immediate approach to layouts, and while it's not really compatible with any of the symfony systems that it does eventually need to be, it gets a baseline in that we can work with when updating to NG.

let's just be clear that if it goes in, it's not a finished API that people should be building on.

EclipseGc’s picture

agreed

sdboyer’s picture

Priority: Normal » Major

ah sorry xpost, restoring priority.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Resolved all of Moshe's feedback that people did not strongly argue with. Also, as people pointed out, parallel work in other areas will shape this in the future after committed, but that is just normal Drupal. Should be back to RTBC then.

fubhy’s picture

Thanks Gabor! Let's get this in asap.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/layout/layout.moduleundefined
--- /dev/null
+++ b/core/modules/layout/layouts/static/onecol/onecol.tpl.phpundefined

+++ b/core/modules/layout/layouts/static/onecol/onecol.tpl.phpundefined
--- /dev/null
+++ b/core/modules/layout/layouts/static/onecol/onecol.ymlundefined

+++ b/core/modules/layout/layouts/static/onecol/onecol.ymlundefined
--- /dev/null
+++ b/core/modules/layout/layouts/static/twocol/twocol.cssundefined

+++ b/core/modules/layout/layouts/static/twocol/twocol.cssundefined
@@ -0,0 +1,17 @@
diff --git a/core/modules/layout/layouts/static/twocol/twocol.tpl.php b/core/modules/layout/layouts/static/twocol/twocol.tpl.php

+++ b/core/modules/layout/layouts/static/twocol/twocol.tpl.phpundefined
--- /dev/null
+++ b/core/modules/layout/layouts/static/twocol/twocol.ymlundefined

(nitpick) We don't smooshwordstogether. Should be "one-col" and "two-col" everywhere. I notice down below though that this is used as part of a theme suggestion, so this might not be possible?

+++ b/core/modules/layout/layouts/static/onecol/onecol.tpl.phpundefined
@@ -0,0 +1,18 @@
+<div class="layout-display layout-onecol clearfix">
+  <div class="layout-region layout-col">
+    <div class="inside"><?php print $content['middle']; ?></div>
+  </div>

+++ b/core/modules/layout/layouts/static/twocol/twocol.tpl.phpundefined
@@ -0,0 +1,24 @@
+<div class="layout-display layout-twocol clearfix">
+  <div class="layout-region layout-col-left">
+    <div class="inside"><?php print $content['left']; ?></div>
+  </div>
+
+  <div class="layout-region layout-col-right">
+    <div class="inside"><?php print $content['right']; ?></div>
+  </div>

Have we validated this markup with any themers? I see mostly back-end guys in this issue.

While I would love to see Panels (lite) in core, I would definitely NOT love to see themers continue to curse Drupal's default markup. Let's run past some of the crew working on Twig.

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/Derivative/Layout.phpundefined
@@ -0,0 +1,117 @@
+  /**
+   * Finds layout definitions by looking for layout metadata.
+   */
+  protected function iterateDirectories($dir, $provider) {

Is there a reason we need our own function here and can't just use CMI's stuff? I thought it was possible to register alternate locations of config. I might be confused.

+++ b/core/modules/layout/lib/Drupal/layout/Plugin/layout/layout/StaticLayout.phpundefined
@@ -0,0 +1,132 @@
+      $build['#content'][$region] = '<h3>' . $title . '</h3>';

Don't we need a theme function for this?

+++ b/core/modules/layout/tests/themes/layout_test_theme/layouts/static/page/page.cssundefined
--- /dev/null
+++ b/core/modules/layout/tests/themes/layout_test_theme/layouts/static/page/page.ymlundefined

+++ b/core/modules/layout/tests/themes/layout_test_theme/layouts/static/page/page.ymlundefined
+++ b/core/modules/layout/tests/themes/layout_test_theme/layouts/static/page/page.ymlundefined
@@ -0,0 +1,21 @@

@@ -0,0 +1,21 @@
+title: Layout test page
+category: Other
+template: test-layout
+stylesheets:
+  - page.css
+regions:
+  header: 'Header'
+  help: 'Help'
+  highlighted: 'Highlighted'
+  featured: 'Featured'
+  content: 'Content'
+  sidebar_first: 'Sidebar first'
+  sidebar_second: 'Sidebar second'

--- /dev/null
+++ b/core/modules/layout/tests/themes/layout_test_theme/layouts/static/page/test-layout.tpl.phpundefined

+++ b/core/modules/layout/tests/themes/layout_test_theme/layouts/static/page/test-layout.tpl.phpundefined
+++ b/core/modules/layout/tests/themes/layout_test_theme/layouts/static/page/test-layout.tpl.phpundefined
@@ -0,0 +1,62 @@

@@ -0,0 +1,62 @@
+<div id="page-wrapper"><div id="page">
+
+  <div id="header"><div class="section clearfix">
+    <?php print $content['header']; ?>
+  </div></div> <!-- /.section, /#header -->
+
+  <?php if ($content['featured']): ?>
+    <div id="featured"><div class="section clearfix">

Could we not just add a layout to Bartik and test that instead? This is an awful lot of code to get out of sync with the "upstream" theme.

Gábor Hojtsy’s picture

(nitpick) We don't smooshwordstogether. Should be "one-col" and "two-col" everywhere. I notice down below though that this is used as part of a theme suggestion, so this might not be possible?

I don't think it would be impossible to do this, will look into this shortly.

Have we validated this markup with any themers? I see mostly back-end guys in this issue.

While I would love to see Panels (lite) in core, I would definitely NOT love to see themers continue to curse Drupal's default markup. Let's run past some of the crew working on Twig.

The introduced markup is pre-existing directly copy-pasted from Panels. That does not in itself validate it obviously. Also, we purposefully only picked a one column and a two column layout for demonstration / testing purposes and this should be possible to refine indefinitely (and will definitely be when/if templates are converted to twig, since twig people will definitely look at this given it is a tpl.php). The goal of this issue was to introduce the plugin layer and the "packaging format" for layouts and to provide some demonstrations for them. Since layouts cannot be used for anything without at least two further followups, the layouts in the patch will just sit around there unused for a definite while. I think its important to have a theme and a module definer layout at least for the test. If you prefer I move this to a test module instead and not have any layout on the main module itself, that is fine.

Is there a reason we need our own function here and can't just use CMI's stuff? I thought it was possible to register alternate locations of config. I might be confused.

I don't know about those capabilities, will shortly take a look, however @EclipseGC specifically said earlier that he was burned by trying to use CMI for non-config data. The layout metainformation file IS NOT configuration! It is metadata about the layout but is not something people can change or configure in any way. It is just there in this .yml format, so we don't need to pick yet another format for it. We can change this to .info files if this helps clear the confusion around CMI-not-CMI or we can use more CMI code, but I'm concerned we'll hit into the area @EclipseGC was criticised in earlier.

Don't we need a theme function for this?

I'd rather not introduce a theme function for a temporary workaround to display something in place of region data to be able to introduce testable layouts. This is just sample data we fill in the layout until we get to actually relating blocks to layouts (or now the intermediary Display's as @sdboyer christened them). Without this landing in some form, we'll not get there.

Could we not just add a layout to Bartik and test that instead? This is an awful lot of code to get out of sync with the "upstream" theme.

The only point of this layout in the test theme is to test that layouts in themes are found. If we put a layout in a theme now (eg. Bartik), then the next logical question is why do we have a layout AND a page.tpl.php and why don't we move the layout CSS to the layout. Well, the answer to that again is that this is a first step and although we do introduce a layout plugin/derivative/packaging format, this looked like enough complexity to review and keep up with and we do not introduce layers to make the themes actually work with layouts. If you want the whole solution, then we should go back to the drawing board for a couple weeks. So we could put a layout in Bartik, but it would have no use AND we could not separate any of Bartik's existing tpl.php's or CSS to the layout, because layouts cannot be used yet for page display. So IF we put it there, THEN it would mean a maintenance problem. The place it is put now it is *not* a maintenance problem because it is a test theme and the only thing its relevant for us is that it is found by the discovery iterator and it can be rendered with the dummy region name renderer that we have now. For what it's worth, it could contain straight divs in succession for all regions (note the clever and complete CSS for example :), but then of course the question would be whether we validated that markup with themers :D

All-in-all I think your concerns are based on assumptions that this system is readily usable and not filled with test data to form a testable first step of introducing this system.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
25.8 KB

Renamed onecol to one-col and twocol appropriately. Needed to rename css selectors, file, directory names, test stuff, etc. Passes on my machine. The rest is debated and better get this tested, so moving back to needs review.

webchick’s picture

Status: Needs review » Needs work

Gábor and I just had a quick pow-wow, and this was the result:

1) Since I'm really not comfortable blindly copy/pasting markup from Panels into core, when we already know fully-well that this markup pisses off front-end devs, and since the idea of one- or two-column layouts is a little bit silly in the first place, what we think we should do is move those to tests instead (one-col to a test module, two-col to a test theme or whatever).

2) This allows us to remove the Bartik-ish layout from the tests (which is a bit overkill) and allow a parallel effort for one or two default Themer Approved™ stock layouts to develop, while we get the necessary foundations in to allow the work at #1787942: Allow assigning layouts to pages to move forward.

3) Good enough about the h3s. I would recommend we mark it with a @todo but there already is one, so we're good there. ;)

4) I was making a false assumption there that YML == CMI, but while that's the case now, it's looking more and more increasingly (e.g. breakpoints, metadata configuration stuff) that this will not be the case at the point 8.x ships. .info files will likely run into nastiness with it not knowing if these are themes or modules or what they are. So let's keep this as-is. But as we were talking, Gábor thought it might make sense to add an abstraction layer that makes this explicit (e.g. YamlStorage, YamlStorageInterface, YamlDiscoveryInterface, YamlStorageDiscoveryDependencyInjectionContainerTest, etc. that Config* extends from). We should create a follow-up for that and see what the CMI people think.

webchick’s picture

Status: Needs work » Needs review

Sorry, cross-post.

Gábor Hojtsy’s picture

FileSize
21.64 KB

Moved one-col to the layout_test module, two-col to the layout_test_theme, no layouts defined anymore in the module per say, only test layouts. Updated tests to test for these two layouts. Consequently, the bartik page copied layout is also gone.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks like this should resolve the concerns related to this concrete patch.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/layout/layout.moduleundefined
@@ -0,0 +1,33 @@
+ * Implements hook_theme().
+ *
+ * Expose all layouts as theme items, so themes can override layout markup.
+ */
+function layout_theme($existing, $type, $theme, $path) {
+  $items = array();
+  foreach (layout_manager()->getDefinitions() as $name => $layout) {
+    $items[$layout['theme']] = array(
+      'variables' => array('content' => NULL),
+      'path' => $layout['path'],
+      'template' => $layout['template'],
+    );
+  }
+  return $items;

Sorry another one. Why are is this dynamically registering theme functions instead of using suggestions? See also #409354: Performance issues with large number imagecache presets and content types.

I opened #1813186: Integrate scripts/stylesheets from info/layout files with libraries, agreed that's a pre-existing issue that shouldn't hold this one up.

catch’s picture

Status: Needs work » Reviewed & tested by the community

Automated status changing :(

Gábor Hojtsy’s picture

@catch: We register the layouts in hook_theme() so that themes can override layout markup as they wish (as-is customary with other .tpl.php files in core). If we'd directly add in the theme suggestions, we'd need to manually process any possible overrides. Also, I assume you are proposing we'd have a general theme function like theme('layout', ...) and then attach suggestions on that right? That would require the suggestion logic to also filter out layouts and only provide the suggested ones for the layout in use based on the conditions present (eg. in the patch we'd need to move the specific layout being asked for as a regular theme variable and then suggest layout templates based on a theme variable). I believe that would be a considerably big hack. So it would be more custom code, less elegant implementation and lack of override-ability for themers. Also as noted above @sdboyer and friends are reworking the rendering layer anyway (and the whole theme system is being reworked for twig), both of which could easily have a heavy effect on this code later on.

So all-in-all I think the current approach is the best we can/should do now.

catch’s picture

OK, I opened #1813206: Dynamically register layouts in hook_theme() or not to keep track of this. The trouble with dynamically registering stuff is that if we end up with 200 layouts defined on a site, then that's going to add a tonne of bloat to the theme registry. If we only have about 10 layouts on any particular site then it's probably not a problem.

Bojhan’s picture

Issue tags: +B&L

Looking forward to seeing this in, looks like we need it for most of the UI stuff.

webchick’s picture

Title: Themes should declare their layouts » Change notice: Themes should declare their layouts
Category: feature » task
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Ok, looks like my feedback was addressed, and catch's has a follow-up, Bojhan and the SCOTCH folks are tentatively +1 too. Heeeeeeeere we gooooo....

Committed and pushed to 8.x. WOOHOO!

I'm a little torn on what to do about a change notice here; both Eclipse and Sam have underscored that this approach is not what we want to do long-term. However, I think in terms of the actual format for Layouts, looks like there's general agreement on? Just the implementation is not optimal?

Tentatively marking for change notice for how a theme declares its layout.

Fabianx’s picture

Yahoo! That is great!

sdboyer’s picture

i'm also not sure what to put into that change notification. one thing that hasn't been tackled here is the notion of region 'roles' - meta-classifications such as "main", "sidebar", "header", "footer", that are not the region name, but identify the general purpose of the region. they're really important for dynamic/suggested remapping of blocks to regions when layouts get hotswitched, however that happens. i know we've had verbal discussions about it, but it didn't make it in here and it does affect the base API that layouts need to implement.

i know we also discussed a parallel categorization axis distinguishing between 'system', 'global', and 'content' regions, and that Spark was already doing some of that. i don't think it's here either, though.

Gábor Hojtsy’s picture

Change notice for layout formats: http://drupal.org/node/1813372, includes detailed format description and graphic.

Gábor Hojtsy’s picture

Title: Change notice: Themes should declare their layouts » Themes should declare their layouts
Category: task » feature
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs change record

Status: Fixed » Closed (fixed)

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

oadaeh’s picture

Issue tags: -B&L

Removing the unnecessary B&L tag.

oadaeh’s picture

Issue summary: View changes

Added images