Abstract 'component type' specific code out of EntityDisplay

So set of components looks like PluginBag already used in core
Maybe we just convert all extra fields to TypedData plugings as #1732730: [meta] Implement the new entity field API for all field types done for fields
Seems it's time to resolve #1954188: Revaluate the concept of 'extra fields' or #1818680: Eliminate hook_field_extra_fields() / Redesign field UI architecture

tagging

format

This would be very helpful to implement node and comment links on top of it!

What's left here?

The code in the sandbox works (and is also green on the test helper issue), but we need to check for interfaces, method names etc. That's at least the first step. Will merge head on the branch this weekend and reroll on the helper issue.

Updated issue summary.

To determine if this is still eligible for D8, it would be very helpful to get an updated issue summary that explains exactly what API changes are entailed. I realize the helper issue isn't passing tests yet, but are we far enough along to know what the API changes are likely to be? I'm adding the "API change" tag based on the assumption that some API change will be needed here. Please remove the tag if I'm wrong about that.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Based on the updated issue summary, I'm reclassifying this as an API addition rather than a change. Please correct me if I'm wrong on that.

..as I mentioned in #1848068-124: Helper issue for "EntityDisplay"

We need to change only EntityDisplay->setComponent() to allow set different types of plugins
EntityDisplay->getComponent($name) could stay the same because component type (currently handler_type, needs better name) already stored in display entity object
Also EntityDisplay->getComponents($type = '') seems better $type = 'field'
For field component type probably better to separate base and configurable fields

I also agree that we should remove $type from getComponent(). It's not needed at all because we don't 'namespace' the components by type. And a default of 'field' for setComponent() sounds great.

more details about API changes

I'll try to reroll... Thats a start

I spend some hours rerolling and fixing bugs!
Testbots aren't back yet but I'm confident I fixed almost everything.

#1848068: Helper issue for "EntityDisplay"

There are some todo's and incomplete docs in the patch. As everything is build on the sandbox on branch "1875974-entity-components" everyone can fix stuff if they see issues.

Especially the extra field stuff needs serious review.

Should be green patch, makes 'type' optional if possible. Done by moving it to the second param everywhere and defaulting to 'field'.
It would be nice if there was an event after saving the fields in the UI that passes the display so "other modules" could use this for additional actions.

Can you expand on the use cases you have in mind ?

When an entity display contains components that are not available anymore. For example, *some* contrib module provides plugins to add fields in code and for a reason you decide to delete that file/plugin. After clearing the caches that field is gone but it's still in the entity display.

After re saving the field UI, a module maybe wants to inspect every component of it's display to make sure it still exists. If it doesn't exist, it should remove it...

That particular module could alter the form and add a second submit callback that does that, but it would nice if core provided such an event somehow.

Either way contrib will need the display in the form system.

I need more help on this one...

- The $type param has a default value of 'field' for all methods, except on getComponents() where it's just '' ?

This different because leaving it to '' will return everything, which is the 80% use case.

- EntityDisplay::$pluginManager shouldn't be needed anymore, the "plugin manager that is supposed to return rendered plugin objects" is a matter of each individual DisplayComponentHandler plugin

The plugin manager is used to fetch the widget for getRenderer on the formdisplay. I think this is a side effect of a base class handling the form and the display side of things.

The third thing:
- I don't understand remove component and what it tries to do o_O, so I have no idea if it's possible to remove $type. For example why are we *settings* stuff in a function that should *remove* something. ANd that remove function calls massageIn, which sounds weird...
$this->content[$name] = array('handler_type' => $type) + $options;
- We can remove it from getComponent if "$this->content[$name]['handler_type']" always contains the correct $type. What should be the case I guess.

IF that is the case can we assume the same for.

public function getRenderer($name, $type = 'field') {
    if (!isset($this->renderers[$name]) && !array_key_exists($name, $this->renderers)) {
      $options = $this->getComponent($name, $type);
      if ($handler = $this->getComponentHandler($type)) {

In general I understand little of functions in the component handler, maybe because there is no documentation or no interface? ;) (massage in / out / prepare stuff) doesn't tell me much even after spending hours in it.
I fixed some bugs by trial and error and some well chosen guesses :p

I've pushed 2 commits on top of @aspilicious refactoring, see interdiff #1848068-160: Helper issue for "EntityDisplay"
Commit b3c7d064ee6437d502ed80039bf2d276842b0f70 properly injects needed services into handler plugins.

Currently DisplayComponentHandlerPluginManager instantiates needed plugins only once and stores them in protected $plugins array, this approach needs to use setContext() each time the handler used.

Another approach could be to use factory pattern for handler plugins and store the instances directly in display entity, this will eat a bit more memory but could allow remove context switching when each component accessed.

There's still a question about validation of component existence #1848068-158: Helper issue for "EntityDisplay" shows thet when field or instance is deleted we still call massageIn() so I've added a check for field but probably we need to check for field instance.

The magic with default values for MassageIn() & massageOut() is really annoying
No idea how to fit into interface

Andypost had the idea to loop through each display component to see if it exist on setComponent.
That way you don't need to pass the type.
It does make set component slower.

I sthta a good or terrible idea?

I just did a short review.

1. That massageIn and out stuff was quite confusing. Not sure why it's needed, but that might become clearer once it has docs ;)

2. +++ b/core/profiles/standard/config/entity.form_display.node.article.default.yml
   @@ -20,6 +22,7 @@ content:
   +    handler_type: field
   

   Would component_type make more sense here? As afaik each of the entries is a display component?

   Otherwise, I could see it even be just "type" and move the field's type to "field_type". The API is not primarily about fields anymore, so the keys should reflect that.

3. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
   @@ -187,64 +202,42 @@ public function getComponents() {
   +    if ($handler = $this->getComponentHandler($type)) {
   

   Do we need the handler term here as well? It's just a display component that I get.

   Instead, imo it's more that the current getComponent() does return the component definition, but not the component object.

@fago There's a new patch about (1) in testing issue #1848068-162: Helper issue for "EntityDisplay" and fixed broken tests in #165 also @yched already commented about approach #166

2) makes sense, probably 'component' key is enough

3) yes, we do special processing in onComponentGet|Set|Remove by the handler plugin mostly because of "extra fields" pita

The idea about using pluginBag exposed by @yched in #1848068-166: Helper issue for "EntityDisplay" makes sense but I also cares about performance overhead by introducing additional wrapper.

The context switching is very light function call comparing to instantiation of component handler plugins for each display and seems that plugin bag would do the same.
So having a lot of display objects on page will bring request time down.

PS: I'll roll new patch here after HEAD would be fixed in #2147581: Testbot has xdebug enabled with xdebug.max_nesting_level = 100

we should switch the internal storage in EntityDisplays

makes sense but this makes ability to use pluginBag is hard from first view, otoh ds-module could split "content" part into region names by overriding base display, code reuse++

We can change storage until beta, so the issue needs to be approved as API change
and 'handler_type' could be easily grep'ed anytime

Related #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title is RTBC now, so here is a question - do we need a different component handlers for field and configurable field in the new world?

PS: will try to try to implement new setComponent() in next weekend, this should move us from API-change area to API-addition because EntityDisplayBaseInterface just extended without BC breaks, and yep, patch should be seriously much less

Issue #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title break a lot of things, so I think that better to postpone this one on

Going to spend some time with it, I'll see how far I can get with it.

It's not working yet, it doesn't save (or load) the correct items, everything is displayed hidden.
But I do think I'm going in the right direction.
At least this version is more of an api addition than the previous versions...

Can someone review the "general direction" of the patch?
I committed it to the 1875974-entity-components-aspilicious branch.

Unassigning, everyone can work on it :)

Reviewing my own patch, ExtraFieldDisplayComponentHandler is missing the "hasElement" function

I'm not going to work on it this weekend, so if anyone wants to finish it go ahead.
I'm still missing something.

At the moment there are two options
1) field is added to content
2) field is hidden

Can't we introduce the notion of regions here?
Makes it easy to introduce multiple regions AND makes the yml file easier to read, else I have to add some kind of funky mapping.

so in stead of going with:
$this->content['title']

We could have something like
$this->regions['content']['title']

EDITED EXAMPLE ABOVE

Pushed some small changes, now a save of the display fatals but thats ok ;)

Ow! And one more thing I noticed, I saw that saving the default display also triggered a save of teaser. Is that by design?

This one at least work. Probably needs some doc updates.

MassageIn and massageOut are still confusing, certainly after my changes.

The massageIn is called AFTER pressing the save button for each element that is inside content.
So it's a function that fetches that manipulate the options. Before getting saved. So that function name is kinda ok, I'm open for suggestions as I think their could be some improvement in the naming.

The massageOut is a different story, originally it was located in the removeComponent function. We don't need any massaging anymore there so I moved it to the getExportOptions. But as this massageOut function has nothing todo with massageIn I think we should rename it to something like: "massageExportOptions()"

Anyway I noticed we are missing an interface with the needed documentation. Anyone want to take that task? ;) :p

The order of parameters for "getInstance" apparantly changed, new patch!

Lets see if this fixes some tests...

Fixed most warnings

+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
@@ -315,41 +277,57 @@ public function getHighestWeight() {
+  public function getComponentHandlerByElementName($name) {
+    if (!isset($this->handlers[$name])) {
+      $handler = NULL;
+      $handlers = $this->handlerManager->getDefinitions();
+      foreach (array_keys($handlers) as $type) {
...
+        if ($handler && $handler->hasElement($name)) {
+          continue;
+        }
+      }
+      $this->handlers[$name] = $handler;
+    }
 
+    return $this->handlers[$name];
+  }

$handler = NULL; isn't doing anything.
I think you mean break instead on continue.
$handler = $this->getComponentHandler($type); should use a different variable or else reset it it after the check, otherwise the last one will win if none match.

So this should be better. Not sure if it will fix some issues.

Should fix most of failures, also adds a @todo.
Plugins now serve view and form both so caching needs work

PS: comment pager bug introduced in #2155387: Multiple comment fields per entity are supported, but break horribly when they need pagers

Added interface for handler plugin and initial doc blocks, alter hook needs work

re-roll

EDIT re-opened #2210177: Use getSetting() in comment formatter

#2095195: Remove deprecated field_attach_form_*() got in, it would be great to get this one forward again!

Last merge in

git.drupal.org:sandbox/yched/1736366.git
   d8ae3fd..9848e1e  1875974-entity-components-andypost

I'm thinking about how to render comment module components to properly test the issue, because history and comment statistics are seriously affected how this fromatter is configured to render a long comment list

Let's see how hard this breaks

Hmm, something really bad in the merge there .. needs fixing

Let's see...

And I'll fix the funky phpunit file stuff tomorrow

Clean file now without the accidental php unit removals

It's green lets review!

1. +++ b/core/lib/Drupal/Core/Entity/DisplayComponentHandlerPluginManager.php
   @@ -0,0 +1,62 @@
   +    // @todo Document the alter hook.
   +    $this->alterInfo('display_component_handler_info');
   

   this needs work

2. +++ b/core/lib/Drupal/Core/Entity/DisplayComponentHandlerPluginManager.php
   @@ -0,0 +1,62 @@
   +    if (!isset($this->plugins[$plugin_id]) && !array_key_exists($plugin_id, $this->plugins)) {
   
   +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -410,4 +401,69 @@ public function onDependencyRemoval(array $dependencies) {
   +    if (!array_key_exists($name, $this->renderers)) {
   

   seems we need to unify that

3. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -410,4 +401,69 @@ public function onDependencyRemoval(array $dependencies) {
   +    if (!isset($this->handlers[$name])) {
   

   This makes to run this each time, probably array_key_exists() should be here

4. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -410,4 +401,69 @@ public function onDependencyRemoval(array $dependencies) {
   +      foreach (array_keys($handlers) as $type) {
   +        $handler = $this->getComponentHandler($type);
   +        if ($handler && $handler->hasElement($name)) {
   +          break;
   +        }
   +        $handler = NULL;
   +      }
   +      $this->handlers[$name] = $handler;
   

   $handler should be defined before "foreach"

5. +++ b/core/lib/Drupal/Core/Entity/Plugin/DisplayComponent/FieldDisplayComponentHandler.php
   @@ -0,0 +1,194 @@
   +    $entity_manager = \Drupal::entityManager();
   

   needs to be injected

While working on a POC for DS I noticed we are lacking something :D.
In the code that renders the field UI overview we are still hardcoding fields and extra fields.
So we still need dirty alter hooks.

Some sinippets from displayOverviewBase

/**
   * {@inheritdoc}
   */
  public function buildForm(array $form, FormStateInterface $form_state, $entity_type_id = NULL, $bundle = NULL) {
    parent::buildForm($form, $form_state, $entity_type_id, $bundle);

    if (empty($this->mode)) {
      $this->mode = 'default';
    }

    $field_definitions = $this->getFieldDefinitions();
    $extra_fields = $this->getExtraFields();

    if (empty($field_definitions) && empty($extra_fields) && $route_info = FieldUI::getOverviewRouteInfo($this->entity_type, $this->bundle)) {
      drupal_set_message($this->t('There are no fields yet added. You can add new fields on the <a href="@link">Manage fields</a> page.', array('@link' => $route_info->toString())), 'warning');
      return $form;
    }

    // Field rows.
    foreach ($field_definitions as $field_name => $field_definition) {
      $table[$field_name] = $this->buildFieldRow($field_definition, $entity_display, $form, $form_state);
    }

    // Non-field elements.
    foreach ($extra_fields as $field_id => $extra_field) {
      $table[$field_id] = $this->buildExtraFieldRow($field_id, $extra_field, $entity_display);
    }

This can easily be solved by abstracting the build form code into the display component but I have no idea if these kinda changes are allowed.

Cleanup + reroll

I'm sure that having of actual usage of component would be a way to core!

Patch looks OK, Currently implementing a component handler for fieldgroup.

It's not yet finished after a talk with Yched on drupalcon we need te rename some stuff and move the build/render logic to the component patch. But I don't think fieldgroup is affected by the build part.

Found a way to move the build part onto the component handler.
Feels extremely dirty as I needed to pass $this->content to the buildMultiple function.

Curious how many things I broke with this...

I'm going to reroll 81

Lets see...

ignore the first lines of the patch ;)

The annotation was in the wrong place bu the plugin manager still doesn't work, debugging...

New try

Ok... apparantly something changed in the last hour

Small fix

Awesome, so now we need a plan to bring that to core at this stage

I think merge should not be hard, but no idea how to write beta evaluation...

Rerolled.

No changes.

I'm sure there's a way to make that in within minor releases

I guess we can try again for 8.1.x

Suppose display should follow changes on #2296423: Implement layout plugin type in core

rerolled path from comment #81

fix not clean merge

fix not clean merge

Here's a re-roll with a fix in the plugin manager that was killing lots of tests.

FormatterPluginManager is throwing errors because it requires $configuration['label'], not sure where that is meant to be initialised, if at all? I've put it in EntityDisplayBase for now to try and work closer to a green patch.

Schema updates required makes me think this is wrong but i'm not sure what FormatterPluginManager would do.

Fixes a bug with the plugin not been unset causing stale settings in the formatter and reverts the label changes from the previous two patches in favour of initialising an empty label in FormatterPluginManager as needed.

The array_filter in FieldDisplayComponentHandler::getFieldDefinitions() was filtering out base fields, in this case the uuid field which meant it could not be rendered with $field->view() because it had no display settings.

@benjy Thanx for taking that!
It's very interesting what plugin has no label and where it fails with empty label. Maybe use assert()

1. +++ b/core/lib/Drupal/Core/Entity/Annotation/DisplayComponent.php
   @@ -0,0 +1,26 @@
   + * Contains \Drupal\Core\Entity\Annotation\DisplayComponent.
   
   +++ b/core/lib/Drupal/Core/Entity/DisplayComponentHandlerBase.php
   @@ -0,0 +1,64 @@
   + * Contains \Drupal\Core\Entity\DisplayComponentHandlerBase.
   
   +++ b/core/lib/Drupal/Core/Entity/DisplayComponentHandlerInterface.php
   @@ -0,0 +1,88 @@
   + * Contains \Drupal\Core\Entity\DisplayComponentHandlerInterface.
   
   +++ b/core/lib/Drupal/Core/Entity/DisplayComponentHandlerPluginManager.php
   @@ -0,0 +1,61 @@
   + * @file
   + * Contains \Drupal\Core\Entity\DisplayComponentHandlerPluginManager.
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/DisplayComponent/ExtraFieldDisplayComponentHandler.php
   @@ -0,0 +1,59 @@
   + * Contains \Drupal\Core\Entity\Plugin\DisplayComponent\ExtraFieldDisplayComponentHandler.
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/DisplayComponent/FieldDisplayComponentHandler.php
   @@ -0,0 +1,194 @@
   + * Contains \Drupal\Core\Entity\Plugin\DisplayComponent\FieldDisplayComponentHandler.
   

   no longer needed, please remove

2. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
   @@ -257,7 +257,7 @@ public function buildMultiple(array $entities) {
   -          // Apply the field access cacheability metadata to the render array.
   +
   

   out of scope

3. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -326,16 +346,21 @@ public function setComponent($name, array $options = array()) {
        // Ensure we always have an empty settings and array.
   +    // @TODO, is this the right place to ensure label is set?
        $options += ['settings' => [], 'third_party_settings' => []];
   

   looks like remains to remove

4. +++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
   @@ -56,6 +56,11 @@ public function createInstance($plugin_id, array $configuration = array()) {
   +    // @TODO, is this missing somewhere else?
   +    if (empty($configuration['label'])) {
   +      $configuration['label'] = '';
   

   I'd better test with isset() and trigger some assert() here
   Did you found what caused missing label?

+++ b/core/lib/Drupal/Core/Entity/Plugin/DisplayComponent/FieldDisplayComponentHandler.php
@@ -188,16 +188,7 @@ protected function getFieldDefinition($field_name) {
-    // The display only cares about fields that specify display options.
-    // Discard base fields that are not rendered through formatters / widgets.
-    return array_filter($definitions, function (FieldDefinitionInterface $definition) use ($display_context) {
-      return $definition->getDisplayOptions($display_context);
-    });
+    return $this->entityManager->getFieldDefinitions($this->context['entity_type'], $this->context['bundle']);

Maybe it makes sense to revert with different function name
getDisplayableFields() ???

We should filter out fields that has no configurable display

We should filter out fields that has no configurable display

Yeah the tests fail in funny ways, shame we don't have any explicit tests for this.

Interdiff against 134, i set some display options for uuid so that UuidFormatterTest passes but maybe that test is just wrong trying to render uuid when it doesn't have display options?

Will address 137 later tonight.

Addressed 137.

UuidFormatterTest passes but maybe that test is just wrong trying to render uuid when it doesn't have display option

I guess it expected because https://www.drupal.org/node/2208327
Probably that points weakness of abstraction in this area

Suppose patch missing fallback logic support when field rendered directly with formatter options instead of usage of entity display settings, see \Drupal\Core\Field\FieldItemListInterface::view()

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
@@ -281,13 +300,14 @@ public function calculateDependencies() {
+    $handlers = $this->handlerManager->getDefinitions();
+    foreach (array_keys($handlers) as $type) {
+      $properties = $this->getComponentHandler($type)->massageOut($properties);
     }

@@ -326,16 +346,20 @@ public function setComponent($name, array $options = array()) {
+    $handler = $this->getComponentHandlerByElementName($name);

Here should be a fallback to rendering field with passed in display options instead of entity display

So, 140 is green, 141 would be if we removed the assertion. I can't do much more here without some direction from someone who knows the end goal better than I.

I don't much like the naming of massageIn/massageOut, they seem more like preSave() and postLoad() methods?

EDIT, crossposted with #144, will try implement that.

One observation, prepareDisplayComponents() is never called right now but i'm not sure where that would happen? Maybe inside setComponent() after we have a handler, rather than calling massageIn()?

I don't see how the fallback is going to work, we need the handler but we don't get the handler because of the filter mentioned in #139.

I think we need tests for new classes added here. Here are some review points.

1. +++ b/core/lib/Drupal/Core/Entity/Annotation/DisplayComponent.php
   @@ -0,0 +1,26 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Entity\Annotation\DisplayComponent.
   + */
   
   +++ b/core/lib/Drupal/Core/Entity/DisplayComponentHandlerBase.php
   @@ -0,0 +1,64 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Entity\DisplayComponentHandlerBase.
   + */
   
   +++ b/core/lib/Drupal/Core/Entity/DisplayComponentHandlerInterface.php
   @@ -0,0 +1,88 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Entity\DisplayComponentHandlerInterface.
   + */
   
   +++ b/core/lib/Drupal/Core/Entity/DisplayComponentHandlerPluginManager.php
   @@ -0,0 +1,61 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Entity\DisplayComponentHandlerPluginManager.
   + */
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/DisplayComponent/ExtraFieldDisplayComponentHandler.php
   @@ -0,0 +1,59 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Entity\Plugin\DisplayComponent\ExtraFieldDisplayComponentHandler.
   + */
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/DisplayComponent/FieldDisplayComponentHandler.php
   @@ -0,0 +1,203 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Entity\Plugin\DisplayComponent\FieldDisplayComponentHandler.
   + */
   

   Not needed anymore.

2. +++ b/core/lib/Drupal/Core/Entity/DisplayComponentHandlerInterface.php
   @@ -0,0 +1,88 @@
   +  public function massageIn($name, array $options);
   ...
   +  public function massageOut($properties);
   

   Method names don't seem right. Can we change these to something better? PreSave and PostLoad perhaps.

3. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
   @@ -257,7 +257,7 @@ public function buildMultiple(array $entities) {
   -          // Apply the field access cacheability metadata to the render array.
   +
   

   Seems unrelated change.

4. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -326,16 +346,21 @@ public function setComponent($name, array $options = array()) {
   +    // @TODO, is this the right place to ensure label is set?
   

   There is not no label key anymore.

@jibran, not sure if you cross posted with about 5 previous comments, but everything you just said is either done, or mentioned above :)

Heh! discard my comment then. :-)

Here should be a fallback to rendering field with passed in display options instead of entity display

To do this we need to be aware that it is a "field" because it would need to use fieldTypeManager to get the field_type for rendering. We could call it FallbackFieldComponentHander? It would basically be FieldDisplayComponentHandler but using the passed in options rather than the display options?

Yep, exactly!

If it is to be outside of the display, it needs to happen in EntityViewBuilder I think, see how the tests go with this patch.

Minor clean-up and fix for assert

Are you sure label is always required? Because, when I did that previously for the views fields, we end up with it been saved into the display and the schema for that doesn't currently exist?

E.g. try this:

diff --git a/core/modules/views/src/Entity/Render/EntityFieldRenderer.php b/core/modules/views/src/Entity/Render/EntityFieldRenderer.php
index 1b05c1b..353f335 100644
--- a/core/modules/views/src/Entity/Render/EntityFieldRenderer.php
+++ b/core/modules/views/src/Entity/Render/EntityFieldRenderer.php
@@ -232,6 +232,7 @@ protected function buildFields(array $values) {
             $display->setComponent($field->definition['field_name'], [
               'type' => $field->options['type'],
               'settings' => $field->options['settings'],
+              'label' => $field->label(),
             ]);

@andypost which patch needs to be re-rolled? #153 ?

Reroll patch uploaded to patch on #153

heervesh@heervesh-VirtualBox:~/drupal$ wget https://www.drupal.org/files/issues/reroll-1875974-160_0.patch
--2016-12-14 14:53:07-- https://www.drupal.org/files/issues/reroll-1875974-160_0.patch
Resolving www.drupal.org (www.drupal.org)... 151.101.17.175
Connecting to www.drupal.org (www.drupal.org)|151.101.17.175|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 26018 (25K) [text/plain]
Saving to: ‘reroll-1875974-160_0.patch’

reroll-1875974-160_0.patch 100%[=================================================================>] 25.41K 132KB/s in 0.2s

2016-12-14 14:53:08 (132 KB/s) - ‘reroll-1875974-160_0.patch’ saved [26018/26018]

heervesh@heervesh-VirtualBox:~/drupal$ git apply --check reroll-1875974-160_0.patch

Patch applied cleanly.

@Kifah Meeran Nope, not for me...

tameeshb@IdeapadY710 /v/w/h/drupal8.3> 
git apply --check 1875974-153\ \(1\).patch 
error: patch failed: core/core.services.yml:1610
error: core/core.services.yml: patch does not apply

The patch applied cleanly on drupal 8.2.4, which is the current stable version of Drupal.

root@masky-VirtualBox:/home/masky/drupal# git pull
remote: Counting objects: 279, done.
remote: Compressing objects: 100% (139/139), done.
remote: Total 142 (delta 113), reused 0 (delta 0)
Receiving objects: 100% (142/142), 26.19 KiB | 29.00 KiB/s, done.
Resolving deltas: 100% (113/113), completed with 90 local objects.
From http://git.drupal.org/project/drupal
fa8939c..5d0814d 8.2.x -> origin/8.2.x
926f9dc..d2b4642 8.3.x -> origin/8.3.x
Updating fa8939c..5d0814d
Fast-forward
core/lib/Drupal/Core/Render/theme.api.php | 47 +++++++++++++++++++++++------------------------
core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php | 15 +++++++++++++++
core/modules/node/src/Form/NodePreviewForm.php | 2 +-
core/modules/node/src/NodeForm.php | 27 ++++++++++++---------------
core/modules/node/src/Tests/PagePreviewTest.php | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
core/modules/node/tests/modules/node_test/node_test.module | 15 +++++++++++++++
6 files changed, 175 insertions(+), 44 deletions(-)
root@masky-VirtualBox:/home/masky/drupal# git apply --check reroll-1875974-160_0.patch
root@masky-VirtualBox:/home/masky/drupal# git apply reroll-1875974-160_0.patch
root@masky-VirtualBox:/home/masky/drupal#

Yes but it needs a reroll to apply to the development version, which is 8.3.x

@Kifah Meeran the Version: in this issue is 8.3.x

Re-roll.

Rerolled on 8.4.x

Looks like the patch in #173 removed some of the new files. Adding those back in.

One more missing file.

The IS proposes a BC break:

the [get/set]Component(s) methods receive an additional parameter, the 'component type', (plugin id of one of the above plugins) - e.g. 'field' for configurable fields, 'extra_field' for extra fields, 'field_group' for field groups...
This changes all existing setComponent() calls (mostly used in tests). The function could possibly default the 'component type' to 'field', keeping most existing calls working.

Maybe D9 is the opportunity for this.

This is a minor-only addition. Since 8.9.x is now in beta, I'm moving this to 9.1.x. Thanks!

This improvement makes a lot of sense. Is there still interest in getting it into core?

A re-rolled patch against 9.3.x. Please review.

Some coding standards issues in EntityDisplayBase.php: https://www.drupal.org/pift-ci-job/2184750

Also, the change to core.services.yml is incorrectly indented.

Addressed #197.
Fixed the coding standard issues along with core.services.yml is incorrectly indented issue.

Came here to bring this up to date.

The failures in the latest patch are 'Error: Class "Drupal\Core\Entity\DisplayComponentHandlerPluginManager" not found. And yes, that class is not in the patch. Then I noticed that the last three patches are significantly smaller than the one in #180. Since the patch in #180 has that class I rerolled from that.

Adding the suggestion in #155. Also make the settings of $handler consistent.

Let's see what breaks now.

edit: s/$handler/$handler consistent/

Thank you! it looks much better

I'm not sure FALSE is good replacement for NULL, since PHP 8.0 it could use https://www.php.net/manual/en/language.oop5.basic.php#language.oop5.basi...

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
@@ -364,8 +364,7 @@
+    if ($handler = $this->getComponentHandlerByElementName($name)) {
       $options = $handler->massageIn($name, $options);

@@ -603,18 +602,19 @@
       $handlers = $this->handlerManager->getDefinitions();
+      $handler = FALSE;
       foreach (array_keys($handlers) as $type) {
         $handler = $this->getComponentHandler($type);
         if ($handler && $handler->hasElement($name)) {

I think instead of NULL/FALSE manager should provide "missing/broken" handler

Which in turn could hide massageIn() call