Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Layouts provide an interface with a single method even though layout plugin have a number of different useful public methods not owned by any interface. We should move these methods to the layout plugin interface since all of these plugins will have/need the methods.
Comment | File | Size | Author |
---|---|---|---|
#8 | 2699841-lp-8.patch | 2.75 KB | tim.plunkett |
#6 | 2699841-lp-6.patch | 3.56 KB | tim.plunkett |
#2 | 2699841-2.patch | 4.19 KB | EclipseGc |
Comments
Comment #2
EclipseGc CreditAttribution: EclipseGc at Acquia commentedCODE!
Eclipse
Comment #4
tim.plunkettNever called anywhere, not in CTools, Panels, Panelizer...
@dsnopek suggests maybe it's being pulled directly from the definition?
Only called as $this->getLibrary, could be protected
Same as getLibrary, but also is getThemeHook not getTheme.
Also I think there should be a
getDefaultRegion()
method. IPE currently does something likereset(array_keys($regions))
, that can be the base implementation, but it'd be useful for a layout to be able to specify thatComment #5
dsnopekI 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.
Comment #6
tim.plunkettI 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.
Comment #7
dsnopekOk, 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 thatgetRegionDefinitions()
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 ;-)
Comment #8
tim.plunkettWell 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.
Comment #9
dsnopekWell, 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?Comment #10
dsnopek... 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 agetLayoutInfo()
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 :-)
Comment #11
dsnopekUpdating issue title to represent recent discussion. Issue summary could use an update too
Comment #12
DamienMcKennaHow does this match what's in core in 8.3?
Comment #13
dsnopekCore 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!
Comment #14
DamienMcKennaThis module is no longer supported, all efforts should be made to switch to core's Layout Discovery system instead.