Problem/Motivation

#1875974: Abstract 'component type' specific code out of EntityDisplay sought to entirely refactor how fields are handled for EntityDisplay objects.
This is still desirable, but is a huge effort with tricky BC concerns.

Proposed resolution

In the meantime, the iterative step is to add native support for placing block plugins within an entity display.
Block plugins are a \Drupal\Core\Block concept, separate from the block.module

Remaining tasks

Write update path and tests

User interface changes

Yes

API changes

API additions only

Data model changes

Yes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
42.17 KB
jonathanshaw’s picture

Thanks Tim! Obviously a huge win for sitebuilders here. Goodbye to a whole class of clunky contrib modules to achieve a very common use case.

swentel’s picture

Awesome. Tested it and works like a charm!

Only quickly scanned the code, looks fine to me.

+++ b/core/modules/field_ui/src/Form/EntityViewDisplayEditForm.php
@@ -83,6 +94,242 @@ protected function buildExtraFieldRow($field_id, $extra_field) {
+      // the new settings fro the next rebuild.

nitpick: missing 'm' in fro

Don't want to set it to needs work for now for missing things (e.g. config dependencies, post update hook to update entity displays) so more people can look at it.

dawehner’s picture

Just some quick comments ...

  1. +++ b/core/lib/Drupal/Core/Entity/Display/EntityViewDisplayInterface.php
    @@ -46,4 +46,45 @@ public function build(FieldableEntityInterface $entity);
    +   * @return array|null
    +   *   The array of display options, keyed by block name.
    ...
    +   *
    +   * @return \Drupal\Core\Block\BlockPluginInterface
    +   *   The instantiated block plugin.
    +   */
    +  public function getBlock($name);
    

    These methods names are weird ... given that one returns an object and the other one an array of configuration.

  2. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
    @@ -263,6 +308,23 @@ public function buildMultiple(array $entities) {
    +
    +        if ($block instanceof ContextAwarePluginInterface) {
    +          $contexts = \Drupal::service('context.repository')->getRuntimeContexts($block->getContextMapping());
    +          \Drupal::service('context.handler')->applyContextMapping($block, $contexts);
    +        }
    

    I'm wondering whether we could pass in the current rendered entity as context. With that we would have to rely less on block runtime contexts

groovedork’s picture

This is great, but as a sitebuilder I would prefer a solution described here, as it has a number of advantages:
- It offers a clean workaround for the problem that the title is not a field.
- It allows for fields to be placed in any of the theme's regions. This would be extremely valuable, and would allow much more flexibilty than this solution offers.

jonathanshaw’s picture

#7 no reason not to have both? They're not way duplicates AFAIK.

tim.plunkett’s picture

That other issue is an interesting idea, but as jonathanshaw points out it is not conflicting with this one.

groovedork’s picture

It may not conflict technically, but they do overlap functionally. Both systems could be used to place blocks in between fields.

Energy to work on Drupal may not be infinite. If energy could be redirected, then I propose to redirect it towards the solution that solves more problems (title does not behave like a field) and offers greater flexibility (mix blocks and fields into any region).

This solution, while wonderful in its own right, could lead to lethargy and slowdown in the development of a solution that I believe would be better, and also covers this use case.

tim.plunkett’s picture

Please comment on the other issue to encourage others to work on it, and stop derailing my efforts here. Thanks!

groovedork’s picture

Ok, sorry.

tim.plunkett’s picture

#5
The typo was "for" :)
What do you mean config dependencies?
Adding update path to IS

#6
1) Agreed. Renamed to match the corresponding method, this is now getBlockComponents()
2) We can pass that in as an available context, but we still need to get the mappings for runtime.

tim.plunkett’s picture

Bojhan’s picture

What? :D

What is a block field?

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
517 bytes
44.21 KB

Totally love this idea :)

This should fix the failing test.

Manuel Garcia’s picture

Status: Needs work » Needs review

OK thats a new failing test, but I've just run it locally and it passes... random failure?

swentel’s picture

@tim.plunkett

What do you mean config dependencies?

Nevermind that one, completely wrong in my mind :)

swentel’s picture

@tim.plunkett actually, I do remember again re: (config) dependencies. If you add a block in the entity display, a dependency should be added (module I guess) depending on the module that provides that plugin ? Probably in EntityDisplayBase::calculateDependencies ?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.