Part of #1949932: [META] Unify entity fields and field API and followup to #1950632-42: Create a FieldDefinitionInterface and use it for formatters and widgets:

In both cases (formatters / widgets), the rendering / form building flow for entities need to be reworked so that formatters / widgets are invoked on "base fields" as well. Right now, they are only invoked for "configurable fields", within field_attach_[prepare_view|view]() / field_attach[form|extract_form_values|form_validate]() respectively.

This issue is the start of that.

Patch summary
Rendering pipeline:

  • Patch changes field_invoke_method*() so that they iterate on all fields (base + configurable) of a entity, instead of just configurable fields so far. Those functions, along with field_attach_form*() / field_attach_view*(), are up for removal / replacement by OO versions in #2095195: Remove deprecated field_attach_form_*(), but this patch here keeps them in field.module for now.
  • As a result, widgets/formatters get invoked on any (base or config) field for which the EntityDisplay used for rendering contains the configuration of a widget/formatter. Free support for "In place editing" ensues.
  • For now, Field UI "Manage display" screens still only list configurable fields, so EntityDisplay objects cannot be saved with widgets/formatters for base fields, thus all of them, except node title, are still rendered with their own custom code, as in HEAD. #2144919: Allow widgets and formatters for base fields to be configured in Field UI has been opened to allow Field UI integration.
  • Patch moves the drupal_alter() step from the callers of entity_get_render_display() / entity_get_render_form_display() into the functions themselves, because per below, now that we have a use case of an alter implementation, we need to ensure that it always runs.

For node title:

  • Patch removes the custom $form['title'] element
  • Uses hook_entity_display_alter() to runtime-alter the EntityDisplay before rendering a node, adding hardcoded widget/formatter display options for the 'title' component
  • Updates all WebTests posting node forms accordingly ($edit['title'] -> $edit['title[0][value]'])
  • Since node title is now themed as a field, the patch adds a theme_field__node__title() to override theme_field()'s generic wrappers with a lighter-weight <span> wrapper. Prior to this patch, the node title was output (in node listings) with no wrapper of its own (only the <h2> and <a> output by node.html.twig), so this results in a new <span> tag where there wasn't one before, but that's needed to hold the attributes required for in-place editing to work.
CommentFileSizeAuthor
#208 core-js-edit-funkyselector-1988612-208.patch777 bytesnod_
#201 interdiff.txt906 byteseffulgentsia
#201 formatters_widgets-base_fields-1988612-201.patch112.07 KBeffulgentsia
#199 interdiff.txt3.5 KBeffulgentsia
#199 formatters_widgets-base_fields-1988612-199.patch111.7 KBeffulgentsia
#194 interdiff.txt7.56 KByched
#194 formatters_widgets-base_fields-2133669-194.patch111.74 KByched
#193 interdiff.txt752 bytesyched
#193 formatters_widgets-base_fields-1988612-192.patch112.07 KByched
#190 formatters_widgets-base_fields-1988612-190.patch112.06 KByched
#186 interdiff-174-186.txt3.44 KBeffulgentsia
#186 formatters_widgets-base_fields-1988612-186.patch111.54 KBeffulgentsia
#183 interdiff.txt1013 byteseffulgentsia
#183 formatters_widgets-base_fields-1988612-183.patch110.95 KBeffulgentsia
#181 interdiff.txt4.05 KBeffulgentsia
#180 formatters_widgets-base_fields-1988612-180.patch110.77 KBeffulgentsia
#179 formatters_widgets-base_fields-1988612-179.patch111.27 KBeffulgentsia
#174 formatters_widgets-base_fields-1988612-174.patch112.16 KByched
#172 interdiff.txt21.64 KByched
#172 formatters_widgets-base_fields-1988612-172.patch112.81 KByched
#167 interdiff.txt3.92 KBeffulgentsia
#167 formatters_widgets-base_fields-1988612-167.patch100.12 KBeffulgentsia
#165 interdiff.txt1.61 KBeffulgentsia
#165 formatters_widgets-base_fields-1988612-165.patch97.44 KBeffulgentsia
#164 interdiff.txt1.58 KBeffulgentsia
#164 formatters_widgets-base_fields-1988612-164.patch97.95 KBeffulgentsia
#163 interdiff.txt814 byteseffulgentsia
#163 formatters_widgets-base_fields-1988612-163.patch97.71 KBeffulgentsia
#161 interdiff.txt658 byteseffulgentsia
#161 formatters_widgets-base_fields-1988612-161.patch97.14 KBeffulgentsia
#159 formatters_widgets-base_fields-1988612-159.patch97.07 KBeffulgentsia
#156 formatters_widgets-base_fields-1988612-156.patch96.54 KBeffulgentsia
#156 interdiff.txt2.98 KBeffulgentsia
#154 formatters_widgets-base_fields-1988612-153.patch95.75 KBeffulgentsia
#154 interdiff.txt890 byteseffulgentsia
#151 formatters_widgets-base_fields-1988612-151.patch95.76 KBeffulgentsia
#151 interdiff.txt464 byteseffulgentsia
#149 formatters_widgets-base_fields-1988612-149.patch95.77 KBeffulgentsia
#147 formatters_widgets-base_fields-1988612-147.patch95.78 KBeffulgentsia
#147 interdiff.txt32.69 KBeffulgentsia
#145 formatters_widgets-base_fields-1988612-145.patch81.79 KBeffulgentsia
#145 interdiff.txt6.77 KBeffulgentsia
#144 formatters_widgets-base_fields-1988612-144.patch79.56 KBeffulgentsia
#143 formatters_widgets-base_fields-1988612-143.patch79.8 KBeffulgentsia
#143 interdiff.txt597 byteseffulgentsia
#141 formatters_widgets-base_fields-1988612-141.patch79.81 KBeffulgentsia
#141 interdiff.txt3.28 KBeffulgentsia
#140 formatters_widgets-base_fields-1988612-140.patch82.06 KBeffulgentsia
#140 interdiff.txt1.42 KBeffulgentsia
#136 formatters_widgets-base_fields-1988612-136.patch81.1 KBeffulgentsia
#136 interdiff.txt617 byteseffulgentsia
#134 formatters_widgets-base_fields-1988612-134.patch80.36 KBeffulgentsia
#134 interdiff.txt5.31 KBeffulgentsia
#131 formatters_widgets-base_fields-1988612-131.patch81.94 KBeffulgentsia
#131 interdiff.txt11.87 KBeffulgentsia
#128 formatters_widgets-base_fields-1988612-128.patch70.01 KBeffulgentsia
#128 interdiff.txt15.37 KBeffulgentsia
#126 formatters_widgets-base_fields-1988612-126.patch54.21 KBeffulgentsia
#126 interdiff.txt24.29 KBeffulgentsia
#123 formatters_widgets-base_fields-1988612-123.patch33.92 KBeffulgentsia
#123 interdiff.txt2.62 KBeffulgentsia
#121 formatters_widgets-base_fields-1988612-121.patch33.01 KBeffulgentsia
#121 interdiff.txt6.86 KBeffulgentsia
#119 formatters_widgets-base_fields-1988612-119.patch27.16 KBeffulgentsia
#119 interdiff.txt781 byteseffulgentsia
#115 formatters_widgets-base_fields-1988612-115.patch27.05 KBeffulgentsia
#112 formatters_widgets-base_fields-1988612-112.patch27.53 KBBerdir
#112 formatters_widgets-base_fields-1988612-112-interdiff.txt860 bytesBerdir
#110 formatters_widgets-base_fields-1988612-110.patch27.54 KBBerdir
#110 formatters_widgets-base_fields-1988612-110-interdiff.txt6.82 KBBerdir
#108 formatters_widgets-base_fields-1988612-108.patch27.33 KBBerdir
#108 formatters_widgets-base_fields-1988612-108-interdiff.txt1.99 KBBerdir
#105 formatters_widgets-base_fields-1988612-105.patch27.08 KBBerdir
#97 formatters_widgets-base_fields-1988612-97.patch32.88 KBeffulgentsia
#95 formatters_widgets-base_fields-1988612-95.patch34.46 KBeffulgentsia
#95 interdiff.txt1.85 KBeffulgentsia
#92 formatters_widgets-base_fields-1988612-92.patch33.56 KBeffulgentsia
#92 interdiff.txt2.04 KBeffulgentsia
#90 formatters_widgets-base_fields-1988612-90.patch32.79 KBeffulgentsia
#86 formatters_widgets-base_fields-1988612-86.patch32.81 KBeffulgentsia
#86 interdiff.txt701 byteseffulgentsia
#84 formatters_widgets-base_fields-1988612-84.patch32.79 KBeffulgentsia
#84 interdiff.txt590 byteseffulgentsia
#83 formatters_widgets-base_fields-1988612-83.patch32.74 KBeffulgentsia
#83 interdiff.txt592 byteseffulgentsia
#82 formatters_widgets-base_fields-1988612-82.patch32.54 KBeffulgentsia
#82 interdiff.txt4.76 KBeffulgentsia
#81 formatters_widgets-base_fields-1988612-81.patch37.14 KBeffulgentsia
#79 formatters_widgets-base_fields-1988612-79.patch37.83 KBeffulgentsia
#79 interdiff.txt942 byteseffulgentsia
#77 formatters_widgets-base_fields-1988612-77.patch36.73 KBeffulgentsia
#74 formatters_widgets-base_fields-1988612-74.patch36.74 KBeffulgentsia
#72 formatters_widgets-base_fields-1988612-71.patch36.73 KBeffulgentsia
#72 interdiff.txt488 byteseffulgentsia
#70 formatters_widgets-base_fields-1988612-70.patch37.35 KBeffulgentsia
#70 interdiff.txt6.95 KBeffulgentsia
#67 formatters_widgets-base_fields-1988612-67.patch32.89 KBeffulgentsia
#67 interdiff.txt2.54 KBeffulgentsia
#64 formatters_widgets-base_fields-1988612-64.patch31.28 KBeffulgentsia
#64 interdiff.txt657 byteseffulgentsia
#62 formatters_widgets-base_fields-1988612-62.patch31.25 KBeffulgentsia
#62 interdiff.txt1.37 KBeffulgentsia
#58 formatters_widgets-base_fields-1988612-58.patch31.24 KBeffulgentsia
#58 interdiff.txt3.86 KBeffulgentsia
#56 formatters_widgets-base_fields-1988612-56.patch31.03 KBeffulgentsia
#56 interdiff.txt4.47 KBeffulgentsia
#53 formatters_widgets-base_fields-1988612-53.patch32.56 KBWim Leers
#47 formatters_widgets-base_fields-1988612-47.patch32.13 KBWim Leers
#47 interdiff.txt4.9 KBWim Leers
#46 formatters_widgets-base_fields-1988612-45.patch30.82 KBWim Leers
#44 formatters_widgets-base_fields-1988612-44.patch30.9 KBWim Leers
#41 formatters_widgets-base_fields-1988612-41.patch54.29 KBeffulgentsia
#38 formatters_widgets-base_fields-1988612-38.patch53.87 KBPancho
#38 interdiff-37-38.do-not-test.diff733 bytesPancho
#37 formatters_widgets-base_fields-1988612-37.patch53.88 KBPancho
#35 formatters_widgets-base_fields-1988612-35.patch53.88 KBeffulgentsia
#35 interdiff.txt4.4 KBeffulgentsia
#34 formatters_widgets-base_fields-1988612-34.patch49.41 KBWim Leers
#34 interdiff.txt991 bytesWim Leers
#33 formatters_widgets-base_fields-1988612-33.patch48.62 KBeffulgentsia
#31 formatters_widgets-base_fields-1988612-31.patch48.2 KBeffulgentsia
#29 formatters_widgets-base_fields-1988612-29.patch28.54 KBeffulgentsia
#28 formatters_widgets-base_fields-1988612-28.patch33.73 KBeffulgentsia
#26 formatters_widgets-base_fields-1988612-26.patch25.69 KBeffulgentsia
#24 formatters_widgets-base_fields-1988612-24.patch21.72 KBeffulgentsia
#22 formatters_widgets-base_fields-1988612-22.patch20.36 KBeffulgentsia
#20 formatters_widgets-base_fields-1988612-20.patch20.35 KBeffulgentsia
#17 formatters_widgets-base_fields-1988612-17.patch19.23 KBeffulgentsia
#15 formatters_widgets-base_fields-1988612-15.patch6.64 KBeffulgentsia
#1 node_created_formatter.patch5.21 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Status: Active » Needs work
FileSize
5.21 KB

This patch is rebased on top of #1950632-44: Create a FieldDefinitionInterface and use it for formatters and widgets. It's still quite ugly, but demonstrates the beginnings of integrating formatters with base fields.

effulgentsia’s picture

Note that one of the intentional features of this is that being rendered with a formatter automatically means going through theme('field'), which is one (but not the only) necessary part of integrating with Edit module.

effulgentsia’s picture

+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.php
@@ -257,6 +257,11 @@ public function baseFieldDefinitions() {
+      'field_definition' => array(
+        'type' => 'number_integer',

Obviously, we'd rather set this to 'datetime'. But that requires other changes to how node.created is stored / registered with EntityNG.

Also, the extra 'field_definition' parent key should hopefully be made unnecessary once #1969728: Implement Field API "field types" as TypedData Plugins is solved, since that's all about unifying the concepts of data type and field type.

Wim Leers’s picture

Note that one of the intentional features of this is that being rendered with a formatter automatically means going through theme('field'), which is one (but not the only) necessary part of integrating with Edit module.

That's not the entire truth.

The point is that edit module needs *some* way of setting a data- attribute on each in-place editable field. Currently that happens via theme_field(), but it doesn't really matter. As long as it happens. Ideally of course, nonconfigurable and configurable fields are treated the same, can be handled the same way via a single API.

+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.phpundefined
+++ b/core/modules/node/lib/Drupal/node/NodeStorageController.phpundefined
@@ -257,6 +257,11 @@ public function baseFieldDefinitions() {

@@ -257,6 +257,11 @@ public function baseFieldDefinitions() {
       'label' => t('Created'),
       'description' => t('The time that the node was created.'),
       'type' => 'integer_field',
+      'field_definition' => array(
+        'type' => 'number_integer',
+        'settings' => array(),
+        'module' => 'number',

It feels strange that non-storage metadata is being added to the entity's storage controller, no? But maybe that's what you're pointing out with #3, that this is non-ideal and will go away once #1969728: Implement Field API "field types" as TypedData Plugins lands?

yched’s picture

@@ -257,6 +257,11 @@ public function baseFieldDefinitions() {
       'label' => t('Created'),
       'description' => t('The time that the node was created.'),
       'type' => 'integer_field',
+      'field_definition' => array(
+        'type' => 'number_integer',
+        'settings' => array(),
+        'module' => 'number',

It feels strange that non-storage metadata is being added to the entity's storage controller, no? But maybe that's what you're pointing out with #3, that this is non-ideal and will go away once #1969728: Implement Field API "field types" as TypedData Plugins lands?

I take this as part of the "proof of concept", just as way quick way to get a "field definition" from a base field for now.
I'm not completely sure how #1969728: Implement Field API "field types" as TypedData Plugins will be able to unify that. In the scenario over there, a "field type" is mostly a class extending FieldItem. In order to build such an object, you need an actual entity, meaning an actual bundle.
And #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets shows that we need to generate "field definitions" in situations when we have no bundle (no $instance).
For example, #1969728: Implement Field API "field types" as TypedData Plugins turns hook_field_schema($field) into a *static* schema($field) method on the ConfigurableFieldItem class.

We'll need to generate it from base field definitions too, and, right, the "how" is not fully decided yet.

Wim Leers’s picture

Title: Apply formatters to rendered entity base fields, starting with node.created » Apply formatters and widgets to rendered entity base fields, starting with node.created
Status: Needs work » Postponed

Expanding scope. This issue should get rid of custom form API code for the node date entirely.

Currently blocked on #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets.

Wim Leers’s picture

Issue tags: +sprint
Wim Leers’s picture

effulgentsia’s picture

Discussed this at DrupalCon with yched and swentel. The approach we came up with is to make EntityDisplay responsible for then entire entity_view() render array. An important step in that direction is being done in #1875974: Abstract 'component type' specific code out of EntityDisplay. Once that and #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets are both in, I'll reroll a patch for this issue that uses that.

Wim Leers’s picture

Status: Postponed » Active

#1950632: Create a FieldDefinitionInterface and use it for formatters and widgets got committed, We can now start working on this! I should be able to work on this in the next few days, unless somebody beats me to it.

yched’s picture

Now that #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets is in, the other part of "make widgets and formatters work on all fields" would be to have them receive EntityNG Field objects, instead of currently plain $items arrays, in their form(), extractFormValues(), prepareView(), view()... methods.

Once #1969728: Implement Field API "field types" as TypedData Plugins is done, widgets and formatters are going to be the only thing that work with the old style arrays.
Aside from API consistency, avoiding that conversion / format change would possibly be a performance win - especially since :
- all those methods are called through a single set of helpers : field_invoke_method() / field_invoke_method_multiple() (they currently work only on Field API fields, that would be changed by making the EntityDisplay responsible for rendering)
- some of those methods (Widget::extractFormValues() & Formatter::prepareView()) do some writing to the $items, and thus the invoke helpers currently do the job of assigning the array values back into the FieldItem objects - for *all* methods, since they don't carry any knowledge of which methods write to the values or not.

It shouldn't be a very complex task in itself, but doing it on all formatters and widgets affects a lot of code...
It should be possible to be smart and temporarily allow both a BC and NG version of each method, with the invoke helpers being smart and calling the right one based on a method_exists('[method]BC') check, thus allowing the "official API" to be ready for code freeze, and leaving the deprecated versions still working and converted one by one. That approach is probably needed anyway in order for a patch to be just workable without having to convert everything as a first step.

Now the awkward part - I'm going afk in two days now and won't be around to work on this before code freeze :-/.
I don't want to sound like slacking off and assigning work, but ... @wim, do you think you could help on that ? ;-)

Wim Leers’s picture

@yched Okay, so it seems that #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets alone was not sufficient to allow this issue to be applied? Or you're saying that it is technically possible, but it'll be hackish/complex, and once we do what you say in #11, it'll become trivial and clean?
In any case: yes, I can definitely help with that! If possible, it'd be great if you could convert one part of the code, then along with the description in #11, I should be able to drive it home. Just having read #11, I have *some* idea of what needs to happen, but I just don't know Field API well enough to say that a light bulb went on and I know exactly what needs to happen. As with #1950632, I can probably get myself up to speed, but that takes considerably more time than if there's an example to start from :)
I presume/hope that effulgentsia already knows *exactly* what you mean, in which case I could also just talk to him to get this done.
You're in the best position to judge if #11 is enough to get me started :)

yched’s picture

@Wim:
It should still be technically possible (formatters and widgets currently work, even though they accept field values in a format that's different than the one in which EntityNG holds them), and so #11 should not delay the work on "make EntityDisplay responsible for the entire entity_view() render array and #1875974: Abstract 'component type' specific code out of EntityDisplay" that @eff mentions in #9.

But this dance between "old (array) format for field values handed to widgets / formatters" and "new (FieldItem objects) native format for field values" is very far from ideal performance wise, and it possibly only works right now because there's an EntityBC layer that we switch the $entity to before calling the field_invoke_method() / field_invoke_method_multiple() helpers that bridge to the widgets & formatters. But that BC layer is intended to go away at some point...

Thanks a lot for accepting to have a try at this! I'll try to whip up an initial patch for directions tomorrow.

yched’s picture

@Wim (and / or anyone willing to help ;-)
I opened #2021817: Make widgets / formatters work on EntityNG Field value objects. with an initial patch and additional explanations.

effulgentsia’s picture

Title: Apply formatters and widgets to rendered entity base fields, starting with node.created » Apply formatters and widgets to rendered entity base fields, starting with node.title
Status: Active » Needs review
FileSize
6.64 KB

A work in progress, but wanting a bot check. Also, decided to switch to node.title instead of node.created as the first use case. So far, this only does the formatter part. Still need to do the widget part.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-15.patch, failed testing.

effulgentsia’s picture

There's some stuff here that will need a bit of cleanup, but let's see if bot likes it more.

effulgentsia’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-17.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
20.35 KB

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-20.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
20.36 KB

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-22.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
21.72 KB

This will hopefully be green. Next step, widgets. Also, this contains a bunch of code that just adds integration between #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets and #1969728: Implement Field API "field types" as TypedData Plugins, since those issues were done in parallel. I may split that out into a separate issue to unclutter it from this one.

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/FieldDataTypeDerivative.phpundefined
@@ -43,6 +43,16 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
+      // The distinction between 'settings' and 'instance_settings' is only
+      // meaningful at the field type plugin level. At the Typed data API level,
+      // merge them.
+      $definition['settings'] = $definition['instance_settings'] + $definition['settings'];
+      unset($definition['instance_settings']);

really interesting... inheritance?

Would be needed for comment-field form settings

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigField.phpundefined
@@ -37,9 +44,14 @@ public function __construct(array $definition, $name = NULL, TypedDataInterface
+        $this->instances = FieldAPI::fieldInfo()->getBundleInstances($parent->entityType(), $parent->bundle());

+++ b/core/modules/node/node.moduleundefined
@@ -743,7 +743,7 @@ function template_preprocess_node(&$variables) {
-  $variables['label'] = check_plain($node->label());
+  $variables['label'] = Drupal::service('plugin.manager.field.formatter')->viewBaseField($node->getNGEntity()->title);

+++ b/core/modules/text/lib/Drupal/text/Plugin/field/field_type/TextItem.phpundefined
@@ -63,6 +63,27 @@ public static function schema(Field $field) {
+    $constraint_manager = \Drupal::typedData()->getValidationConstraintManager();
...
+      $constraints[] = $constraint_manager->create('ComplexData', array(

is not it needs injection?

effulgentsia’s picture

This implements the widget side. All the BC decoration is quite ugly. Looking forward to when that's removed. I still need to respond to #25, but first, let's see what bot thinks of this.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-26.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
33.73 KB

Ugh. So many of our tests hardcode assumptions about input element names. Would be nice to centralize to some helper functions that in the future could be changed in one place. Not doing that yet though. Meanwhile, this fixes some of them.

effulgentsia’s picture

#28 went in the wrong direction. Instead of fixing every test, we can instead make a custom widget that conforms to test expectations. That's what this patch does.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-29.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
48.2 KB

This includes the start of making in-place editing work on node title.

@Wim Leers: When I'm on the home page viewing node teasers, I can go into "Quick edit" mode, and see via Firebug Net tab that metadata is being sent back for both title and body, but there's only an outline around body, and not on title. Can you debug that?

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-31.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
48.62 KB

Fixed the 3 test failures. Still need to get in-place editing working, fill in the missing docs, update the issue summary, and respond to #25. More reviews also welcome at this point for anyone willing to do so without an updated issue summary.

Wim Leers’s picture

Fixed #31.

Bugs

Found while testing:

  1. saving the title does not work (the original title remains), nor does rerendering the title (empty string is returned in the AJAX response as the "data").
  2. this does not yet make it work on the full node page (e.g. node/1), where the node title is explicitly not rendered as part of the node, but lifted into the page title. Fixing that could be a follow-up though.
  3. the langcode set on the title is en, but it should be und, like for the other entity fields.

Review

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityFieldAccessCheck.phpundefined
@@ -41,7 +41,7 @@ public function access(Route $route, Request $request) {
-    return $entity->access('update') && ($field = field_info_field($field_name)) && field_access('edit', $field, $entity->entityType(), $entity);
+    return $entity->access('update'); // && ($field = field_info_field($field_name)) && field_access('edit', $field, $entity->entityType(), $entity);

How do you plan to fix this? I guess we need some way of detecting if a field is nonconfigurable, and if so, solely the entity access check should be performed.

Do you plan to make this part of FieldDefinition?

+++ b/core/modules/field/field.moduleundefined
@@ -173,6 +173,9 @@ function field_theme() {
+    'field__title' => array(
+      'base hook' => 'field',

Yay!

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.phpundefined
@@ -134,7 +134,7 @@ protected function checkFieldAccess($op, $entity) {
-      return FALSE;
+      return TRUE;
+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetBase.phpundefined
@@ -449,7 +449,7 @@ protected function checkFieldAccess($op, $entity) {
-      return FALSE;
+      return TRUE;

Is this temporary? If not, then I don't understand it.

effulgentsia’s picture

#34.1 fixed and now in-place editing of node titles (when viewing teasers, see #34.2) works!!

Re the access check removals in #34, yes, that still needs fixing. Just wanted to see things working first.

Wim Leers’s picture

Yay :D

Pancho’s picture

Straight reroll of #35 which didn't apply anymore after #1754246: Languages should be configuration entities landed.

Pancho’s picture

Straight reroll is not quite enough after #1754246: Languages should be configuration entities. Small fix should do it though.
Confirm that in-place editing works again.

Still no widget settings in "Manage Form Display", and from "Manage Fields" and "Manage Display", the Title field is still completely removed (wasn't it at least listed some days ago?). Will be next step probably.

Pancho’s picture

Status: Needs review » Needs work

Nice that this part works again.
However, overall this still needs work.

fago’s picture

+++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
@@ -0,0 +1,107 @@
+  /**
+   * @todo Document.
+   */
+  public function __construct(FieldInterface $field) {
+    $this->field = $field;
+    $this->definition = $field->getDefinition() + \Drupal::typedData()->getDefinition($field->getType());

hm, this really makes field definition a totally different beast than the data definition. That's a bit weird as right now it's the same except for entity field definitions supporting some additional keys. So why not just mimic that and make a DataDefinition class + a FieldDefinition class that extends it? Then the FieldDefinition class could directly implement the FieldDefinitionInterface...

+++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
@@ -23,7 +24,13 @@ public function build(array $form, array &$form_state, EntityInterface $entity,
+    $field = $entity->getNGEntity()->get($field_name);
+    if ($field->getFieldDefinition() instanceof FieldInstanceInterface) {
+      field_attach_form($form_state['entity'], $form, $form_state, $form_state['langcode'], array('field_name' =>  $form_state['field_name']));
+    }
+    else {
+      $form[$field_name] = \Drupal::service('plugin.manager.field.widget')->baseFieldForm($field, $form, $form_state, $form_state['langcode']);

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.php
@@ -176,4 +177,48 @@ public function getDefaultSettings($type) {
+  public function baseFieldForm(FieldInterface $field, array &$form, array &$form_state, $langcode) {

Couldn't that be just the same flow? e.g. go with a regular field-invoke-multiple? Probably that needs more refactoring though, so maybe that's better topic of a follow-up.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
54.29 KB

Just a reroll.

effulgentsia’s picture

Thanks for all the reviews so far. This issue is going to take longer than the next 12 or so hours (when API freeze kicks in) to complete. So I spun off the API changes being introduced here into #2032393: Limit APIs to FieldDefinitionInterface when FieldInstance isn't needed which I think is straightforward enough to be able to land quickly. Please review that. Then, I believe the rest of the work here is eligible for commit post API feeze.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-41.patch, failed testing.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review
FileSize
30.9 KB

Rerolling #41 now that #2032393: Limit APIs to FieldDefinitionInterface when FieldInstance isn't needed landed. Quite the nontrivial reroll! This is a straight reroll of #41, minus the changes committed as part of #2032393: Limit APIs to FieldDefinitionInterface when FieldInstance isn't needed, plus rebased on latest HEAD.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-44.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
30.82 KB

#44 was the wrong patch. Sorry.

Wim Leers’s picture

Various things were broken in #44/#46 when you actually tried to use it. In this reroll, I got everything to work again: the node form, the node view (front page and full node view), and in-place editing of the title. Various things were completely broken.

I also fixed Edit's access checking to only do entity access checking for nonconfigurable fields/base fields/properties (whatever the right name might be right now, the current code base uses both!) and do both entity and field access checking for configurable fields.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-47.patch, failed testing.

Wim Leers’s picture

Assigned: Wim Leers » effulgentsia

#47 has the same failures as #41, as expected, but with a much smaller patch because #2032393: Limit APIs to FieldDefinitionInterface when FieldInstance isn't needed got committed. I hope that effulgentsia can take it from here.

fago’s picture

hm, this really makes field definition a totally different beast than the data definition. That's a bit weird as right now it's the same except for entity field definitions supporting some additional keys. So why not just mimic that and make a DataDefinition class + a FieldDefinition class that extends it? Then the FieldDefinition class could directly implement the FieldDefinitionInterface...

I opened #2047229: Make use of classes for entity field and data definitions for that - please provide your thoughts about it there.

yched’s picture

webchick’s picture

Priority: Normal » Major

As far as Edit module is concerned, this is major. It blocks being able to edit node titles, authors, and other non-primary fields.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
32.56 KB

Here is a straight reroll of #47, but things are utterly broken again. Not yet able to even use the node/add form…

Wim Leers’s picture

Seems a pretty big change was introduced in #2021817: Make widgets / formatters work on EntityNG Field value objects., it was even spun off from this issue, specifically from #11, but never reported back here. So the patch on that issue got committed :)

This is also indirectly the cause for the node form no longer working.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-53.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
4.47 KB
31.03 KB

Some interim progress.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-56.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
3.86 KB
31.24 KB

Node form works now. And data attribute is output for the title, but I can't select it when using Quick Edit.

jessebeach’s picture

Quick editing a node title with the patch in #58 worked for me. Here's proof: http://www.youtube.com/watch?v=Jbxozsn4eto&feature=youtube_gdata_player

yched’s picture

Thanks @effulgentsia.

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/FieldItemDeriver.php
@@ -77,6 +77,16 @@ public function getDerivativeDefinitions(array $base_plugin_definition)
+      // The distinction between 'settings' and 'instance_settings' is only
+      // meaningful at the field type plugin level. At the Typed data API level,
+      // merge them.
+      $definition['settings'] = $definition['instance_settings'] + $definition['settings'];
+      unset($definition['instance_settings']);

Hm, interesting / puzzling. Why is this needed ?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigField.php
   public function getInstance() {
-    if (!isset($this->instance) && $parent = $this->getParent()) {
-      $instances = FieldAPI::fieldInfo()->getBundleInstances($parent->entityType(), $parent->bundle());
-      $this->instance = $instances[$this->getName()];
+    if (!isset($this->instance)) {
+      if (!isset($this->instances) && $parent = $this->getParent()) {
+        $this->instances = FieldAPI::fieldInfo()->getBundleInstances($parent->entityType(), $parent->bundle());
+      }
+      $field_name = $this->getName();
+      if (isset($this->instances[$field_name])) {
+        $this->instance = $this->instances[$field_name];
+      }
     }
     return $this->instance;

Why do we need to keep a copy of $this->instances within the FieldItem object ? FieldInfo::getBundleInstances() is statically cached already (and refreshed when needed), so duplicating the info sounds more like trouble with stale stuff for no real gain ?

On a side note, I'd really want to get rid of getInstance(). It prevents using "configurable field types" on base fields, which de facto means two separate sets of field types for base fields and configurable fields, which is wrong (shameless plug to #2052135: Determine how to deal with field types used in base fields in core entities).
Opened #2066507: remove ConfigField/ConfigFieldItem::getInstance() for this.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterPluginManager.php
+  public function viewBaseField(FieldInterface $field)

Yeah, so, this does worry me :-). Am I correct in understanding that this is the equivalent of field_view_field() for base fields ?
The widget / formatter plugin managers seem like a weird place for this.

field_view_field() / field_view_value() suck anyway - I've been considering replacing / deprecating those with a unified view() method on FieldInterface / FieldItemInterface(). Would seem like a better fit ?

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-58.patch, failed testing.

effulgentsia’s picture

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

Just addressing test failures, not #60 yet.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-62.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
657 bytes
31.28 KB

Chasing HEAD.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-64.patch, failed testing.

Wim Leers’s picture

Great progress effulgentsia! :)


The remaining failures in \Drupal\node\Tests\NodeValidationTest are legitimate. Node title validation doesn't work correctly anymore: a too long title no longer triggers a violation, nor does a non-existing title. So either the Node::set('title', 'something') API for setting the title no longer works, or retrieving the new title is what fails.

I started debugging this, but given I have zero knowledge about \Symfony\Component\Validator\Validator, my time is probably better spent elsewhere. Hope this helps a tiny bit.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.54 KB
32.89 KB

This fixes NodeValidationTest. Hopefully doesn't introduce some other bug.

tim.plunkett’s picture

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeValidationTest.php
@@ -55,7 +55,7 @@ public function testValidation() {
+    $this->assertEqual($violations[0]->getMessage(), t('<em class="placeholder">Title</em>: the text may not be longer than 255 characters.', array('%limit' => 255)));

This should be '%name: the text may not be longer than @max characters.' with appropriate replacements, otherwise it will get picked up for translation as a new string.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-67.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
6.95 KB
37.35 KB

Re #68, this patch just removes the t() from tests, since that shouldn't be there in the first place. The rest of this interdiff ain't pretty, and I'll move it into its own issue about us having various problems related to the way we integrate validation of required fields. Here for now to see if it makes bot happy.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-70.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
488 bytes
36.73 KB

Removing a stray hunk that was only there to assist with testing.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-71.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
36.74 KB

Straight reroll.

yched’s picture

Ouch, not pretty indeed.

I *think* changing FieldInstanceEditForm::getFieldItem() this way would work:

protected function getFieldItem(EntityInterface $entity, $field_name) {
  // In all cases, EntityNG or not:
  $this->instance->required = FALSE;
  $this->instance->description = '';
  $definition = _field_generate_entity_field_definition($this->instance->getField(), $this->instance);
  return \Drupal::typedData()->create($definition, array(), $field_name, $entity)->offsetGet(0);
}

That results in injecting a modified $instance in the FieldItem as its fieldDefinition. This is a trick that was needed during field purge to be able to construct an $items object using a specific injected $instance. I hope at some point it will be easier to inject a FieldDefinitionInterface object into $items / $item objects, but right now it's all we have...

(then the code that currently does this job in FieldInstanceEditForm::getDefaultValueWidget() can be removed)

[edit: sorry, scratch comment about t(), didn't see you fixed it]

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-74.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
36.73 KB

Straight reroll.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-77.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
942 bytes
37.83 KB

Trying to outdo the ugliness from #74 with even more ugliness. Ok, so this should be green, but now I'm on to creating a separate issue to deal with all this mess related to validating required fields.

effulgentsia’s picture

effulgentsia’s picture

Straight reroll.

effulgentsia’s picture

Reverting the stuff that I spun off to #2070429: Configurable fields aren't validated against the "required" constraint except in forms. I'm going to follow up with a different approach for this issue. That other one still needs to be fixed on its own merits, but I don't want this one held up on it.

effulgentsia’s picture

effulgentsia’s picture

And, the much simpler and more correct solution to #66. I feel pretty silly now for going down the rabbit hole of the last 15 comments.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-84.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
701 bytes
32.81 KB

Sigh. Missed a hunk that should have been part of #82.

effulgentsia’s picture

Ok, so I think the next steps here are:
- get #2047229: Make use of classes for entity field and data definitions in, which will remove some of what's in this patch with equivalent code that's been more thoroughly reviewed
- get #1994140: Unify entity field access and Field API access in, which will remove some of the hacks related to access checks
- address #60
- see what else is left here that's questionable

I think those can all be done in parallel. Keeping this assigned to me; I'll post more when there's an update.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Field API, +sprint, +Spark, +Entity Field API

The last submitted patch, formatters_widgets-base_fields-1988612-86.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
32.79 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-90.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
33.56 KB

NodeFormController now has a buildEntity() implementation in HEAD, so this merges the one added by the patch into it.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-92.patch, failed testing.

Berdir’s picture

Drupal\Core\TypedData\TypedDataManager->createInstance('filter_format', Array, 'format', Object)
Drupal\Core\TypedData\TypedDataManager->create(Array, NULL, 'format', Object)
Drupal\Core\TypedData\TypedDataManager->getPropertyInstance(Object, 'format', NULL)
Drupal\Core\TypedData\Plugin\DataType\Map->get('format')
Drupal\text\TextProcessed->setContext('processed', Object)
Drupal\Core\TypedData\TypedDataManager->getPropertyInstance(Object, 'processed')
Drupal\Core\Entity\Field\FieldItemBase->__construct(Array, 0, Object)
Drupal\Core\TypedData\TypedDataManager->createInstance('field_item:text', Array, 0, Object)
Drupal\Core\TypedData\TypedDataManager->create(Array, NULL, 0, Object)
Drupal\Core\TypedData\TypedDataManager->getPropertyInstance(Object, 0, NULL)
Drupal\Core\TypedData\ItemList->createItem(0)
Drupal\Core\Entity\Field\Field->__construct(Array, 'title', Object)
Drupal\Core\TypedData\TypedDataManager->createInstance('field_item:text', Array, 'title', Object)
Drupal\Core\TypedData\TypedDataManager->create(Array, NULL, 'title', Object)
Drupal\Core\TypedData\TypedDataManager->getPropertyInstance(Object, 'title', NULL)
Drupal\Core\Entity\EntityNG->getTranslatedField('title', 'und')
Drupal\Core\Entity\EntityNG->get('title')
Drupal\Core\Entity\EntityNG->getProperties()
Drupal\Core\Entity\EntityNG->getIterator()
Drupal\Core\Entity\DatabaseStorageControllerNG->create(Array)
Drupal\node\NodeStorageController->create(Array)
entity_create('node', Array)
Drupal\node\Tests\Condition\NodeConditionTest->testConditions()
Drupal\simpletest\TestBase->run()
simpletest_script_run_one_test('107', 'Drupal\node\Tests\Condition\NodeConditionTest')

That's because filter_format is now a thing provided by filter.module so that needs to be enabled in those DUBT tests.

However, this also indicates that the title is now a string field with a format. And that seems wrong?

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.85 KB
34.46 KB

That's because filter_format is now a thing provided by filter.module so that needs to be enabled in those DUBT tests.

Thanks. That explains 2 of the 3 failures. Since filter.info.yml says it's a required module, enabling it in those tests seems legitimate.

The 3rd failure is due to Symfony's ChoiceValidator::validate() throwing an exception if $constraint->choices is an empty array *before* checking for $value being null. That makes no sense. If null is allowed, then $constraint->choices being empty should not be an error. Rather than fixing Symfony, this patch just fixes it in the AllowedValuesConstraintValidator subclass.

However, this also indicates that the title is now a string field with a format. And that seems wrong?

Because text.module doesn't provide a field type that's just 'value' without 'format'. Maybe it should? But, since format is allowed to be NULL when the 'text_processing' setting is FALSE, and since filter.module is a required module, I don't think it's a problem for $node->title to support a NULL $node->title->format. If we ever decide to have text.module provide a value-only field type, then we can switch $node->title to use it.

jibran’s picture

Status: Needs review » Needs work

Yay!! it is green. Let's fix all the @todo's.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
32.88 KB

Reroll, which also removes the offending code of #60.2. Next steps are to get #1994140: Unify entity field access and Field API access in and address #60.1 and #60.3.

effulgentsia’s picture

Status: Needs review » Needs work

Regardless what bot says, this still needs work per #97.

effulgentsia’s picture

Status: Needs work » Needs review
yched’s picture

Does #75 still apply ? (avoid "required" validation errors on the widget in the "default value" UI)

effulgentsia’s picture

Does #75 still apply ?

Decoupled from this issue. See #82 and #84.

yched’s picture

@effulgentsia: OK, makes sense.
Do you want eyes on the current patch, or is it still in your yard for now ?

effulgentsia’s picture

Status: Needs review » Needs work

I think it makes sense to at least have some variant of #2047229-17: Make use of classes for entity field and data definitions committed, and this patch rerolled without it, before further eyes on the rest of the patch are useful. #1994140: Unify entity field access and Field API access would also be nice, but not as necessary, though I wonder if that might be the quickest way to get the first one in.

yched’s picture

Berdir’s picture

Status: Needs work » Needs review
FileSize
27.08 KB

Here's a re-roll. That was fun, really nice to see how HEAD improved since Sept 5th :)

Let's see how well this works, might have removed a few things that I shouldn't.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-105.patch, failed testing.

yched’s picture

#2095195: Remove deprecated field_attach_form_*() contains an initial version of how the integration of widgets / formatters in forms / entity_views would work after #2019055: Switch from field-level language fallback to entity-level language fallback gets in. Still only works on configurable fields, but In the end, that same code should work on all fields (base, config) that are known by the EntityFormDisplay.
I'll be afk for the next two weeks, so I posted what I had over there.

Berdir’s picture

Improving viewBaseField().

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-108.patch, failed testing.

Berdir’s picture

Fixed editing too.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-110.patch, failed testing.

Berdir’s picture

Too much interface. Something about validation seems to be broken for real, though.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-112.patch, failed testing.

Berdir’s picture

  1. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -350,14 +350,13 @@ public static function baseFieldDefinitions($entity_type) {
           'description' => t('The title of this node, always treated as non-markup plain text.'),
    -      'type' => 'string_field',
    +      'type' => 'field_item:text',
    +      'list_class' => '\Drupal\Core\Entity\Field\FieldItemList',
    

    I still think this change is wrong.

    field_item:text means we also get a format in the structe and more importantly, it instantiates a TextProcessed instance for the computed process property that we'll never use. We already have a custom widget for the title, I'd rather add a formatter for string_field too. Also saves us the list_class definition.

  2. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -350,14 +350,13 @@ public static function baseFieldDefinitions($entity_type) {
    -      'property_constraints' => array(
    -        'value' => array('Length' => array('max' => 255)),
    -      ),
    +      'default_widget' => 'node_title',
    

    I guess this is causing the test fail there. Wrong merge or was that explicitly removed?

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
27.05 KB

In theory, this is just a straight reroll, but I did need to manually fix some conflicts. Let's start by seeing if bot finds anything horribly broken.

Status: Needs review » Needs work
Issue tags: -Field API, -sprint, -Spark, -Entity Field API

The last submitted patch, formatters_widgets-base_fields-1988612-115.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Field API, +sprint, +Spark, +Entity Field API

The last submitted patch, formatters_widgets-base_fields-1988612-115.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
781 bytes
27.16 KB

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-119.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
6.86 KB
33.01 KB

Let's see if bot likes this more. If so, I'll split these unit test changes into a separate issue.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-121.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
33.92 KB

Just getting bot to pass again. Still need to address #60.3 and #114.1.

Berdir’s picture

We might want to wait with #114 until #2050835: Move Widget, Formatter, and FieldType plugin types to the Core\Field system is in, I'd be fine with doing that in a follow-up issue if this would be ready otherwise.

effulgentsia’s picture

+++ b/core/modules/edit/tests/Drupal/edit/Tests/Access/EditEntityFieldAccessCheckTest.php
@@ -149,6 +138,12 @@ public function testAccess(EntityInterface $entity, FieldInterface $field = NULL
+    $entity_with_field->expects($this->any())
+      ->method('getPropertyDefinition')
+      ->with('example')
+      ->will($this->returnValue(array(
+        'field_name' => 'example',
+      )));

Having to write mock code like this in unit tests (#121) made me feel icky, so I opened #2112759: Add ContentEntityInterface::hasField().

effulgentsia’s picture

We already have a custom widget for the title

This removes that.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-126.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
15.37 KB
70.01 KB

A whole bunch more appending of [0][value]. Wow, we have a lot of tests that submit node forms.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-128.patch, failed testing.

Berdir’s picture

  1. +++ b/core/modules/edit/lib/Drupal/edit/EditController.php
    @@ -133,12 +134,15 @@ public function metadata(Request $request) {
    +      if (!($field_definition = $entity->getTranslation($langcode)->get($field_name)->getFieldDefinition())) {
    +        throw new NotFoundHttpException();
    +      }
    

    The check doesn't seem to make much sense? If there's a field then there has to be a field definition. So if the field doesn't exist, this should throw an exception on get() and never get to the next method call?

    Sounds like another use case for hasField() ?

  2. +++ b/core/modules/edit/lib/Drupal/edit/EditController.php
    @@ -216,7 +220,14 @@ public function fieldForm(EntityInterface $entity, $field_name, $langcode, $view
    +      $field = $entity->get($field_name);
    +      if ($field->getFieldDefinition() instanceof FieldInstanceInterface) {
    +        $output = field_view_field($entity, $field_name, $view_mode_id, $langcode);
    +      }
    +      else {
    +        $output = \Drupal::service('plugin.manager.field.formatter')->viewBaseField($field);
    +      }
    

    What about moving this logic into the entity render controller/view builder and add a viewField() in there? Then we can hide the configurable/base logic from the outside, as long as it still exist and can refactor it within there?

  3. +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
    @@ -87,7 +88,13 @@ public function buildForm(array $form, array &$form_state, EntityInterface $enti
    -    field_attach_form($form_state['entity'], $form, $form_state, $form_state['langcode'], array('field_name' =>  $form_state['field_name']));
    +    $field = $entity->get($field_name);
    +    if ($field->getFieldDefinition() instanceof FieldInstanceInterface) {
    +      field_attach_form($form_state['entity'], $form, $form_state, $form_state['langcode'], array('field_name' =>  $form_state['field_name']));
    +    }
    +    else {
    +      $form[$field_name] = \Drupal::service('plugin.manager.field.widget')->baseFieldForm($field, $form, $form_state, $form_state['langcode']);
    +    }
    

    This one I guess is more problematic, as we don't really have an easy way to do this.

effulgentsia’s picture

Still hunting down all the places that need [0][value] appended. Will address #130 after we're green again.

effulgentsia’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-131.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
5.31 KB
80.36 KB

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-134.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
617 bytes
81.1 KB

This fixes one of the failing tests, but the one that's left is real: we need to somehow allow base fields to have per-bundle definitions (similar to instances for configurable fields, but not that exactly), since node types have a configurable title label. I remember fago starting on that in Prague, but I don't know the current status of that.

Berdir’s picture

Yes, we need that, there's a @todo for it in #2047229: Make use of classes for entity field and data definitions, no issue yet for it, we should open one. The conclusions of the prague discussion are in https://docs.google.com/document/d/1A5BLd-KmrLnJW88SdLX5HG3Xm7AyinxwXNAG..., with example code to implement it, we just need to get the other issue in first, that one is big enough on it's own I think.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-136.patch, failed testing.

xjm’s picture

Issue tags: +Prague Hard Problems
effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.42 KB
82.06 KB

Re #137, ok, I opened #2114707: Allow per-bundle overrides of field definitions and added a hack here with a @todo linking to it.

I still need to address #130 and other feedback, but let's see if we're back to green now.

effulgentsia’s picture

A few easy cleanups, including #130.1. Not using hasField() though until #2112759: Add ContentEntityInterface::hasField() lands.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-141.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
597 bytes
79.8 KB

Oops.

effulgentsia’s picture

Straight reroll for HEAD changes. No actual changes.

effulgentsia’s picture

#2112759: Add ContentEntityInterface::hasField() landed, so this incorporates that and simplifies the unit test.

effulgentsia’s picture

Status: Needs review » Needs work

I'm in progress on the other feedback that's been raised in earlier comments.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
32.69 KB
95.78 KB

Alrighty. I think this incorporates all the remaining feedback (#60.3 and #130.2 and .3).

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-147.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
95.77 KB

And rerolled for HEAD.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-149.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
464 bytes
95.76 KB

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-151.patch, failed testing.

Berdir’s picture

Sounds like you have some old namespaces, search for "\Core\Entity\Field\" and replace with "\Core\Field\" :)

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
890 bytes
95.75 KB

Status: Needs review » Needs work

The last submitted patch, formatters_widgets-base_fields-1988612-153.patch, failed testing.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
FileSize
2.98 KB
96.54 KB

This one should be green. I'm unassigning myself pending reviews. I do still need to fill in the docs where there's a @todo for docs, and I need to file followup issues for the other @todos, but other than that, is there anything else that's needed?

fago’s picture

wow, great to see this working. Looking through the patch it seems reasonable first step, it would be good to get an overview of what it does already and what the follow-ups left are (yaml!).

fago’s picture

Issue summary: View changes

shortened issue summary

Wim Leers’s picture

Berdir, fago, any chance you could review this?

effulgentsia’s picture

Reroll.

Status: Needs review » Needs work

The last submitted patch, 159: formatters_widgets-base_fields-1988612-159.patch, failed testing.

effulgentsia’s picture

FileSize
97.14 KB
658 bytes

Status: Needs review » Needs work

The last submitted patch, 161: formatters_widgets-base_fields-1988612-161.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
97.71 KB
814 bytes

This should be green again.

effulgentsia’s picture

This moves theme_field__title() from field.module to theme_field__node__title() in node.module, and documents it, resolving one @todo.

effulgentsia’s picture

Now that #2019055: Switch from field-level language fallback to entity-level language fallback is in, this resolves one language related todo, and links the other one to the appropriate followup.

yched’s picture

Reviewing - sorry, I'm behind on a lot of reviews, slowly catching up...

As a side note:
- Now that #2019055: Switch from field-level language fallback to entity-level language fallback is in, we can move forward on #2095195: Remove deprecated field_attach_form_*(). The goal is to replace field_attach_view() / field_attach_form(), that currently iterate only on configurable fields, with iterator methods on entity view / form controllers, that can iterate on all (base & configurable) fields of an entity indifferently. It should simplify a lot of the code in this patch, that currently has to go through hoops to execute widgets/formatters on base fields ?

- #2090509: Optimize entity_get_render_display() for the case of "multiple entity view" moves entity_get_render_display() to a public method in EntityViewBuilder, that includes the hook_entity_display_alter invocation. This should let us revert some of the changes here.

effulgentsia’s picture

This resolves another todo. I think that's the last one that's in scope for this issue. The rest will be follow ups. I'll update the issue summary and add links to those follow ups shortly.

effulgentsia’s picture

Re #166, thanks for those links, but please tell me we don't need to hold this issue up on them :)

yched’s picture

@effulgentsia: Definitely no need to hold this up on #2090509: Optimize entity_get_render_display() for the case of "multiple entity view", that's a detail.
#2095195: Remove deprecated field_attach_form_*() is open for debate ;-)

I have no real opinion yet, I'm still in the middle of the review. For now, I was just pointing that this other issue should settle the "final" "clean" "D8 style" version of the "iterate widgets / formatters on Entity Fields" code, and is thus closely related.

I guess it's a matter of how intrusive the patch is, and to what extent the changes made here to have this work on top of the "old" code spill over a lot of different places that will be hard to revert. Looks like it should be ok - and sure, I'd very much favor getting this in first and polishing in that other issue. At least that's the angle of my review :-)

yched’s picture

Partial review so far, sorry, posting what I have.

  1. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
    @@ -20,6 +20,9 @@ class AllowedValuesConstraintValidator extends ChoiceValidator {
    +    if (!isset($value)) {
    +      return;
    +    }
    

    Looks like a sign something else is going wrong somewhere ?

  2. +++ b/core/modules/edit/js/editors/directEditor.js
    @@ -21,7 +21,13 @@ Drupal.edit.editors.direct = Drupal.edit.EditorView.extend({
    +    var $textElement;
    +    if (this.$el.is(':has(.field-item)')) {
    +      $textElement = this.$textElement = this.$el.find('.field-item:first');
    +    }
    +    else {
    +     $textElement = this.$textElement = this.$el;
    +    }
    

    I'll leave it to @Wim to vet this ;-)

  3. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -194,9 +205,9 @@ public function getComponent($name) {
    -          return array(
    -            'weight' => $this->content[$name]['weight'],
    -          );
    +          $options = $this->content[$name];
    +          unset($options['visible']);
    +          return $options;
    

    Why this change ?

  4. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -290,4 +301,18 @@ public function getHighestWeight() {
    +    if (($this->targetEntity instanceof ContentEntityInterface) && $this->targetEntity->hasField($field_name)) {
    +      $field_definition = $this->targetEntity->get($field_name)->getFieldDefinition();
    +      return $field_definition;
    +    }
    

    This means the method can still return NULL, which would result in EntityDisplay::getRenderer() calling Widget/FormatterPluginManager->getInstance(array('field_definition' => NULL)), which will break ?
    So it looks like EntityDisplay::getRenderer() should check if ($field_definition = $this->getFieldDefinition()) before calling pluginManager->getInstance() ?

  5. +++ b/core/modules/field/field.attach.inc
    @@ -294,12 +301,24 @@ function _field_invoke_get_instances($entity_type, $bundle, $options) {
    +  elseif (isset($options['display'])) {
    +    // @todo Replace with \Drupal::entityManager()->getFieldDefinitions() after
    +    //   #2047229: Make use of classes for entity field and data definitions lands.
    +    $entity = _field_create_entity_from_ids((object) array('entity_type' => $entity_type, 'bundle' => $bundle, 'entity_id' => NULL));
    +    $field_definitions = array();
    +    foreach ($options['display']->getComponents() as $name => $component_options) {
    +      if ($entity->hasField($name)) {
    +        $field_definitions[] = $entity->get($name)->getFieldDefinition();
    +      }
    +    }
    

    That new $options['display'] param for field_invoke_method() is not documented - but I'm not too fond of the idea.
    The logic about "only iterate on fields that are actually visible in a given EntityDisplay" should already be taken care of by the $target_function closure passed to field_invoke_method().
    The role of $target_function($field_definition) is "for this field, give me the object (e.g the widget plugin) on which to call the method, or NULL if none"
    The functions used as $target_function, like _field_invoke_widget_target(), use $display->getRenderer(), which takes care of "NULL if the field is hidden".
    So there should be no need of making field_invoke_method() aware of the notion of "EntityDisplay", this logic is abstracted and injected in the $target_function parameter already ?

Wim Leers’s picture

#170.2: That is correct, I made that change :)

yched’s picture

Regarding my last remark in #170 - $options['display'] in field_invoke_method():

With the various Field API cleanups and refactors that went in the past couple weeks, we can now simplify field_invoke_method*() a lot, and get rid of most "options": run on deleted fields, run on one single field specified by UUID, and most of all get rid of the old multilingual logic - the functions are now only ever called in a context where we run on one single language, which has already been resolved upstream in the received $entity.

It makes sense to do this as part of this issue : the old field_language*() logic is outdated after #2019055: Switch from field-level language fallback to entity-level language fallback, the functions are now mere BC layers that reproduce the old behavior for configurable fields only. This issue makes field_invoke_method() run on all fields, so it shouldn't rely on the config-only language logic anymore - especially since it's useless and slow as hell.

So I explored this a bit, and skimmed the functions down to what is strictly needed now. I removed the $options['display'] param as well - as explained in #170.5, the logic about "only invoke the method on fields that are visible in the EntityDisplay" is already taken care of by the $target_function closures.

This works pretty well and simplifies code a lot, but uncovered some additional fails.Turns out, with the previous patches, ContentEntityFormController::validate() was actually not passing $options['display'] when calling field_invoke_method('flagErrors'), so the function was running in its "old" mode (only on configuration fields), and validation errors were not reported for base fields.
With this new patch, validation errors are reported for every field (base or config) present in the FormDisplay, causing issues with validation errors on empty 'title':

- The NotNull violation is at the ItemList level, i.e not on a specific Item/delta, and the code in WidgetBase::flagErrors() choked with notices.
--> patch fixes that by flagging those errors with no $delta to $element instead of $element[$delta]

- Once this gets fixed, an "empty title" error is reported twice: first as a pure Form API '#required' fail, then by EntityValidation and the NotNull constraint
--> patch fixes that as was discussed in #2070429: Configurable fields aren't validated against the "required" constraint except in forms, by having WidgetBase::flagErrors() *not* report errors found on a field if the form already has errors on that same field. As mentioned in that issue, it would be best to not run validation at all on a field if there were Form API-level errors on it, but IMO that improvement can be left for that other issue to tackle.

- node/Tests/PagePreviewTest was drunk: It tries to preview and submit changes to a node iteratively, but the actual submits, with a partial $edit without a 'title', were being made to node/add instead of node/[nid]/edit. The submissions are actually failing in current HEAD, but nothing checks that.

yched’s picture

yched’s picture

Just a reroll for now.

Status: Needs review » Needs work

The last submitted patch, 174: formatters_widgets-base_fields-1988612-174.patch, failed testing.

yched’s picture

Working on this.

Reading through the patch, I stumbled on MetadataGeneratorInterface::generateField(), and posted suggestions at #2144879: Brush up MetadataGeneratorInterface::generate(Field|Entity)(): use FieldItemListInterface + better naming :-)

yched’s picture

This is very close to being RTBCable IMO.

- I opened #2144919: Allow widgets and formatters for base fields to be configured in Field UI for the next step (allow actual configurability instead of the - smart - hack done here for node title)
- I'm still exploring a couple adjustments in a helper issue.

Meanwhile, a couple questions for @effulgentsia:

  1. +++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
    @@ -139,18 +139,24 @@ public function getInstance(array $options) {
    +    // If the field can be displayed (has a formatter), fill in the defaults
    +    // needed to display it.
    +    if (isset($configuration['type'])) {
    +      $configuration += array(
    +        'label' => 'above',
    +        'settings' => array(),
    +      );
    +      $configuration['settings'] += $this->getDefaultSettings($configuration['type']);
         }
    -    // Fill in default settings values for the formatter.
    -    $configuration['settings'] += $this->getDefaultSettings($configuration['type']);
    

    The changes in FormatterPluginManager/WidgetPluginManager::prepareConfiguration() do not seem to be able to rescue anything: if we're not able to come up with a 'type' of widget/formatter, then the rest of the code will fail anyway ?

  2. +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    @@ -2273,7 +2273,7 @@ protected function assertNoLinkByHref($href, $message = '', $group = 'Other') {
    -    $urls = $this->xpath('//a[normalize-space(text())=:label]', array(':label' => $label));
    +    $urls = $this->xpath('//a[normalize-space()=:label]', array(':label' => $label));
    

    Why this ?

effulgentsia’s picture

Re #177.1:
The issue is "extra fields" in entity displays that are the same name as a base field of a type for which there is no formatter/widget. For example, taxonomy term name. That overlap should go away with #2144919: Allow widgets and formatters for base fields to be configured in Field UI, but what to do in the meantime? What "fails anyway"?

Re #177.2:
Because theme_field__node__title() outputs a span wrapper in order to house the attributes needed by edit.module.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
111.27 KB

Straight reroll to remove the change to an Overlay test now that Overlay module is gone. #2047229: Make use of classes for entity field and data definitions landed, so I'd like to resolve the @todos referencing that, which I'll work on now, unless yched tells me not to :)

effulgentsia’s picture

Resolved those todos, and made sure all the remaining ones link to a follow up issue.

effulgentsia’s picture

FileSize
4.05 KB

Interdiff between #179 and #180.

The last submitted patch, 179: formatters_widgets-base_fields-1988612-179.patch, failed testing.

effulgentsia’s picture

The last submitted patch, 183: formatters_widgets-base_fields-1988612-183.patch, failed testing.

yched’s picture

Regarding the @todos on #2047229: Make use of classes for entity field and data definitions :
I'm not sure we can remove them yet, EntityManager->getFieldDefinitions($entity_type, $bundle) currently contains the $fields, not the $instances :-/. That's for #2114707: Allow per-bundle overrides of field definitions to solve, for now $items->getFieldDefinition() is the only way to get the $instance.

Actually, in my local patch, I kept the @todos but moved the link to 2114707. Sorry, I should have posted that here, but I'm still working on fails on other parts :-/

effulgentsia’s picture

Assigned: Unassigned » yched
FileSize
111.54 KB
3.44 KB

I see. Ok, here's a reversion of that, and a complete interdiff from #174. Looking forward to your next patch once you resolve the stuff you're working on :)

The last submitted patch, 186: formatters_widgets-base_fields-1988612-186.patch, failed testing.

The last submitted patch, 180: formatters_widgets-base_fields-1988612-180.patch, failed testing.

The last submitted patch, 180: formatters_widgets-base_fields-1988612-180.patch, failed testing.

yched’s picture

Ok, sorry for the confusion.
Here's a patch built on top of @eff's latest #186, with a couple trivial changes:
- add a missing typehint
- update @todo issue URL
- fix a recently added test involving a node title in a form

Still experimenting a couple possible cleanups in a local branch, not included here for now.

The last submitted patch, 180: formatters_widgets-base_fields-1988612-180.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 190: formatters_widgets-base_fields-1988612-190.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
112.07 KB
752 bytes

Should fix the fail ?

yched’s picture

OK, I think I'm done.

- Attached patch clarifies the handling for "fields" and "extra fields" in EntityDisplayBase, trying to keep a "you're one or the other but not both" line, which we very much need for #1875974: Abstract 'component type' specific code out of EntityDisplay.
- This also lets us revert the changes in Widget/FormatterPluginManager (we don't try to run Widget/FormatterPluginManager::prepareConfiguration() on "base field"-style display options)

This is ready to fly as far as I'm concerned - but obviously I can't push the RTBC button myself now...

yched’s picture

Issue summary: View changes

Added a patch summary to the OP

yched’s picture

Issue summary: View changes
yched’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
@@ -359,13 +359,13 @@ public static function baseFieldDefinitions($entity_type) {
     $properties['title'] = array(
       'label' => t('Title'),
       'description' => t('The title of this node, always treated as non-markup plain text.'),
-      'type' => 'string_field',
+      'type' => 'field_item:text',
+      'list_class' => '\Drupal\node\NodeTitleItemList',
       'required' => TRUE,
       'settings' => array(
         'default_value' => '',
-      ),
-      'property_constraints' => array(
-        'value' => array('Length' => array('max' => 255)),
+        'max_length' => 255,
+        'text_processing' => 0,
       ),
       'translatable' => TRUE,

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -101,18 +101,6 @@ public function form(array $form, array &$form_state) {
-    $node_type = node_type_load($node->getType());
-    if ($node_type->has_title) {
-      $form['title'] = array(
...
-        '#title' => check_plain($node_type->title_label),
-        '#required' => TRUE,
-        '#default_value' => $node->title->value,
-        '#maxlength' => 255,
-        '#weight' => -5,
-      );
-    }

+++ b/core/modules/node/node.module
@@ -191,14 +195,42 @@ function node_entity_bundle_info() {
 function node_entity_display_alter(EntityDisplay $display, $context) {
-  // Hide field labels in search index.
-  if ($context['entity_type'] == 'node' && $context['view_mode'] == 'search_index') {
-    foreach ($display->getComponents() as $name => $options) {
-      if (isset($options['label'])) {
-        $options['label'] = 'hidden';
-        $display->setComponent($name, $options);
+  if ($context['entity_type'] == 'node') {
+    // Hide field labels in search index.
+    if ($context['view_mode'] == 'search_index') {
+      foreach ($display->getComponents() as $name => $options) {
+        if (isset($options['label'])) {
+          $options['label'] = 'hidden';
+          $display->setComponent($name, $options);
+        }
       }
     }
+    // @todo Manage base field displays in the YAML:
+    //   https://drupal.org/node/2144919.
+    $display->setComponent('title', array(
+      'label' => 'hidden',
+      'type' => 'text_default',
+    ));
+  }
+}
+
+/**
+ * Implements hook_entity_form_display_alter().
+ */
+function node_entity_form_display_alter(EntityFormDisplay $form_display, $context) {
+  if ($context['entity_type'] == 'node') {
+    // @todo Manage base field displays in the YAML:
+    //   https://drupal.org/node/2144919.
+    $node_type = node_type_load($context['bundle']);
+    if ($node_type->has_title) {
+      // Title is also registered in node_field_extra_fields().
+      $options = $form_display->getComponent('title') ?: array('weight' => -5);
+      $options['type'] = 'text_textfield';
+      $form_display->setComponent('title', $options);
+    }
+    else {
+      $form_display->removeComponent('title');
+    }
   }
 }

These are the "choice quotes" from the patch. Basically instead of defining it as a string field and providing our own form / display, we provide it as a field.

Discussed this with @yched and @fago in detail in Vienna. Basically the main problem is we treat title as both an extra field (so it shows up on the list of fields to configure) and as a field on the entity display that we need to alter. This is not very nice, but best we can do with current tools.

#2114707: Allow per-bundle overrides of field definitions and #2144919: Allow widgets and formatters for base fields to be configured in Field UI as well as other issues mentioned in the patch will go a long way to clean this up. Given that this is an infrastructure issue to set us up to get widgets for base fields available, I'd vote to move forward with the current approach instead of jumping around more.

Ps. #2111887: Regression: Only title (of base fields) on nodes is marked as translatable also needs this infrastructure to have widgets for base fields proper to attach translatability information without needing to hack around in the form structure.

effulgentsia’s picture

Assigned: yched » Unassigned
FileSize
111.7 KB
3.5 KB

Thanks for the RTBC! I looked this over one more time in depth, and am happy with all of yched's changes as well.

Just trivial cleanup here, so leaving at RTBC.

fago’s picture

I agree with others that it's a good idea to move on with this now and work in the remaining issues as follow-ups.

I had a detailed look at the patch, in particular at the entity validation changes - those are good as well, great work!

The only minor thing (besides the commented interim hacks) I found:

+ * This is an override of theme_field() for the node title field. See that
+ * function for documentation about its details and overrides.
+ *
+ * @param $variables
+ * An associative array. See theme_field() for details.
Should be array $variables.

Also the function has no @return, what could be ok as this is an override and has an @see on the original function. But if so, @param should go as well -> either add both, or none.

That's nothing what should hold up this important step though, and can be fixed in a separate issue.

effulgentsia’s picture

Added the type to the @param of the new theme function and also to theme_field() to keep them consistent, but did not add the @return to either due to #2148429: Add @return to theme.inc functions.

effulgentsia’s picture

Issue summary: View changes
effulgentsia’s picture

Issue summary: View changes
fago’s picture

Thanks - improvements are good! Thanks for opening #2148429: Add @return to theme.inc functions - I replied there .

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK there's a few @todos in here but they're all related to existing issues and/or need to be resolved when everything has been converted. Committed/pushed to 8.x, thanks!

Wim Leers’s picture

OMG YAY!!!!! My sincere thanks to go to everybody who helped get this patch to RTBC, especially effulgentsia and yched! :)

nod_’s picture

Tag and some refactor of the JS.

$el.is(':has(.field-item)')? I think not :D

plach’s picture

Awesome!

However, don't we need a change notice for this?

yched’s picture

I'll let @Wim chime on the JS followup :-)

@plach: the functionality will definitely deserve a change notice, but for now it's still relatively hidden and clunky to leverage. I think it would make more sense to write a change notice after the "right" API and UI have landed in #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title)

plach’s picture

Ok, works for me :)

Wim Leers’s picture

effulgentsia’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

Title: Apply formatters and widgets to rendered entity base fields, starting with node.title » Change record: Apply formatters and widgets to rendered entity base fields, starting with node.title
Priority: Major » Critical
Status: Closed (fixed) » Active
Issue tags: +beta blocker, +Missing change record

The followup issue has now turned into a meta issue, and I just tried to reference the change record from this one there, except there wasn't one. :) So let's create the change record here, since this is a monumental improvement, and then we can add to it as needed.

yched’s picture

@xjm: This issue here did the internal legwork of adapting the field rendering system to run on base fields too, but the actual API to specify widget and formatter options for your entity type's base fields was added by #2144919: Allow widgets and formatters for base fields to be configured in Field UI.

The change record was thus created for the latter : https://drupal.org/node/2186583
IIRC, this issue here doesn't need a change record of its own.

xjm’s picture

Title: Change record: Apply formatters and widgets to rendered entity base fields, starting with node.title » Apply formatters and widgets to rendered entity base fields, starting with node.title
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -beta blocker, -Missing change record

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.