Problem/Motivation

Drupal entities should be able to control their own layout. They can't right now.

Proposed resolution

At Drupalcon Baltimore's sprints, we (@tim.plunkett, @DyanneNova, @eaton, @EclipseGc, @pixelite) discussed at length an approach to the naming and storage for this process. Stealing heavily from Panelizer, we've settled on a multi-value layout section field which stores individual layout ids and corresponding plugin configuration for each block within the layout. This approach allows multiple layouts on the page to be stacked on top of each other. The plan will likely include some simplified layouts MEANT to be stacked on top of others in this category allowing the end user the control to build a layout that works for their needs.

Remaining tasks

Test coverage for the new field
A UI for interacting with it.

User interface changes

It will add a whole new UI for interacting with this field to control layout of an entity.

API changes

None

Data model changes

None

Comments

EclipseGc created an issue. See original summary.

EclipseGc’s picture

Status: Active » Needs work
FileSize
7.54 KB

Initial code for this approach.

Eclipse

tim.plunkett’s picture

Component: other » layout.module
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Blocks-Layouts
Parent issue: » #2811175: [plan] Add layouts to Drupal
FileSize
10.54 KB
11.71 KB

Updated the patch for coding standards fixes
Renamed a couple things for clarity
Added rudimentary coverage of the formatter

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
13.08 KB

composer.json and hook_help are required.

EclipseGc’s picture

Ok, I TOTALLY added the block I'm using for testing to the completely wrong place, but I'm certain Tim can help me out with this. I wasn't certain how best to do this.

I expanded the test to include multi-value. I'll probably try to make certain we have multi-lingual test coverage shortly.

Eclipse

tim.plunkett’s picture

In the interdiff:
CS fixes for previous fix
Add support for context-aware blocks
Not in the interdiff:
Removed TestContentBlock since TestBlockInstantiation works just fine

larowlan’s picture

  1. +++ b/core/modules/layout_builder/layout_builder.module
    @@ -0,0 +1,19 @@
    +      $output .= '<p>' . t('Layout Builder provides layout building utility, surprisingly.') . '</p>';
    

    Yes hook_help is a bit of a white elephant, but not sure folks will appreciate this self-deprecating humour.

  2. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldFormatter/LayoutSectionFormatter.php
    @@ -0,0 +1,166 @@
    +      foreach ($blocks as $uuid => $configuration) {
    +        /** @var \Drupal\Core\Block\BlockPluginInterface $block */
    +        $block = $this->blockManager->createInstance($configuration['plugin_id'], $configuration);
    +        if ($block instanceof ContextAwarePluginInterface) {
    +          $contexts = $this->contextRepository->getRuntimeContexts(array_values($block->getContextMapping()));
    +          $this->contextHandler->applyContextMapping($block, $contexts);
    +        }
    +        $access = $block->access($this->account, TRUE);
    +        if ($access->isAllowed()) {
    +          $regions[$region][$uuid] = $block->build();
    +        }
    

    this looks generic and useful enough to be somewhere else? Putting blocks into regions, applying context and checking access. I'm assuming we do the same thing in multiple places. And I'm not sure its the domain of a formatter to have all that knowledge, should defer to something else.

  3. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldType/LayoutSectionItem.php
    @@ -0,0 +1,87 @@
    +    $properties[static::mainPropertyName()] = MapDataDefinition::create('map')
    +      ->setLabel(new TranslatableMarkup('Layout Section'))
    +      ->setRequired(FALSE);
    

    eww serialized data. This means we *cannot* support updating this over REST (unserializing user provided content is a security hole). We need to make sure that we have a normalizer that explicitly filters this out.

  4. +++ b/core/modules/layout_builder/tests/src/Kernel/LayoutSectionFormatterTest.php
    @@ -0,0 +1,212 @@
    +  public function providerTestLayoutSectionFormatter() {
    

    there are no tests here that cover the access check (but again I think that logic belongs somewhere else outside a formatter, and hence should be tested outside the formatter too).

tim.plunkett’s picture

First up! We're moving this work to a core sandbox https://www.drupal.org/sandbox/timplunkett/2874553
It's not really ready for review yet, and this will become an epic and terrible thread if we keep posting every patch here.

To address #8 directly

1) Yes. Just wanted to get the failure out of the way for the time being

2) Absolutely. Page Manager, Panels, and Block all have their own versions of this, but with other domain-specific bits sprinkled in. This absolutely needs to be a stand-alone thing.

3) I defer to @EclipseGc on this, it was his idea

4) Also true.

EclipseGc’s picture

Ok, so wrt 3...

I'm open to suggestions, but the basic requirement is that each block & its configuration would be stored in this field and sorted into each region of the corresponding layout. In Panelizer, we serialize this data today. I'm open to building some sort of validation layer here, but we really can't normalize this effectively without a REALLY messy table somewhere.

Thoughts?

Eclipse

EclipseGc’s picture

Is some serialization OTHER than php a viable option? or does the security issue persist across all serialization types?

Eclipse

larowlan’s picture

Only PHP is the issue, so json and yaml are fine

yoroy’s picture

Issue tags: +Usability
xjm’s picture

Sweet!

I see this doesn't depend directly on field_layout; how will they interact?

"Per-entity overrides" was a "Could" have on the roadmap of #2795833: [plan] Add layouts to entity displays (both form and view). It sounds like feedback at DrupalCon indicated this feature was essential enough to reprioritize the roadmap?

I'm a little concerned about the experimental module proliferation, also. It would be good to see other parts of the layout initiative become stable before we add additional features.

EclipseGc’s picture

WRT how this interacts with field_layout. I think that's TBD to a certain degree. Conceptually, this is just a field that renders, so if field_layout placed that field somewhere, it'd render it's own contents within that layout region (kinda mini-panels-ish). That's not to say that is desirable or undesirable, I'm just explaining what would happen as the code sits right now. I'm fairly neutral on this opinion.

Tim and I have discussed unifying the rendering process between the two approaches so we have one thing (probably a service) which does that work. I've already abstracted a service out in my branch on tim's sandbox for this purpose. It is different from field layout though since this is explicitly blocks and field layout is laying out fields (i.e. not blocks). We'll need to either rectify that difference or simply agree they are completely different things. Again, I have no opinion on this topic.

I think this was reprioritized in part because I had an architecture in my head I thought would work, ran it past tim.plunkett, DyanneNova, eaton and pixelite and we spent a good amount of that time agreeing on and tweaking the basics, so I started doing it with tim's guidance and help in terms of test coverage and commentary about how we get this in. Tim has expressed similar concerns about the proliferation of experimental modules as well. Again I'm just following people's leads so if that's a concern, happy to help address it. Tim gave me an example patch once on this topic and we agreed we could revisit the idea as the code became more mature.

DyanneNova's UI designs (which we should really maybe post on this issue at some point) are a WAAAAAY shinier version of something I've been talking about with the lightning team for quite some time now, so to see it fully-formed and more well-thought-out, I figured it was worth doing everything I could to make it happen.

Eclipse

eaton’s picture

Sorry for the long delay, as mentioned in the other issue I got hit pretty hard with Drupalflu and am just prairie-dogging back into the world of the living.

Aside from experimental/architectural considerations, one thing I want to bring up is concern about the nomenclature. Right now it's called a "Section" field and I think we run a serious risk of confusing users who are adjusting to Drupal's naming conventions, as well as needlessly obscuring the nature of the field type. Why not name it a "Layout" field, since that's what it explicitly is? That preserves "section" as yet another abstract noun in our dwindling strategic reserve, and (I think) makes the field type's intended use case more transparent. It also makes the connection between "layouts" (when mentioned in contrib, documentation, etc) and the options that appear in the field's configuration, less mystery-meat.

Finally, are that any objections to tightening up the title of the issue to: "Introduce field type that renders arbitrary elements in a selectable layout"? The current title crosses over into the domain of "Field Layouts" and per-entity field layout overrides, which should definitely remain a different beast architecturally (IMO).

eaton’s picture

Also, as a point of philosophy: I'd be REALLY worried if this feature made it into a core release but Field Layouts didn't; if it's the only option in core for layout control, people will use the hammer they have, and we'll quickly encounter a generation of sites composed of nothing but layout fields — a structured content nightmare, for all their usefulness.

This field is a godsend for the edge cases that Field Layouts wouldn't help — one-off landing pages composed mostly of blocks and other bits that really only appear on that particular page, ever. For cases where users just want more layout and positioning control over otherwise (fairly) structured content, plopping a layout field in there would kill any hope of decoupling, or heck, generating a meaningful RSS feed.

EclipseGc’s picture

I'm not opposed or in favor of your semantics argument. I think I'd rather others make that determination. It seems like a pretty simple change in the code either way when the time comes.

Do you want to spawn off a separate issue for that or do it here?

Eclipse

yoroy’s picture

If it helps: yes please, lets use the words that go with the concept and go with "layout field". No need to inject a new term that creates more dots for people to connect in their mental model of things.

EclipseGc’s picture

That seems pretty authoritative. I'll get this in the next reroll.

Eclipse

tim.plunkett’s picture

Title: Introduce architecture and UX to allow individual entities to control their layout » Introduce field type that renders arbitrary elements in a selectable layout

Well said.

EclipseGc’s picture

Tim and I discussed this this morning and while we're both on board with changing this, we feel like "layout" is a bit generic. This isn't push-back, just that we're unwilling to change the strings at this time because if they were to change again before we get everything hashed out, it could be quite a bit more difficult to change them to whatever the new thing is. So we are on board with the change, but are going to delay it to the last step (or one of the last) in order to prevent problems in-case another rename were to emerge in the mean time.

TL:DR; Yes, later.

Eclipse

Bojhan’s picture

Can we get some screens?

yoroy’s picture

Or even some conceptual diagrams of what's the goal here. Is this about (eventually) implementing the "layout blocks" UI concepts @dyannenova designed?

Also: slightly amused by the decision to *leave open* the possibility to bikeshed the naming of things ;-P

tacituseu’s picture

Status: Needs review » Needs work

Changing to NW per #9 to avoid pre-bikesheding.

EclipseGc’s picture

Yoroy, Bojhan,

We are working from @dyannenova's designs. I'm certain you've probably seen them at least once. I'll see if we can get them documented here shortly.

Also, not trying to leave that bikeshed open, just pointing out that the name we've all agreed to would be really hard to rename away from if before this lands we wanted to do that. We're all still on board with the name.

Eclipse

EclipseGc’s picture

Forgot she gave me the files. I'd say these 3 designs are the inspiration for what we're trying to essentially accomplish. Obviously everything is subject to some degree of change as we get in and start working with it, but I think this gives a basic idea of the direction pretty clearly.

Eclipse

EclipseGc’s picture

EclipseGc’s picture

I'd encourage focussing on what's happening in the settings tray and the main content region. The toolbar is going to be interesting due to various technical concerns and the squishiness of that api as it stand currently.

Eclipse

yoroy’s picture

Thanks for the update!

andypost’s picture