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.
Files: 
CommentFileSizeAuthor
#208 core-js-edit-funkyselector-1988612-208.patch777 bytesnod_
#201 interdiff.txt906 byteseffulgentsia
#201 formatters_widgets-base_fields-1988612-201.patch112.07 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 59,105 pass(es).
[ View ]
#199 interdiff.txt3.5 KBeffulgentsia
#199 formatters_widgets-base_fields-1988612-199.patch111.7 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 59,210 pass(es).
[ View ]
#194 interdiff.txt7.56 KByched
#194 formatters_widgets-base_fields-2133669-194.patch111.74 KByched
PASSED: [[SimpleTest]]: [MySQL] 59,093 pass(es).
[ View ]
#193 interdiff.txt752 bytesyched
#193 formatters_widgets-base_fields-1988612-192.patch112.07 KByched
PASSED: [[SimpleTest]]: [MySQL] 59,096 pass(es).
[ View ]
#190 formatters_widgets-base_fields-1988612-190.patch112.06 KByched
FAILED: [[SimpleTest]]: [MySQL] 58,808 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#186 interdiff-174-186.txt3.44 KBeffulgentsia
#186 formatters_widgets-base_fields-1988612-186.patch111.54 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,796 pass(es), 3 fail(s), and 1 exception(s).
[ View ]
#183 interdiff.txt1013 byteseffulgentsia
#183 formatters_widgets-base_fields-1988612-183.patch110.95 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,716 pass(es), 155 fail(s), and 5 exception(s).
[ View ]
#181 interdiff.txt4.05 KBeffulgentsia
#180 formatters_widgets-base_fields-1988612-180.patch110.77 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,836 pass(es), 220 fail(s), and 2,711 exception(s).
[ View ]
#179 formatters_widgets-base_fields-1988612-179.patch111.27 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,926 pass(es), 111 fail(s), and 3,322 exception(s).
[ View ]
#174 formatters_widgets-base_fields-1988612-174.patch112.16 KByched
FAILED: [[SimpleTest]]: [MySQL] 58,837 pass(es), 3 fail(s), and 1 exception(s).
[ View ]
#172 interdiff.txt21.64 KByched
#172 formatters_widgets-base_fields-1988612-172.patch112.81 KByched
PASSED: [[SimpleTest]]: [MySQL] 60,368 pass(es).
[ View ]
#167 interdiff.txt3.92 KBeffulgentsia
#167 formatters_widgets-base_fields-1988612-167.patch100.12 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 60,299 pass(es).
[ View ]
#165 interdiff.txt1.61 KBeffulgentsia
#165 formatters_widgets-base_fields-1988612-165.patch97.44 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 60,177 pass(es).
[ View ]
#164 interdiff.txt1.58 KBeffulgentsia
#164 formatters_widgets-base_fields-1988612-164.patch97.95 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 59,643 pass(es).
[ View ]
#163 interdiff.txt814 byteseffulgentsia
#163 formatters_widgets-base_fields-1988612-163.patch97.71 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 60,382 pass(es).
[ View ]
#161 interdiff.txt658 byteseffulgentsia
#161 formatters_widgets-base_fields-1988612-161.patch97.14 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 60,370 pass(es), 6 fail(s), and 6 exception(s).
[ View ]
#159 formatters_widgets-base_fields-1988612-159.patch97.07 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 60,383 pass(es), 23 fail(s), and 164 exception(s).
[ View ]
#154 formatters_widgets-base_fields-1988612-153.patch95.75 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,529 pass(es), 53 fail(s), and 31 exception(s).
[ View ]
#154 interdiff.txt890 byteseffulgentsia
#151 formatters_widgets-base_fields-1988612-151.patch95.76 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,776 pass(es), 168 fail(s), and 12,125 exception(s).
[ View ]
#151 interdiff.txt464 byteseffulgentsia
#149 formatters_widgets-base_fields-1988612-149.patch95.77 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#147 formatters_widgets-base_fields-1988612-147.patch95.78 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch formatters_widgets-base_fields-1988612-147.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#147 interdiff.txt32.69 KBeffulgentsia
#145 formatters_widgets-base_fields-1988612-145.patch81.79 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 59,369 pass(es).
[ View ]
#145 interdiff.txt6.77 KBeffulgentsia
#144 formatters_widgets-base_fields-1988612-144.patch79.56 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 59,054 pass(es).
[ View ]
#143 formatters_widgets-base_fields-1988612-143.patch79.8 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 59,286 pass(es).
[ View ]
#143 interdiff.txt597 byteseffulgentsia
#141 formatters_widgets-base_fields-1988612-141.patch79.81 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/edit/lib/Drupal/edit/EditController.php.
[ View ]
#141 interdiff.txt3.28 KBeffulgentsia
#140 formatters_widgets-base_fields-1988612-140.patch82.06 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 59,519 pass(es).
[ View ]
#140 interdiff.txt1.42 KBeffulgentsia
#136 formatters_widgets-base_fields-1988612-136.patch81.1 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 59,311 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#136 interdiff.txt617 byteseffulgentsia
#134 formatters_widgets-base_fields-1988612-134.patch80.36 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,941 pass(es), 42 fail(s), and 9 exception(s).
[ View ]
#134 interdiff.txt5.31 KBeffulgentsia
#131 formatters_widgets-base_fields-1988612-131.patch81.94 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,859 pass(es), 56 fail(s), and 9 exception(s).
[ View ]
#131 interdiff.txt11.87 KBeffulgentsia
#128 formatters_widgets-base_fields-1988612-128.patch70.01 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 57,957 pass(es), 171 fail(s), and 17 exception(s).
[ View ]
#128 interdiff.txt15.37 KBeffulgentsia
#126 formatters_widgets-base_fields-1988612-126.patch54.21 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 56,660 pass(es), 260 fail(s), and 42 exception(s).
[ View ]
#126 interdiff.txt24.29 KBeffulgentsia
#123 formatters_widgets-base_fields-1988612-123.patch33.92 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 59,190 pass(es).
[ View ]
#123 interdiff.txt2.62 KBeffulgentsia
#121 formatters_widgets-base_fields-1988612-121.patch33.01 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 59,103 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#121 interdiff.txt6.86 KBeffulgentsia
#119 formatters_widgets-base_fields-1988612-119.patch27.16 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 59,092 pass(es), 8 fail(s), and 1 exception(s).
[ View ]
#119 interdiff.txt781 byteseffulgentsia
#115 formatters_widgets-base_fields-1988612-115.patch27.05 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#112 formatters_widgets-base_fields-1988612-112.patch27.53 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,308 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#112 formatters_widgets-base_fields-1988612-112-interdiff.txt860 bytesBerdir
#110 formatters_widgets-base_fields-1988612-110.patch27.54 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,949 pass(es), 20 fail(s), and 997 exception(s).
[ View ]
#110 formatters_widgets-base_fields-1988612-110-interdiff.txt6.82 KBBerdir
#108 formatters_widgets-base_fields-1988612-108.patch27.33 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 54,812 pass(es), 521 fail(s), and 571 exception(s).
[ View ]
#108 formatters_widgets-base_fields-1988612-108-interdiff.txt1.99 KBBerdir
#105 formatters_widgets-base_fields-1988612-105.patch27.08 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 52,780 pass(es), 1,451 fail(s), and 1,069 exception(s).
[ View ]
#97 formatters_widgets-base_fields-1988612-97.patch32.88 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 58,390 pass(es).
[ View ]
#95 formatters_widgets-base_fields-1988612-95.patch34.46 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 58,157 pass(es).
[ View ]
#95 interdiff.txt1.85 KBeffulgentsia
#92 formatters_widgets-base_fields-1988612-92.patch33.56 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,132 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#92 interdiff.txt2.04 KBeffulgentsia
#90 formatters_widgets-base_fields-1988612-90.patch32.79 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/lib/Drupal/node/NodeFormController.php.
[ View ]
#86 formatters_widgets-base_fields-1988612-86.patch32.81 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch formatters_widgets-base_fields-1988612-86.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#86 interdiff.txt701 byteseffulgentsia
#84 formatters_widgets-base_fields-1988612-84.patch32.79 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 53,741 pass(es), 540 fail(s), and 363 exception(s).
[ View ]
#84 interdiff.txt590 byteseffulgentsia
#83 formatters_widgets-base_fields-1988612-83.patch32.74 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 53,378 pass(es), 726 fail(s), and 447 exception(s).
[ View ]
#83 interdiff.txt592 byteseffulgentsia
#82 formatters_widgets-base_fields-1988612-82.patch32.54 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 47,974 pass(es), 2,630 fail(s), and 1,451 exception(s).
[ View ]
#82 interdiff.txt4.76 KBeffulgentsia
#81 formatters_widgets-base_fields-1988612-81.patch37.14 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 45,607 pass(es), 2,996 fail(s), and 1,515 exception(s).
[ View ]
#79 formatters_widgets-base_fields-1988612-79.patch37.83 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 57,925 pass(es).
[ View ]
#79 interdiff.txt942 byteseffulgentsia
#77 formatters_widgets-base_fields-1988612-77.patch36.73 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 57,981 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#74 formatters_widgets-base_fields-1988612-74.patch36.74 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,113 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#72 formatters_widgets-base_fields-1988612-71.patch36.73 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch formatters_widgets-base_fields-1988612-71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#72 interdiff.txt488 byteseffulgentsia
#70 formatters_widgets-base_fields-1988612-70.patch37.35 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch formatters_widgets-base_fields-1988612-70.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#70 interdiff.txt6.95 KBeffulgentsia
#67 formatters_widgets-base_fields-1988612-67.patch32.89 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,194 pass(es), 9 fail(s), and 36 exception(s).
[ View ]
#67 interdiff.txt2.54 KBeffulgentsia
#64 formatters_widgets-base_fields-1988612-64.patch31.28 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,342 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#64 interdiff.txt657 byteseffulgentsia
#62 formatters_widgets-base_fields-1988612-62.patch31.25 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 48,086 pass(es), 2,424 fail(s), and 1,363 exception(s).
[ View ]
#62 interdiff.txt1.37 KBeffulgentsia
#58 formatters_widgets-base_fields-1988612-58.patch31.24 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 48,012 pass(es), 2,423 fail(s), and 1,363 exception(s).
[ View ]
#58 interdiff.txt3.86 KBeffulgentsia
#56 formatters_widgets-base_fields-1988612-56.patch31.03 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 53,838 pass(es), 505 fail(s), and 2,245 exception(s).
[ View ]
#56 interdiff.txt4.47 KBeffulgentsia
#53 formatters_widgets-base_fields-1988612-53.patch32.56 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 51,949 pass(es), 1,371 fail(s), and 3,517 exception(s).
[ View ]
#47 formatters_widgets-base_fields-1988612-47.patch32.13 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 57,080 pass(es), 2 fail(s), and 2,450 exception(s).
[ View ]
#47 interdiff.txt4.9 KBWim Leers
#46 formatters_widgets-base_fields-1988612-45.patch30.82 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 51,245 pass(es), 2,250 fail(s), and 2,805 exception(s).
[ View ]
#44 formatters_widgets-base_fields-1988612-44.patch30.9 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Entity/Field/Field.php.
[ View ]
#41 formatters_widgets-base_fields-1988612-41.patch54.29 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 56,451 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#38 formatters_widgets-base_fields-1988612-38.patch53.87 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 56,678 pass(es).
[ View ]
#38 interdiff-37-38.do-not-test.diff733 bytesPancho
#37 formatters_widgets-base_fields-1988612-37.patch53.88 KBPancho
FAILED: [[SimpleTest]]: [MySQL] 56,784 pass(es), 1 fail(s), and 1,071 exception(s).
[ View ]
#35 formatters_widgets-base_fields-1988612-35.patch53.88 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 56,601 pass(es).
[ View ]
#35 interdiff.txt4.4 KBeffulgentsia
#34 formatters_widgets-base_fields-1988612-34.patch49.41 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 56,600 pass(es).
[ View ]
#34 interdiff.txt991 bytesWim Leers
#33 formatters_widgets-base_fields-1988612-33.patch48.62 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 59,135 pass(es).
[ View ]
#31 formatters_widgets-base_fields-1988612-31.patch48.2 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 57,966 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#29 formatters_widgets-base_fields-1988612-29.patch28.54 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,297 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#28 formatters_widgets-base_fields-1988612-28.patch33.73 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#26 formatters_widgets-base_fields-1988612-26.patch25.69 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 53,592 pass(es), 810 fail(s), and 223 exception(s).
[ View ]
#24 formatters_widgets-base_fields-1988612-24.patch21.72 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 58,022 pass(es).
[ View ]
#22 formatters_widgets-base_fields-1988612-22.patch20.36 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 56,598 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#20 formatters_widgets-base_fields-1988612-20.patch20.35 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 56,444 pass(es), 38 fail(s), and 21 exception(s).
[ View ]
#17 formatters_widgets-base_fields-1988612-17.patch19.23 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 53,413 pass(es), 1,127 fail(s), and 456 exception(s).
[ View ]
#15 formatters_widgets-base_fields-1988612-15.patch6.64 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,223 pass(es), 5 fail(s), and 4,895 exception(s).
[ View ]
#1 node_created_formatter.patch5.21 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_created_formatter.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs work
StatusFileSize
new5.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_created_formatter.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

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

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?

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

Title:Apply formatters to rendered entity base fields, starting with node.createdApply 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.

Issue tags:+sprint

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.

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.

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 ? ;-)

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

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

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

Title:Apply formatters and widgets to rendered entity base fields, starting with node.createdApply formatters and widgets to rendered entity base fields, starting with node.title
Status:Active» Needs review
StatusFileSize
new6.64 KB
FAILED: [[SimpleTest]]: [MySQL] 58,223 pass(es), 5 fail(s), and 4,895 exception(s).
[ View ]

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.

StatusFileSize
new19.23 KB
FAILED: [[SimpleTest]]: [MySQL] 53,413 pass(es), 1,127 fail(s), and 456 exception(s).
[ View ]

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new20.35 KB
FAILED: [[SimpleTest]]: [MySQL] 56,444 pass(es), 38 fail(s), and 21 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new20.36 KB
FAILED: [[SimpleTest]]: [MySQL] 56,598 pass(es), 2 fail(s), and 3 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new21.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,022 pass(es).
[ View ]

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.

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

StatusFileSize
new25.69 KB
FAILED: [[SimpleTest]]: [MySQL] 53,592 pass(es), 810 fail(s), and 223 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new33.73 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

StatusFileSize
new28.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,297 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new48.2 KB
FAILED: [[SimpleTest]]: [MySQL] 57,966 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new48.62 KB
PASSED: [[SimpleTest]]: [MySQL] 59,135 pass(es).
[ View ]

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.

StatusFileSize
new991 bytes
new49.41 KB
PASSED: [[SimpleTest]]: [MySQL] 56,600 pass(es).
[ View ]

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.

StatusFileSize
new4.4 KB
new53.88 KB
PASSED: [[SimpleTest]]: [MySQL] 56,601 pass(es).
[ View ]

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

Yay :D

StatusFileSize
new53.88 KB
FAILED: [[SimpleTest]]: [MySQL] 56,784 pass(es), 1 fail(s), and 1,071 exception(s).
[ View ]

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

StatusFileSize
new733 bytes
new53.87 KB
PASSED: [[SimpleTest]]: [MySQL] 56,678 pass(es).
[ View ]

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.

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new54.29 KB
FAILED: [[SimpleTest]]: [MySQL] 56,451 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Just a reroll.

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.

Assigned:Unassigned» Wim Leers
Status:Needs work» Needs review
StatusFileSize
new30.9 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Entity/Field/Field.php.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new30.82 KB
FAILED: [[SimpleTest]]: [MySQL] 51,245 pass(es), 2,250 fail(s), and 2,805 exception(s).
[ View ]

#44 was the wrong patch. Sorry.

StatusFileSize
new4.9 KB
new32.13 KB
FAILED: [[SimpleTest]]: [MySQL] 57,080 pass(es), 2 fail(s), and 2,450 exception(s).
[ View ]

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.

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new32.56 KB
FAILED: [[SimpleTest]]: [MySQL] 51,949 pass(es), 1,371 fail(s), and 3,517 exception(s).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new4.47 KB
new31.03 KB
FAILED: [[SimpleTest]]: [MySQL] 53,838 pass(es), 505 fail(s), and 2,245 exception(s).
[ View ]

Some interim progress.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.86 KB
new31.24 KB
FAILED: [[SimpleTest]]: [MySQL] 48,012 pass(es), 2,423 fail(s), and 1,363 exception(s).
[ View ]

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

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

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.

Status:Needs work» Needs review
StatusFileSize
new1.37 KB
new31.25 KB
FAILED: [[SimpleTest]]: [MySQL] 48,086 pass(es), 2,424 fail(s), and 1,363 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new657 bytes
new31.28 KB
FAILED: [[SimpleTest]]: [MySQL] 58,342 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Chasing HEAD.

Status:Needs review» Needs work

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

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.

Status:Needs work» Needs review
StatusFileSize
new2.54 KB
new32.89 KB
FAILED: [[SimpleTest]]: [MySQL] 58,194 pass(es), 9 fail(s), and 36 exception(s).
[ View ]

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

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

Status:Needs work» Needs review
StatusFileSize
new6.95 KB
new37.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch formatters_widgets-base_fields-1988612-70.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new488 bytes
new36.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch formatters_widgets-base_fields-1988612-71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new36.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,113 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Straight reroll.

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.

Status:Needs work» Needs review
StatusFileSize
new36.73 KB
FAILED: [[SimpleTest]]: [MySQL] 57,981 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Straight reroll.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new942 bytes
new37.83 KB
PASSED: [[SimpleTest]]: [MySQL] 57,925 pass(es).
[ View ]

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.

StatusFileSize
new37.14 KB
FAILED: [[SimpleTest]]: [MySQL] 45,607 pass(es), 2,996 fail(s), and 1,515 exception(s).
[ View ]

Straight reroll.

StatusFileSize
new4.76 KB
new32.54 KB
FAILED: [[SimpleTest]]: [MySQL] 47,974 pass(es), 2,630 fail(s), and 1,451 exception(s).
[ View ]

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.

StatusFileSize
new592 bytes
new32.74 KB
FAILED: [[SimpleTest]]: [MySQL] 53,378 pass(es), 726 fail(s), and 447 exception(s).
[ View ]

StatusFileSize
new590 bytes
new32.79 KB
FAILED: [[SimpleTest]]: [MySQL] 53,741 pass(es), 540 fail(s), and 363 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new701 bytes
new32.81 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch formatters_widgets-base_fields-1988612-86.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new32.79 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/lib/Drupal/node/NodeFormController.php.
[ View ]

Reroll.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.04 KB
new33.56 KB
FAILED: [[SimpleTest]]: [MySQL] 58,132 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

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.

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?

Status:Needs work» Needs review
StatusFileSize
new1.85 KB
new34.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,157 pass(es).
[ View ]

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.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new32.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,390 pass(es).
[ View ]

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.

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

Does #75 still apply ?

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

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

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.

Status:Needs work» Needs review
StatusFileSize
new27.08 KB
FAILED: [[SimpleTest]]: [MySQL] 52,780 pass(es), 1,451 fail(s), and 1,069 exception(s).
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new1.99 KB
new27.33 KB
FAILED: [[SimpleTest]]: [MySQL] 54,812 pass(es), 521 fail(s), and 571 exception(s).
[ View ]

Improving viewBaseField().

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new6.82 KB
new27.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,949 pass(es), 20 fail(s), and 997 exception(s).
[ View ]

Fixed editing too.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new860 bytes
new27.53 KB
FAILED: [[SimpleTest]]: [MySQL] 58,308 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

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.

  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?

Status:Needs work» Needs review
StatusFileSize
new27.05 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new781 bytes
new27.16 KB
FAILED: [[SimpleTest]]: [MySQL] 59,092 pass(es), 8 fail(s), and 1 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new6.86 KB
new33.01 KB
FAILED: [[SimpleTest]]: [MySQL] 59,103 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.62 KB
new33.92 KB
PASSED: [[SimpleTest]]: [MySQL] 59,190 pass(es).
[ View ]

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

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.

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

StatusFileSize
new24.29 KB
new54.21 KB
FAILED: [[SimpleTest]]: [MySQL] 56,660 pass(es), 260 fail(s), and 42 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new15.37 KB
new70.01 KB
FAILED: [[SimpleTest]]: [MySQL] 57,957 pass(es), 171 fail(s), and 17 exception(s).
[ View ]

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.

  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.

StatusFileSize
new11.87 KB
new81.94 KB
FAILED: [[SimpleTest]]: [MySQL] 58,859 pass(es), 56 fail(s), and 9 exception(s).
[ View ]

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.31 KB
new80.36 KB
FAILED: [[SimpleTest]]: [MySQL] 58,941 pass(es), 42 fail(s), and 9 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new617 bytes
new81.1 KB
FAILED: [[SimpleTest]]: [MySQL] 59,311 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

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.

Issue tags:+Prague Hard Problems

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
new82.06 KB
PASSED: [[SimpleTest]]: [MySQL] 59,519 pass(es).
[ View ]

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.

StatusFileSize
new3.28 KB
new79.81 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/edit/lib/Drupal/edit/EditController.php.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new597 bytes
new79.8 KB
PASSED: [[SimpleTest]]: [MySQL] 59,286 pass(es).
[ View ]

Oops.

StatusFileSize
new79.56 KB
PASSED: [[SimpleTest]]: [MySQL] 59,054 pass(es).
[ View ]

Straight reroll for HEAD changes. No actual changes.

StatusFileSize
new6.77 KB
new81.79 KB
PASSED: [[SimpleTest]]: [MySQL] 59,369 pass(es).
[ View ]

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new32.69 KB
new95.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch formatters_widgets-base_fields-1988612-147.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new95.77 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

And rerolled for HEAD.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new464 bytes
new95.76 KB
FAILED: [[SimpleTest]]: [MySQL] 58,776 pass(es), 168 fail(s), and 12,125 exception(s).
[ View ]

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new890 bytes
new95.75 KB
FAILED: [[SimpleTest]]: [MySQL] 58,529 pass(es), 53 fail(s), and 31 exception(s).
[ View ]

Status:Needs review» Needs work

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

Assigned:effulgentsia» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.98 KB
new96.54 KB
PASSED: [[SimpleTest]]: [MySQL] 59,368 pass(es).
[ View ]

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?

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

Issue summary:View changes

shortened issue summary

Berdir, fago, any chance you could review this?

Issue summary:View changes
StatusFileSize
new97.07 KB
FAILED: [[SimpleTest]]: [MySQL] 60,383 pass(es), 23 fail(s), and 164 exception(s).
[ View ]

Reroll.

Status:Needs review» Needs work

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

StatusFileSize
new97.14 KB
FAILED: [[SimpleTest]]: [MySQL] 60,370 pass(es), 6 fail(s), and 6 exception(s).
[ View ]
new658 bytes

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new97.71 KB
PASSED: [[SimpleTest]]: [MySQL] 60,382 pass(es).
[ View ]
new814 bytes

This should be green again.

StatusFileSize
new97.95 KB
PASSED: [[SimpleTest]]: [MySQL] 59,643 pass(es).
[ View ]
new1.58 KB

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

StatusFileSize
new97.44 KB
PASSED: [[SimpleTest]]: [MySQL] 60,177 pass(es).
[ View ]
new1.61 KB

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.

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.

StatusFileSize
new100.12 KB
PASSED: [[SimpleTest]]: [MySQL] 60,299 pass(es).
[ View ]
new3.92 KB

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.

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

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

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

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

StatusFileSize
new112.81 KB
PASSED: [[SimpleTest]]: [MySQL] 60,368 pass(es).
[ View ]
new21.64 KB

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.

StatusFileSize
new112.16 KB
FAILED: [[SimpleTest]]: [MySQL] 58,837 pass(es), 3 fail(s), and 1 exception(s).
[ View ]

Just a reroll for now.

Status:Needs review» Needs work

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

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

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 ?

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.

Status:Needs work» Needs review
StatusFileSize
new111.27 KB
FAILED: [[SimpleTest]]: [MySQL] 58,926 pass(es), 111 fail(s), and 3,322 exception(s).
[ View ]

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

StatusFileSize
new110.77 KB
FAILED: [[SimpleTest]]: [MySQL] 58,836 pass(es), 220 fail(s), and 2,711 exception(s).
[ View ]

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

StatusFileSize
new4.05 KB

Interdiff between #179 and #180.

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

StatusFileSize
new110.95 KB
FAILED: [[SimpleTest]]: [MySQL] 58,716 pass(es), 155 fail(s), and 5 exception(s).
[ View ]
new1013 bytes

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

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

Assigned:Unassigned» yched
StatusFileSize
new111.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,796 pass(es), 3 fail(s), and 1 exception(s).
[ View ]
new3.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.

StatusFileSize
new112.06 KB
FAILED: [[SimpleTest]]: [MySQL] 58,808 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new112.07 KB
PASSED: [[SimpleTest]]: [MySQL] 59,096 pass(es).
[ View ]
new752 bytes

Should fix the fail ?

StatusFileSize
new111.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,093 pass(es).
[ View ]
new7.56 KB

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

Issue summary:View changes

Added a patch summary to the OP

Issue summary:View changes

Issue summary:View changes

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.

Assigned:yched» Unassigned
StatusFileSize
new111.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,210 pass(es).
[ View ]
new3.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.

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.

StatusFileSize
new112.07 KB
PASSED: [[SimpleTest]]: [MySQL] 59,105 pass(es).
[ View ]
new906 bytes

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

Issue summary:View changes

Issue summary:View changes

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

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!

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

Issue tags:+JavaScript
StatusFileSize
new777 bytes

Tag and some refactor of the JS.

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

Awesome!

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

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)

Ok, works for me :)

Status:Fixed» Closed (fixed)

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