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
Comment | File | Size | Author |
---|---|---|---|
#64 | 1787942-64.patch | 17.93 KB | MerryHamster |
Comments
Comment #1
Gábor HojtsyJust had a quick discussion with @effulgentsia about this:
Comment #2
andypostThemes 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
Comment #3
Bojhan CreditAttribution: Bojhan commentedRelabeling, I am pretty sure we all understand users/humans should do this :D
Comment #4
sdboyer CreditAttribution: sdboyer commentedthis 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 :)
Comment #5
webchickTagging with Spark.
Comment #6
Gábor HojtsyLet 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)!
Limited options and some foo/bar/baz layouts for you:
Now bring your nerf guns out! :D
Comment #7
Gábor HojtsyWanted 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).
Comment #8
Bojhan CreditAttribution: Bojhan commentedThis 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:
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
Comment #9
Gábor HojtsyResolved 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:
(Yes, there is some discrepancy around page titles, have not been able to figure that out yet).
Comment #10
Gábor HojtsyI 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].
Comment #11
Gábor HojtsyAdded "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.
Comment #12
Gábor HojtsyRenamed "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.
Comment #13
sdboyer CreditAttribution: sdboyer commentedso, 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.
Comment #14
andypostUse
!$page->isNew()
also see #1813832-2: Entity wrongly checks existence of ID in isNew() methodBut what's about $visibility;$paths;$layout;
Comment #15
Gábor HojtsyNow 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.
Comment #16
Gábor HojtsyComment #17
Gábor Hojtsy@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.
Comment #18
Gábor Hojtsy@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).
Comment #19
jide CreditAttribution: jide commentedNice ! Just a note: I would find "Pages" simpler and more clear than "Page library".
Comment #20
Gábor HojtsyChanged 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.
Comment #21
tim.plunkettThat interdiff looks good to me. Didn't review the whole patch.
Comment #22
Gábor HojtsyUpdated 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:
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!
Comment #23
Gábor HojtsyTruth 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 :)
Comment #24
Gábor HojtsyNow 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.
Comment #25
andypost@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
Comment #26
Gábor Hojtsy@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 :/
Comment #27
tim.plunkettIf you extend ConfigEntityListController instead you shouldn't need to override load(), ConfigEntityList:sort() should be exactly what you need
Comment #28
Gábor Hojtsy@timplunkett: great tip :)
Comment #29
sdboyer CreditAttribution: sdboyer commentedglancing 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?
Comment #30
podarok#28 looks nice
Comment #31
Gábor Hojtsy@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...
Comment #32
sdboyer CreditAttribution: sdboyer commented@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.Comment #33
Gábor Hojtsy@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).
Comment #34
sdboyer CreditAttribution: sdboyer commented@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 :)
Comment #35
xjmWe talked briefly today about the possibility of having the paths be set with views-style patterns, e.g.
foo/%/%
ratherfoo/*
, but that can easily be a followup.Comment #36
jessebeach CreditAttribution: jessebeach commentedSo 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?
Comment #37
jessebeach CreditAttribution: jessebeach commentedThe testing patch for #36.
Comment #39
Gábor Hojtsy@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.
Comment #40
Gábor Hojtsy#28: pages-28.patch queued for re-testing.
Comment #41
Gábor HojtsyUpdated 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
Comment #42
Gábor HojtsyBTW 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.
Comment #44
Gábor HojtsyAll the test fails are due to #1831774: Config import assumes that 'config_prefix' contains one dot only, not introduced with this patch.
Comment #45
Gábor HojtsyThe 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 blank page creation capability. We might or might not be able to return to here after feature freeze, depending on how you define feature freeze.
Comment #46
Gábor HojtsyGiven 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.
Comment #47
andypostctools is alive again! Page manager does nearly the same
Comment #48
sdboyer CreditAttribution: sdboyer commentedi 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.
Comment #49
oadaeh CreditAttribution: oadaeh commentedAssigning the Blocks and Layouts tag that is used everywhere else.
Comment #50
catchNot impossible to add in a minor version.
Comment #51
EclipseGc CreditAttribution: EclipseGc commentedThis is clearly postponed based upon version already and will need to evaluate what is happening in contrib when the time comes.
Eclipse
Comment #52
andypostComment #54
andypostThat really depends on #2296423: Implement layout plugin type in core
Comment #56
tkoleary CreditAttribution: tkoleary at Acquia commentedThe time has come. :)
Comment #59
andypostComment #60
andypostComment #61
jofitz CreditAttribution: jofitz at ComputerMinds commentedCan you provide details of why this requires a re-roll? The patch applies cleanly to 8.5.x.
Comment #64
MerryHamster CreditAttribution: MerryHamster at Skilld commentedReroll for 8.7.x
The patch from #41 applies cleanly to 8.7.x.
Comment #65
MerryHamster CreditAttribution: MerryHamster at Skilld commentedComment #67
savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commented