Problem/Motivation
The layout module was removed in #2053879: Remove layout.module from core
We're not proposing adding it back in this issue, but instead just putting a very simple plugin manager for a layout plugin type into core.
Many modules need to declare or use layouts, for example: Display Suite, Panels, Layout, etc. We want to avoid the situation in D7 and earlier, where each module has their own silo'ed way of doing this.
We already have created the Layout Plugin module in contrib, which several modules (including Panels and Display Suite) have already standardized on and now have interchangeable layouts!
But we think it should be in core, in order to:
- Promote standardization on a single layout plugin type in contrib. It's sooo simple that it's almost a shame that it takes a whole module that other modules have to depend on. :-)
- Take the first little baby step towards adding more complete layout functionality in core. We're continuing to work on layouts in contrib, with modules like Tim Plunkett's Page Manager module, but once those have matured we'd like to progressively merge parts of it into core. This is the most minimal step in that direction.
Proposed resolution
Move Layout Plugin into core as an experimental module.
In 'layout_plugin', layouts are declared in a .yml file and simply give a template and library definition. Here's an example layout .yml file:
http://cgit.drupalcode.org/layout_plugin/tree/modules/layout_plugin_exam...
If you want to learn more about declaring layouts with layout_plugin here are the docs:
https://www.drupal.org/node/2578731
Remaining tasks
User interface changes
In this proposal, there is no user interface!
API changes
This would include API additions (no changes) to support this plugin type, ex: LayoutPluginInterface
, LayoutPluginManager
, etc.
Comment | File | Size | Author |
---|---|---|---|
#188 | 2296423-layout_discovery-188.patch | 66.14 KB | tim.plunkett |
#188 | 2296423-layout_discovery-188-interdiff.txt | 732 bytes | tim.plunkett |
#182 | 2296423-layout_discovery-182.patch | 66.02 KB | tim.plunkett |
#182 | 2296423-layout_discovery-182-interdiff.txt | 858 bytes | tim.plunkett |
#160 | layout-interdiff.txt | 41.58 KB | tim.plunkett |
Comments
Comment #1
dsnopekfrega has refactored just the plugin part of his 'layout' module on GitHub into a sandbox on Drupal.org. Updated the issue summary to reflect where the continuing work on that will be!
Comment #2
dsnopekIf we can't get this into core, another possibility is merging it into the Layout module. However, that depends on what that team's plans are for the future: #2317333: Roadmap / future plans for layout.module
Comment #3
seanrI definitely support this - it's a pain having multiple different kinds of layout engines in a single site, arbitrarily chosen by whatever dev I inherited the site from. :-/
Comment #4
dsnopekUpdate issue summary now that we've promoted layout_plugin to a full project.
Comment #5
andypostThere's a couple of questions:
1) Any reason to separate layout yml and plugin? Looks that plugin annotation could define the same properties of layouts.
2) Region definition in yml has only Label so why not merge them into one line:
Comment #6
dsnopekThe layout yml is actually an alternate discovery mechanism for the plugin, they're the same thing. See the
LayoutPluginManager
:http://cgit.drupalcode.org/layout_plugin/tree/src/Plugin/Layout/LayoutPl...
It just wraps it's
$this->discovery
inYamlDiscoveryDecorator
.So, developers have the choice of implementing either the layout yml (for the simple case where it's just a template) or a plugin (where it's more advanced, like some kind of flexible layout builder).
So, "label" is the only required attribute of a region that we expect all layouts to implement. But we allow custom layout plugin classes to use other attributes on the region configuration for any purpose. For example, frega's layout module on GitHub implements a flexible layout builder which uses other region arguments, and we may need something similar for all of Panels layouts.
This should probably be improved in the documentation!
Comment #7
andypost@dsnopek thanx for clarity and taking that!
2) is a question still because core uses "third_party_settings" to extend any yml because of schema issues.
Actually I think that part needs more work
Comment #8
dsnopekSo, I think this is different. This is plugin configuration, not configuration in CMI. Also, it's not really "third party" since any additional settings will be read by the plugin class itself, so it's still first party. The extra keys just aren't defined by the "standard interface" and plugin classes can define some new ones of their own.
Comment #9
YesCT CreditAttribution: YesCT commentedneeds a beta evaluation. added link to instructions to do that in the summary.
Comment #10
dawehnerLooks certainly more than 8.1.x
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commented:'(
Comment #12
dsnopekModernizing the issue summary for the current state of layout_plugin.
Comment #13
dsnopekHere's a patch that just copies layout_plugin into core and removes the example modules and some deprecated code. Please let me know what you think!
Comment #15
swentel CreditAttribution: swentel commentedMostly nictpicks
I don't know exactly what our coding standards say, but sometimes the @file docblock starts at the top, sometimes with a new line in this patch. Looking at core, it seems a newline is the standard, so let's make it consistent everywhere.
should we add a @see here to Layout::layoutPluginManager()->alterTheme... ?
newline to much
One newline to much
same here
I should probably create a patch for the actual module now ;)
The biggest question is: do we need some sort of upgrade path? If a site upgrades to 8.1.x and it had the contrib enabled, how will extension discovery behave then after a full cache clear. Will it pick up the contrib module or the one from core (my guess is the contrib because that's how it works now IIRC).
Another name for the module wouldn't work since the contrib modules would still have their dependencies set to layout_plugin, or we need to coordinate releases, but that seems hard, and we don't control custom code either right ? Moving the code into system module wouldn't work either for the dependency problem as well for the modules.
Other than that, RTBC of course ;)
(oh irony, we need a hook help haha)
Comment #16
swentel CreditAttribution: swentel commentedFix my nitpicks and composer test. No hook_help() for now though ...
Comment #18
swentel CreditAttribution: swentel commentedGuess this needs a change record too.
Comment #19
benjy CreditAttribution: benjy at PreviousNext commentedShould this move to \Drupal?
Needs fixing?
Comment #20
dsnopekRe #19.1: If layout_plugin stays being a separate module, I don't think we should put that method on \Drupal because it will only return a valid value if the module is installed.
However, given how simple layout_plugin is, I could see an argument for merging it directly into \Drupal\Core (then adding the plugin manager to core.services.yml, and putting the hook implementations into 'system') in which case putting that method on \Drupal totally make sense!
The block system has it's code in \Drupal\Core and its plugin manager is registered in core.services.yml, so there is a similar-ish precedent! And this would solve our issues with an upgrade path from a contrib 'layout_plugin' module to the core code. But it's really the hook implementations in layout_plugin.module that give me pause... I think we need feedback from a core committer on if this would be acceptable.
Re #19.2: Ah, yes! I almost totally forgot about that. :-) I'll fix it in my next patch.
Comment #21
dsnopekHere's a patch that adds a hook_help() and fixes #19.2.
Comment #23
dsnopekOk, second try at getting the hook_help() right!
Comment #24
benjy CreditAttribution: benjy at PreviousNext commentedAdding the committer feedback tag to get some input on #20.
Comment #25
dawehnerIMHO we don't need the layout package or do we? IMHO it belongs into the main one as all the other ones?
I kinda dislike to have those static wrappers
module / theme
Any reason to not also support js files? I could imagine JS files could be useful for layouts as well?
What is the point of having just one key in the value? Why is it not a map of machine name and label like in
.info.yml
files?Shouldn't it be 'A remaining' instead?
IMHO we should get rid of that, it just sets a bad example.
Does that interface have to expose all of the subinterfaces automatically? For example the ConfigurablePluginInterface and PluginFormInterface fells totally optional and orthogonal.
Is there a reason the LayoutManager doesn't leverages the CategorizingPluginManagerInterface machinery?
This is confusing, we just allow absolute css paths?
IMHO you could split up this test method into multiple single ones to be more descriptive.
Comment #26
andypostSuppose in real would layouts should be a part of library so separate CSS/JS assets are question, here a question howto aggregate this assets
Also relation with entity displays a bit depends on that issue cos they could really a nice example of "usage" instead of "unusable module"
main code related concerns are about location of code:
1) base class instead of base implementation
2) interface definition
3) boilerplate code without traits
I find this name very technical, maybe Layout management or Layouts?
In long run that should be a part of core folder somehow
Please make it Experimental to polish in next minors
>this module does not do anything.
bad part of help...sentence
This is a part of interface, is not it?
I think this should be optional (empty array by default, not bool type)
Why so slightly theme, that can be a render element
$build['#type'] = 'layout' . '__' . $this->getPluginDefinition();
#settings and #attached
this should simplify default template
there should be traits somehow
Why that is not fallback implementation for other plugins
toRenderable() existing interface method
why not simply '#type' = 'layout'
in case of render element plugin is not needed
so this is a options of the layout or categorizable list?
any strong reason to expose theme implementations?
I suppose they defined by modules.
Library contains also JS and could carry SVG of preview
Comment #27
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedComment #28
Wim LeersI'm reviewing this specifically from the asset library system POV.
Let us please not do this.
We standardized on asset libraries in Drupal 8. Let's not knowingly add exceptions to that rule.
If even themes can do that (issue: #2377397: Themes should use libraries, not individual stylesheets, CR: https://www.drupal.org/node/2379475), then so can this module.
The Drupal base path? And with or without a leading slash? This is not sufficiently well defined.
And what about a HiDPI version?
"library name" is wrong.
Given the library
core/ckeditor
, "ckeditor" is the name, "core" is the extension, "core/ckeditor" is the library.I think this is supposed to be the latter.
"theme" vs "theme function", which is it?
This leads to the confusing code cited.
If the asset library is provided by a different extension, this is wrong.
Eh… this looks extremely bizarre.
What about CSS for separate media types? And why the
theme
SMACSS category, shouldn't this belayout
, since this is CSS for … layout plugins?https://www.drupal.org/developing/api/8/assets
All the more reason to let layout plugins be more specific, and not automatically generate asset libraries for them. One way to do something versus two.
Comment #29
dsnopekThanks, everyone, for all the review! I had a saved response for #25 from last week that I didn't quite finish (because I got caught up in other things) but now I've got a quite a bit more to chew on. :-) I'll do my best to follow-up ASAP!
Comment #30
tim.plunkettI think this should be in \Drupal\Core, as opposed to a module. There is no UI, nor will there ever be for this functionality.
Comment #32
dsnopekOk! I had these changes and response from 3 months ago - there's loads more changes I want to make based on some conversations with @tim.plunkett, but getting this out there because I have it. More to come!
@dawehner: Thanks for the review!
#25.1: The logic was the same as how modules that provide field types are in the "Field types" category. There are contrib modules like Radix Layouts and Bootstrap Layouts that also use the "Layout" category, and so this groups all this stuff together. However, I suppose the "Field" module itself is under the "Core" category! So, I'll move this to the "Core" category as well.
#25.2: I'm not attached to the
Layout::layoutPluginManager()
, it's just a convenience. If necessary, I'd be happy to remove it (that would solve the "should it be on \Drupal?" question from #19 as well. Leaving it in for now.#25.3: Fixed!
#25.4: We do support Javascript, you just have to declare your own asset library and use the 'library' key instead. The 'css' key is a convenience for super simple layouts, that declares the asset library for you. Using Javascript is an advanced enough that I'm (personally) fine requiring developers to declare the asset library for the Javascript themselves.
#25.5: This is to allow layouts to define their own extra data that could be attached to regions, to be used by the layout plugin for its own purposes. It's used by frega's flexible layout builder (which is now totally out of date and won't work any more :-/) and there's talk of putting "region weights" in the Panels layouts to allow the Panels region changing to work better. There's an attempt at an explanation further in the docblock which was quoted in #25.6, but with a type-o.
#25.6: Actually, the "An remaining" was meant to be "Any remaining". Fixed! Any suggestions on making this explanation clearer are welcome!
#25.7: Responded in #2 above. Happy to remove if others agree!
#25.8: Hm, yeah. So, you're thinking that layouts which support configuration would implement
ConfigurablePluginInterface
andPluginFormInterface
themselves and we'll do an instanceof check for them before trying to show the settings form or doing setConfiguration()? This makes sense to me. Moved those interfaces toLayoutBase
.#25.9: Heh, because I didn't know that existed. :-) Fixed!
#25.10: No, we just convert the relative path (from the module or theme) to a path relative to the Drupal install in
processDefinition()
. The slash is added because its required by the libraries asset API (where as$definition['path']
is a filesystem path we can dofile_exists()
on).Comment #33
dsnopekThanks, @Wim Leers, for your review in #28!
@tim.plunkett and I discussed #28.1 a month or so ago, and decided that you're right - if this is going to be part of core, it shouldn't provide a convenience feature to help people not directly use the library asset system. We don't want one part of core to undermine another. :-)
Here's a patch that simply pulls out that functionality - it doesn't address any of the rest of the review in #28.
Comment #35
swentel CreditAttribution: swentel commentedSo if we drop the css property, that would mean we have to create a new branch for DS because our default layouts use that feature,
Or we need to register it ourselves for backwards compatibility in the same branch ?
In both cases, I feel nervous dropping it because I don't know how exactly this will effect existing sites that already use DS.
Can we mark it as deprecated somehow to be removed in D9 ?
Comment #36
tim.plunkettI still believe this should not be a module, meaning these will move to system.module, which is very strange.
As far as #35 goes, we should talk/think more about that. Putting a BC layer into core seems strange. Perhaps the module could live on in contrib just to provide that?
Comment #37
dsnopek@swentel: A separate branch shouldn't be necessary - DS could start manually registering it's libraries right now! It's just an additional YAML file.
As far as backwards compatibility: we could make the layout_plugin module add a shim to support the 'css' property so layouts created by users continue to work?
Comment #38
dsnopekI agree with Tim in #36 that this shouldn't be a module and I may have an idea to remove the preprocess function, which I'll attempt at the sprint tomorrow. But those two hooks will still need to be in system in that case.
Comment #39
swentel CreditAttribution: swentel commented@dsnopek Sure, DS can register it for our layouts, and adding it then if wanted - we actually have a killswitch option to remove the css, oh irony :)
I was indeed more wondering about people who've created their own layouts and used DS as a (bad) example using a css file.
Granted, I guess that number will be fairly small, so maybe I'm worrying to much.
Comment #40
dsnopek@swentel: I think that's a valid worry, for sure! But I think we make the version of the contrib layout_plugin module act as a backwards compatibility shim after this merged into core.
Comment #41
dsnopekHere's a patch where I went through and tried to fix the docblocks for the
Layout
annotation class make them match withLayoutBase
to address Wim's concerns in #28.2, 3 and 4. #28.7 was already address by removing the automatic library registration in #33.#28.6: The
LayoutDefault
class is the one used if you register your layout via YAML. It doesn't need anything beyond what's inLayoutBase
, but it seems weird/wrong to have layout plugins which are instances ofLayoutBase
(ie. concrete instances of a base class). :-) MaybeLayoutBase
is doing too much and we should move some of that stuff intoLayoutDefault
? If nothing else, this reserves the possibility that the layouts registered via YAML could do something differently than theLayoutBase
.#28.5: Is a valid issue (thanks for catching it, Wim!) that still needs to be handled.
Comment #42
dsnopekHere's an attempt to fix #28.5!
Comment #44
dawehnerJust another driveby review.
This feels just wrong ... aren't we returning strings / translation objects?
Comment #45
dsnopekHere's a patch that kills
hook_theme_registry_alter()
and the preprocess function, but in order to do it it has to put render arrays under '#content' in the layout render array, which isn't super clean, but it allows removing a whole lot of code.Thanks @dawehner! My latest patch switches all of those to strings too.
Comment #46
dsnopekAlright! Here's a patch with the big move from a module to being under
\Drupal\Core\Layout
. It only requires minimal stuff added to the system module.Comment #47
dsnopekHere's a patch to remove a left-over function that should have been removed when I pulled out the automatic library registration in #33
Comment #48
swentel CreditAttribution: swentel commentedCan't we move this into a file in core/config/schema ? Otoh, if we can't move the call in system_theme, it kind of make sense for it to be here I guess.
Comment #49
dsnopekre #48: I think it could be moved there from a technical perspective! I'd love some feedback from someone on the core team if that's preferable or not?
Comment #50
jibranI think we should add a layout test module with some kernel tests to wire all this up and check it is really working. It'd be a good example for dev's as well.
Comment #51
dsnopek#50: I concur! In the contrib version of layout_plugin we have functional tests that depend on Page Manager, which we obviously can't use here, but kernel tests that go further than the unit tests would be really, really good. Or I suppose we could even do some functional tests with a completely contrived example (ie. configuring a set of three hard-coded strings to appear in layout regions), but I'm not sure it's worth all the extra boilerplate?
Comment #52
jibranIt's always worth it.
Let's move the config changes to
core/config/schema/core.layout.schema.yml
as per #48.Comment #53
andypostLooks missed release window
Comment #54
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedMoving layout schema into its own file as per #48.
Comment #55
andypostgreen, so now NW for tests and reviews
Comment #56
dsnopekHere's a new patch that has some kernel tests that actually renders some example layouts. The layouts were ported from the 'layout_plugin_example' module.
Comment #57
dsnopekYay, the tests pass!
The one thing that still isn't really tested is the config schema stuff. Here's a new patch that adds a config schema for the one layout with settings, but doesn't add tests for it yet. I'm not super sure how to test that (without building a whole thing to configure and save layout configuration) so if any one has any ideas, please go for it!
Comment #58
mlhess CreditAttribution: mlhess as a volunteer commentedComment #59
romainj CreditAttribution: romainj as a volunteer commentedIssue https://www.drupal.org/node/2786541 is a start to a UI that list registred layouts provided by any module/theme. It could be improved for sure but at least it helps to know all available layouts.
Comment #60
dsnopekThere's been some minor changes in layout_plugin in contrib that need to get integrated into this patch, so marking as "Needs work" so that we don't push forward without doing that. I'd still love some more reviews of the current patch, though! :-)
Comment #61
tim.plunkettRerolling as an experimental module again. Yes we'd like to get it into \Drupal\Core eventually, but this will expedite things.
Made some tweaks, and removed some things. It now works for DS and for field_layout (#2796173: Add experimental Field Layout module to allow entity view/form modes to switch between layouts). Need to decide if we want to include getLayoutOptions() or not.
Comment #63
tim.plunkettCopied a bunch more code over from layout_plugin.
Comment #64
tim.plunkettThis patch incorporates the changes due for #2749375: LayoutBase incorrectly implements PluginFormInterface and ConfigurablePluginInterface and #2699841: Provide better API for getting information about layouts (without putting more methods on the interface)
It fixes the interface of LayoutInterface, and it switches from array-based definitions to object-based definitions.
Comment #65
tim.plunkettHad to reimplement CategorizingPluginManagerInterface directly due to object-based definitions.
Added test coverage for everything in LayoutPluginManager.
Comment #68
tim.plunkettTHAT old chestnut
Comment #70
tim.plunkettApparently PHPUnit is "broken" in 5.5/5.6: If you have an expectation of an exception, it should ignore all other predictions.
This works fine in PHP 7.
Working around it!
Comment #71
tedbowLooking good!
Here is a patch that is mostly doc fixes.
I will continue to look at not non-nit things.
Comment #72
tim.plunkettMore nits I noticed.
Comment #73
tim.plunkettOkay final set. Going to try and stop now :)
Comment #74
jhedstromIf the theme implementations are meant to be optional, could this instead be a separate interface that relevant layouts could implement?
This could could then just use
instanceof ...
:Comment #75
tim.plunkett99.9% of these plugins will be defined via YAML. There is no mechanism for them to choose which implementation of LayoutDefinitionInterface they use.
Also, those methods are really only used internally by the manager themselves.
If you feel strongly about inventing a new mechanism for that, I can try. But otherwise I'd just as soon leave it as is.
Comment #76
jhedstrom@tim.plunkett ah, I get it now. I hadn't made the connection to the yaml definitions. I don't feel very strongly, it just stood out as a potential simplification :)
Comment #77
jhedstromThe IS needs to be updated to reflect the experimental module direction. As I understand it, there will then need to be a meta issue that tracks remaining tasks to make it non-experimental...
Comment #78
tim.plunkettUpdated. There is a parent issue in the Ideas queue, it needs more detail.
Comment #79
andypostThe only confusing here is usage of *theme* in variables and methods, I suggest to use themeHook
Also wondered mix of template & them, not sure it's possible to use template without themehook defined in registry
I'd better named this as
$theme_hook
maybe add plugin definition provider by default?
Any reason for that? just for typehinting...
This is a bit confusing.
How it's possible to have template without theme hook?
Also I think getThemeHook() better name
ThemeHook to prevent confusion with theme (theme could be provider)
any reason for empty file, maybe better to leave this without library for testing layout without library
Comment #80
dsnopekThanks @tim.plunkett for pushing this forward! I'm super behind on reviews here -- sorry about that -- but I'm going to start by looking at changes in this patch relative to my last core patch in #57, to try and make sure we aren't losing anything from previous reviews/changes here.
The current patch is going back to a theme hook based on 'render element' (like in the contrib layout_plugin). Switched to using 'variables' back in #45 per review from #36, so that we could elminate the
hook_theme_registry_alter()
(which also been added back in the current patch) to reduce the number of hooks it needs to just one and move it into\Drupal\Core
rather than having it be a module.Since the ultimate goal is still for it to not be a module and move it into
\Drupal\Core
, I think we should stick with that change, so we aren't changing the theme hook definition for all layouts at that point when other code depends on it. (We didn't make this change in the contrib layout_plugin for backcompat figuring we can make the transition with the transition to core and the other changes it needs).This (and the code its calling) is what we can eliminate by switching from 'render element' to 'variables' in the point above.
Why is this being removed? Is there another property on the definition that's replacing this?
This is a valid accessibility problem (originally raised by the UC Berkeley accessibility team for Panels in D7 - which is still unresolved there :-/) and I'd prefer to start out on the right foot in D8!
Why was the 'icon' removed? This is used in both Panels and DS.
Why was this removed? It was added in #42 in order to address #28.5
Ditto
Comment #81
dsnopekOnly one bit of review when looking at the latest full patch:
Do we really want/need an interface for a value object? I worry this could limit us changing or evolving the LayoutDefinition, similar to the problems with FormState/FormStateInterface
Comment #82
tim.plunkett#79
1) Good idea! Changed.
2) See \Drupal\Core\Plugin\PluginDependencyTrait::calculatePluginDependencies(), this is the responsibility of the caller
3) Just typehinting.
4) Indeed confusing, I simplified this.
5) Yep.
6) Good idea, removed.
#80
1+2) Thanks! I really hated that code anyway.
3+4+5) Restored
6) Switched to using 'config_dependencies' as documented by \Drupal\Core\Config\Entity\ConfigDependencyManager and used by \Drupal\Core\Plugin\PluginDependencyTrait::calculatePluginDependencies()
#81 Also a very good idea, did that
Already ported my field_layout work to this, and it works 100%. Now going to try converting DS and Panels!
Comment #83
drumm(Seeing if a new comment triggers testing here.)
Comment #84
drumm(Sorry for the noise, one more after this.)
Comment #85
drummComment #86
tim.plunkettWorked on converting Display Suite and Panels to the new system, each module has an issue now:
#2821201: Update Display Suite for layout_plugin in core
#2821226: Update Panels for layout_plugin in core
While doing the conversion, I made some changes to the new core module:
Opened #2821189: Allow object-based plugin definitions to be processed in DerivativeDiscoveryDecorator and #2821191: Allow object-based plugin definitions to be processed in PluginDependencyTrait as follow-ups in addition to #2818653: Allow object-based plugin definitions to be processed in DefaultPluginManager::findDefinitions()
Added code to handle manually declared layout classes with leading slashes
Allowed arbitrary properties to be set on the definition, the same way as EntityType
Fixed the schema name to be layout_plugin.settings, was layout.settings
Restored a simplified version of getLayoutOptions() to the manager
Added a new getRegionLabels() to the definition object to replace the generated 'region_names' property in contrib.
Comment #87
dsnopekI don't really have an opinion about the new
DerivablePluginDefinitionInterface
andObjectDefinitionContainerDerivativeDiscoveryDecorator
, other than to say that derivatives are very important for layout_plugin! So, so long as they work, I'll leave review of those to their independent issues.But as for the rest, the changes in #82 and #86 look great to me and I don't have any further review. :-) The patches to Panels and DS both look great too.
I think this ready to go up to framework/product owners and committers to look at, so, RTBC!
Comment #88
tim.plunkettYay! Added a change record https://www.drupal.org/node/2821240
Comment #89
tim.plunkettI think this qualifies as a major feature, considering the follow-ups are major, and Panels/DS are pretty widely used.
Comment #90
tim.plunkettNoticed one further issue while updating field_layout with the recent changes.
Because of this change, layouts containing form elements stopped working properly. #content is needed as a key because of the change to hook_theme, switching from 'render element' to 'variables'. But some mechanism is needed to allow forms to be processed by layouts AND the form builder.
Comment #91
tim.plunkettOne mistake in the PASS patch, meant to delete this.
Comment #93
dawehner<3
We can remove that later, as its an experimental module, right?
It would be nice to have that a generic feature of the plugin system
It is a bit weird that we care about that.
This is quite jerkable code.
Can we have a todo to move this to core? This sounds like a great idea!
Given that most layouts won't be configureable, I'm wondering whether layouts should really be configurable by default.
Comment #94
tim.plunkett1) I've learned my lesson :)
2) Yes, completely removable
3) We absolutely need more helpers/support for object-based definitions, but I'm not sure how abstractable this is. Will ponder.
4) Yes it is weird, but DS has
if ($info->getClass() == DsLayout::class) {
and some passed and some didn't, better to keep it clean.5) Not sure what jerkable means in this context, but I'll take it as a compliment? :)
6) Sure, #2822752: Allow object-based plugin definitions to be created by non-annotated discovery
7) #2749375: LayoutBase incorrectly implements PluginFormInterface and ConfigurablePluginInterface says:
This is an issue for the contrib layout_plugin, and @dsnopek explicitly asked that I follow that for this conversion. And honestly it DOES make things easier.
Leaving at RTBC since I'm just adding an @todo
Comment #95
BerdirLooked at this for the first time, if things were discussed before, just tell me :)
Would it be worth to use array_multisort for this to avoid the @stuff? Should also be faster but I guess that makes no difference for the amount of layouts compared to everything else that's going on during plugin discovery.
still technically not allowed to do this :(
do we really need a default and a base class?
we use the original class in EntityType to be able reliably identify the entity_type_id based on the class. There is no usecase for this here, why have it?
in the initial layout module (which I'm still using..), there used to be a type, additionally to the category: https://github.com/frega/layout/blob/master/modules/layout_example/layou...
The idea was to have different types of templates, like a whole page and something like a small two-column layout that you'd embed somewhere.
Is there no use case for this in panels/DS? how are they solving this now? is every layout available for everything?
Comment #96
tim.plunkett1) This is identical to \Drupal\Core\Plugin\CategorizingPluginManagerTrait::getSortedDefinitions(), except with
$a->getCategory()
instead of$a['category']
. So if you think array_multisort would be a better fit, let's do that in both as a follow-up2) It's still all over core, is prevalent and necessary throughout the plugin system, and is valid in major IDEs.
3) No we don't.
4) No reason, just copied. Good call.
5) Opened #2822758: Introduce a way to distinguish between different types of layouts
Comment #97
xjmGreat to see this RTBC! Looks like this needs both framework manager review for the architecture and release manager review for the plan to make the plugin stable, so tagging for both per discussion with @webchick and @effulgentsia.
I think #2811175: [plan] Add layouts to Drupal probably at least needs to have the list of followups and the timeframe for completing them, at least.
Comment #98
dsnopekRegarding #93.7 (which @tim.plunkett also responded to in #94):
Even though most layout plugin implementations won't be configurable (although, all DS layouts are and all Panels layouts will be once they reach feature parity with D7), all code that uses layout plugins needs to take configurable layouts into account.
A module that wants to use layout plugins to render stuff can conceivably get passed any layout plugin, including configurable ones, and failing to take into account that the plugin might be configurable will cause everything to break as soon as configurable layout is used. (We've encountered this in contrib a couple time already.) So, from the perspective of consumers of this API, all layouts are effectively configurable!
Having
LayoutInterface
extendConfigurablePluginInterface
prevents all code using layouts from having to do$layout instanceof ConfigurablePluginInterface
in order to do something that effectively required in order to use the API.I kinda went in circles there, but hopefully that isn't total gibberish :-)
Comment #99
dawehnerI still don't get why the rare case have to dictate the common case. In my naivety I would assume that doing instanceof checks should be always possible.
Comment #100
tim.plunkett#97, I expanded #2811175: [plan] Add layouts to Drupal
#98/99, I don't feel strongly either way. Also keep in mind that this is experimental, and we can iterate on that.
Comment #101
alexpottJust some thoughts - not a final review at all.
Do we want to pass the arguments in?
Can use array_multisort() instead of the silencing. Bit more complex and less readable though... php--
Missing @param and @return.
Comment #102
tim.plunkettOminous.
1) Removing the unused params.
2) As I said in #96
3) Fixed.
Comment #103
xjmNotes from reading the patch:
This probably isn't the best user-facing name for the module (as the word "plugin" is overloaded for the average web admin), but since it's experimental, we can make picking the best user-facing name a followup that's part of adding the whole suite of intended modules.
What kinds of categories are these?
Also, I noticed there are a whole lot of public properties, but looking at other instances in core I guess that's normal for plugin annotation classes.
Typo: handler.
Seems odd to hardcode this in this specific plugin implementation.
Shouldn't this use
DIRECTORY_SEPARATOR
instead of/
?So based on this it looks like implementations can define any categories they want, and we compile the list from what they've defined.
So we would want to either do this before it's in beta, or provide BC when we do move it into core.
Moves the content?
Ah. It moves the content from
'content'
to'#content'
. Er. So why is it set in'content'
in the first place? I guess whatever reason could just be documented here.Other things that would need to be done before beta, or provide BC.
I think it would be good to have @Cottser or a theme maintainer review the implementation of the theme bits. Hopefully there is a test implementation or such later in the patch. :) Edit: And indeed there is.
These made me "hmm". Aren't they essentially making every protected property public API? Is this to support extensibility somehow, or?
Eh?
Going over https://www.drupal.org/core/experimental#requirements:
The one potential risk for data integrity problems I saw would be if there were bugs with config dependencies. The implementation looked correct (however many hours or days ago I read it), but that's one spot to keep an eye on for this API as it moves from experimental to stable. Couldn't think of any security risks.
Since the module provides an as-yet unimplemented API, and the only data impacted will be implementations of these plugins, there is no disruption to stable functionality. I did manually test and confirm that the module can be uninstalled. ;) (IIRC InstallTest does not test the experimental module group, or at one point it didn't. I can't remember if we actually fixed that or not.)
The API
includeser, has functional and unit test coverage, including a test theme implementation.Docs are fine. I made a couple notes above about questions I had while reading the patch (e.g. about the category key).
N/A, no UI.
Nothing else I found.
Usability: We should have a followup to consider the name for this module in the context of the other intended core module names on the roadmap, as well as considering whether having three modules (versus adding additional functionality to this one) is a concern. That's literally the only user-facing part to review since this is an API prototype.
Accessibility: N/A
Documentation: https://www.drupal.org/node/2619128 exists and is actually pretty thorough. Is it accurate for the core experimental implementation?
Testing: I spent a good hour reading the tests and did not catch any missing coverage.
Performance: No implications I could think of.
This will depend on @alexpott's review I guess. Edit: and @Cottser's if applicable.
There are a few small things in the issue above that might merit followups, e.g. the sorting thingy. I haven't read the issue thoroughly to ensure there aren't other points that might be missing a followup, but plenty of feedback does have followups filed already. There is a higher-level question in that the roadmap for this hasn't gotten "idea" approval, but it does seem promising to me.
We would expect this to be stable according to #2811175: [plan] Add layouts to Drupal (including the contrib implementations being available at least as a forward dev branch) by 8.5.0-alpha1, around August 2017. I think there is a good chance of success.
So other than checking for other needed followup work, backend and maybe frontend framework manager review, and approval of the overall plan, I didn't see anything that needed to be a blocker, and overall this is in great shape I think.
Comment #104
tim.plunkett1) I should describe this on the plan issue, but when this is stable it would cease being a module and move to \Drupal\Core\Layout, making the naming much less consequential
2) There's no telling what $category will be, but it is used for the optgroup of the select list.
Blocks have a similar category, their docblock says "The category in the admin UI where the block will be listed.", which isn't that much more informative.
Adding an @see to \Drupal\Component\Plugin\CategorizingPluginManagerInterface, which is what this is consumed by.
And yes, plugin annotation properties are always public
3) Fixed
4) Opened #2824655: Plugin definitions should store their class in a consistent manner (without leading slashes) to make this generic.
5) \Drupal\Core\Theme\Registry uses '/', and that's where this ends up. So does Views, which this mimics.
6) Yep! That's also identical code to \Drupal\Core\Plugin\CategorizingPluginManagerTrait::getCategories(), but for object-based definitions instead of arrays.
7) Yes.
8) Expanded the documentation a bit.
9) Yep.
10) Fine by me, but this matches how Views does things, and incidentally also how the former core Layout module did it
11) This is absolutely necessary for extensibility, DS uses this. Consider that the only other object-based definition, EntityType, also has these. Yes, it makes protected properties available, but at least you can track WHERE that's being called from.
12) This is related to 8. We need layouts for straight Render API as well as Form API.
It is the responsibility of those storing the data of configurable plugins to properly manage dependencies. Field Layout, DS, Panels, etc would need to do this. Core has a PluginDependencyTrait intended for this, it needs to be improved in #2821191: Allow object-based plugin definitions to be processed in PluginDependencyTrait
Comment #105
tim.plunkettMeant to add the new issue as a related one
Comment #106
xjmSo this merits discussion. If that's the plan, I think we should document it explicitly on the classes and interfaces that are going to move, and have an explicit followup issue. Presumably Panels and DS maintainers are in the loop about a planned BC break there, but we want to make sure everyone who implements the API knows that might happen and what they will need to change (hopefully just some use statements). And I guess we should have framework manager signoff on that end goal as well. And yeah, let's make sure the plan reflects that.
Comment #107
xjmMmm, also, what about upgrade paths for configured layout plugins? That could be nontrivial. We ran into this question as well for the
datetime_range
experimental module. Edit: Though this is less complicated than that since that has plugin backing config storage backing content storage, and this is just plugin backing config storage.Comment #108
tim.plunkettOpened #2824667: Update Layout Plugin handbook documentation for updating the docs.
#107, the plugin IDs won't change, only the classes they point to, so
\Drupal::service('plugin.manager.layout_plugin')->clearCachedDefinitions()
should be sufficient.Comment #109
xjm@alexpott and @Berdir, do you think we should have a followup to discuss using
array_multisort()
instead of@uasort()
forgetSortedDefinitions()
here and elsewhere?Comment #110
alexpottThere's a number of issues around sorting - see #2757207: Introduce "stable sort by weight" functions to sort arrays (and objects), #2759479: Proof of concept: Replace uasort() with dedicated stable-sort-by-weight functions, where possible and #2522712: Simplify Element::children() stable sort.. Imo since this the @ it should do the
array_multisort()
since the usage of the silence operator can hide other errors. There's only one runtime usage of@u.?sort
in core and thats to sort blocks which I guess is why @tim.plunkett thinks it is okay but in other patches we've been moving toarray_multisort()
when this occurs. For example the recent criticals around sorting config dependencies.Comment #111
tim.plunkettThat sort is admin-runtime not real runtime
So, blocker or follow-up? We'd need a follow-up anyway to fix the core trait.
Comment #112
tim.plunkettOpened #2824890: Convert uasort to array_multisort in \Drupal\Core\Plugin\CategorizingPluginManagerTrait::getSortedDefinitions() for the existing code, which this mimics exactly. If this lands first (I hope it does!) we can fix both at once.
Comment #113
alexpott\Drupal\Core
. If we add a service to core with no usages other than tests the code will not be loaded at runtime. And we could add @internal annotations and note that it is experimental and remove these when the classes at not. OTOH we could do this as is now and just make the last step of making this stable in Drupal 8 moving everything to core and gutting all the code from the module apart from a very thin BC layer. I'm not sure which is best but given recent discussions in #2779647: Add a workflow component, ui module, and implement it in content moderation we should seriously consider putting this straight into\Drupal\Core
as this will result in less busy-work. Before starting on that I think we need a +1 from @catch and @xjm to ensure we have consensus.Mixing @inheritdoc and other stuff doesn't work :(
Unnecessary empty lines
Comment #114
alexpottThis is the one problem with putting this straight into core. We'd need to move this to system.module. (from @tim.plunkett in IRC)
I think that making this change is okay as opposed to the issues of moving all the code around and maintaining an experimental module that does not need to exist.
Comment #115
catchThere aren't any real layouts in the patch yet, which is fine. However I'm not sure about defaulting to a theme hook + template per layout. It's going to result in a lot of theme hooks.
Both of these templates are the same, just with different class names. I'd expect the majority of layouts to be like this, but with a varying number of regions on top.
So why not default to a single template with a wrapper, that loops over regions and the the layout name as a theme suggestion?
If there's a layout that deviates from that pattern it could still provide its own template/theme hook.
Comment #116
dsnopekWhile this would make an interesting contrib module for people who don't want to write Twig/HTML, I don't think it makes sense as a default.
Most layouts in practice aren't just a variable number of columns, but also include rows, and have things that can be configured by the user. Depending on how the layout is implemented (ie. which grid system is used, or how the custom CSS is done) the markup for this will have a different structure. So, the only practical way to implement most layouts, would be to describe the full structure of the HTML markup in YAML. In the default case, I'd really prefer not to invent another way to express HTML - in Drupal 8, at least to me, it seems like if you want to write HTML, you should create a Twig template.
Here's some example real world layout templates in contrib right now...
Radix Layouts (based on Bootstrap - needs 'container' and 'row' div's):
http://cgit.drupalcode.org/radix_layouts/tree/plugins/layouts/radix_hews...
http://cgit.drupalcode.org/radix_layouts/tree/plugins/layouts/radix_sutr...
Display Suite (markup is highly configurable):
http://cgit.drupalcode.org/ds/tree/templates/ds-3col-stacked-fluid.html....
Bootstrap Layouts (also based on Bootstrap - decided to completely omit regions that are empty, which is a valid choice, but different than Radix):
http://cgit.drupalcode.org/bootstrap_layouts/tree/templates/bootstrap-6-...
Panels (custom CSS with layouts about as simple as they can be made):
http://cgit.drupalcode.org/panels/tree/layouts/threecol_33_34_33_stacked...
http://cgit.drupalcode.org/panels/tree/layouts/twocol/panels-twocol.html... (even Panels' twocol is a little more complicated, because you can configure the CSS ID - for parity with D7, you'll eventually be able to configure the CSS classes too)
Anyway, my point is really that complex HTML/templates for layouts is really the rule. Super simple layouts like used in the tests are the exception. We hope that dynamic layout builders will eventually be a thing, whether that's in the UI, or even a special syntax in YAML, but based on what people are doing in contrib right now (and have been doing in D7 a long time), I don't think that makes sense as a default.
Also, putting these in templates makes it easy to replace them in the theme. If you're using Panels, and your theme is based on Bootstrap, you probably want to replace the templates for any Panels layouts you use to be based on Bootstrap, so that they use the same responsive behavior and breakpoints as you setup in your Bootstrap variables. With a dynamic layout builder of some kind, that's also a solvable problem, but more complex.
Comment #117
tim.plunkettI happen to agree with @dsnopek 100%.
@catch does that answer your question?
Additionally, @alexpott raised the question of lib vs module. We've had this discussion before in this issue, and flip-flopped several times.
To me, it is better to have the not-yet-solidified interfaces in an experimental module than to put them into core/lib marked as @internal and un-internal them later.
Contrib using this functionality is already depending on a separate module for this, this is no different. If it were a subset of a whole module's functionality that we were moving in, I'd say this approach was wrong. But this is the whole module.
Comment #118
catchI don't like the potential explosion of theme hooks, but don't see a way 'round it if each layout needs its own template.
Overall I'd lean more towards having this in core than a module, but as with the workflow patch we're seeing complications when we try to do that. Still haven't reviewed the whole patch so unassigning.
Comment #119
alexpottFor me since this is replacing https://www.drupal.org/project/layout_plugin and using the same module name I really think that this is a bad idea. It creates all sorts of problems for both consumers of the API having to detect whether or not the core experimental or contrib module is being used - for no gain since eventually the core experimental module is going to be gutted and moved to core. If we move everything straight to core then we completely avoid this scenario and sites have to remove the layout plugin module because panels, ds etc can just migrate to core/lib and be done.
Comment #120
alexpottDiscussed with @catch on IRC and we agreed to go straight to core/lib because of all of the problems with module name clashes between core and contrib and breaking things the least number of times. In #2779647: Add a workflow component, ui module, and implement it in content moderation, I added
To all the class docblocks - we should do something similar here.
Comment #121
tim.plunkettThanks for the feedback!
Comment #122
catchThis also means that we save one set of breakages for modules depending on this - i.e. if it goes in as a module, then again moves to core/lib, modules get broken at least twice as opposed to at least once.
Comment #123
tim.plunkettI'm having déjà vu all over again!
Comment #124
tim.plunkettForgot the @internal parts, hah. Will get to it first thing tomorrow
Comment #125
tim.plunkettThere we go.
Comment #126
alexpottAs per #113 we still need consensus from @xjm on the approach to move into core/lib.
For me core/lib is the correct approach because it values the 10,000 existing sites using the contrib layout_plugin module rather than theoretical problems from being able to autoload experimental code on a live site. If non-experimental code does that then it is a bug in that code.
Comment #127
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI like the move of classes to
core/lib
, but I'm concerned about this being insystem.module
. It means the theme registry will be filled with theme hooks for every provided layout, even if no layout rendering module (like Panels, DS, etc.) is enabled. It also means that enabling a module like Panels or DS won't trigger any UI warning that it depends on an experimental core feature. I suggest moving just this line, and the service definition, to an experimental module.Per above, this is the other hunk to move into an experimental module. I think the module could perhaps then be named
layout_discovery
? I think the service ID can stay the same though.I don't like this. I read the issue comments to see how this came about, and it looks like it was done to avoid needing a
hook_theme_registry_alter()
implementation that inserts a preprocess function for every layout. However, what if we registered these theme hooks as suggestions (i.e.,'base hook' => 'layout'
)? Couldn't we then get*_preprocess_layout()
functions registered for free? Which would allow for both the template_preprocess_layout() that we need, plus make room for modules to implement hook_preprocess_layout() if they need to. Note that I'm not disagreeing with #116: I agree that most layouts need their own template file: I'm only saying that they can and should share common preprocess functions, which would allow for the above code to be cleaned up. However, to be honest, I haven't fully followed all of the changes to how the theme registry works for theme hook suggestions between Drupal 7 and Drupal 8, so maybe there's some implementation reason why this idea isn't feasible. But if that's the case, I'd like to know what that reason is before we proceed with the above hack. Then again, I'd also be ok with this getting punted to a follow-up, so long as folks wouldn't be upset with the corresponding disruptions (e.g., the layout and theme hook names changing to follow alayout--foo
instead oflayout-foo
pattern) getting committed later.Comment #128
tim.plunkettOkay, the proposals are now
I leave it to the 3 committers that are proposing each to decide that. I'm here if you need some insight, but I have no strong opinion.
As for #127.3, it gets ugly.
We'd need to have an empty 'layout' theme hook that does nothing but allow preprocessing.
Or we'd need to have a functioning 'layout' theme hook that is generic, which we don't want.
Note that #104, #125, and #128 are all functionally equivalent.
Comment #130
tim.plunkettSide effects of the changes. Not going to bother fixing it until an approach had been decided upon
Comment #131
webchickIMO, @effulgentsia's proposal seems like it makes everyone happy since:
1) It keeps the various dread warnings about experimental functionality intact (which xjm will like)
2) It avoids all the nasty consequences in implementing modules when that code eventually moves from layout_disovery to system; simply a cache clear, no? (which catch/alexpott will like)
I like it. ;) Now there are 4 core committers with 3 different opinions. :D
Comment #132
larowlanIf we do item 2, we could put items in a Drupal\Core\Experimental namespace
Drupal\Core\Experimental\...
then it is clear they're experimental.
e.g.
Drupal\Core\Experimental\SomeApi\SomeClass
When we move them to stable, we end up with
Drupal\Core\SomeApi\SomeClass
and we leave the old class behind, extending the new one, as an empty shell and mark it as deprecated.
We did something similar with BrowserTestBase, its old skeleton is still present in the Drupal\simpletest namespace
Comment #133
alexpottI didn't think
Drupal\Core\Experimental\
was a good way to go because that just means more breakage for no reason when we move the classes to the correct place. But then I realised that you're proposing to leave them behind. I think we should leave behind till until 9.x. So contrib can upgrade as they want - they never have to version juggle in 8.x - once both the patch version and next minor version have the non-experimental classes they can move to them. This is a good idea. And combining that with @effulgentsia's proposal in #127 (also a good idea) seems clever.So I'm +1 to #132 and #127.
The one reservation I had when thinking about things was that the service name might exist in the existing contrib module... however the service name is
plugin.manager.layout_plugin
. Which brings me to something I've been thinking for a while. We really should have a naming standard for services where they begin with their provider.whatever to avoid name clashes. Which means that I think the service should be named:core.plugin.manager.layout
since at some point we're going to remove thelayout_discovery
module and put it into core. We can document this in the services.yml file.Comment #134
tstoecklerWell we currently have a standard of all plugin managers being called
plugin.manager.*
, so - while I don't necessarily disagree your proposal is good - that would break the existing standard, which we are actually pretty consistent with in core at the moment.Comment #135
Berdir@alexpott: In general, I agree with prefixing things (also plugins and so on). However, the plugin.manager prefix is special, see \Drupal\Core\Plugin\PluginManagerPass::process(), we don't *have to* use that but then we need to explicitly add a tag.
Given that, I'd recommend plugin.manager.core.layout.
Comment #136
alexpottOkay +1 to #135... for plugin managers we go with plugin.manager.PROVIDER.whatever... I thought adding the tag was okay - less magic but this is also okay.
Comment #137
alexpottAfter discussing with @catch for a bit about the core/experimental idea - he's moved my position back to "just means more breakage for no reason" since contrib will have to update twice and there's no real difference with having it in an experimental module then. I think we should be prioritising the least number of breaks for people experimenting once we've worked out how to definitely not affect live sites. So that seems fulfilled by #127. I remove my support for #132.
Damn BC and experimental is tricky.
Comment #138
tim.plunkettIn order to continue with #127, we need a generic 'layout' theme hook. So we get to do what @catch wanted in #115 after all.
I still agree with @dsnopek that 99% of layout authors will want their own template, but if they are lazy then they can have this one.
As of #125 I renamed the service to plugin.manager.layout anyway (away from plugin.manager.layout_plugin). Begrudgingly adding the .core because you's asked for it, not because I think it's in scope to redefine our plugin manager naming conventions here.
Comment #140
tim.plunkettRandom fail from #2828045: Tests failing with unrelated „Translation: The French translation is not available”
@alexpott's comment in #137 implies that @catch is also in favor of @effulgentsia's proposal, as is @webchick, so the only one left is @xjm
Comment #141
dsnopekIf this is going to be suggestions on a single theme hook (ick!) then at the very least we need to make sure we're packing enough information to allow people to preprocess the theme hook and target a specific layout, since it's super normal for a module providing layouts to also preprocess some or all of them. I didn't have time to look at the patch on #138 to check if that's the case or not, but without that we'd make current use cases in contrib impossible.
For what it's worth, I think having a single theme hook with suggestions is going to make it harder and more confusing to use this API in practice, but it seems like that's the way this is going :-/
Comment #142
tim.plunkettIn the last RTBC patch, there were N theme hooks for every layout that specifies a template.
Now there are N+1.
There is always a 'layout' theme hook present, but it will almost never be used.
Specifying it as a 'base hook' does only a couple things:
template_preprocess_layout()
(why we want it)hook_theme_suggestions_HOOK()
(not important)This is NOT converting the theme hooks for layouts to suggestions, they are full fledged theme hooks.
Comment #143
dsnopekAh, ok, thanks! That sounds good. Sorry, I didn't actually read the last patch, just skimmed the comments (I'm on paternity leave now - so, not much time)
If we're still able to use full fledged theme hooks (as opposed to converting all to single theme hook with suggestions which is what I thought was happening) then this is totally fine and contrib will be able to keep on doing what it's doing :-)
Comment #144
tim.plunkettThe only change for existing contrib is that their preprocess will change from
mymodule_preprocess_mylayoutname()
to
mymodule_preprocess_layout__mylayoutname()
which *looks* very suspiciously like a suggestion, but is not.
\Drupal\Core\Theme\Registry::completeSuggestion() is used to process this, which once again seems like it's a suggestion, but it's just confusingly named.
As the calling code says:
Comment #145
andypostI think having experimental module is good idea
1) It will allow to bring default one col layout as BC for fields so it can be used for default "layout" theme hook
2) Theme suggestions are runtime so we have hew hooks https://www.drupal.org/node/2100775
3) Later will allow more layouts to polish in minors
why not move this "one col" to module itself? and move test module here as well?
Comment #146
andypostAlso having default fallback "Reset" layout very useful
Comment #147
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI agree with @tim.plunkett. I don't see why this issue should start prefixing
.core
when no existing core plugin manager service IDs currently do. What if we rename the module to simplylayout
? The info.yml could still retain the human-friendly name of 'Layout Discovery' if we think that's helpful.https://www.drupal.org/project/usage/layout currently lists 0 sites using the 8.x version, and with no commits in 3 years, I doubt those branches even work at all, so I don't see much of a conflict problem with that. Then the service ID could be
plugin.manager.layout
, which would adhere to our current naming patterns both while it's in layout.module, and when it eventually moves to core.services.yml.What about renaming these from
layout_plugin
tolayout
as well? Also, it doesn't look like we have any config objects anywhere testing the.*
one, or do we? There's the schema inlayout_test.schema.yml
, but that's explicit rather than a reference tolayout_plugin.settings.*
, and even that one, is that schema ever tested, since I thinktestRenderLayout()
bypasses actual configuration read/write?Comment #148
tim.plunkettI'm very hesitant to call this layout.module. To me, and many others, that name implies a UI. Specifically, a UI for building layouts (aka "flexible layout builder"). Note that's what layout.module in D7 contrib did.
If layout.module *were* to exist, it would like be stored as a config entity with a plugin deriver, similar to how menu entities and menu blocks work.
So in the schema I'd prefer to keep "layout" for that eventuality, and stick with layout.plugin here for clarity.
As far as
layout_plugin.settings.*
is concerned, I don't know that we need explicit tests for this, but it is an extremely good practice IMO, and one I think we're mistaken in skipping before. For example, #2815829: Adding or editing a block through the UI saves the entity twice is a follow-up to #2811519: Blocks do not appear after being placed with the Rules module enabled (or other missing schemata for Condition plugins), both are trying to add this to save contrib from itself:In fact, my addition of
layout_plugin.settings.*
was explicitly called out in #93.1 as a good thing.For the service name, I really don't care. I think adding .core. is out of scope and could be discussed elsewhere, but it doesn't hurt anyone to add it here.
It's a trivial change to fix, it only is used in 3 places:
I can reroll without .core. if needed, or it could be fixed on commit.
Comment #149
tim.plunkett#145.1 Yes, that's what #2796173: Add experimental Field Layout module to allow entity view/form modes to switch between layouts is for
#145.2 Theme suggestions are runtime, but these are full theme implementations using a base hook.
#145.3 We don't want to add any real layouts yet to runtime until #2822758: Introduce a way to distinguish between different types of layouts is resolved.
Comment #150
xjmSo I've read all the comments since my previous review and spent a good time thinking about this. I think we have a number of separate questions that affect some of the same things, and so they are being discussed together. But they are separate questions:
\Drupal\Core
?Honestly, I'm still stuck on #1. I don't think this belongs in
core/lib
(not in Drupal 9 either). @tim.plunkett has said that it belongs there, but I am not sure what the case is for it being there.Let's take a look at what's in
core/lib/Drupal/Core/
:Most of those things are low-level "you can't have a Drupal without this" functionality. Forms, routing, content rendering, data storage, configuration. Lower subsystems and utilities. There are a few exceptions. (I've thought numerous times over the past year that we should have put BTB and kin in a module as well.)
I was surprised to see Block in there since there is also an optional Block module. Then I opened a class in there (in BlockBase) and saw this:
So from that the Block module is not just a Block UI module, and we have something in Drupal core that depends on code in an optional module. Oopsie. That could lead to some interesting errors.
Do you need to have a layout API on every Drupal site? I'm not sure you do. Do we have any case for code in
core/lib
to depend on Layout API code? The two related things I see in there are Block (but see caveat above) and Theme (I don't think we want our theme system of the future to depend on these plugins... or do we)?I think things should be in modules if:
I think things should be in
core/lib
if:core/lib
that might need to depend on them. (Circular but true.)Our answers to questions #2-4 depend on #1, so let's evaluate what the answer to #1 should be. (I have thoughts on questions #2-4 for either answer to question #1, but don't want to add even more of a wall of text here if there is a strong answer to #1.)
(Aside: as much as I am in favor of the continuous upgrade path, I don't think we should make decisions for Drupal 8 based only on what Drupal 9 might look like. Really, we don't know yet. We might want to get rid of System module finally somehow, or make our namespacing truly unique and safe, or even decide we want to do away with the module system as it currently exists, but I wouldn't make a decision for this issue based only on such far-off ideas.)
Comment #151
xjmOh and I guess question #5 might be "What should its name actually be?"
Comment #152
xjmAlso just noticed that we do have a README.txt in there, which says:
Comment #153
tim.plunkettOpened #2829680: BLOCK_LABEL_VISIBLE is defined on the wrong interface to fix the Drupal\Core\Block vs Drupal\block issue.
Here are my reasons for layout being in \Drupal\Core.
As stated on the parent issue, there are two issues "to validate the implementation".
Both would initially add experimental modules at first.
But both are altering existing functionality in a way that we would eventually want baked into core itself, not living forever as a module containing only alters. Once stable, ideally each would be merged directly into the system they are altering.
#2796173: Add experimental Field Layout module to allow entity view/form modes to switch between layouts would need to rewrite parts of Field UI, but also code in \Drupal\Core\Entity.
#2811179: Allow themes to provide Layout API integration would need to rewrite parts of system.module itself.
I highly doubt we'd want to add another module as a dependency of system.module, and we couldn't have \Drupal\Core\Entity using module code as would be necessary.
Yes this is true. Except for all the things that aren't essential. Which happen to be the ones I'm changing. Maybe they don't truly belong in Drupal\Core and should be somewhere else (\Drupal\MaybeNotEssentialButNotAModule ?)
But if we're going to continue to have things like theme regions and entity displays outside the module system, then to improve them we need the Layout API available as a subystem as well.
There are no UIs changed here, and even the follow-up issues have their UIs and their functionality working separately. For example, the field layout one either changes \Drupal\Core\Entity or field_ui.module. There's already a pre-existing split there, but both halves need to be modified.
In the theme layout issue, the underlying code modifies the results of system_get_info(), and the UI is left for the Block UI to handle.
The tl;dr is the answer to your 5 questions.
Comment #154
tim.plunkettPer discussion in IRC with @xjm, expanding the @internal docs to explicitly link to the experimental code policy.
Comment #156
tim.plunkettRandom fail from DrupalCI, and now it's retesting twice. First retest passed!
Comment #157
tim.plunkett@xjm points out that one of the kernel tests in Drupal\KernelTests\Core\Layout depends on layout_discovery, moving that to Drupal\Tests\layout_discovery\Kernel.
Comment #158
xjmThanks @tim.plunkett. The potential of entity displays depending on this API matches the requirement for things in
core/lib
to only depend on other things in core/lib, and so supports this belonging in\Drupal\Core
in 8.x at least, since there is not currently any plan to move entity display functionality into a module.I talked about this more with @tim.plunkett in IRC.
Things I like about this approach:
layout_discovery
is a better name. (I didn't like having "plugin" in a module name.)layout_discovery
plus core library has less risk of namespace collisions with previous implementations.Things I don't like:
core/lib
in general. I just don't like the precedent it sets.core/lib
when we already have a solution for modules.To mitigate #1, we should remove the @internal tags only in a minor, with a change record, once the API is stable. Increasing the visibility of code from @internal to public in a minor is equivalent with an API addition and allowed by semver. #2 is more of a problem, and I don't know how to solve it.
#3 is already mitigated by having the discovery module, and well we never got actual separate version numbers for experimental modules working anyway. (I did like @larowlan's idea of an experimental namespace but apparently @catch did not.) I noticed that api.d.o doesn't even surface or format @internal in a useful way, but then, you don't get any special message on code that is actually in experimental modules either. (It used to hide @internal code completely, so at least we don't have that to contend with anymore.)
For #4, I guess the solution would be for committers to agree "Don't use it as a precedent", but we already have another issue in the RTBC queue that wants to do the same thing. I guess, at the least, we need to have a really compelling reason that the code belongs in
core/lib
and not a module, and a really strong expectation that it is going to be a stable, required part of core in 9.x.So, overall, I guess I'm in agreement that the current approach is better than the alternatives, despite the drawbacks, and that most of the drawbacks are sufficiently mitigated. But let's be careful in similar issues to always ask if the code really needs to be in
core/lib
and whether there is any possible impact on installed sites.Thanks @tim.plunkett for taking the time to work through this with us.
Comment #159
xjmComment #160
tim.plunkettSince originally being RTBC'd in #87 by @dsnopek, this has bounced around a lot.
The committers seem to have reached consensus on the direction.
So, here's an interdiff between the patch in #86 and #157!
Comment #161
xjmThe CR and handbook should be updated for the current approach (totally makes sense to wait on that until the issue has settled). I think it would be worth mentioning that the core plugin type is declared as internal while it's under development because there's no BC promise yet, but intended to be public API, and that modules wishing to implement the API should declare a dependency on the (currently experimental) discovery module.
Comment #162
EclipseGc CreditAttribution: EclipseGc at Acquia commentedI spent a bit of time reading through 157 today, and this looks very convincing to me. I'm very happy to RTBC this.
Eclipse
Comment #163
alexpottBefore this issue is committed we need to address @xjm's points in #161 - so we need to update policy docs and the CR.
Comment #164
tim.plunkettUpdated the CR: https://www.drupal.org/node/2821240
The handbook already has a dedicated follow-up: #2824667: Update Layout Plugin handbook documentation
https://www.drupal.org/core/experimental needs to be updated with something reflecting #158
However I'm not clear on whether we do want to document this as a precedent, or not.
Maybe something like this?
Comment #165
xjmI only meant update the change record to describe the changes being made, including the expectation of stability/public API vs. internal for this API only in this issue's CR only; and update https://www.drupal.org/node/2619128 to use the correct module name etc. (because it's really confusing otherwise).
I definitely do not want to update any policy docs as part of this issue, because as I've said, I do not want this to be a basis for a policy, and we should avoid it as much as possible.
Comment #166
alexpottSo I misinterpreted @xjm's comment
thinking that this meant the experimental module page needed to be be updated to explain what experimental code was doing in core/lib. I'm ok with not changing the handbook in order to make progress in this issue. However, in my mind, not documenting on the experimental page that experimental code can be found in core/lib in order to not set a policy about this feels awkward. Because we have experimental code in core/lib. But discussing this elsewhere is best.
Comment #167
dsnopekReviewing the interdiff on #160:
If we're no longer calling anything 'layout_plugin' internally, should this just be 'layout.settings' and 'layout.settings.*'? I think that's what we used in the last patch that had the code in core/lib rather than a module. (Of course, having it as 'layout_plugin.settings.*' would help with migrating from the contrib module, but would be confusing without the history.)
Otherwise, this all looks good to me! Thanks @tim.plunkett for continuing to push this forward!
Comment #168
tim.plunkettI purposefully did not rename the schema because there is a distinct possibility that we end up with something like a flexible layout builder that would need a config object, and that layout.* would be better for it.
I don't feel strongly either way.
Comment #169
alexpottSo I don't know if this is a problem or not but if you have both layout_discovery and layout_plugin enabled everything blows up. I'm pondering about how a module like Display Suite moves to the core plugin type once the core part is stable. It can't disable the layout_plugin module because it does not know if there is another module that depends on it. Or does it depend on a new version of layout_plugin that is no code but just exists so that things can transition to core's plugin. And if we do that then all things that depend on layout_plugin need to add be updated. And ideally we'd have this all mapped out and documented in the CR.
So, I think we have a problem but one that can be mitigated by having a good plan and communicating it. My plan would be to:
Comment #170
alexpott#169 another option would be to use a different annotation - that should fix this...
\Drupal\layout_plugin\Annotation\Layout
and\Drupal\Core\Layout\Annotation\Layout
. This is one reason why Doctrine don't like SimpleAnnotationReader and not adding use statements for annotations.Comment #171
swentel CreditAttribution: swentel commented@alexpott
I haven't wrapped my mind mind completely around it either. At first I though a new branch would be ok for DS. That's why there's a 8.x-3.x branch already with a dependency on core >= 8.3.x (although it still has a dependency on layout_plugin now haha). It made sense in my head because when this patch, and also field layouts would land, we need to rewrite large parts of our code base anyway if want to use the core modules.
However, #2796581: Fields must store their region in entity displays affects us as well so, I'm starting to wonder if we need an additional branch so that we end up with this:
8.x-2.x: core 8.2.x for people who never upgrade (tsss)
8.x-3.x: for #2796581: Fields must store their region in entity displays and dependency on core >= 8.3.x
8.x-4.x: for dependency on the new experimental modules in core
see also #2821201: Update Display Suite for layout_plugin in core
Comment #172
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI wonder if @version 0.1.0 (or whatever other number seems appropriate) instead of
@internal
would address this concern. A practical difference between the two is that IDEs show you (e.g., via a strikethrough) when you're referencing@internal
code, but I don't think they do anything like that for@version 0.*
code.Comment #173
alexpottRe #172. I think that the @internal is actually fine for this. Because it is internal until we decide to make it stable. Panels, DS etc can't do stable releases relying on this functionality. If they choose to experiment with it they are relying on internal APIs. These APIs are internal because they are no yet finished and therefore we can't consider them public API. And when we make it stable then great we just remove the @internal tags. I think this is simpler than using @version and deciding what
@version 0.
means in terms of a stable 8.3.x Drupal core release.Comment #174
tim.plunkettThe contrib layout_plugin only has alpha releases, we can do whatever is necessary in the next alpha release, or just mark it obsolete and abandon the project.
Comment #175
alexpott@tim.plunkett but Display Suite has a stable release and Panels a beta. They can't just move to use core's layout plugin once this is in. And once a site has those modules it becomes problematic to install the layout_discovery. So if we introduce an experimental module in core that using layouts for entity display then sites with display suite or panels can't experiment with it.
Comment #176
xjmYesterday the committers discussed this issue and agreed to go forward with the @internal notation currently in the patch, and explore whether to add @version and/or the idea proposed in #2833347: Allow experimental modules to rely on \Drupal\Core\ namespaced classes that are only loadable when the module is enabled as part of a followup.
So that means AFAIK the one thing we need to do is make sure we have an agreed solution for #171. Part of the reason @alexpott is concerned is from experience with a similar contrib to core change for the Workflow Initiative that has turned out to be a lot more difficult than expected.
I'll try to follow up with @tim.plunkett and @alexpott to agree on the way forward.
Comment #177
xjmComment #178
tim.plunkettIf this lands in 8.3.x, why wouldn't that just be two branches?
Sure you'd be depending on an experimental core module/subsystem, but is that really any worse than depending on an alpha release of a contrib module?
Comment #179
tim.plunkettI went to add a hook_requirements and hit this bug #2833462: hook_requirements($phase = 'install') does not work as expected for experimental modules
Comment #181
tim.plunkettRandom failure
Comment #182
tim.plunkettPer discussion with @alexpott, @xjm, and @swentel in IRC, adding a hook_requirements that explicitly checks for layout_plugin.
Note that #2833462: hook_requirements($phase = 'install') does not work as expected for experimental modules was committed to 8.3.x already.
@xjm reviewed this interdiff so leaving at RTBC
Comment #183
tim.plunkettMeant to leave at RTBC
Comment #184
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI discussed this with @tim.plunkett. Turns out that this is needed due to a bug with the way that
Drupal\Core\Theme\Registry
collects preprocess functions. Seems that without the theme hook name starting with'layout__'
, we don't gettemplate_preprocess_layout()
executing for it, but we need that. Ideally, the theme registry would collect preprocess functions purely based on the'base hook'
hierarchy, not on anything magical with'__'
. But fixing that is out of scope for this issue.However, I'm somewhat concerned with theme hook names not matching up with template names. In principle, it's ok. In fact, drupal_find_theme_templates() has some code in it already to handle such a situation. Except, that code is incomplete. If a theme wants to implement an even more specific suggestion of such a template, I don't think that would work. E.g., if a module defines
foo-1col.html.twig
, but this gets entered into the theme registry aslayout__foo_1col
, then a theme could implement eitherfoo-1col.html.twig
orlayout--foo-1col.html.twig
to override it, butfoo-1col--SOME_SUGGESTION.html.twig
would fail to get discovered, whereaslayout--foo-1col--SOME_SUGGESTION.html.twig
would be discovered ok I think (just basing all this on reading the code in drupal_find_theme_templates(), not in trying any of this).Maybe suggestions of specific layouts is not a needed use case, I don't know? But I'm just highlighting some of the WTFs that occur when the theme hook name and the template name don't match up. While such a mismatch has always been a possibility in theory (given that people can put whatever value they want into the 'template' key), I don't think we've ever yet committed code to core that deliberately made use of it, so if we do so now, we're opening ourselves to having to deal with that latent technical debt getting surfaced.
What if, instead, we created a convention that layout templates should be defined here as
layout--MODULE_NAME-*
? In other words, this would becometemplate: templates/layout--layout-test-1col
. That way, the LayoutPluginManager code above wouldn't need to prefix the hook name and create a mismatch between the hook name and the template name. But we'd still get the benefit of the magic naming that allowstemplate_preprocess_layout()
to run.I'm leaving this issue as RTBC, because potentially, since this is all experimental code, we can deal with all this in a follow-up. However, if we end up changing the naming convention of templates, as I'm proposing, then that affects modules that will want to start providing layouts, so maybe we'd rather settle on it before commit?
Comment #185
tim.plunkettI don't think this is a commit blocker, since this is experimental.
I agree with the WTF, I'd rather fix the Registry.
I think forcing templates to be layout-- prefixed is worse, I'd rather not dictate such things to themers.
Also the #theme part isn't used directly, only by LayoutDefault::build()
Comment #186
BerdirSee #2559825: Preprocess functions from base hooks not triggered for theme hooks not using the __ pattern for that problem, has AFAIK a working patch and needs reviews :)
Comment #187
xjm@alexpott and I have both updated #2811175: [plan] Add layouts to Drupal with additional followups that have been identified since it was posted. Now would be a good time to review/give feedback there!
Additionally, here are other followups I think we may still need:
Am I missing anything?
Comment #188
tim.plunkett#186, good to know there's an issue!
#187.1 #2834025: Mark Layout Discovery as a stable module
#187.2 #2834019: Remove the "layout__" prefixing of theme hooks
#187.3 #2834023: Discuss service ID and config schema naming for Layouts
#187.4 I haven't seen or heard anything about @version, so I don't know enough to file this one.
#187.5 Also not filing this one
Adding explicit @todo for 2, confirmed that all the @internal's are sufficient and don't need @todo's in addition.
Comment #189
xjmAdded this note to all three handbook pages for the module:
https://www.drupal.org/node/2619128/revisions/view/9126312/10243380
Comment #191
xjmAlright, I think this has ended up in a really good spot, we've covered our bases as best we can for the path forward for core and contrib, and the best way to make improvements is to get this in and move forward on both the field layout work and the contrib implementations.
I'll file followups for #187 points 4 and 5.
Committed and pushed to 8.3.x. Thanks everyone for your patience!
Comment #192
xjmIf anyone has any additional feedback or potential followups, please add them to #2811175: [plan] Add layouts to Drupal.
Comment #194
tim.plunkett