Issue #1875974 by swentel, andypost, yched, amateescu aspilicious: Abstract 'component type' specific code out of EntityDisplay.

The EntityDisplay class introduced in #1852966: Rework entity display settings around EntityDisplay config entity includes hardcoded logic to handle "field" and "extra field" components.

Problem:

- It currently only deals with "Field API field" & "extra field" (aka arbitrary render array snippet) components. There is hardcoded logic to determine which is which and how to handle each.
--> Contrib "component types" are currently not possible, and it's very much needed for Display Suite / Field groups.
The entity render flow is based on the fact that a single EntityDisplay object, loaded and altered once as a whole, contains the whole "display recipe" for an entity in a given view mode. Display suite / Field groups need to be able to participate in that flow.

- Now that widgets / formatters are theoretically usable on "base fields", EntityDisplay objects are the natural (and only) place to store widget / formatter configuration on base fields.
They thus need to be uncoupled from the notion of "configurable Field API field" (i.e field_info_*() API calls)

Proposed solution
If we want EntityDisplay objects to hold display options for every component in a entity view, including contrib stuff like field_groups, the EntityDisplay methods should be agnostic about what type of "thing" they contain (field, 'extra field', field_group, possibly blocksNG...), and the "component-type" specific logic needs to be split out into pluggable handler classes.

Thus, EntityDisplay objects become a collection of configured plugins, which is a well known pattern.

  • Component types are plugins.
  • Each component type ("field API field", "extra field", "field_group", ... possibly "block") has a handler class (one object of each is enough for the whole request).
  • The EntityDisplay defers handling of setComponent() / getComponent() methods to the relevant component type handler, and just stores the result (i.e no more massaging code specific to fields or extra fields in EntityDisplay methods).
  • the entity display config entities store the additional 'component type' property in each entry.

API Changes (D8 only - this whole API is new in D8)

  • Introduce "entity display component" plugin type (API addition + is only of interest for very few contrib modules)
  • 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.

Work happens in http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/1875... - helper issue is over at #1848068: Helper issue for "EntityDisplay" - http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/enti... (old branch)

Files: 

Comments

yched’s picture

Issue summary: View changes

fix method names

andypost’s picture

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

swentel’s picture

Issue tags: +Field API

tagging

swentel’s picture

Issue summary: View changes

format

andypost’s picture

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

andypost’s picture

What's left here?

swentel’s picture

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.

swentel’s picture

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

Issue tags: +API change

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.

effulgentsia’s picture

Issue summary: View changes

Updated issue summary.

swentel’s picture

Issue summary: View changes

Updated issue summary.

swentel’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

@swentel & I updated the summary with "reasons for the change" and "API impact"

yched’s picture

Issue summary: View changes

Added reasons for the change.

yched’s picture

Assigned: yched » Unassigned

Unassigning, I'm not working on this right now :-/

effulgentsia’s picture

Issue tags: -API change +API addition

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.

yched’s picture

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.

If this works, then yes, it would keep 95% existing code working (code that accesses EntityDisplay->getComponent($name) for an 'extra field' would still break)

andypost’s picture

..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

yched’s picture

Hmm, I don't remember why my initial patch added a $type to getComponent(). You're right, probably best if we can avoid it.

For field component type probably better to separate base and configurable fields

Quite the contrary, the whole point of "unify base fields & config fields, make widgets / formatters work on both" is that most our code base should make no difference and just work with "fields". A field is a field, where it comes from (code or config) is irrelevant.

amateescu’s picture

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.

amateescu’s picture

Issue summary: View changes

more details about API changes

yched’s picture

Priority: Normal » Major
Issue summary: View changes

Bumping priority. This is needed to be able to generate proper config schemas for EntityDisplay config entities.
Anyone up for grabbing this ? ;-)

aspilicious’s picture

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

aspilicious’s picture

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.

aspilicious’s picture

FileSize
124.32 KB
FAILED: [[SimpleTest]]: [MySQL] 58,999 pass(es), 1 fail(s), and 0 exception(s). View

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.

aspilicious’s picture

Status: Active » Needs review
yched’s picture

Awesome! Thanks a ton @aspilicious!
I'll try to look at the code soon - others are welcome to beat me to it ;-)

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 ?
Not sure if that's related, but #2094499: Convert "Manage (form) display" forms to be the official entity edit forms for EntityView[Form]Display objects would make the "Manage display" forms be the "official entity forms" for the EntityDisplay / EntityFormDisplay config entities. Which might make it easier for 3rd party code to work on the $entity in $form_state before it's saved.

Status: Needs review » Needs work

The last submitted patch, 17: 1875974-17.patch, failed testing.

aspilicious’s picture

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.

yched’s picture

@aspilicious: form submit should start with a fresh, empty EntityDisplay::$content, and then only populate it with stuff that are actually part of the submitted form ?
Would it solve the case you're talking about ?

yched’s picture

A couple remarks after an initial cursory look:

- 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 $type param has a default value of 'field' for all methods, except on getComponents() where it's just '' ?

- More generally, an EntityDisplay object does not accept two components of different "types" with the same name, so:
we do need a $type parameter for setComponent(),
a $type param could be useful for getComponents() (give me all the components of this type),
but the $type param looks useless for other methods (getComponent(), removeComponent(), getRenderer()) ?

aspilicious’s picture

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

yched’s picture

re #24:

- $type param in getComponents():
Ah, right, makes sense of course. But then NULL would be clearer than ''.

- EntityDisplayBase::$pluginManager
Well, EntityDisplay::getRenderer() gets deferred to DisplayComponentHandler::getRenderer(), so it's the DisplayComponentHandler, not the EntityDisplay that knows whether it needs a plugin manager, and which one.
I think what's currently preventing the removal of EntityDisplayBase::$pluginManager is the fact that the patch keeps the current Entity*Form*Display::getRendered(), which does the job itself and still uses $this->pluginManager, instead of deferring to DisplayComponentHandler::getRenderer()
(EntityFormDisplays were added after the initial versions of this patch)

The above means FieldDisplayComponentHandler needs to use either plugin.manager.field.formatter or plugin.manager.field.widget, depending on whether $this->context['display_context'] is 'display' or 'form'.
Then, we can remove the old EntityFormDisplay::getRendered().

- removeComponent() / $type:
yeah, sorry about the lack of interface for and the not super clear API, that's my bad :-/
That's the state the initial patch was in, and I was not too happy with it either (which is why I didn't freeze an interface yet...)
massageIn() is about "massage data that gets written into the EntityDisplay"
massageOut() is about "massage data that is read from the EntityDisplay"

And basically, yes, we only need to explicitly know the type of a component when we write it in (setComponent()).
When we remove a component (removeComponent()), we don't the caller to give us the type, just the component $name, then we already know the type in $this->content[$name]['handler_type']

And, at least for now, what happens on EntityDisplay::removeComponent() depends on the component type. So this currently goes through ComponentHandler::massageIn() (a "remove" is a "write"...)

Again, fully agreed that this can probably be made much cleaner...

I do intend to look into refining as soon as I can, but I really appreciate the help here :-)

-

andypost’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
4.35 KB
119.68 KB
FAILED: [[SimpleTest]]: [MySQL] 59,141 pass(es), 4 fail(s), and 8 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 27: 1875974-entity-components-27.patch, failed testing.

aspilicious’s picture

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?

fago’s picture

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.

andypost’s picture

@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

yched’s picture

re @fago:

+ 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.

AAMOF in my initial iterations of the patch I went with "type" for the component type, and moved the existing 'type' to 'widget/formatter'.
Later rerollers reverted that, and I can totally understand because it makes the patch size and conflict area explode, making rerolls quite painful. So, while I still think the above would be more intuitive in the end, it might be wiser to keep the current for now, and switch to the above either when the patch is otherwise "ready", or in a followup :-/

re @andypost:

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

True, but actually this exact same pattern of "check default settings defined in code if we have nothing configured in the EntityDisplay" is going to extend to regular fields as well when "base fields" kick in with #2144919: Allow widgets and formatters for base fields to be configured in Field UI. Base fields are going to behave the exact same way as extra fields do currently : defined in code, with default display / form settings.

This probably means we should switch the internal storage in EntityDisplays that way:
- content:
like currently, but only contains the *visible* components. Extra fields that are hidden are no longer listed here, and extra fields no longer have a 'visible: true/false' entry, just a weight.
Bonus points for sorting by weight before saving :-)
- hidden:
lists the names of the components (whichever, fields, extra fields...) that have been explicitly configured to be hidden.
That way we can easily differenciate between "hidden" and "no configured settings, switch back to the defaults for this component". This should also simplify the code quite a bit (probably no need for onComponentRemove anymore).

yched’s picture

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?

Been toying with that idea as well - on setComponent('foobar', $array), ask each handler "hey, which of you owns 'foobar' ?". Might be a good idea, especially since #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title puts us in a weird situation where some components can be seen either as a field or as an extra field (see the OP there - this is supposed to be temporary, but not sure how long this "temporary" will last). So this idea might let us account for that "flexibility" better.

Performance should not be too much of a concern if this only happens on setComponent(). Not sure if there would be other pitfalls, but might be worth a try.

andypost’s picture

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

yched’s picture

@andypost : why do you think switching to 'content' / 'hidden' would make things harder ?

do we need a different component handlers for field and configurable field [after #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title] ?

No, one single handler for "base field" & "configurable field", the content of content[field_name] will be the same in both cases, and widgets/formatters do not care.
This single handler might need to have a couple "if (base field) {...}" or "if (configurable field) {...}" code branches, but the entries in 'content' will be structured the same, and the overall logic should be vastly the same - that's the whole point of "unified Field API" :-)

yched’s picture

#2144919: Allow widgets and formatters for base fields to be configured in Field UI is in, this can now resume.

@andypost, @aspilicious : feel like going for it ? :-)

Food for thought: even though we need to have a 'type' for each component ('field', 'extra field', 'field_group'...) we could consider keeping the 'field' handling code hardcoded within EntityDisplay classes rather than in a separate handler plugin, to avoid the overhead on the most frequent case ?

Or we can also consider this is premature optimization for now, get to a working patch, and see then...

aspilicious’s picture

Assigned: Unassigned » aspilicious

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

aspilicious’s picture

Assigned: aspilicious » Unassigned
FileSize
27.03 KB

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 :)

aspilicious’s picture

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

yched’s picture

Thanks a lot @aspilicious ! This is looking nice.

One thing regarding getComponentHandlerByElementName() / "which handler handles which component" :
This should only be computed when a new component gets added in setComponent(), and then be kept as $options_form_the_component['handler'], so that it's known for the rest of the processing, saved in the yaml for each component, and then present again when the display is loaded.
-->
- No need to recompute it each time
- We need that info in the yml to be able to generate config schemas

aspilicious’s picture

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

aspilicious’s picture

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?

yched’s picture

I don't think we should aim at regions atm. That's not anything formalized in core, and it would really complicated config schemas.

Some component types will want to add nesting but that's their business. They'll have a 'children' entry that lists their children, but I don't think we can have the structure of the display->content reflect that, would take us far far away ...

yched’s picture

I saw that saving the default display also triggered a save of teaser. Is that by design?

Hm - definitely not :-) Weird.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
27.54 KB
FAILED: [[SimpleTest]]: [MySQL] 56,324 pass(es), 669 fail(s), and 1,417 exception(s). View

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

aspilicious’s picture

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

Status: Needs review » Needs work

The last submitted patch, 46: 1875974-entity-components-46.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
27.54 KB
FAILED: [[SimpleTest]]: [MySQL] 56,317 pass(es), 670 fail(s), and 1,417 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 49: 1875974-entity-components-49.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
27.5 KB
FAILED: [[SimpleTest]]: [MySQL] 59,706 pass(es), 30 fail(s), and 7,504 exception(s). View

Lets see if this fixes some tests...

Status: Needs review » Needs work

The last submitted patch, 51: 1875974-entity-components-51.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
27.72 KB
FAILED: [[SimpleTest]]: [MySQL] 62,967 pass(es), 22 fail(s), and 453 exception(s). View

Fixed most warnings

twistor’s picture

+++ 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.

Status: Needs review » Needs work

The last submitted patch, 53: 1875974-entity-components-53.patch, failed testing.

aspilicious’s picture

FileSize
27.71 KB
FAILED: [[SimpleTest]]: [MySQL] 62,847 pass(es), 13 fail(s), and 443 exception(s). View

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

aspilicious’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 56: 1875974-entity-components-56.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
6.07 KB
29.57 KB
PASSED: [[SimpleTest]]: [MySQL] 63,615 pass(es). View

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

andypost’s picture

FileSize
9.25 KB
31.72 KB
PASSED: [[SimpleTest]]: [MySQL] 63,690 pass(es). View

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

andypost’s picture

FileSize
31.74 KB
FAILED: [[SimpleTest]]: [MySQL] 64,181 pass(es), 2 fail(s), and 1 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 61: 1875974-entity-components-61.patch, failed testing.

swentel’s picture

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

yched’s picture

Right. This is (finally...) #1 in my list of things to review.

yched’s picture

@andypost : did you push your latest reroll in a sandbox somewhere ?

andypost’s picture

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

swentel’s picture

Issue summary: View changes
swentel’s picture

Status: Needs work » Needs review
FileSize
75.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,840 pass(es), 1,917 fail(s), and 1,370 exception(s). View

Let's see how hard this breaks

Status: Needs review » Needs work

The last submitted patch, 68: 1875974-68.patch, failed testing.

swentel’s picture

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

aspilicious’s picture

FileSize
61.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,485 pass(es), 1,652 fail(s), and 844 exception(s). View
aspilicious’s picture

Status: Needs work » Needs review

Let's see...

aspilicious’s picture

And I'll fix the funky phpunit file stuff tomorrow

Status: Needs review » Needs work

The last submitted patch, 71: 1875974-71.patch, failed testing.

swentel’s picture

Issue summary: View changes
swentel’s picture

Status: Needs work » Needs review
FileSize
26.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,494 pass(es), 1,656 fail(s), and 844 exception(s). View

Clean file now without the accidental php unit removals

Status: Needs review » Needs work

The last submitted patch, 76: 1875974-75.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
26.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,629 pass(es). View

It's green lets review!

andypost’s picture

  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

aspilicious’s picture

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.

aspilicious’s picture

FileSize
27.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,762 pass(es). View
7.38 KB

Cleanup + reroll

andypost’s picture

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

Stalski’s picture

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

aspilicious’s picture

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.

yched’s picture

This will also need to take #2271419: Allow field types, widgets, formatters to specify config dependencies (almost RTBC) into account - see item 2. in comment #37 over there)

In short, patch over there adds EntityWithPluginCollectionInterface to EntityDisplayBase for config dependency handling,
and currently implements the corresponding getPluginCollections() method in EVD / EFD by hardcoding the list of widgets / formatters.

Assembling that list will need to be delegated to the component handlers.

Also, well, since the generic tool for "config dependencies for config entities holding a list of plugins" (EntityWithPluginCollectionInterface) relies on the notion of PluginCollections (new name for PluginBags), it definitely raises the question of internally using PluginCollections to store our lists of widgets / formatters.

aspilicious’s picture

FileSize
31.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,552 pass(es), 7 fail(s), and 0 exception(s). View
5.51 KB

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...

Status: Needs review » Needs work

The last submitted patch, 86: 1875974-86.patch, failed testing.

aspilicious’s picture

I'm going to reroll 81

aspilicious’s picture

Status: Needs work » Needs review
FileSize
27.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,181 pass(es), 3,654 fail(s), and 10,090 exception(s). View

Lets see...

ignore the first lines of the patch ;)

Status: Needs review » Needs work

The last submitted patch, 89: 1875974-89.patch, failed testing.

aspilicious’s picture

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

aspilicious’s picture

Status: Needs work » Needs review
FileSize
27.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1875974-92.patch. Unable to apply patch. See the log in the details link for more information. View

New try

Status: Needs review » Needs work

The last submitted patch, 92: 1875974-92.patch, failed testing.

aspilicious’s picture

FileSize
27.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,240 pass(es), 216 fail(s), and 36 exception(s). View

Ok... apparantly something changed in the last hour

aspilicious’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 94: 1875974-94.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
28.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Small fix

Status: Needs review » Needs work

The last submitted patch, 97: 1875974-97.patch, failed testing.

Status: Needs work » Needs review

aspilicious queued 97: 1875974-97.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 97: 1875974-97.patch, failed testing.

aspilicious’s picture

FileSize
28.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1875974-101.patch. Unable to apply patch. See the log in the details link for more information. View
aspilicious’s picture

Status: Needs work » Needs review
andypost’s picture

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

ivanjaros’s picture

So...any plans for this yet?

andypost’s picture

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

mlevasseur queued 101: 1875974-101.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 101: 1875974-101.patch, failed testing.

mlevasseur’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
28.02 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 82,005 pass(es), 4,430 fail(s), and 1,376 exception(s). View

Rerolled.

No changes.

Status: Needs review » Needs work

The last submitted patch, 108: 1875974-102.patch, failed testing.

Status: Needs work » Needs review

jibran queued 108: 1875974-102.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 108: 1875974-102.patch, failed testing.

andypost’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed

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

swentel’s picture

Status: Postponed » Active
Issue tags: -Needs beta evaluation

I guess we can try again for 8.1.x

andypost’s picture

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

andypost’s picture

Status: Active » Needs work
Issue tags: +Needs reroll
nabiyllin’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
25.17 KB

rerolled path from comment #81

Status: Needs review » Needs work

The last submitted patch, 118: 1875974-103.patch, failed testing.

nabiyllin’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 118: 1875974-103.patch, failed testing.

nabiyllin’s picture

Status: Needs work » Needs review
FileSize
684.69 KB

fix not clean merge

Status: Needs review » Needs work

The last submitted patch, 122: 1875974-104.patch, failed testing.

nabiyllin’s picture

Status: Needs work » Needs review
FileSize
25.05 KB

Status: Needs review » Needs work

The last submitted patch, 124: 1875974-105.patch, failed testing.

nabiyllin’s picture

Status: Needs work » Needs review
FileSize
24.71 KB

fix not clean merge

Status: Needs review » Needs work

The last submitted patch, 126: 1875974-106.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
24.63 KB
1.27 KB

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

Status: Needs review » Needs work

The last submitted patch, 128: 1875974-128.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
25.43 KB
1.36 KB

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.

benjy’s picture

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

The last submitted patch, 130: 1875974-130.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 131: 1875974-131.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
25.54 KB
2.82 KB

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.

Status: Needs review » Needs work

The last submitted patch, 134: 1875974-133.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
25.09 KB
1.27 KB

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.

andypost’s picture

@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?

Status: Needs review » Needs work

The last submitted patch, 136: 1875974-136.patch, failed testing.

andypost’s picture

+++ 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

benjy’s picture

Status: Needs work » Needs review
FileSize
26.22 KB
699 bytes

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.

benjy’s picture

Addressed 137.

Status: Needs review » Needs work

The last submitted patch, 141: 1875974-141.patch, failed testing.

andypost’s picture

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

andypost’s picture

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

benjy’s picture

Status: Needs work » Needs review

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.

benjy’s picture

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.

jibran’s picture

Issue tags: +Needs tests

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.

benjy’s picture

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

jibran’s picture

Heh! discard my comment then. :-)

benjy’s picture

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?

andypost’s picture

Yep, exactly!

benjy’s picture

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.

andypost’s picture

Minor clean-up and fix for assert

Status: Needs review » Needs work

The last submitted patch, 153: 1875974-153.patch, failed testing.

benjy’s picture

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(),
             ]);

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 152: 1875974-152.patch, failed testing.

andypost’s picture

Issue tags: +Needs reroll
tameeshb’s picture

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

tameeshb’s picture

Assigned: Unassigned » tameeshb
tameeshb’s picture

Reroll patch uploaded to patch on #153

tameeshb’s picture

Status: Needs work » Needs review
tameeshb’s picture

Assigned: tameeshb » Unassigned

Status: Needs review » Needs work

The last submitted patch, 161: reroll-1875974-160.patch, failed testing.

Heervesh’s picture

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

Kifah Meeran’s picture

Patch applied cleanly.

Kifah Meeran’s picture

Issue tags: -Needs reroll
tameeshb’s picture

@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
tameeshb’s picture

Issue tags: +Needs reroll
Kifah Meeran’s picture

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#

tim.plunkett’s picture

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

tameeshb’s picture

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

Jo Fitzgerald’s picture

Jo Fitzgerald’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 173: reroll-1875974-173.patch, failed testing.

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

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

rlmumford’s picture

Status: Needs work » Needs review
FileSize
9.56 KB

Rerolled on 8.4.x

Status: Needs review » Needs work

The last submitted patch, 177: 1875974-177.patch, failed testing.

rlmumford’s picture

Status: Needs work » Needs review
FileSize
24.03 KB

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

rlmumford’s picture

FileSize
25.34 KB

One more missing file.

The last submitted patch, 179: 1875974-179.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 180: 1875974-181.patch, failed testing.

pk188’s picture

Status: Needs work » Needs review
FileSize
8.14 KB

Status: Needs review » Needs work

The last submitted patch, 183: 1875974-183.patch, failed testing.