Problem/motivation

Currently people can either use theme provided layouts that are hardwired (according to well set patterns) or override the whole page rendering with tools like Panels to get their way and ignore regions and theme provided layouts altogether.

Proposed solution

Once we make it possible for themes to declare their provided layouts formally (#1787846: Themes should declare their layouts) - instead of just tying them to specific paths or using baked-in logic, we should be able to provide a user interface to assign layouts based on conditions. The most basic condition would be the path obviously, making the path => layout assignment configurable instead of hard-coded with the path components. If/once block visibility conditions as plugins work out (#1743686: Condition Plugin System), this can extend to other conditions like permissions or language.

Dependencies

We should figure out how to declare layouts first in #1787846: Themes should declare their layouts and then when we have layouts get back here. UI discussions should be possible in the meantime.

Related issues

#1787634: [META] Decouple layouts from themes

Files: 
CommentFileSizeAuthor
#41 interdiff.txt5.51 KBGábor Hojtsy
#41 pages-41.patch17.87 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 48,086 pass(es), 0 fail(s), and 9 exception(s).
[ View ]
#37 controller.patch283.67 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] 47,610 pass(es), 695 fail(s), and 168 exception(s).
[ View ]
#28 pages-28.patch17.44 KBGábor Hojtsy
PASSED: .
[ View ]
#28 interdiff.txt1.76 KBGábor Hojtsy
#24 interdiff.txt1.29 KBGábor Hojtsy
#24 pages-24.patch18.18 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,285 pass(es).
[ View ]
#23 pages-23.patch17.65 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,270 pass(es).
[ View ]
#23 interdiff.txt574 bytesGábor Hojtsy
#22 PageTableDrag.jpg39.07 KBGábor Hojtsy
#22 interdiff.txt6.95 KBGábor Hojtsy
#22 pages-22.patch17.68 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,268 pass(es).
[ View ]
#20 interdiff.txt1.45 KBGábor Hojtsy
#20 pages-20.patch15.32 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 46,109 pass(es).
[ View ]
#17 pages-17.patch15.16 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 42,635 pass(es).
[ View ]
#17 interdiff.txt625 bytesGábor Hojtsy
#15 interdiff.txt4.13 KBGábor Hojtsy
#15 pages-15.patch15.17 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 42,637 pass(es).
[ View ]
#12 pages-12.patch14.3 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 42,193 pass(es).
[ View ]
#12 interdiff.txt1.01 KBGábor Hojtsy
#11 pages-11.patch14.3 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 42,193 pass(es).
[ View ]
#11 interdiff.txt811 bytesGábor Hojtsy
#11 LibraryExcept.jpg31.68 KBGábor Hojtsy
#10 pages-10.patch14.16 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 42,197 pass(es).
[ View ]
#10 interdiff.txt1.06 KBGábor Hojtsy
#9 PageLibrary2.jpg31.19 KBGábor Hojtsy
#9 interdiff.txt3.44 KBGábor Hojtsy
#9 pages-9.patch14.13 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 42,201 pass(es).
[ View ]
#6 pages-6.patch12.58 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 42,193 pass(es).
[ View ]
#6 PageLibrary.jpg34.39 KBGábor Hojtsy
#6 EditPage.jpg49.8 KBGábor Hojtsy

Comments

Just had a quick discussion with @effulgentsia about this:

Gábor Hojtsy @ 6:10 I've been playing around with the theme.inc code quite a bit earlier today trying to figure out how to avoid the .tpl.php route for pages but then still be able to run that later, and was not really happy with my stuff, so no patch yet :)

Alex Bronstein @ 6:11 my thought from earlier was that we'd have, say, layout.module that would be enabled instead of block.module
so, layout_page_build() would do something similar to what block_page_build() does, but different as needed
one of the things it can do is set $page['#pre_render'] to something like layout_page_render()
which can then do its plugin-oriented logic

Gábor Hojtsy @ 6:15 hm, can I quote you on that?

Alex Bronstein @ 6:16 yes
you can also set $page['#theme'] to NULL to prevent theme('page') from getting called

Gábor Hojtsy @ 6:18 Alex Bronstein: yeah, well, the idea is a clean D8 solution, not a hack really :)
once we have themes defining possibly multiple layouts, if we skip theme('page') all the time we need something dynamic, we would never call theme('page') again
so I think we should just as well fix theme page :)

Alex Bronstein @ 6:20 Gábor Hojtsy: once layout.module actually works, and we can remove block.module, then i agree, we can remove page.tpl.php, and change the 'page' element in system_element_info() to not have '#theme' set to 'page'

Gábor Hojtsy @ 6:21 well, the idea is that themes could still define layouts in tpl.php or twig or whatever if they prefer, we would not enforce Drupal layouts to be created with a Drupal UI or geneated via a code generator from a .yml

Alex Bronstein @ 6:21 Gábor Hojtsy: Also, sdboyer is working on replacing drupal_render_page() entirely with something more Symfony oriented

Gábor Hojtsy @ 6:21 we want to make themers happy that they can do custom stuff if they want
so themes could define any number of layouts in .tpl.php => UI for selecting from those => Drupal page render needs to pick the layout execution based on the config

Alex Bronstein @ 6:23 yes, i still think we'll allow layout plugins that render templates, and they can reuse functions in theme.inc to do that, like theme_render_template()
but not theme()

Gábor Hojtsy @ 6:23 layout execution might be "running" the .tpl.php file or the dynamic markup generator or whatever
well, there is a lot of logic in theme() for .tpl.php files that is outside of theme_render_template() like inherited templates for subthemes
I think :)

Alex Bronstein @ 6:24 which is exactly what is not wanted for layout plugins
themes can provide layouts, but the goal is for layouts to be decoupled from themes

Gábor Hojtsy @ 6:26 Alex Bronstein: yeah, well, I don't really see how "layout plugin" is defined from a theme point of view… I think we want to keep that simple still to provide layouts from themes

Alex Bronstein @ 6:26 so theme() can render templates and layouts can render templates, but theme() and layouts are decoupled

Gábor Hojtsy @ 6:27 Alex Bronstein: there are a lof of things happening in preprocess, process, etc. that is useful / needed for page rendering regardless of a .tpl.php or a dynamic layout
Alex Bronstein: if that is all abstracted out elsewhere, then sure
btw need to go in a few minutes, sorry :/
will post this discussion on the issue (http://drupal.org/node/1787942)

Themes could define their layouts in different theme engines... not sure.

Suppose a layout module should register a LayoutBundle for request
Also this should be applicable for response object, probably @Crell explains it better

Title:Make it possible for users to assign layouts to pagesAllow assigning of layouts to pages

Relabeling, I am pretty sure we all understand users/humans should do this :D

this is, essentially, a problem of attaching config to the routing system.

i'm thinking directly about this in discussions such as #1784040: NestedMatcher may be too slow and non-deterministic; i'm just about ready to work on the controller that binds these things together. i may piggyback on this issue :)

Issue tags:+Spark

Tagging with Spark.

Status:Active» Needs review
StatusFileSize
new49.8 KB
new34.39 KB
new12.58 KB
PASSED: [[SimpleTest]]: [MySQL] 42,193 pass(es).
[ View ]

Let me introduce you to.... page module! TADA! I think this will be controversial for a few reasons:

- **OMG, YET ANOTHER NEW MODULE**: well, trying to get this into the layout module added in #1787846: Themes should declare their layouts looked odd from many perspectives; that module deals with layouts which might be used for things that are not pages for example

- **OMG, THIS ONLY DEALS WITH PATH PATTERNS**: yeah, conditions like that for blocks are not yet abstracted and **they should be**, but they are not, so when they are, this could re-integrate with that at that point; this should support more conditions definitely; we don't have the infrastructure for that yet and @EclipseGC is working on that to the best of my knowledge

- **OMG THIS DOES NOT ACTUALLY WORK WITH LAYOUTS**: right, to make this patch independently work without #1787846: Themes should declare their layouts applied, I just hard-coded three layouts: foo, bar and baz, obviously this should change and the layout mapper from #1787846: Themes should declare their layouts should be included here once that issue is committed, and in that case, this code will very quickly turn into a great vehicle to actually apply layouts to pages; it currently emits useless dsm()s - fun stuff to prototype with

The patch comes with three silly preconfigured pages with patterns and associated layouts. The page patterns are proven to work manually. Given that we don't have any priority order for page patterns, the user pattern will never match, because the non-admin page pattern matches (and obviously one page can only use one layout after all). This is demonstrating an interesting problem that we need to resolve. Also I assume we should have a * pattern that cannot be deleted / removed and cannot be modified in terms of patterns and would serve as the absolute fallback. This is a very interesting thing to achieve if we want to make conditions pluggable and still want to make sure there is one generally applicable page.

I'm not sure about the desired abstractions here, I'm very confident others have stronger opinions :D The code I propose clearly provides this functionality now (without the layout being applied, which is in part already in the RTBC patch in #1787846: Themes should declare their layouts).

Screenshot fun:

LOOK MA, page library under structure (with some silly defaults)!
PageLibrary.jpg

Limited options and some foo/bar/baz layouts for you:
EditPage.jpg

Now bring your nerf guns out! :D

Wanted to add that I did not pick this UI out of the blue :) It was based on Bojhan's excellent work on the Page library prototype, see Page library page at http://bojhan.nl/drupal/prototype4/index.html (and Add new page right there).

Issue tags:+Usability, +B&L

This is an interesting development, it is a first step in what we set out to do. Although it clearly isn't near what we want, it is a minimalistic step that allows us to progress without introducing usability issues. I think the community needs to decide how we wish to continue on these SCTOCH things, I think it makes sense to start with small UI patches that establish the fundamentals.

A couple things on the actual UI:

  • We probably want to link the path in the table - even thought it could stretch several rows, lets worry about how to solve that later. Lets remove the machine_name from the table, that to my mind should mostly be on the object not the listing.
  • We should have a help page, or more useful descriptions for paths. Dharmesh clearly noted previously, that this part of the UI is often alien for first time site-builders.

Other than that, I currently see few issues. Obviously we need a more beautiful layout selector. I also think, that it should probably be between name and path. Instead of at the bottom

StatusFileSize
new14.13 KB
PASSED: [[SimpleTest]]: [MySQL] 42,201 pass(es).
[ View ]
new3.44 KB
new31.19 KB

Resolved Bojhan's concerns. Replaced machine name with paths (required an entity list controller override too). Also moved layout selection to inbetween title and paths. Screenshot from the first change, the second should be pretty obvious:

PageLibrary2.jpg

(Yes, there is some discrepancy around page titles, have not been able to figure that out yet).

StatusFileSize
new1.06 KB
new14.16 KB
PASSED: [[SimpleTest]]: [MySQL] 42,197 pass(es).
[ View ]

I should obviously use the right storage controller class, so that there is no page error :) This fixes the titles. Also, previous patch lacked hunk where the new controller is actually used. This should be good with these now :) [Imagine screenshot saying "Page library" in place of "Error" above].

StatusFileSize
new31.68 KB
new811 bytes
new14.3 KB
PASSED: [[SimpleTest]]: [MySQL] 42,193 pass(es).
[ View ]

Added "Except:" to where the paths are to be applied negatively, so the confusion around "Not admin pages" with patterns "admin" and "admin/*" will not come up. Noted by @webchick over in IRC, discussed with Bojhan. It does become a pretty extensive table row obviously. @Bojhan said above not to be bothered about this, so I'm fine not to be bothered for this.

http://drupal.org/files/LibraryExcept.jpg

StatusFileSize
new1.01 KB
new14.3 KB
PASSED: [[SimpleTest]]: [MySQL] 42,193 pass(es).
[ View ]

Renamed "pages" to "paths" in the radio labels, because @tkoleary pointed out that "pages apply to pages" is not a very good pattern to think of :D See interdiff.

Status:Needs review» Active

so, Gabor has gone the direction of focusing on the UI for managing this stuff, which definitely is something we need to tackle. however, some aspects of it are putting the cart before the horse (but only a bit), as we have not yet defined the final mechanism by which we actually DO the binding of these things together.

fortunately, i've now finally gotten the preliminary work on that up: #1812720: Implement the new panels-ish controller [it's a good purple]. so, these systems which build 'pages' out of block instances placed onto layout instances have a config object to drive towards: a Display (name subject to change :P).

i haven't quite yet defined the structure of the display object itself, but we can get to that RIGHT now, and we know how it connects back to everything else.

i'm not sure what that means about the direction of this issue - it seems to me that the UI work absolutely ought to continue, but we should talk ASAP about the config object you're saving to so that it can integrate with the new approach.

+++ b/core/modules/page/lib/Drupal/page/PageFormController.phpundefined
@@ -0,0 +1,105 @@
+      '#disabled' => (bool) $page->id(),

Use !$page->isNew() also see #1813832-2: Entity wrongly checks existence of ID in isNew() method

+++ b/core/modules/page/page.moduleundefined
@@ -0,0 +1,159 @@
+    'entity keys' => array(
+      'id' => 'id',
+      'label' => 'label',
+      'uuid' => 'uuid',

But what's about $visibility;$paths;$layout;

Title:Allow assigning of layouts to pagesAllow assigning layouts to pages
StatusFileSize
new15.17 KB
PASSED: [[SimpleTest]]: [MySQL] 42,637 pass(es).
[ View ]
new4.13 KB

Now that [#178784] is committed and #1812720: Implement the new panels-ish controller [it's a good purple] is in very early stages, I've introduced direct layout assignment in the patch. I also added two sample layouts on top of the three sample page assignments. These are *obviously* demonstrations and not targeted at being committed.

Status:Active» Needs review

StatusFileSize
new625 bytes
new15.16 KB
PASSED: [[SimpleTest]]: [MySQL] 42,635 pass(es).
[ View ]

@andypost: implemented !$page->isNew() as suggested. I don't understand your other suggestion. As per http://api.drupal.org/api/drupal/core%21includes%21entity.api.php/functi..., the 'entity keys' property should contain mappings for a few key pieces, like id, uuid, label, etc. It should not contain references to arbitrary data elements of the entity, they are specific to the config entity and are not keys.

@sdboyer set up #1817044: Implement Display, a type of config for use by layouts, et. all for the displays. Looks like in line with that plan this issue would relate paths to those display config objects (and create those display config objects in the background).

Nice ! Just a note: I would find "Pages" simpler and more clear than "Page library".

StatusFileSize
new15.32 KB
PASSED: [[SimpleTest]]: [MySQL] 46,109 pass(es).
[ View ]
new1.45 KB

Changed paths to conform to the current '/manage/' style paths and local tasks as in the best practice followup at #1588422: Convert contact categories to configuration system. I believe/hope I'm applying the best practices right.

That interdiff looks good to me. Didn't review the whole patch.

StatusFileSize
new17.68 KB
PASSED: [[SimpleTest]]: [MySQL] 46,268 pass(es).
[ View ]
new6.95 KB
new39.07 KB

Updated patch with entity list controllers integrated with tabledrag. We need weight support so we can decide which pattern to apply when conflicts arise, like in the sample data. The saving of the weights does not seem to actually work proper yet, but that is not the point so far :) The tabledrag works as intended. I discussed this with @fubhy and @timplunkett earlier today and its not very nice at all. Not sure how could we properly bridge the function based drupal_get_form() and form theming with the OO based entity list controllers without much pain. For now:

- The page needs to be a form since it has weights that can be changed and saved => instead of rendering a table, I needed a form callback
- Consequently I needed a form save callback;
- I needed to introduce a hook_theme() in my module for the form table theming
- I needed to actually introduce the theme function for the form

I did channel the form getter and submission handler to the OO listing code but the theming I left in the function because if I reference the OO code from there, its hard to imagine themers will ever sanely track that down :/

Obviously I also introduced the weight on the config object and weight defaults in the sample config objects. All is changed on the UI is:

PageTableDrag.jpg

If we expect the discussion of who's responsibility it is to do what here and where to put abstractions would go on long, then we should break this out to its own abstraction issue. If we consider to quickly come to an OK solution then we can introduce a separate base class that supports tabledrag and use that here. I integrated the code for now to show the differences. I used the existing entity list controller based implementation, but only reused the operations building, nothing else was useful to reuse here (I needed different header labels due to the added weight and the form needed to be built, not the table, so the row and form renderers were useless).

Feedback welcome!

StatusFileSize
new574 bytes
new17.65 KB
PASSED: [[SimpleTest]]: [MySQL] 46,270 pass(es).
[ View ]

Truth is, the weights are properly saved. The listing code does not take weights into account when loading entities, so they are not loaded in weight order. Did not fix that yet, but removed some crap debug code :D So once entities are listed in order of weight, this should flawlessly work (despite the ugly underpinnings). I'm going to sign off for the day, so feedback and help in solving entity ordering is welcome :)

StatusFileSize
new18.18 KB
PASSED: [[SimpleTest]]: [MySQL] 46,285 pass(es).
[ View ]
new1.29 KB

Now fixes the ordering in the table as well. Looks like Drupal's drupal_sort_weight() only supports arrays, so we either patch that and make it more general or need our own object-compatible sorting function. I opted for the last option here. Obviously it will need to be generalised later. So this now fully demonstrates tabledrag with the entity list controllers, even though I'm not happy with how the responsibilities are separated, hook_menu() form and theme hooks require this kind of separation. Drupal is just not OO yet. So any comments on how to make this better are welcome.

@Gabor, There's already sort method in Drupal\Core\Config\Entity\ConfigEntityBase::sort() see ConfigEntityListController
But we have strange issue with sorting #1799600: Add test of sorting for configuration entities

@andypost: thanks! How would we use it though? The Page class is the instantiable extension of ConfigEntityBase, so when we get a list of pages, would you pop the first one and invoke the sorting with it?! The sorting is not implemented on the list class currently, its on the item's class :/

+++ b/core/modules/page/lib/Drupal/page/PageListController.phpundefined
@@ -0,0 +1,87 @@
+use Drupal\Core\Entity\EntityListController;
...
+class PageListController extends EntityListController {
...
+   * Overrides Drupal\Core\Entity\EntityListController::load().
+   */
+  public function load() {
+    $items = $this->storage->load();
+    uasort($items, 'page_sort_weight');
+    return $items;

If you extend ConfigEntityListController instead you shouldn't need to override load(), ConfigEntityList:sort() should be exactly what you need

StatusFileSize
new1.76 KB
new17.44 KB
PASSED: .
[ View ]

@timplunkett: great tip :)

glancing through the patch here, i'm wondering how we fit Displays in. do the page config objects hold a reference to a Display config object?

#28 looks nice

@sdboyer: I would assume so, however so far the display object would also only refer to the layout, since the are no blocks as plugins yet to relate to in the display...

@Gábor Hojtsy - ugh. yeah, i tend to forget that since the code you're working on here can actually work *right now*, it is blocked by #1535868: Convert all blocks into plugins.

nevertheless, it seems wasteful to me to develop a ton of logic here that is likely to be be removed, or at least significantly altered, when these other things land. then again, i haven't taken a detailed look at the patch, and maybe the reworking wouldn't be too significant. it's really just that the discussion of yet another ConfigEntity raised a concerning flag.

@sdboyer: well, unless we could package the page conditions somehow into the display, we need this yet another config entity, the page entitiy. I think this is going to be reasonably easy to work out once blocks as plugins and displays land, so we can convert pages to refer to displays and blocks. So far this patch seems to be furthest along. So I would prefer to continue write tests here and let this land to be adapted latermto displays and blocks as plugins (the later of which will even need a massive new UI for block assignment).

@Gábor Hojtsy - yeah ok, that's fair. we will be looking to package block-level conditionality into displays, but not where pages attach. that's a fundamentally different layer of data.

actually, yeah. this makes perfect sense - not sure why i didn't see it before. when we convert this to the new router system, we probably won't have to get the page config entities at ALL at runtime - only at router compilation time, when we'll convert it all to first-class routing logic.

brilliant. carry on. sorry to derail :)

We talked briefly today about the possibility of having the paths be set with views-style patterns, e.g. foo/%/% rather foo/*, but that can easily be a followup.

So I'm trying to work on the Responsive Layout Designer patch in the D8 Spark build, but I ran into the same error that Gàbor got ("500 Fatal Error") when we were at the sprint at BADcamp in SF when you tried to show us the page manager.

Here's the stack trace: https://gist.github.com/4048387

I bisected the issue using the attached patch applied to 8.x at 3b4548f90a192bac1226bef1cabeaf7945e6bb1f. That was the good state. I then merged in the rest of the 8.x commits to HEAD (9c79f870bbdb743a863c12e09dd58ac360ec2a77) -- that's the bad state. Bisecting brought me down to this commit: 0142ce1f40c4fe6ba026ab8ecc7a8f2f5f55d0cc, where page stuff breaks, and the commit just before it, where page stuff works: b9c4744e0296550f1fcf16df7594a954d734eb86.

I think the error has to do with the introduction of a service controller.

Specifically, PHP fails near line entity_get_controller function of entity.inc

https://gist.github.com/4048393

$type_info['controller_class']; is being referenced as an array, but the value of $type_info is the string "page" when the code runs. $controllers is an empty array.

Perhaps some new property of an entity was introduced in 0142ce1f40c4fe6ba026ab8ecc7a8f2f5f55d0cc that we need to reflect in the Page module?

StatusFileSize
new283.67 KB
FAILED: [[SimpleTest]]: [MySQL] 47,610 pass(es), 695 fail(s), and 168 exception(s).
[ View ]

The testing patch for #36.

Status:Needs review» Needs work

The last submitted patch, controller.patch, failed testing.

@jessebeach: I don't think the error you are experiencing or the patch you posted belongs here. I'm looking into rerolling and retesting the dynamic layout patches these days, and will get to the responsive layout one too. It it certainly not yet converted to the new annotated core entity classes, which in itself should break it completely.

Status:Needs work» Needs review

#28: pages-28.patch queued for re-testing.

StatusFileSize
new17.87 KB
FAILED: [[SimpleTest]]: [MySQL] 48,086 pass(es), 0 fail(s), and 9 exception(s).
[ View ]
new5.51 KB

Updated pages patch to latest core and other changes carried over form similar codebase in #1813910: Add region module to Drupal core (for editable responsive layouts):

- moved page entity to annotated plugin
- renamed delete_confirm to confirm_delete as per core standards
- added an initial short help text
- removed $items init from hook_menu

BTW did reproduce the additional uncaught exception that @jessebeach saw. It seems to be due to layout_theme() in the core module which is invoked in the module enable process before the dependency injection container got a chance to register the layout manager, so the theme items are not properly returned. Not sure how to solve this :/ The workaround I got was to comment out layout_theme() at first, that let the module being enabled and then rerun it later through a cache clear.

Status:Needs review» Needs work

The last submitted patch, pages-41.patch, failed testing.

All the test fails are due to #1831774: Config import assumes that 'config_prefix' contains one dot only, not introduced with this patch.

Status:Needs work» Postponed

The approach here was deemed to have significant usability issues, since you cannot actually create a page, but instead you map layouts to path patterns. A simpler page creation system is proposed in #1840500: Add simple landing page creation capability. We might or might not be able to return to here after feature freeze, depending on how you define feature freeze.

Version:8.x-dev» 9.x-dev
Status:Postponed» Needs work

Given feature freeze, this is not going to happen in Drupal 8. It is possible Panels will serve this purpose in Drupal 8. Moving this to Drupal 9.

ctools is alive again! Page manager does nearly the same

i do think that the assigning of layouts to pages, specifically, will be a task for contrib in D8. i'm actually happy about that (working on a blog post that explains why). but we'll want to grab the work that's been done here on the editor, i think, and use it for admin/structure/blocks. that's why i marked #1841584: Add and configure master displays back to a task and 8.x.

Issue tags:-B&L+Blocks-Layouts

Assigning the Blocks and Layouts tag that is used everywhere else.