Provide better API for getting information about layouts (without putting more methods on the interface)

1. +++ b/src/Plugin/Layout/LayoutInterface.php
   @@ -18,6 +18,65 @@ use Drupal\Core\Plugin\PluginFormInterface;
   +  public function getDescription();
   ...
   +  public function getBasePath();
   ...
   +  public function getIconFilename();
   

   Never called anywhere, not in CTools, Panels, Panelizer...

   @dsnopek suggests maybe it's being pulled directly from the definition?

2. +++ b/src/Plugin/Layout/LayoutInterface.php
   @@ -18,6 +18,65 @@ use Drupal\Core\Plugin\PluginFormInterface;
   +  public function getLibrary();
   

   Only called as $this->getLibrary, could be protected

3. +++ b/src/Plugin/Layout/LayoutInterface.php
   @@ -18,6 +18,65 @@ use Drupal\Core\Plugin\PluginFormInterface;
   +  public function getTheme();
   

   Same as getLibrary, but also is getThemeHook not getTheme.

Also I think there should be a getDefaultRegion() method. IPE currently does something like reset(array_keys($regions)), that can be the base implementation, but it'd be useful for a layout to be able to specify that

I can't imagine a getDefaultRegion() method ever being dynamic, since the list of regions can't be (if you wanted to make a layout builder, it would use a deriver to generate a layout plugin definition, and that definition's region list would be static).

So, to me, that could also be a new property on the definition that can documented on \Drupal\layout_plugin\Annotation\Layout. Adding a new method would make the interface bigger (and a little bit harder to implement) without much gain.

Although, I'd be open to an argument that core wants all plugins to have methods for everything on their annotations (since the ultimate goal is core inclusion)? I don't know if that's the case, just throwing that out there.

I also just independently arrived at the conclusion that getDefaultRegion is NOT needed, and it's only due to the lack of a mechanism to map regions between layouts, and we should just fix that instead.

Not every annotation property is always represented by a method, even in core.

Here's my proposed changed now, discussed with EclipseGC and japerry.

Ok, thanks!

So, your're basically adding convenience methods to the interface for getting label, category and region definitions from the plugin definition?

For at least label and category, the plugin definition is the definitive source of information on this, as we don't want to instantiate every plugin in order to make a list of all the plugins (for a selection UI or whatever). For the region definitions, I could reasonable see a case for instantiating the plugin to get that, so we could potentially say that that method is the definitive source of that? In either case, we should document the method's relationship to the property on the annotation.

I guess my main worry is someone implementing the interface, and thinking that overriding getLabel() would actually have an effect on a listing of the layouts, or something along those lines. Or, a more likely case, if we say that getRegionDefinitions() ISN'T the definitive source of information for that, and someone overrides that method and puts some logic that makes certain regions exist or not exist under certain conditions, and it's ignored for the information on the annotation (or even worse - it's used inconsistently and one bit of code uses the annotation and another the method).

Not having the methods makes all these questions very easy ;-)

Well argued. As much as I hate just relying on "use the definition directly", it's already happening, ever since #2364821: Simplify LayoutInterface to require implementing fewer methods

This patch should have been part of that issue.

As much as I hate just relying on "use the definition directly", it's already happening ...

Well, we do have the opportunity to change that since this is still -alpha and not in core!

If we decided it did make sense for getRegionDefinitions() to be the definitive source of the region definitions, we could document it, patch Panels and Display Suite and get the word out.

That said, at the moment I'd be against that change because I think it'd encourage people to make layouts with dynamic regions, and I think having static regions and using derivatives to make dynamic layouts is the way to go. Dynamic regions moves us towards the "Flexible" layout we had in D7, and I'd rather not do that again. :-)

Maybe we could put convenience methods on the plugin manager instead? It could be like ->getRegionDefinitions($layout_name_or_object)? Then we have a method as API, but without allowing implementors of the Layout interface to override the method?

It could be like ->getRegionDefinitions($layout_name_or_object)? Then we have a method as API, but without allowing implementors of the Layout interface to override the method?

... or maybe a better API would be a LayoutInfo value object that had all the convenience methods for information about a layout, and the plugin manager would have a getLayoutInfo() method?

Or, can we have the plugin definition be itself an object? I actually have a vague recollection of seeing some plugin that used an object rather than an array as the plugin definition, but I might have dreamt that.

Now I'm just spit balling :-)

Updating issue title to represent recent discussion. Issue summary could use an update too

How does this match what's in core in 8.3?

Core introduced a class to represent the plugin definitions which are used to get information about the layouts. So, when we backport all of that to contrib, we'll solve this problem!

This module is no longer supported, all efforts should be made to switch to core's Layout Discovery system instead.