Problem/Motivation
Currently we have this interface with these methods
interface SectionStorageInterface extends \Countable {
// Section specific
public function getSections();
public function getSection($delta);
public function appendSection(Section $section);
public function insertSection($delta, Section $section);
public function removeSection($delta);
// Integral to the runtime usage
public function getContexts();
// Needed for storage and routing
public function getStorageId();
public static function getStorageType();
// Storage
public function save();
// Only used by Forms and Controllers
public function label();
public function getCanonicalUrl();
public function getLayoutBuilderUrl();
}
The last 3 methods bug me, as they are not necessarily needed to store sections. Just to present them.
Proposed resolution
Split the interface up into SectionStorageInterface and SectionListInterface.
Introduce a new plugin type SectionStorage.
This retains the SectionStorageInterface, which can delegate back to the underlying object (field, config entity, etc.) if it so chooses.
The field/entity/whatever now only implements the SectionListInterface, and is only responsible for maintaining the list of sections.
Remaining tasks
N/A
User interface changes
N/A
API changes
Yes, but to alpha-stability experimental pre-release code.
The following methods on SectionStorageInterface have moved to SectionListInterface
getSections()
getSection()
appendSection()
insertSection()
removeSection()
count()
The following methods are staying on SectionStorageInterface:
getContexts()
getStorageId()
getStorageType()
save()
label()
getLayoutBuilderUrl()
getRedirectUrl() [renamed from getCanonicalUrl()]
The following methods have been added to SectionStorageInterface:
setSectionList()
getSectionListFromId()
buildRoutes()
extractIdFromRoute()
Data model changes
N/A
Comments
Comment #2
tim.plunkettNaming TBD. Also this patch is on top of #2922033: Use the Layout Builder for EntityViewDisplays
Comment #3
larowlanWe'd want to do this before beta
Comment #4
tim.plunkettFirst thing to note: the interface itself might be misnamed.
As outlined by my inline comments in the interface in the issue summary, there are several different things happening here.
Right now the code expects a single object to be passed around that can do all of those things.
And all of those methods are equally needed to make the Layout Builder UI work.
A system that wants to provide a Layout Builder UI experience for its data model needs only to implement this interface (and a param converter) and that's it.
An object implementing this interface says "yes, not only do I know what sections I have, but I know *why I have them*" (meaning, it can provide the appropriate contexts for the underlying system, it can provide a human readable label to communicate that to a user, and it knows where it can be viewed and edited).
Comment #5
samuel.mortensonTo me, a StorageInterface in core should always define CRUD methods that take in an ID where appropriate. This is consistent with almost every *StorageInterface.php file I've found. Following that, I would expect that a SectionStorageInterface is something that can at least load, save, and delete a Section or list of Sections based on an ID. Instead, as #4 outlines, StorageInterface does quite a few things that are mostly unrelated to normal *StorageInterface operations.
With that, I would suggest renaming the interface to something like SectionListInterface, which is consistent with the getSections/getSection/appendSection/insertSection methods. The other methods are tricky, here are my thoughts on them:
$something->load('entity:node:1')->getSections()
? From what I understand, these are only used to determine a unique tempstore ID, but can't be used to perform CRUD operations on the storage from another place.Splitting these methods up is hard, but here's a hypothetical setup:
User interfaces can then be written generically and invoke the service like
$service->load('entity:node:1')->insertSection(...)->save()
. The service would know to load Node 1 and pass relevant data to the LayoutSectionItemList field.Comment #6
tim.plunkettI need to update the IS, but here's a big refactor to address the above concerns.
Built on top of #2922033: Use the Layout Builder for EntityViewDisplays. Interdiff is just this issue's worth.
Comment #8
tim.plunkettAs we (@EclipseGC, @samuel.mortenson, and I) dug into this issue, one thing became clear:
The steps necessary to define a new type of section storage (overrides, defaults, etc.) were not clear.
First there is the magic string returned by a *static method* (yuck) that has to match a portion of a magic string on a service definition.
Then there is the situation described in the original issue summary here, about the underlying object being responsible for way too much knowledge about itself (why should the field or config entity care how the URL parameters for the Layout Builder UI are structured?)
Finally, there is the way routes are defined for each. Without adding more code directly into LayoutBuilderRoutes, there was no reasonable way to hook into the Layout Builder UI.
Hence, a new plugin type SectionStorage.
This retains the SectionStorageInterface, which can delegate back to the underlying object (field, config entity, etc.) if it so chooses.
The field/entity/whatever now only implements the SectionListInterface, and is only responsible for maintaining the list of sections.
Still blocked on defaults, the do-not-test patch is just this issue's code.
Comment #9
EclipseGc CreditAttribution: EclipseGc at Acquia commentedI find the naming between LayoutBuilderRoutes and LayoutBuilderRoutesTrait to be confusing. One is an event subscriber and the other is a trait the plugins (which the event subscriber delegates to) happen to us. And that extra layer of abstraction between the two makes drawing a line between them sort of weird. Maybe I'm being overly nitpicky here.
?? is this like... necessary rather than just calling createInstance()?
ok, so forgive my complete craziness, but when I see this method I expect to see loadFromRoute(Route $route). I know the ID is from the route defaults. Feels weird to parse it out before it gets to this method. Is that nuts to say?
I'll review for strings and more detail shortly.
Eclipse
Comment #10
tim.plunkettThose methods (the two above plus
loadFromObject()
) were all quickly named. I would love feedback on this.The intention behind loadEmpty vs createInstance was that 99% of the time you'd expect your plugin to be able to respond to all methods.
loadEmpty()
only exists for the single usecase of routing. Which maybe isn't worth an API level method?But the point was, if you call
createInstance()
yourself, you're not going to get what you want.In fact, I went so far as to have
interface SectionStorageManagerInterface extends DiscoveryInterface
instead of extending PluginManagerInterface, since I only want getDefinition[s] and these load* methods to be called, and the fact that it uses createInstance internally is an implementation detail.I'd appreciate further feedback on the manager here, since it's the New Big Thing, but I'm also welcome to hashing this stuff out in realtime in the #layouts channel on Drupal Slack.
Comment #11
larowlanNot sure of the naming of this method, I realise its a param converter in disguise, but perhaps we can find a name that means more in the domain of layout builder.
Can you elaborate a bit on why this has changed? I assume it is because of
but its not obvious
Comment #12
larowlanand then some!
Comment #13
larowlanI originally was going to say the same thing, but then realised that `loadEmpty` is the correct naming in the context of a layout domain, while `createInstance` is a plugin-api naming.
I think it is fine to name the methods in the context of the domain.
Comment #14
EclipseGc CreditAttribution: EclipseGc at Acquia commentedFWIW, I completely agree, I just think a pure passthrough method is sort of weird. Not a hill I care to go die on, and Tim's reasoning seems sound.
Eclipse
Comment #15
tim.plunkett#11
1) Yep, that'd be good
2) Oops, that's left over from an earlier version where I dropped the bit in hook_entity_type_alter() that sets up the link template. Those hunks should be reverted.
Comment #16
larowlanSo thinking about a use case for another section type I came up with the following scenario.
How would that look here?
So I think this is a pretty good fit, just wondering how/where getCanonicalUrl fits
PS, when this goes beta, I might actually build such a solution.
Whilst I like the idea of content-editors being able to customise the layout of their content entity, I also like the structure this provides to keep them on the rails.
Comment #17
tim.plunkettThat sounds cool!
Thanks for walking through that.
Currently for Defaults we have the canonical URL returning the user to Manage Display.
In an earlier iteration, I just had getCanonicalUrl return getLayoutBuilderUrl...
Comment #18
larowlanmissing return point? also means missing a test for when no section list
One of these has 6 args, the other has 4. I think 4 is the right amount.
Comment #19
tim.plunkettRerolled for latest changes in #2922033: Use the Layout Builder for EntityViewDisplays
#18
1)
No return missing, but a missing @return typehint and docblock.
Added a test to prove the expectations.
2)
Also fixed the 2nd point
Additionally, renamed s/loadFromObject/loadFromSectionList as per a recommendation of @samuel.mortenson.
Comment #20
larowlanComment #21
tim.plunkettRerolled now that #2922033: Use the Layout Builder for EntityViewDisplays is in
Comment #22
tim.plunkettLayoutBuilderRoutes was split up but the test coverage wasn't. Fixing that now.
Comment #23
tim.plunkettPer @EclipseGc's request, I cleaned up SectionStorageBase's handling of sectionList. Since it's not guaranteed to be set, we should check first.
Comment #24
EclipseGc CreditAttribution: EclipseGc at Acquia commentedThis is a review of 21. I'll post subsequent reviews of interdiffs. I was fairly picky with this and even gave the regular text strings a pretty good looking at during the review just to make sure we didn't have obvious typos or other english-weirdness.
Ok, so I mentioned this to Tim on chat already, but both of the actual plugin classes have proxy methods they use instead of going directly to $this->sectionList. That's beneficial for a number of reasons but especially since we can throw a sane exception there if there is no SectionList object present (since setSectionList() is sort of like a views plugin's init() method). Subsequent calls to methods of the SectionListInterface could in theory be called on a null value, which throws an error to the effect of "called getSections() method on NULL object". That would be especially confusing in the case of this method since it proxies to a method of the exact same name. I suspect Tim will have a fix for this before I even finish this review, but I'm including it for completeness on this patch.
ok, I still think this is totally weird. That being said, there's a certain degree of "I like this" going on too. In a perfect world, we'd overwrite createInstance() with some additional logic that could tell if the createInstance method were called internally or externally and would throw an exception for externally sourced calls that gave some reasoning for using these methods instead. Still that is likely a huge degree of overkill and I DO really like how clean and utilitarian these methods are.
Tim and I discussed this at length. We're both generally in favor of exposing "hasThingy" methods, but Tim chose not to in this case because hasSection(7) seems sort of ambiguous and unhelpful. I agree completely, and I'm just documenting it for future reviewers.
Overall, this patch moves a lot of things around to organize them better, introduces a new plugin system and puts almost all the domain knowledge for introducing a new layout builder use case into that plugin. What is left is fairly standard sorts of things (like local tasks) which are considered temporary UI solutions by the layout builder team for our UI. All in all, I'm very happy with the state of this patch and it moves the LB code base in a really good direction without sacrificing any flexibility and instead really improves our DX.
Eclipse
Comment #25
EclipseGc CreditAttribution: EclipseGc at Acquia commentedI think this makes a lot of sense. There are a few places where we just might not need an additional prefix, so ++ to this change.
Would it make sense for both of these methods to call to $this->getSectionList() so that they and their methods also benefit from the exception that method can throw?
Eclipse
Comment #26
tim.plunkett#24
1)
Fixed in #23
2)
Interesting to note:
SectionStorageManagerInterface doesn't have a method
createInstance()
, since we extend DiscoveryInterface directly instead of PluginManagerInterface. (We do use createInstance internally since we extend DefaultPluginManager)#25
2) Fixed here, and made SectionStorageBase::$sectionList private to enforce use of the getter.
Comment #27
EclipseGc CreditAttribution: EclipseGc at Acquia commentedSo I've spent a bit of time looking at #2924058: Discuss using Layout Builder to control full site layout and replace Block UI which has an implementation for both before and after this patch and I think it's telling. Tim's gone the extra mile, so there's some non-essential-to-this-review stuff in that patch, so I'm going to highlight what is essential for the sake of this patch.
Routing
I'm comparing the patches provided in posts 2 and 13. The first real section of note is the LayoutBuilderRoutes class. The patch in 2 actually edits this class, which is something core modules can get away with, but a contrib module wishing to implement a Layout Builder solution would have to provide a whole new event subscriber, so that's an entry in the services.yml file and a php class, just to get at the route alterations which need to happen.
The patch in this issue removes that need and is illustrated by the patch on comment 13 of #2924058: Discuss using Layout Builder to control full site layout and replace Block UI because there is an alterRoutes() method on the plugin type which was implemented.
Parameter conversion/upcasting
In the patch on 2, Tim had to provide a custom class to do parameter upcasting. This means another entry in the ole services.yml file and a corresponding php class. After this issue lands, the patch in 13 illustrates again that this is delegated to a method of the plugin which again simplifies the dx of building one of these implementation.
Section Storage
The section storage system as it stands in core currently revolves around a magic naming convention in the services.yml file. This puts us up to three services.yml entries and 3 corresponding php files just to setup your own implementation of layout builder. Again, after this patch, we're still just creating the plugin class.
There are of course a few things this doesn't resolve (like the local task deriver) but I've explained all of those in previous reviews. This patch radically improves the dx of using layout builder, and moves the whole of this code base in a clearly better and cleaner direction. I've looked over this code pretty carefully and fell good about rtbcing it pending tests passing.
Eclipse
Comment #30
EclipseGc CreditAttribution: EclipseGc at Acquia commentedre-affirming testbot is weird and my rtbc.
Comment #32
tim.plunkettNo idea why
private
causes this to fail, but I think there is some strange interaction between the base class and a trait.Switching to protected (which is the 99.999% standard in Drupal) and putting back.
Comment #33
andypostPatch introduces more changes that described, also queued tests for other php versions
Comment #34
tim.plunkett@andypost, no idea what you're describing. Please update the IS as you see fit, or ask specific questions and I can answer them.
Comment #35
tim.plunkettI reread the issue, and realized I skipped over #11.1
Working on this now.
Comment #36
tim.plunkettAfter discussion with @EclipseGc and @larowlan, split ::convert() into ::extractIdFromRoute() and ::getSectionListFromId()
Comment #37
larowlannit, should we use the third arg here, passing 3?
same deal here
changes look good in my book
Comment #38
EclipseGc CreditAttribution: EclipseGc at Acquia commentedAssuming this passes tests, I am completely on board with this further detangling of the dubiously named convert() method into two new methods that actually say what they do. RTBC pending passing tests.
Eclipse
Comment #39
tim.plunkett#37
I had originally not done this on purpose, but on looking again my reason has been refactored away.
And also switching the Overrides case to use `.` too instead of `:` for consistency.
(Also, a small joy of this patch: before that change of separator would have required changes in 2 classes and 2 tests, now it's 1 class and 1 class)
Leaving at RTBC.
Comment #40
EclipseGc CreditAttribution: EclipseGc at Acquia commentedre-affirming rtbc pending passing tests.
Eclipse
Comment #41
larowlanNeeded re-roll after #2914484: Prevent the layout field from being removed if overrides exist, testing locally
Comment #42
larowlanTook for a manual test, no issues. Tested on block content and node.
Comment #43
tim.plunkettReroll looks good to me, thanks @larowlan!
Comment #44
larowlanCouple of things I picked up so far, going through the process of building the module described in #16 to really put this through its paces
I'm not sure we need this to be public. The base class already has
\Drupal\Core\Entity\EntityDisplayBase::getTargetEntityTypeId
There are three calls:
- one from default storage - which is a plugin that can inject the entity type manager (and already does)
- one from the form, which already has the entity type manager injected
- one from self, which obviously can still be called if it is made protected
Feels like unnecessary API surface that can be removed
I don't think this API belongs on this object. If another module needs it, it can't call it without instantiating the display, but there is no use of
$this
, so the display is not needed.This whole method can be removed from the interface and moved to a standalone service in my opinion. Again, smaller API surface for the view display (albeit moved elsewhere)
Then the only usage (in the section storage) can have the service injected via CFPI.
And other modules can use it.
Comment #45
larowlanWhen implementing this, it took me a while to work out what this method is for.
It is used to redirect the user back to somewhere after saving the layout.
So I think perhaps
getRedirectUrl
might be a more intuitive name?Comment #46
larowlanwe need real docs here, there is no parent method for the
{@inheritdoc}
Comment #47
larowlanOK, I think I've completed the process of implementing one of these plugins, my observations are as above comments.
I think it works well, the alterRoutes still feels a bit light on for docs, that was probably the most confusing/difficult bit to implement/understand - this is what we have
If it could go into detail about what routes to alter, and how to work with the
buildRoutes
method, that'd go a long way to making this a largely smooth process.The project WIP is https://www.drupal.org/project/layout_library, @tim.plunkett, @samuel.mortenson, @EclipseGC you all have push access.
The plan is I'll flesh it out further to the point of being useful in coming months.
Comment #48
larowlanFor #44 to #47
Comment #49
tim.plunkett#44.1
Good point, fixed
#44.2
Added a new service. Purposefully did not include an interface, as this should eventually be considered for a generic service.
#45
Agreed!
#46
Inlined the one instance, and documented the other.
#47
Documented, and renamed from alterRoutes to buildRoutes. I don't think the altering aspect is relevant.
https://github.com/timplunkett/d8-layouts/commits/2937483-layout_section...
Comment #50
EclipseGc CreditAttribution: EclipseGc at Acquia commentedThe interdiff looks really good to me. RTBC pending passing tests.
Eclipse
Comment #51
larowlanValidated the API of a new layout section storage plugin with Layout library module
Screenshots of editing layouts from the library
Winning!
Comment #52
larowlanThe only extension point/API missing is in this code on
\Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::getRuntimeSections
It effectively embeds knowledge of the override storage handler in the defaults, but doesn't allow other modules to interact, limiting us to two plugin implementations without needing to do some hacky stuff like #2941808: Subclass \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::getRuntimeSections.
I'm not sure how to unpick that though.
This is an entity, so using module handlers or plugin managers is even more hacky.
Open to suggestions but I don't really see any better way of doing it
Comment #53
tim.plunkett\Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::buildMultiple() is effectively just a hook_entity_view_alter() moved into OO code.
It's only coincidentally located in the implementation of a SectionListInterface, it could easily be moved back out to demonstrate that this is a separate system you are hooking into, like this https://gist.github.com/timplunkett/0443bdd20d1d80ed2dbb8ce9b86ed761
Not all implementations will be related to rendering fieldable entities, which is why this isn't something on the plugin itself.
It is unfortunate indeed that we're spreading around knowledge of how to get at the sections directly.
This is why I opened #2936360: Remove duplicate references to the "layout_builder__layout" field from Layout Builder, where we can continue this discussion.
But I feel that it is out of scope for this issue, as we are currently not changing either buildMultiple() or getRuntimeSections() here.
Comment #54
tim.plunkettComment #55
larowlanBut I feel that it is out of scope for this issue, as we are currently not changing either buildMultiple() or getRuntimeSections() here
Agree
Regardless, I like the current code more than that in the gist that uses
hook_entity_view_alter()
And #2941808: Subclass \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::getRuntimeSections can just use hook_entity_view_alter() if it has to.
Comment #56
larowlansaving review credits
Comment #58
larowlanThis has been reviewed extensively, and I even took the extraordinary step of writing a contrib module to verify the API.
Committed as 10eb692 and pushed to 8.6.x
🦅🍒
Comment #59
tim.plunkett