This is part of #1949932: [META] Unify entity fields and field API.

Updated: Comment #111

Problem/Motivation

Formatter and widget constructors currently receive a $instance config entity, with no type hint, and use it as an array, thereby potentially relying on any arbitrary (public or private) FieldInstance property. This couples formatters and widgets too tightly on $instance as a config entity, preventing them from being reliably applied to node titles or any other nonconfigurable field, effectively also preventing node titles and other nonconfigurable fields from being translatable or in-place editable without adding lots of new custom code for each such field.

Proposed resolution

  • Create a FieldDefinitionInterface that encapsulates the information that formatters and widgets need about a field (other than its item values within each entity, since those are passed separately to the formatter and widget interface methods as needed so that a single formatter or widget instance can operate on multiple entities).
  • Have $instance config entities implement that interface.
  • Change formatter and widget constructors from receiving an untyped $instance config entity to receiving a $field_definition object typed to FieldDefinitionInterface.
  • EntityDisplay and EntityFormDisplay can still pass $instance as that object, since it implements that interface, though see #1875974: Abstract 'component type' specific code out of EntityDisplay about abstracting that.
  • Also make $field config entities implement that interface, and remove the Drupal\field\Plugin\views\field\Field::fakeFieldInstance() hack currently employed for allowing Views to collect formatter settings to apply to results across multiple bundles.

Remaining tasks

Review.

User interface changes

None.

API changes

In addition to what's listed above in "Proposed resolution", various functions and hooks called by formatters and widgets are affected:

  • Changed hook_field_is_empty() from receiving $field to receiving $field_type, since it should only depend on the type, not anything else in the field definition.
  • Changed file_field_widget_upload_validators() and file_field_widget_uri() from receiving $field and $instance to receiving $field_settings, since that's the only part of the field definition they depend on.
  • Changed entity_reference_get_selection_handler() and the corresponding ER selection plugin constructors from receiving $field and $instance to receiving $field_definition.
  • Changed hook_options_list() and options_allowed_values() from receiving $field and $instance to receiving $field_definition.
  • Made $entity a required parameter to options_allowed_values(), just as it is for hook_options_list(), and because 'entity_type' and 'bundle' can no longer be retrieved from $instance.
  • Removed $field_state['field'] and $field_state['instance'] and its accessor functions field_widget_field() and field_widget_instance(), because there's no good reason for element callbacks to need them. Instead, widgets should set $element['#custom'] properties that are needed by those callbacks. Updated the docs in WidgetInterface accordingly.
  • Changed the $context received by hook_field_widget_form_alter() and hook_field_widget_WIDGET_TYPE_form_alter() from having 'field' and 'instance' to having 'widget', 'field_definition', and 'entity'.

Also, strengthened hook_field_access() to type hint $field, which is already the intent, but the lack of it being explicit caused tests to pass that shouldn't have (see #62).

Once this issue and #1875974: Abstract 'component type' specific code out of EntityDisplay are both in, work on having nonconfigurable fields implement FieldDefinitionInterface and apply formatters and widgets to them: see #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title and #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title).

CommentFileSizeAuthor
#142 field-definition-1950632-142.patch161.17 KByched
#142 interdiff.txt396 bytesyched
#136 field-definition-1950632-136.patch161.55 KBamateescu
#136 interdiff.txt1.28 KBamateescu
#132 field-definition-1950632-132.patch160.74 KBamateescu
#129 field-definition-1950632-129.patch160.69 KBeffulgentsia
#129 interdiff.txt3.94 KBeffulgentsia
#128 field-definition-1950632-128.patch160.51 KBeffulgentsia
#128 interdiff.txt2.76 KBeffulgentsia
#125 field-definition-1950632-125.patch159.11 KBeffulgentsia
#123 field-definition-1950632-123.patch159.77 KBeffulgentsia
#123 interdiff.txt4.33 KBeffulgentsia
#121 field-definition-1950632-121.patch156.25 KBeffulgentsia
#121 interdiff.txt4.63 KBeffulgentsia
#119 interdiff.txt2.32 KBeffulgentsia
#118 field-definition-1950632-118.patch156.69 KBeffulgentsia
#111 field-definition-1950632-111.patch156.39 KBeffulgentsia
#111 interdiff.txt6.81 KBeffulgentsia
#107 field-definition-1950632-107.patch156.42 KBeffulgentsia
#107 interdiff.txt19.29 KBeffulgentsia
#100 field-definition-1950632-100.patch154.37 KBamateescu
#100 interdiff.txt1.27 KBamateescu
#93 field-definition-1950632-93.patch154.57 KBeffulgentsia
#93 interdiff.txt1.17 KBeffulgentsia
#88 field-definition-1950632-88.patch154.55 KBeffulgentsia
#85 formatters_field_definition-1950632-85.patch134.61 KBeffulgentsia
#82 formatters_field_definition-1950632-82.patch131.57 KBeffulgentsia
#80 formatters_field_definition-1950632-80.patch124.91 KBeffulgentsia
#72 formatters_field_definition-1950632-72.patch130.32 KBeffulgentsia
#72 interdiff.txt7.31 KBeffulgentsia
#71 formatters_field_definition-1950632-71.patch128.66 KBeffulgentsia
#71 interdiff.txt690 byteseffulgentsia
#69 formatters_field_definition-1950632-69.patch127.83 KBeffulgentsia
#69 interdiff.txt1.16 KBeffulgentsia
#67 formatters_field_definition-1950632-67.patch127.82 KBeffulgentsia
#67 interdiff.txt7.38 KBeffulgentsia
#66 formatters_field_definition-1950632-66.patch127.44 KBeffulgentsia
#66 interdiff.txt1.43 KBeffulgentsia
#65 formatters_field_definition-1950632-65.patch127.24 KBeffulgentsia
#65 interdiff.txt2.31 KBeffulgentsia
#62 formatters_field_definition-1950632-62.patch126.3 KBeffulgentsia
#62 interdiff.txt2.75 KBeffulgentsia
#61 formatters_field_definition-1950632-61.patch123.31 KBeffulgentsia
#61 interdiff.txt5.64 KBeffulgentsia
#58 formatters_field_definition-1950632-58.patch129.91 KByched
#58 interdiff.txt3.04 KByched
#56 formatters_field_definition-1950632-56.patch128.5 KByched
#56 interdiff.txt2.57 KByched
#54 formatters_field_definition-1950632-54.patch128.23 KBeffulgentsia
#54 interdiff.txt835 byteseffulgentsia
#48 formatters_field_definition-1950632-48.patch131.95 KBWim Leers
#48 interdiff.txt3.1 KBWim Leers
#44 formatters_field_definition-1950632-44.patch128.32 KBeffulgentsia
#36 formatters_field_definition-1950632-36.patch132.47 KBWim Leers
#36 interdiff.txt8.09 KBWim Leers
#30 formatters_field_definition-1950632-30.patch125.81 KBWim Leers
#30 interdiff.txt102.17 KBWim Leers
#29 formatters_field_definition-1950632-29.patch27.57 KBWim Leers
#29 interdiff.txt24.5 KBWim Leers
#25 formatters-field_definition-25.patch4.28 KBWim Leers
#19 formatters-field_definition.patch4.03 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Status: Active » Postponed
Issue tags: +Spark

Postponing on #1735118: Convert Field API to CMI, and tagging for Spark, because it's needed to support inline editing of node title and other nonconfigurable fields.

jibran’s picture

Status: Postponed » Active

As per #1

yched’s picture

I'm not sure this can happen before #1969728: Implement Field API "field types" as TypedData Plugins, though ?

swentel’s picture

Issue tags: +Field API

Tagging

fago’s picture

I think it could, since all field types have already their entity field item classes. But we need to have all fieldable entities converted first, such that we can start building upon entity field classes.

Thus, we have users+terms left from #1818580: [Meta] Convert all entity types to the new Entity Field API - but there are already patches.

Wim Leers’s picture

Issue tags: +sprint

I really need to make it possible to in-place edit node title/author/date ASAP now. Is this really blocked on #1969728: Implement Field API "field types" as TypedData Plugins, which sounds like it's a lot of work?

Please help me understand what needs to happen in order for this issue's title to become reality.

fago’s picture

I think what's required is that the passed $items are actually instance of the respective Field and FieldItem classes, i.e. have all fieldable entities converted to EntityNG.

Once, we have that we can start building upon it to get the $definition from them, e.g. do $items->getDefinition(). Then, for everything to work out we have to make sure all relevant settings are available in $definition, e.g. add in any settings defined at $instance in getDefinition().

fago’s picture

Citing the part from #1949932: [META] Unify entity fields and field API which is actually relevant for this issue:

Improve formatters and widgets to make them usable with all fields

  • Formatters and widgets depend on field API $field and $instance data structures, so currently we cannot use them with entity fields.
  • We don't want to require config entities for base entity fields defined by the entity type (node title, node language, node nid, ..) as those are critical for the entity type to work, so it should not depend on any CMI files to be there and everything be in sync. (Not mentioning field configs are entities as well ...)
  • We discussed multiple options for making formatters and widgets work independently from field instances and $field. We figured out that formatters and widgets mostly rely on instance settings and field settings plus some foundational field information. After discussing separate proxy objects, mappings etc. we figured that a merged representation of the field and instance object would cut it. Fortunately, we could serve that via the entity field object $items as we can merge settings in its getDefinition() method, e.g. see this code snippet:
    class Formatter {
    
      function render (FieldInterface $items) {
    
         // getDefinition() of a configurable field would merge instance and field settings,
         // while an entity field would just have to take care to provide the necessary settings.
         $definition = $items->getDefinition();
         $default_value = $items->getDefaultValue();
         $settings = $definition[‘settings’];
         $cardinality = $definition[‘cardinalty’];
      }
    
     function settingsForm($definition) {
    
     }
    }
    

    That way, an entity field that wants to make use of a field formatter or widget would just have to make sure it has the necessary settings in place also. Formatters and widgets still explicitly state which field types they support.

Wim Leers’s picture

Hm, okay. I wish I'd have a deeper understanding of what you're saying :)

So, let me put it this way: if you have bite-sized tasks (patches) to be done, that don't require deep or broad Field or Entity API knowledge, then I can help push this forward. Otherwise, I'm afraid I won't be of much use.

Do you have a rough ETA? :)

fago’s picture

I'd be happy to discuss things more via IRC, skype, whateve if that helps.

>Do you have a rough ETA? :)

Nope. As far as I'm concerned this is falls under the "would be awesome to have" umbrella, but there are quite some "*must* be solved before release" issues on my todo list queued before that :-/

fago’s picture

So, let me put it this way: if you have bite-sized tasks (patches) to be done, that don't require deep or broad Field or Entity API knowledge, then I can help push this forward. Otherwise, I'm afraid I won't be of much use.

I'd be happy to help splitting this up, but I think we'll need yched's input as well. I guess a good start would be to nail down a list of stuff that needs to be in $definition and make sure that's there. Once we'd have that *and* fieldable entities are converted, I think we could start with actually re-factoring formatters/widgets.

effulgentsia’s picture

Once we'd have that *and* fieldable entities are converted, I think we could start with actually re-factoring formatters/widgets.

I think it's possible to break off chunks even before those prerequisites. Here's a rough idea of a way it might be possible to proceed:

1) Change FormatterBase::__construct() to receive a $field_definition array as its third parameter instead of the $instance object. Remove $this->field and $this->instance from all formatters; instead whatever formatters would get from these, they should instead get from $this->field_definition. For now, make FormatterFactory::createInstance() create this $field_definition array by piecing together information that it gets from $configuration['instance'] and the corresponding field that it can get from field_info_field($configuration['instance']->field_name). $field_definition should only contain one set of keys though. For example, it should contain a single 'settings' that is a merge of $instance->settings and $field->settings. The entire point is that from the perspective of a formatter, there needs to be no such concept as field vs. instance. To start with, which keys are needed in $field_definition could be figured out by looking at what the union of core formatters actually use; eventually, we'll want to evaluate all the keys to determine if it makes architectural sense for formatters to need them.

2) Do the same for widgets.

3) Once all entities are converted to EntityNG, then we can replace $field_definition as something that is created by FormatterFactory::createInstance() into something that formatters and widgets can retrieve from $items->getDefinition().

Steps 1 and 2 alone don't solve the needs of Edit module, since FormatterFactory and WidgetFactory would still be limited to configurable fields only. But, they do get us a step closer, and possibly, can allow some Edit module patches to experiment with hacky glue code that only hacks around FormatterFactory and WidgetFactory, but not around every individual formatter and widget.

yched’s picture

#12 sounds like a good plan.
Merging $field and $instance might be a bit more complex now that they are objects rather than arrays.
Also, aside from merging $field['settings'] with $instance['settings'], I fear it's going to be hard to draw a line of "there's no reasonable use case for widgets or formatters to use this $field / $instance property".
I think the conclusion during the GHangout was that, if you look at what's currently in an (EntityNG) $field_definition, the diff was not that big ?

Having all core widgets and formatters converted to Plugins would help too I guess.
We still need to get those committed:
#1751234: Convert option widgets to Plugin system
#1796316: Convert Link/URL widgets / formatters to plugin system
#1787248: Convert taxonomy module formatters to Plugin system
First two are green & RTBC, the third looks currently stalled.

effulgentsia’s picture

Also, putting some attention on #1875992: Add EntityFormDisplay objects for entity forms and getting it over the finish line would be awesome, since that decouples widget configuration from field configuration, so is also a step towards enabling widgets on nonconfigurable fields.

effulgentsia’s picture

I fear it's going to be hard to draw a line of "there's no reasonable use case for widgets or formatters to use this $field / $instance property"

At a minimum, I would hope that id, uuid, deleted, and storage have no reasonable use case for formatters and widgets. I think entity_type, bundle, active, schema, and indexes are also suspect. I'm sure there'll be some that need discussion, but I think the goal is to have many fewer keys than just merging everything from $field and $instance.

fago’s picture

I'm sure there'll be some that need discussion, but I think the goal is to have many fewer keys than just merging everything from $field and $instance.

Yep, I think we need to identify what we really need and only put that stuff in there.

1) Change FormatterBase::__construct() to receive a $field_definition array as its third parameter instead of the $instance object

OK. Just to clarify, I think we'll go with the definition of an field here and not with the definition of the field item, as formatters, widgets usually work on multiple items.

Oh, or better said we could go with the definition of the type we are formatting. I don't think that's reasonable now, but we could think about splitting out a general formatting plugin based upon typed-data types and definitions later on. Imho an interesting idea, but out of scope now :-)

effulgentsia’s picture

effulgentsia’s picture

And now a harder one: #1974444: Narrow the API of hook_options_list() and friends. Do we think it makes sense for that function/hook to get all of $field_definition (whatever that ends up being)? Or, can we narrow that one a bit more than we narrow formatters and widgets generically?

effulgentsia’s picture

Status: Active » Needs review
FileSize
4.03 KB

@Wim: sorry, I was hoping to have more done here by now, but in case you have time to continue on this over the weekend or early next week, here's a small patch to get you started. This will fail bot. Try making it pass by adjusting all formatters and the rabbit hole of things they invoke to act on $this->field_definition instead of $this->field and $this->instance.

Status: Needs review » Needs work

The last submitted patch, formatters-field_definition.patch, failed testing.

yched’s picture

Doesn't affect the immediate next steps, but not sure why $instance is optional. We're always going to have an instance here ?
AAMOF I'd tend to put the method on FieldInstance rather than Field. Instances are usually the entry point when doing stuff on an entity, and once #1967106: FieldInstance::getField() method is in, getting the field from the instance will be straightforward.

effulgentsia’s picture

I thought there was something in Views that invoked one of the functions that will need conversion, and did it without a $instance, but I can't find it now.

effulgentsia’s picture

Found it: Drupal\field\Plugin\views\argument\FieldList::init() calls options_allowed_values() without a $instance.

yched’s picture

Oh. Just to be able to generate a admin configuration forms and summaries for views arguments on a 'list' field. Sad panda.
OK, too bad, nevermind.

Other than that, if this $field_definition is the only thing widgets are going to receive, it needs to include $field->cardinality and $instance->required (= 1 if no $instance).

Wim Leers’s picture

Straight reroll — HEAD changes broke the patch in #19.

arosboro’s picture

I applied this patch and began to get a 404 ajax error as soon as the home page loads. The article I had written is also missing it's body, but text appears in ckeditor if you click on the edit tab. Article started as unpublished, and then was published.

An AJAX HTTP error occurred.
HTTP Result Code: 404
Debugging information follows.
Path: /drupal8/edit/metadata
StatusText: Not Found
ResponseText: A fatal error occurred:
  • Fresh install of D8
  • Create an Article with Tags and Body
  • Apply patch in #25
Wim Leers’s picture

This patch is also not yet ready for review :P

The patch in #19/#25 is not at all working yet, it was just an indication of how the code *should* work in order to fulfill the goals of this issue :) I'm currently working on this!

arosboro’s picture

Ok sorry, I keep jumping in on these issues before they're ready to review :)

Wim Leers’s picture

@arosboro: np :)


This reroll completes the conversion from $field and $instance to $field_definition for formatters (and hence also for options_allowed_values()), as well as field_access(). I still have to do everything on the widget side.

This would trigger a lot of test failures, so keeping at NW until I do the widgets part.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
102.17 KB
125.81 KB

Widgets done. Whew.

Required a modification to entity_reference_get_selection_handler() and subsequently pretty much all of the Entity Reference module…
Similar story for hook_options_list(): lots of dependent changes.

I found out this the hard way:

Other than that, if this $field_definition is the only thing widgets are going to receive, it needs to include $field->cardinality and $instance->required (= 1 if no $instance).

But more than just those, also (all were on $instance before): description, columns, module, entity_type, bundle.

Also replaced field_widget_field() and field_widget_instance() by field_widget_field_definition().

This passes most Field API-related tests locally (running tests takes *forever* locally, so I haven't tested them all), so hopefully also on d.o's testbot.

Status: Needs review » Needs work

The last submitted patch, formatters_field_definition-1950632-30.patch, failed testing.

Dave Reid’s picture

Coming from #1974444: Narrow the API of hook_options_list() and friends

How would an existing D7 field type that has a field and instance setting that uses the same key but stores different values work in this new hybrid 'field definition' system? I want to ensure there is a plan or 'you must do this' in place for those field modules (or even modules that alter in field/instance options to existing fields).

function mymodule_field_info() {
  $info['myfield'] = array(
    ...
    'settings' => array(
      'text' => 'Default of a field-level setting',
    ),
    'instance settings' => array(
      'text' => 'Default of an instance-level setting',
    ),
    ...
  );
}
effulgentsia’s picture

The only examples in core of 'settings' and 'instance_settings' having the same key are image_field_info() for 'default_image' and translation_entity_field_info_alter() for 'translation_sync'. And in both cases, they refer to the same concept, so #30's behavior of merging the two with $instance overriding $field would do what's expected. Note that the merge is only for passing information to formatters and widgets. Field configuration UIs still have independent access to $field and $instance.

If in the hypothetical example of #32, the two 'text' settings refer to conceptually different things, such that a formatter or widget would need simultaneous access to both, then the "you must do this" plan if this issue lands is for the module to rename one of the two keys to match the fact that they refer to different things.

Dave Reid’s picture

Ok, then we need to consider and ensure that those modules would need to be able to have an opportunity to make that change before anything alters existing field definitions in this issue. And ensure that it is referenced in the change notice. And that this restriction is documented in however we are provided hook_field_info(), and hook_field_info_alter().

The chances of this situation are likely very slim, but considering that any module can alter in settings to existing field types, it is a possibility. That said, +1 to the effort here, I think it makes a lot of sense.

yched’s picture

hook_field_info() will most likely be moving to Plugins / Annotations as part of #1969728: Implement Field API "field types" as TypedData Plugins. hook_field_info_alter() will stay (probably renamed though)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.09 KB
132.47 KB

Reroll to fix the last few test failures.

Shouldn't this issue be major, considering that it blocks in-place editing of the most fundamental thing possible: a node's title?

swentel’s picture

First review. I felt kind of sad at first to loose the field and instance objects, otoh, this is a relatively straight forward change and if in the end we can edit the node title, then we all should be glad :)

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityFieldAccessCheck.phpundefined
@@ -42,9 +42,12 @@ public function access(Route $route, Request $request) {
+    $field = field_info_field($field_name);
+    $field_definition = $field->getFieldDefinition($instance);

Just wondering, maybe we should put getFieldDefinition in the FieldInfo service ?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/entity_reference/selection/SelectionBase.phpundefined
@@ -30,20 +30,6 @@
-  /**
-   * The instance array.
-   *
-   * @var array
-   */
-  protected $instance;

Should we add a protected $field_definition here ?

+++ b/core/modules/field/field.api.phpundefined
@@ -2138,8 +2138,9 @@ function hook_field_storage_purge(\Drupal\Core\Entity\EntityInterface $entity, $
- * @param $field
- *   The field on which the operation is to be performed.
+ * @param array $field_definition
+ *   The field definition array for the instance of the field on which the

The doc sounds weird to me, can't think of anything better right now though :)

+++ b/core/modules/field/field.moduleundefined
@@ -941,9 +941,10 @@ function field_has_data($field) {
+ * @param array $field_definition
+ *  The field definition array for the instance of the field on which the

Same here

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.phpundefined
@@ -17,20 +16,6 @@
-  /**
-   * The field instance definition.
-   *
-   * @var \Drupal\field\Plugin\Core\Entity\FieldInstance
-   */
-  protected $instance;

Same here, should we add a protected $field_definition ?

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -36,12 +36,12 @@
+    reset($this->field_definition['columns']);

It's not an object anymore, so the reset() can go away.

+++ b/core/modules/options/options.moduleundefined
@@ -414,7 +412,8 @@ function _options_values_in_use($field, $values) {
 function options_field_validate(EntityInterface $entity = NULL, $field, $instance, $langcode, $items, &$errors) {
-  $allowed_values = options_allowed_values($field, $instance, $entity);
+  $field_definition = $field->getFieldDefinition($instance);

It's kind of redundant to get the field definition if the $field and $instance are available no ?

Wim Leers’s picture

Thanks for the review!

RE: protected $field_definition: I probably should indeed do that. Will fix.
RE: "weird doc" -> I also couldn't think of anything better, I defer that to the Field API experts :)
RE: reset(): will fix.
RE: "redundant $field_definition" — not really. Look at the next line: I need it to call options_allowed_values():

 function options_field_validate(EntityInterface $entity = NULL, $field, $instance, $langcode, $items, &$errors) {
-  $allowed_values = options_allowed_values($field, $instance, $entity);
+  $field_definition = $field->getFieldDefinition($instance);
+  $allowed_values = options_allowed_values($field_definition, $entity);

More reviews are definitely needed.

What would be next steps? Does this patch implement the complete scope of what we want to achieve in this issue? Or do we already want to convert e.g. node title to leverage this? Or is more necessary to be able to do that? (I personally don't see how this directly enables using Field API Widgets for the node title.)

Gábor Hojtsy’s picture

Priority: Normal » Major

I think both the scope/intent and the size of this patch makes it pretty major.

Looking at the changes it looks to be applying a pattern mostly, so far as we agree on the way it applies the pattern, we don't necessarily need to scrutinise each line AFAIS. I looked at several random places where the pattern was applied and it looks good.

smiletrl’s picture

Priority: Major » Normal

Have the same questions with @Wim Leers.

As Entity properties, like node.title, node.created are working as Field API fields, there're no formatters nor widgets avaviable in UI for cnfiguration. Perhaps that's one of the reasons why they are not converted to config entity, because there's no configuration at all:) Of curse, the main concern is

those are critical for the entity type to work, so it should not depend on any CMI files to be there and everything be in sync.

But now, we are trying to make formatters and widgets work for those special fields, which brings two concerns for me.

One is how are we going to make that happen? For instance, I want to create a formatter/widget serving node.title, what's the field_types = {} in this formatter's @plugin definition? And If this formatter/widget could be created, will this formatter/widget be available at the UI for user to configure? What if I want to make formatter work for node.nid? And I guess there's no sense to make widgets avavilable for nid, since it's generated automatically.

The other one is since these fields could use formatters and widgets, they could have configuration associated with them. Then I guess it would be necessary to make them config entities?

Currently, the usage of field configuration to me is to sync the entity/node types, which may have many fields attached. If we make node.title not configurable, then in the sync process, we need to manually create this node type(this creation will build 'node title field' for us) via UI before import other fields configuration. Maybe it's a bad user experience for the sync process? Or we import the node type configuration(and this configurtion will include 'node title field') firstly and then import fields configuration? But it seems Class NodeType is not configurable either. Then how are we going to make use of this field configuration? Will node.title also configurable make the mentioned sync process easier?

smiletrl’s picture

Priority: Normal » Major

Didn't mean to change priority:)

yched’s picture

like node.title, node.created are working as Field API fields, there're no formatters nor widgets avaviable in UI for cnfiguration. Perhaps that's one of the reasons why they are not converted to config entity, because there's no configuration at all:)

Let's clarify a couple things to avoid misunderstandings.

- node.title, node.created will remain "base fields" and will *not* be turned into "Field API field" (i.e. "configurable fields", i.e. fields that can be created or removed by admins in the UI). The point of the issue is to make widgets and formatters be able to work on "base" fields as well as "configurable fields" instead of just the latter currently.

- This patch does not provide a UI to actually select the widgets or formatters to use for a given "base field". This will be a topic for a separate issue.
For widgets, #1875992: Add EntityFormDisplay objects for entity forms is a prerequisite to have a place to store the corresponding config.
For formatters, the EntityDisplay objects introduced in #1852966: Rework entity display settings around EntityDisplay config entity already provide a location for the config.

- 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.
That's a non minor task, that is probably best addressed in a followup...

I want to create a formatter/widget serving node.title, what's the field_types = {} in this formatter's @plugin definition?

That's a good question, still to be adressed. Right now widgets / formatters specify which "configurable field" types they can work on. If they can work on base fields as well, how do they identify the base fields they can work on ? Their data type I guess ?

#1969728: Implement Field API "field types" as TypedData Plugins is about "merging" the notions of "configurable field" type and data type somehow, but this is not nearly done yet.
So I guess for now this means two separate entries in a widget / formatter Plugin definition : the list of applicable field types, and the list of applicable data types.

Strictly speaking, though, this is only needed when we want to present a UI. This does not necessarily prevent starting to assign widgets / formatters to base fields in the base field definitions (or possibly just as "default widget' / 'default formatter' properties in the data types definitions for starters) and make them actually trigger.

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

I'll work on a separate issue that demonstrates a proof of concept for connecting a text formatter and widget to node.title to help clarify #42. Will link to it from here when it's ready. Code reviews on #36 welcome in the meantime. I'll try to do one soon as well.

effulgentsia’s picture

Just a reroll of #36 for HEAD changes. Feedback since then NOT implemented.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned

I opened #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title for #43, except for node.created, because node.title presents its own additional challenges.

Gábor Hojtsy’s picture

So #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title answers Wim's question about the scope. To me looks like the scope of the issue is well covered by the proposed patch. @yched: any feedback on the patch itself?

yched’s picture

Reviewing.

Wim Leers’s picture

Reroll that fixes 2 out of 3 things that needed to be fixed according to #37. What hasn't been fixed yet, is a better doc than this:

+ * @param array $field_definition
+ *   The field definition array for the instance of the field on which the
smiletrl’s picture

Great work! This is totally a new pattern design to me. As bot is green, hardly to find some issues in code to me:)

Just one question:

+    $field = field_info_field($instance['field_name']);
+    $field_definition = $field->getFieldDefinition($instance);

I thought $field returned by field_info_field() is an field array, can we treat it as field object? But bot is green, I'm confused:(

yched’s picture

Assigned: effulgentsia » Unassigned

First of all, thanks @Wim for the awesome/massive work so far !

This is definitely going somewhere. I do think this new notion of "field definition array" needs some more conceptual polish (current name notably seems like a total confusing WTF to me), but I still need to articulate my thoughts on this - please bear with me, this patch is about a *major* turn in our concepts and data structures, takes some time to process and fully ponder :-)

Meanwhile, here are my remarks so far:

  1. DateTimeDatelistWidget::__construct()
    -    $instance['default_value_function'] = $this->defaultValueFunction();
    -    parent::__construct($plugin_id, $plugin_definition, $instance, $settings, $weight);
    +    $field_definition['default_value_function'] = $this->defaultValueFunction();
    

    Wow, the original code is *weird*, I'm not sure what this was doing (or assuming it was doing), but it most definitely can't do it with the new code :-).
    Opened #1989468: Weird messing with 'default_value_function' in date widgets ? about that, can we leave a @todo on that meanwhile ?

  2.    public function accessEditEntityField(EntityInterface $entity, $field_name) {
    -    return $entity->access('update') && field_access('edit', $field_name, $entity->entityType(), $entity);
    +    $instance = field_info_instance($entity->entityType(), $field_name, $entity->bundle());
    +    $field = field_info_field($field_name);
    +    $field_definition = $field->getFieldDefinition($instance);
    +    return $entity->access('update') && field_access('edit', $field_definition, $entity->entityType(), $entity);
       }
    

    Could be:

    $instance = field_info_instance($entity->entityType(), $field_name, $entity->bundle());
    $field_definition = $instance->getField()->getFieldDefinition($instance);
  3. Which questions: should there be a $instance->getFieldDefinition() shorthand ? something like return $this->getField()->getFieldDefinition($this); ?
    Patch currently has this all over the place:
    $field_definition = $field->getFieldDefinition($instance);
    

    Could turn into just

    $instance->getFieldDefinition(), 
    

    and does not need to bring a $field into scope if the current code doesn't have one.

    + If $field_name really is the name of a field in accessEditEntityField(), the old code was probably not working ? (field_access() currently takes a $field, not a $field_name)

  4. More generally on field_access():
    I'm wary of the patch turning field_access() from reasoning on a Field API $field to reasoning on a "generic" $field_definition. The reason for the change is that field access checks on entity view and entity forms are currently performed by widgets and formatters: FormatterBase and WidgetBase set '#access' = field_access('view[edit', $field) themselves. Since they don't receive a $field anymore, but just a $field_definition, patch switches field_access() over to that structure across all of core.
    But the result is that field_access() is not a Field API thing anymore.

    There could be two approaches here:

    A) "field access" remains a Field API thing for now,
    field_access() remains about $field structures and stays in field.module,
    formatters and widgets do not care about it, they generate render arrays, then the code that invoked them on Field API fields (field_attach_form() / field_attach_view()) calls field_access() and assigns #access.

    B) "field access" API becomes "entity property access" API,
    field_access() becomes "something else somewhere in EntityNG API" - but where ?
    Checking access rights and assigning #access remains the job of widgets and formatters.

    Approach B) sounds more solid to me (widgets and formatters can be trusted, rather than having external code needing to manually check access), but is also more involved. I don't know if the EntityNG folks had plans on that regard.
    If this is the road we want to take, I'd be fine with deferring the task of moving field_access() to Entity API to a followup, if we add an epxlicit @todo on field_access() noting that it's going to die.

  5. WidgetFactory:
       public function createInstance($plugin_id, array $configuration) {
         $plugin_definition = $this->discovery->getDefinition($plugin_id);
         $plugin_class = static::getPluginClass($plugin_id, $plugin_definition);
    -    return new $plugin_class($plugin_id, $plugin_definition, $configuration['instance'], $configuration['settings'], $configuration['weight']);
    +    $instance = $configuration['instance'];
    +    $field = field_info_field($instance['field_name']);
    +    $field_definition = $field->getFieldDefinition($instance);
    +    return new $plugin_class($plugin_id, $plugin_definition, $field_definition, $configuration['settings'], $configuration['weight']);

    This is not consistent with the change made on entity_reference Selection plugins, where it's left to the caller of PluginManager->getInstance() to provide $configuration['field_definition'] instead of $configuration['instance']. If we want widgets to work on non-Field API fields, then the whole instanciation flow needs to be $field / $instance free, no ?
    Also applies to FormatterFactory.

  6. FieldInstanceEditForm::validateForm()
          // Grab the field definition from $form_state.
          $field_state = field_form_get_state($element['#parents'], $field_name, LANGUAGE_NOT_SPECIFIED, $form_state);
          $field_definition = $field_state['field_definition'];
    
          // Validate the value.
          $errors = array();
          $function = $field_definition['module'] . '_field_validate';
          if (function_exists($function)) {
            $field = field_info_field($field_name);
            $function(NULL, $field, $this->instance, LANGUAGE_NOT_SPECIFIED, $items, $errors);
          }

    There is actually no need to fetch the $field_state here, this can be simplified to just:

          // Validate the value.
          $errors = array();
          $field = $form['#field'];
          $function = $field['module'] . '_field_validate';
          if (function_exists($function)) {
            $function(NULL, $field, $this->instance, LANGUAGE_NOT_SPECIFIED, $items, $errors);
          }
    
  7. Then a couple points about the content of the "field definition" itself (currently held in Field::getFieldDefinition()):
          'cache_id' => $instance ? $instance->uuid() : $this->uuid(),
    

    Seems a bit hackish. Looks like this is only ever used for some ad-hoc static cache in options_allowed_values() (allowed values for 'list' fields). Doesn't look like a need that should leak upstream to the genaral "definition".
    options_allowed_values() should be safe keing its static cache by 'field_name'.

  8.       'field_name' => $this->id,
    

    Should be id(), the $this->id property is only assigned during save(), so this wouldn't work on a runtime created Field structure.
    Also, 'field_name' is a thing from the past - Fields, being config entities, have an id instead of a name. $field['field_name'] only works as part of the temporary BC syntax, but is going to be replaced by $field->id() everywhere. I'd suggest 'id' or 'field_id' ?
    OTOH, given that we hope to use this "definition" format for base properties, and those have "names", then maybe 'name' might be the best choice - just not 'field_name' :-).

  9.       'settings' => $instance ? NestedArray::mergeDeep($this->settings, $instance->settings) : $this->settings,
    

    Not fully sure about the mergeDeep. "If a field setting and an instance setting have the same name, instance wins" is one thing, but deep merging scares me a bit. depending on what's in the settings (and we don't know that), it could mess things up. I'd stick with top-level merging for now.

  10. Still about 'settings':
    With the current code, depending on whether the "definition" was generated from a $field or a $field + $instance, $defintion['settings']['some_instance_setting'] might or might not be present. That's a real DX issue, and since at least CCK D6 we take extra care that all expected settings are present no matter what (filling default values), so that consuming code need not litter with isset() checks all over the place.
    When generating the definition against just a $field, we could add in default values for the instance settings . Not sure I fully ponder the exact impact on the semantics of $definition['settings']...
  11.       'required' => $instance ? $instance->required : 1,
    

    Hm, outside of a specific instance, I'd say "non required", no ?
    Also, this should be a boolean.

  12.       'columns' => $this['columns'],
    

    This should probably rely on EntityNG's getPropertyDefinitions() in the end. What the consuming code needs here are the names of the properties that compose a field value, not their SQL schema.
    Meanwhile this should be $this->getColumns(), though - let's avoid BC syntax within the Field / FieldInstance classes :-).

  13.       'entity_type' => $instance ? $instance->entity_type : NULL,
          'bundle' => $instance ? $instance->bundle: NULL,
    

    Hmm. So this is where Field API and EntityNG data structures are fundamentally different, and that's the part where trying to merge those into a single generic format shows cracks at the seam..
    Like for 'settings' above, this means any code receiving a $field_definition needs to account for the fact that maybe there are no 'entity_type' / 'bundle' properties to work with. That's a heavy DX impact in practice.
    So far this typically impacted code that received $field + $instance params, with $instance "being possibly NULL" - but it was usually documented in the function or hook signature. With this now being a single "definition" param, the uncertainty now spills to every code that now receives a "definition" - including code that so far had guaranteed $field and $instance structs to work with. Like, "no one is safe"...

    Crazy thought : what if the "field definition" contained no 'entity type' and 'bundle' entries altogether ?
    Code that runs in a context where there the entity type and bundles are known needs to find another way to get it.

Wim Leers’s picture

Assigned: Unassigned » effulgentsia

Assigning to effulgentsia to answer #50.

Note that I was just following effulgentsia's lead, I personally don't have enough insight into Field API to intelligently answer these excellent remarks :)

fago’s picture

Assigned: Unassigned » effulgentsia

@field-access:

Approach B) sounds more solid to me (widgets and formatters can be trusted, rather than having external code needing to manually check access), but is also more involved. I don't know if the EntityNG folks had plans on that regard.

We've got already Field::access() in EntityNG- entity field access that mimics field_access() but takes it to EntityNG. Thus, moving that check to $field->access() should do it.

Crazy thought : what if the "field definition" contained no 'entity type' and 'bundle' entries altogether ?
Code that runs in a context where there the entity type and bundles are known needs to find another way to get it.

Yes, that sounds like a good approach to me.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.php
@@ -493,6 +494,44 @@ public function getBundles() {
+   * @return array
+   *   An array with the following keys:
+   *   - cache_id
+   *   - field_name
+   *   - type
+   *   - label
+   *   - translatable
+   *   - settings
+   *   - cardinality
+   *   - required
+   *   - description
+   *   - columns
+   *   - module
+   *   - entity_type
+   *   - bundle

ok, in preparation for step 3) here a summary of what we have not in $field->getDefinition() of that right now:

cache_id (-> ?)
field_name (would be $field->getName())
cardinality (-> constraint?)
columns (-> getPropertyDefinitions())
module ( NA, is really needed here?)
entity_type (on render, $field->getParent->entityType())
bundle (on render, $field->getParent->bundle())
effulgentsia’s picture

Approach B) sounds more solid to me (widgets and formatters can be trusted, rather than having external code needing to manually check access)

Per #52, we can make that work, but should we? Maybe decoupling access has its merits: #1991248: Decouple field access checks from formatters and widgets.

effulgentsia’s picture

what if the "field definition" contained no 'entity type' and 'bundle' entries altogether ?

I agree. Let's see what breaks.

Status: Needs review » Needs work

The last submitted patch, formatters_field_definition-1950632-54.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
2.57 KB
128.5 KB

Should fix most of the warnings.

Status: Needs review » Needs work

The last submitted patch, formatters_field_definition-1950632-56.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
3.04 KB
129.91 KB

Should fix the rest of the fails.

Wim Leers’s picture

Nice! Much better indeed :)

Feel free to delegate work to me — I'd rather have you spend time on reviewing patches so this can keep moving forward than on grunt work. So: let me know when I can do something.

yched’s picture

@Wim: Thanks for the offer :-)
Well, the field_access() thing possibly needs some discussion in Portland, but there are other actionable points in #50.

effulgentsia’s picture

This reverts the API change of field_access(), but does not yet fix FormatterBase or WidgetBase accordingly, so if we have adequate test coverage, it should fail. I'll follow up with a fix to those.

effulgentsia’s picture

That's disturbing that #61 passed: means we're missing test coverage. Let's first try adding type hints to see if that's enough to cause failure.

Status: Needs review » Needs work

The last submitted patch, formatters_field_definition-1950632-62.patch, failed testing.

yched’s picture

It's not that we miss tests, it's that the BC ArrayAccess layer present on Field objects makes it so that the hook_field_access() implemenation used by the tests works the same when passed a Field or a "field definition" (the ['field_name'] is the same).
The type hints should make a difference though.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.31 KB
127.24 KB

Yay that the type hints are sufficient to make the tests robust. Now here's a fix.

effulgentsia’s picture

Small fix to FieldInstanceEditForm.

effulgentsia’s picture

This adds a getFieldDefinition() method to FieldInstance to complement the one in Field, and in the process, addresses some, but not all, of #50.

Status: Needs review » Needs work

The last submitted patch, formatters_field_definition-1950632-67.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
127.83 KB

Sorry for the noise. Per #50.12, I'd actually like to remove 'columns' from the definition, but for now, just wanting to get this green again.

Status: Needs review » Needs work

The last submitted patch, formatters_field_definition-1950632-69.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
690 bytes
128.66 KB

Alright, this one should be green. Working on remaining feedback.

effulgentsia’s picture

I think this resolves everything from #50 except for items 1 and 5.

yched’s picture

Status: Needs work » Needs review
+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -520,8 +518,7 @@
-      'columns' => $this['columns'],
-      'module' => $this->module,
+      'property_names' => array_keys($schema['columns']),

Yay, +1.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -34,12 +34,22 @@
+    $field_type_info = field_info_field_types($this->field_definition['type']);
+    $this->field_type_module = $field_type_info['module'];
   }

This info (field type module) is only needed within the getOptions() method. Couldn't we fetch it in there instead of adding a new class member ?

Especially since this notion of "the module providing the field type, which is therefore the module we're asking to for the the list of candidate values to display in option widgets" is very Field API centric, and it's not clear how it can be formulated when the widget is applied to a non-Field-API field.
But at least isolating the issue in getOptions() would seem best IMO.

+++ b/core/modules/options/options.moduleundefined
@@ -237,7 +237,8 @@
-  if (!isset($allowed_values[$field_definition['cache_id']])) {
+  $cache_id = serialize($field_definition);
+  if (!isset($allowed_values[$field_definition[$cache_id]])) {

A bit weird, but yeah, works, I guess :-)

[edit: removed duplicate snippets - dreditor...]

Status: Needs review » Needs work

The last submitted patch, formatters_field_definition-1950632-72.patch, failed testing.

effulgentsia’s picture

Status: Needs review » Needs work

I came up with another idea on how to do this. What if instead of having getFieldDefinition() that returns an array, we instead define the following interfaces:

MinimalFieldInterface:
- getName()
- getType()
- getSettings()
- getPropertyNames()
- isTranslatable()

FormattableFieldInterface extends MinimalFieldInterface:
- getLabel()

EditableFieldInterface extends FormattableFieldInterface:
- getDescription()
- getCardinality()
- isRequired()

Then, the FieldInterface that's in HEAD can extend MinimalFieldInterface along with the ConfigEntityInterface that it currently extends. Alternatively, we can rename FieldInterface to ConfigurableFieldInterface, and then rename MinimalFieldInterface above to FieldInterface.

InstanceInterface would then extend EditableFieldInterface along with ConfigEntityInterface.

Instead of receiving a $instance or $field_definition, formatters would receive a FormattableFieldInterface and widgets would receive a EditableFieldInterface.

When Field API needs to instantiate formatters and widgets, it can do so with a $instance object, as it currently does in HEAD, but because the formatters and widgets would type hint that parameter to FormattableFieldInterface or EditableFieldInterface, they'd be expected to conform to that interface.

For nonconfigurable fields, we can create a generic implementation of EditableFieldInterface that receives a TypedData definition and implements the methods above.

This would also allow for conditional upcasting as needed. For example, if a specific formatter wants to act specially on information available only in configurable fields (e.g., uuid()), it could do so within a if ($field instanceof ConfigEntityInterface) as long as it provides fallback behavior in the else part.

@yched, @fago: what do you think?

yched’s picture

So, yeah, using Interfaces instead of yet another notion of "field definition", related to but slightly different than the current EntityNG/propertyDefinition and FieldAPI/Field/FieldInstance (and array-based like it's still D6 :-p) was what I was toying around when I wrote in #50: "I think this new notion of 'field definition array' needs some more conceptual polish, but I still need to articulate my thoughts on this".

I wouldn't have thought of slicing it down the way #75 does (FormattableFieldInterface, EditableFieldInterface... - notably, not sure whether differentiating the two or having one extend the other makes sense), but right, passing either a $field or, when available (i.e entity type and bundle context are known) an $instance as a "field definition" could be handy.

At least, this is totally an approach we should consider IMO.

I'll be mostly afk til this w-e's sprint. Maybe we should discuss that over there ? (well, this not to shut down any discussion here meanwhile, of course - just to say I won't be able to chime much more than this "conceptual +1")

Wim Leers’s picture

I would also love to have an interface rather than a far less structured/more error prone array. Conceptual +1 :)

Wim Leers’s picture

We discussed this at DrupalCon Portland. We'll have an interface.

However, I think it should be decided by Entity API/Field API guys what that interface looks like. yched, swentel, fago, effulgentsia …, can you provide guidance? You only need to provide guidance and reviews, I'll work on the patch! :)

Wim Leers’s picture

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
124.91 KB

Re #78, will do, but for now, here's #72 rebased to HEAD. There were quite a few conflicts I had to resolve manually, and it's possible I didn't do all them correctly, but the patch will end up changing a lot for #78 anyway, so I'm not too worried about that. But it helps to have a starting point of something that applies.

Status: Needs review » Needs work

The last submitted patch, formatters_field_definition-1950632-80.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
131.57 KB

This still needs work, but is a start. It adds Drupal\Core\Entity\Field\FieldDefinitionInterface and makes FieldInstanceInterface extend it. It also updates a lot of the formatter and widget code, but not all of it.

Status: Needs review » Needs work

The last submitted patch, formatters_field_definition-1950632-82.patch, failed testing.

effulgentsia’s picture

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
134.61 KB

Throwing another patch at bot.

Status: Needs review » Needs work

The last submitted patch, formatters_field_definition-1950632-85.patch, failed testing.

effulgentsia’s picture

Switching to #2014163: Bot checks for #2021817 for bot checks. Will post a patch here when it passes, or at least doesn't fail so badly.

effulgentsia’s picture

Issue summary: View changes

Added related issues to the summary

effulgentsia’s picture

Title: Make formatters and widgets work on nonconfigurable fields » Create a FieldDefinitionInterface, decoupled from $field and $instance config entities, and use it for formatters and widgets
Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
FileSize
154.55 KB

This is ready for brand new reviews now, given the change in direction in #78. I'll post interdiffs from here on out. I also rewrote the issue summary to focus on what the patch does at an API level, and here's an updated issue title as well.

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

The last submitted patch, field-definition-1950632-88.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

#88: field-definition-1950632-88.patch queued for re-testing.

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

The last submitted patch, field-definition-1950632-88.patch, failed testing.

swentel’s picture

This looks very nice on a first glance. One quick question:

+++ b/core/modules/file/lib/Drupal/file/Plugin/field/widget/FileWidget.phpundefined
@@ -177,13 +176,19 @@ public function formElement(array $items, $delta, array $element, $langcode, arr
+      // Add properties needed by file_field_widget_value() and
+      // file_field_widget_process().
+      '#display_field' => (bool) $this->getFieldSetting('display_field'),
+      '#display_default' => $this->getFieldSetting('display_default'),
+      '#description_field' => $this->getFieldSetting('description_field'),
+      '#cardinality' => $this->getFieldSetting('cardinality'),

Wouldn't it be easier to pass along the field_definition into say '#field_definition' instead of creating all these new element properties ?

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
154.57 KB

Minor fixes for test failures.

Wouldn't it be easier to pass along the field_definition into say '#field_definition' instead of creating all these new element properties ?

Possibly, but then the element callbacks become coupled to knowledge about where the info comes from. For example, in ImageWidget::formElement(), #preview_image_style comes from a widget setting while #alt_field_required comes from a field setting. So, I think element callbacks not having access to $widget or $field_definition or even $field_settings, but only to the specific properties they need, keeps things cleaner. But I'm open to changing that here or in a follow up if there's opinions to the contrary.

yched’s picture

+1 on #93

Will try to review tonight / tomorrow.

amateescu’s picture

The Entity reference part of the patch looks good to me, even though there's some unrelated cleanup in there :)

effulgentsia’s picture

Re #95: thanks! What cleanup is unrelated?

amateescu’s picture

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteWidgetBase.php
@@ -50,24 +50,14 @@ public function settingsForm(array $form, array &$form_state) {
   public function formElement(array $items, $delta, array $element, $langcode, array &$form, array &$form_state) {
-    $instance = $this->instance;
-    $field = $this->field;
-    $entity = isset($element['#entity']) ? $element['#entity'] : NULL;
+    $entity = $element['#entity'];
 
     // Prepare the autocomplete path.
     $autocomplete_path = $this->getSetting('autocomplete_path');
-    $autocomplete_path .= '/' . $field['field_name'] . '/' . $instance['entity_type'] . '/' . $instance['bundle'] . '/';
-
-    // Use <NULL> as a placeholder in the URL when we don't have an entity.
-    // Most web servers collapse two consecutive slashes.
-    $id = 'NULL';
-    if ($entity && $entity_id = $entity->id()) {
-      $id = $entity_id;
-    }
-    $autocomplete_path .= $id;
+    $autocomplete_path .= '/' . $this->fieldDefinition->getFieldName() . '/' . $entity->entityType() . '/' . $entity->bundle() . '/' . $entity->id();

This part. We needed the 'NULL' string in there when the search string was the last part of the url, but now that it's sent as a query string, we can just omit the entity id from the url.

Now that I look at it again, there were probably legitimate cases where $element['#entity'] was not set, so I wonder what changed in the meantime.

effulgentsia’s picture

Ah, thanks. There's always a $element['#entity'], even on a node/add/TYPE page and the FieldInstanceEditForm::buildForm(). However, in both of those cases, $element['#entity']->id() might be NULL, so potentially that hunk needs some fixing and tests.

The assumption of $element['#entity'] existing is related to this issue though, in that we chose not to include 'entity_type' or 'bundle' in $field_definition (see #50.13).

yched’s picture

Right, I made those changes in #56, just after we stopped including entity_type and bundle in the $field_definition.
And yes, those changes reflect the fact that there is actually always an $entity - but right, I overlooked that its ->id() could be NULL...

amateescu’s picture

So how about leaving that part for another issue?

Edit: I know if ($entity.. is not needed anymore, I just wanted to leave out that entire hunk out of this patch :/

yched’s picture

@amateescu: that would be the issue where you intend to rewrite autocomplete widgets altogether ?
Works for me.

amateescu’s picture

Yep, that's the one: #1959806: Provide a generic 'entity_autocomplete' Form API element

This info is not yet posted there, but @dawehner's review from #7 made me think that we can rewrite the widgets and url handling to just reuse the generic form element that the issue is trying to introduce.

yched’s picture

This looks plain awesome !
Partial review, I stopped at Fieldinstance.php

  1. +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -82,7 +82,7 @@ function datetime_theme() {
    -function datetime_field_is_empty($item, $field) {
    +function datetime_field_is_empty($item, $field_type) {
    

    No biggie considering this moves to FieldItem::isEmpty() in #1969728: Implement Field API "field types" as TypedData Plugins but why is this change needed ?

  2. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteWidgetBase.phpundefined
    @@ -120,4 +118,12 @@ protected function getLabels(array $items) {
    +  protected function getSelectionHandlerSetting($setting_name) {
    +    $settings = $this->getFieldSetting('handler_settings');
    +    return isset($settings[$setting_name]) ? $settings[$setting_name] : NULL;
    +  }
     }
    

    Nitpick: missing empty line after last method.
    (+ missing @param & @return docblocks)

  3. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/Views/SelectionTest.phpundefined
    @@ -39,8 +39,8 @@ public function testSelectionHandler() {
    -    // Build a fake field instance.
    -    $field = array(
    +    // Create a field and instance.
    +    $field = field_create_field(array(
    

    New code should use the new entity_create('field_entity', array(...)) / entity_create('field_instance', array(...)).
    Same in EntityReferenceSelectionAccessTest, EntityReferenceSelectionSortTest

  4. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -512,6 +512,78 @@ public function getBundles() {
    +  public function getFieldName() {
    +    return $this->id;
    +  }
    

    I can't really figure out if we should be using ->id or ->id(). I think there are edge cases where the id property is not set for the whole span of the entity lifecycle.

    (same for FieldInstance::getFieldName())

  5. +++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
    @@ -512,6 +512,78 @@ public function getBundles() {
    +  public function getFieldSetting($setting_name) {
    +    $settings = $this->getFieldSettings();
    +    return isset($settings[$setting_name]) ? $settings[$setting_name] : NULL;
    +  }
    

    I'm a bit worried of the performance impact of each individual call to getFieldSetting() would cause an array merge. Should we have the getFieldSettings() method do the merge once and store the result in a protected class member ?

    (same for FieldInstance::getFieldSetting())

  6. +++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetBase.phpundefined
    @@ -75,11 +66,9 @@ public function form(EntityInterface $entity, $langcode, array $items, array &$f
         // Store field information in $form_state.
         if (!field_form_get_state($parents, $field_name, $langcode, $form_state)) {
           $field_state = array(
    -          'field' => $field,
    -          'instance' => $instance,
    -          'items_count' => count($items),
    -          'array_parents' => array(),
    -          'errors' => array(),
    +        'items_count' => count($items),
    +        'array_parents' => array(),
    +        'errors' => array(),
           );
           field_form_set_state($parents, $field_name, $langcode, $form_state, $field_state);
    

    It's possible that we have discussed this already but - why aren't we replacing 'field' and 'instance' by 'field_definition' in there ?

yched’s picture

yched’s picture

End of the review :

  1. +++ b/core/modules/field/field.api.phpundefined
    @@ -207,6 +207,16 @@ function hook_field_info_alter(&$info) {
    +function hook_field_info_cache_clear() {
    +  // Reset the static value that keeps track of allowed values for list fields.
    +  drupal_static_reset('options_allowed_values');
    +}
    

    Didn't look into it precisely, but this smells weird. Why isn't options_field_update_field() good enough anymore ?

  2. +++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.phpundefined
    @@ -91,14 +81,14 @@ public function view(EntityInterface $entity, $langcode, array $items) {
    +        '#field_name' => $this->fieldDefinition->getFieldName(),
    

    Minor: the field name is used twice, maybe extract it to a $field_name variable ?
    (like WidgetBase::form() does)

  3. +++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.phpundefined
    @@ -131,4 +121,26 @@ public function settingsSummary() {
    +  protected function getFieldSetting($setting_name) {
    +    return $this->fieldDefinition->getFieldSetting($setting_name);
    +  }
    

    Nitpick: missing empty line after last method, & missing @param / @return docblocks.

    (same in WidgetBase)

    + some formatters or widgets could also use a similar getFieldSetting*s*() shorthand for consistency ?

  4. +++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetBase.phpundefined
    @@ -221,9 +208,9 @@ protected function formMultipleElements(EntityInterface $entity, array $items, $
    +        '#field_name' => $this->fieldDefinition->getFieldName(),
    +        '#cardinality' => $this->fieldDefinition->getFieldCardinality(),
    

    There are existing $field_name and $cardinality variables in the method body.
    (+ the cardinality is accessed once more a couple lines below)

  5. +++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.phpundefined
    @@ -32,11 +32,11 @@ class Field extends FieldPluginBase {
    -  public $field_info = array();
    +  public $field_info;
    

    Nitpick of all nitpicks: while we're in here, we could remove the extra newline just below :-)

  6. +++ b/core/modules/file/lib/Drupal/file/Plugin/field/widget/FileWidget.phpundefined
    @@ -150,25 +149,25 @@ protected function formMultipleElements(EntityInterface $entity, array $items, $
    

    Minor: that method could be streamlined by extracting $field_settings = $this->fieldDefinition->getFieldSettings() upfront ?

  7. +++ b/core/modules/options/options.moduleundefined
    @@ -400,7 +400,16 @@ function _options_values_in_use($field, $values) {
    +  // When configuring a field instance, the default value is validated without
    +  // an entity, but options_allowed_values() and the callback it invokes
    +  // require an entity, because the result can depend on entity type, bundle,
    +  // and other entity data.
    +  if (!isset($entity)) {
    +    $ids = (object) array('entity_type' => $instance->entity_type, 'bundle' => $instance->bundle, 'entity_id' => NULL);
    +    $entity = _field_create_entity_from_ids($ids);
    

    Hm, actually FieldInstanceEditForm::formValidate() does have an $entity available, but currently doesn't pass it along to hook_field_validate(), but don't really see why it wouldn't ?

  8. +++ b/core/modules/views/lib/Drupal/views/Plugin/entity_reference/selection/ViewsSelection.phpundefined
    @@ -27,6 +28,20 @@
    +  /**
    +   * The entity object, or NULL
    +   *
    +   * @var NULL|EntityInterface
    +   */
    +  protected $entity;
    

    Is that the same "possibly NULL because of Field UI 'default value' validation" entity we're talking about ?
    Then maybe similarly, it doesn't really have to be NULL ?

yched’s picture

Edited #103 & #105, numbered the items

effulgentsia’s picture

This addresses #103 and #105, except for the following:

#103.1: Because WidgetBase::extractFormValues() calls _field_filter_items(), and if we change that to take a FieldDefinitionInterface, then we'd be implicitly passing it instance-specific settings, which we probably don't want hook_field_is_empty() implementations relying on.

#103.4: offsetGet() for 'field_name' returns id, not id(), so I implemented the same. They can be changed together if need be. Or, I can change it here for getFieldName() if you think that's needed, considering that offsetGet() will eventually be removed anyway.

#103.6: Per the issue summary, I can't think of a use case where it's a good idea for field_definition to be in $field_state.

#105.1: This patch changes options_allowed_values() to not use UUID for its static cache. So, the static cache needs to be cleared on other operations too, such as a delete followed by create of the same name. There's a test already in HEAD for exactly that.

#105.7 and #105.8: That deals with validation, and I didn't want to change the API of that in this issue. I'd be fine with doing so in a different issue though.

Status: Needs review » Needs work

The last submitted patch, field-definition-1950632-107.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

#103.1 : right, makes sense. Non-issue after #1969728: Implement Field API "field types" as TypedData Plugins anyway.

#103.4 ->id / ->id(): OK, I have a clearer recollection now.
->id is problematic for an $instance object, because it is only set when saving the entity - so will be NULL for an $instance created at runtime with entity_create('field_instance', $values).
Not a problem for a $field, though, because unlike with instances, $values['id'] is required for entity_create('field_entity', $values).
A field will always have an id (the "machine name"), but instances don't "really" have an id of their own (they have a composite one because of CMI).
Not fully intuitive, but in short: we're fine here.

#103.6 Agreed.

#105.1 Would the following work ?

function options_allowed_values(...) {
  if ($field_definition instanceof FieldInterface) {
    $cache_id = ... $field_definition->uuid ...
  }
  elseif ($field_definition instanceof FieldInstanceInterface) {
    $cache_id = ... $field_definition->field_uuid ...
  }
  else {
    $cache_id = ... $field_definition->getFieldName() ...
  }

Which in fact means that maybe a FieldDefinitionInterface::getFieldUniqueId() method would make sense ?
For config fields, it returns $field->uuid / $instance->field_uuid.
For base fields, it returns the "field name", which *is* a unique id for a base field ?
It's more or less the $field_definition['cache_id'] entry that was present in the initial, array-based iterations of this patch - 'cache_id' sounded really weird, but it addressed a need that might not be so uncommon ?

#105.7 and #105.8: Agreed, will be easier to cleanup as part of / after #1969728: Implement Field API "field types" as TypedData Plugins.

So, aside from hook_field_info_cache_clear() ("#105.1"), that I'd really wish we could find a way to avoid, we're very close !

yched’s picture

Oh, forgot the obvious: FieldDefinitionInterface documentation is still @todo :-)

effulgentsia’s picture

Issue tags: -sprint
FileSize
6.81 KB
156.39 KB

This hopefully fixes the test failures, and it removes the hook_field_info_cache_clear() by implementing hook_field_delete_field().

#110 still needs doing, but hopefully, this is now complete other than that.

effulgentsia’s picture

Issue summary: View changes

Rewrote issue summary.

effulgentsia’s picture

Didn't intend to remove that tag.

Status: Needs review » Needs work
Issue tags: +sprint

The last submitted patch, field-definition-1950632-111.patch, failed testing.

yched’s picture

hook_field_delete_field() is being deprecated in #2013939: Remove hook_field_[CRUD]_() in favor of hook_ENTITY_TYPE_[CRUD]() calls, let's use hook_entity_field_entity_delete() instead.
Strictly speaking, we should be reacting to field update as well ?

I still feel that FieldDefinitionInterface::getFieldUniqueId() would be more robust here (like allowing to work with runtime-created fields), but I can live with the current solution if you think it's not a good idea.

yched’s picture

Non blocking, but we might want to keep an eye on #2018685: Remove field_is_translatable()

yched’s picture

Also, #2004626: Make non-configurable field translation settings available in the content language settings means that isTranslatable() might get out of the FieldDefinitionInterface down the road (comments #9 / #10) over there.

fago’s picture

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
156.69 KB

This fixes the test failures, but includes a @todo. Any thoughts on whether that's acceptable for now, or if we need to resolve it in this issue, and if so, how?

hook_field_delete_field() is being deprecated in #2013939: Remove hook_field_CRUD_() in favor of hook_entity_TYPE_CRUD() calls, let's use hook_entity_field_entity_delete() instead.

Done.

Strictly speaking, we should be reacting to field update as well ?

We already are in HEAD. I left that alone though, since the patch in #2013939: Remove hook_field_[CRUD]_() in favor of hook_ENTITY_TYPE_[CRUD]() calls is already changing it to the new hook.

I still feel that FieldDefinitionInterface::getFieldUniqueId() would be more robust here (like allowing to work with runtime-created fields)

Let's discuss that in a follow up. Given that even with UUIDs, we still need to react to updates, I'm not yet convinced there's much benefit to adding that method, but perhaps other real or hypothetical use cases can convince me.

effulgentsia’s picture

FileSize
2.32 KB

Forgot to include the interdiff in #118.

yched’s picture

"even with UUIDs, we still need to react to updates"
True, agreed then.

About the @todo / need to refresh $mergedSettings when a setting changes :
Ouch, right... That would mean turning settings into a protected var, with a setter that clears $mergedSettings on each write. Patch scope ++...

Then what about ditching $mergeSettings and do something like:

public function getSettings() {
  merge each time
}

public function getSetting($setting) {
  if (isset($this->settings[$setting]) {
    return $this->settings[$setting]:
  else {
    return $this->field->settings[$setting];
}

At least the most commonly used getSetting() would still be acceptable performance wise ?

effulgentsia’s picture

Like so? Note that we can probably speed this up more by applying defaults during __construct() rather than save(), but I'm hesitant to do it in this issue, unless you think we should.

Also, array_key_exists() is supposedly fast now on PHP 5.4 when not using xdebug, so I refrained from the isset() || array_key_exists() technique we employed in some places in D7.

yched’s picture

Yup, that looks fine.
With #1969728: Implement Field API "field types" as TypedData Plugins, field_info_field_types() will be a wrapper around a PluginManager::getDefinition() - I don't think we want to worry about adding another cache on this data.

Aside from the doc @todo in FieldDefinitionInterface , I think we're RTBC here ?

effulgentsia’s picture

First pass at docs.

Status: Needs review » Needs work

The last submitted patch, field-definition-1950632-123.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
159.11 KB

Chasing HEAD.

yched’s picture

Status: Needs review » Needs work
+ * Fields can be defined entirely via code, entirely via UI configuration, or
+ * via a combination.

Not sure what this refers to. Are we talking about "there are base fields and config fields" ?
Then what would the combination be ?

+ * (...) Also, some field definitions can be relatively static
+ * while others can vary for each request based on runtime context.  Therefore,
+ * different classes can implement this interface, based on the needs of the
+ * field.

Confuses me as well :-)

+   * Field types are defined by modules that implement hook_field_info().

That's only true for config fields. EntityNG-wise, field types are exposed as TypedData types (although not all TypedData types are "field" types). Hm. Maybe we could explain what the "feild type" means (the kind of data it stores, the widgets and formatters that it can use...) rather than how they are defined ?

The rest of the doc looks great !

yched’s picture

Status: Needs work » Needs review

crosspost

effulgentsia’s picture

Man, this stuff is hard to write docs for while the related unification issues are still in progress. How's this?

effulgentsia’s picture

I couldn't put it down. Here it is with a little more polish. I think I'm done for now though, pending feedback.

fago’s picture

+++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinitionInterface.php
@@ -0,0 +1,164 @@
+ * from the corresponding "field_entity" configuration, as appropriate.

"field_entity" has been a bit confusing for me when reading this. Just having "field" configuration would have been less confusing I think - or should we start calling it "field definition" configuration entity?

+++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinitionInterface.php
@@ -0,0 +1,164 @@
+  public function getFieldName();

As this is a field definition interface, do we have to repeat the word "Field" in every method?

+++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinitionInterface.php
@@ -0,0 +1,164 @@
+  public function getFieldPropertyNames();

Can we replace that with entity-ng's getPropertyDefinition()? All field types will have it anyway.

yched’s picture

Will review asap.

For now, just notifying of #2019601: Rename config Field / FieldInstance structures ? :-/

amateescu’s picture

yched’s picture

The docs look great IMO. Explaining this while it's still in flux is indeed mighty tricky, but the current is way more than good enough IMO. This can always be refined later as we have more actual use cases.

In short, we should get this in asap IMO, and as far as I'm concerned, this is good to go.
So, temptatively marking RTBC - discussion, notably on the method names, can still proceed...

re @fago #130:

- The name of the entity types ('field_entity', 'field_instance') are up for debate in #2019601: Rename config Field / FieldInstance structures ?. Meanwhile, I think it's fine to leave the current names in there.

- getFieldName() / repeating "Field" in every method.
I guess that's a precaution when being an interface than different classes will implement in addition to their own. "Namespace" the methods to avoid clashing with unrelated business methods in the classes (that's what #1986802: rename PluginInspectionInterface::getDefinition() to getPluginDefinition() was about).
Now, given that those classes will probably very much be about "fields" anyway, maybe that's not as important as in that other issue, and this is being needlessly verbose.

My only concern would be getSettings() / getSetting() - for config $field and $instance structs, this is related to but subtly different from their 'settings' property.
$instance->getSettings() = $instance->settings + $field->settings
$field->getSettings() = $field->settings + "the default values for instance-level settings"
I'd say we can live that with @eff is willing to go that way.

- getFieldPropertyNames() / getPropertyDefinitions()
well, the two are related (the former would return the array keys of the latter), but it's not the same data.
Also, in order to call FieldItem::getPropertyDefinitions(), you need a FieldItem object, which you might not have if you're the "definition" object.
So, still deserves a separate method IMO.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

I guess that's a precaution when being an interface than different classes will implement in addition to their own.

Exactly. Since configurable field definitions implement both FieldDefinitionInterface and ConfigEntityInterface, one example of a clash is isTranslatable(). The others aren't clashes yet, but easily could be: both the getSettings() example from #133, but also potentially getName() and getType() in the future.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, field-definition-1950632-132.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.28 KB
161.55 KB

Fixed that new code.

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

The last submitted patch, field-definition-1950632-136.patch, failed testing.

amateescu’s picture

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

#136: field-definition-1950632-136.patch queued for re-testing.

A fail in Drupal\user\Tests\UserPasswordResetTest->testUserPasswordReset() can only be a random testbot fluke..

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the reroll and fix.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Very minor nit... but I don't think I should commit with the following issue unanswered...

+++ b/core/modules/field/field.info.incundefined
@@ -39,6 +39,8 @@ function field_info_cache_clear() {
+
+  Drupal::moduleHandler()->invokeAll('field_info_cache_clear');

Is this additional hook necessary? I can't find any implementations...

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.phpundefined
@@ -435,11 +427,8 @@ public function buildOptionsForm(&$form, &$form_state) {
-    // Provide an instance array for hook_field_formatter_settings_form().
-    $this->instance = $this->fakeFieldInstance($format, $settings);

Lovely! Less fakeness FTW

fago’s picture

Exactly. Since configurable field definitions implement both FieldDefinitionInterface and ConfigEntityInterface, one example of a clash is isTranslatable().

True - that sucks a bit, but that's how it is.

- getFieldPropertyNames() / getPropertyDefinitions()
well, the two are related (the former would return the array keys of the latter), but it's not the same data.
Also, in order to call FieldItem::getPropertyDefinitions(), you need a FieldItem object, which you might not have if you're the "definition" object.
So, still deserves a separate method IMO.

So the field property names are derived from the schema, however with the "field types" patch this will be derived via the field type plugin as well. So it's probably the same then?
Howsoever, having a separate method just for the names is some syntactic sugar which we can have, of course.
Apart from that, I think something as getPropertyDefinitions() belongs also into it - in particular in the lights of #2002134: Move TypedData metadata introspection from data objects to definition objects, but we can take care of it there also.

yched’s picture

@alexpott:
Oops, right, nice catch.
The actual implementations of hook_field_info_cache_clear() have disappeared, along with the documentation, in the interdiff from #111. So this invokeAll('field_info_cache_clear') is definitely a leftover.

Reroll just for that.

yched’s picture

Status: Needs work » Reviewed & tested by the community

@fago:

So the field property names are derived from the schema, however with the "field types" patch this will be derived via the field type plugin as well. So it's probably the same then?

In the current patch, yes (ConfigEntity)Field::getFieldPropertyNames() is derived from hook_field_schema()
AFAICT, after #1969728: Implement Field API "field types" as TypedData Plugins it will be still derived from the schema, that will live as a static schema() method in the FieldItem class.
(ConfigEntity)Field cannot call FieldItem::getPropertyDefinitions() because that method is not static, and we don't have any actual FieldItem object to invoke it on...

Pancho’s picture

Very nice. Didn't find anything except a typo that was introduced way before with entity_reference:
assertReferencable would correctly read assertReferenceable.
Not introduced here, so we don't want to lose more time and nerves here. Spun off to #2020405: Correct misspelling of 'referenceable'.

Docs are superb, I just stumbled over the "best-guess definition", which sounds more fuzzy than it is:

+ * Field definitions may fully define a concrete data object (e.g.,
+ * $node_1->body), or may provide a best-guess definition for a data object that
+ * might come into existence later.

How about "abstract definition" as referred to some lines further down, or maybe a bit more concrete "base definition" which IMHO nicely corresponds to the "base field".
Didn't want to change status, as this is a very minor nitpick.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Field API, -sprint, -Spark

The last submitted patch, field-definition-1950632-142.patch, failed testing.

yched’s picture

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

grumblecantreproducegrumble...

#142: field-definition-1950632-142.patch queued for re-testing.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Re #144, thanks for the review. Let's see what names get chosen in #2019601: Rename config Field / FieldInstance structures ?, and then we can refine that doc.

The reason I went with "best-guess" for now, is that some of the methods, like isFieldRequired() and getFieldLabel() really are guesses in a way. In the absence of a concrete field, we just assume that FALSE and the field name are what make sense to return there (that assumption has come from years of Views experience, primarily). But I agree that replacing that with better terminology would be good once we have it.

alexpott’s picture

Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed d9f7b3a and pushed to 8.x. Thanks!

... can't add "Change notice" to title as it's too long :)

Wim Leers’s picture

Yay! Thanks, Alex, Alex and Yves! :) Now we can start working on #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title! :)

yched’s picture

Awseome ! Thanks @effulgentsia !

andypost’s picture

@yched @effulgentsia Does this means that we should re-roll all patches in #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API to replace $field->id() for $field->getFieldName() because we already do $field['field_name'] ==> $field->id()?

alexpott’s picture

Issue tags: -Needs change record, -sprint, -Spark

Re #151 I don't think so because I think getFieldName() and id() are going to always be equivalent for a field entity … what's nice about getFieldName() is that it means you don't have to know whether you are working with an instance or a field… in these conversions we always know...

effulgentsia’s picture

Title: Create a FieldDefinitionInterface, decoupled from $field and $instance config entities, and use it for formatters and widgets » Needs change notice: Create a FieldDefinitionInterface and use it for formatters and widgets
Issue tags: +Needs change record

Right. In that issue, we're dealing with ConfigEntity CRUD, so using EntityInterface::id() is appropriate. We'll want calling code using getFieldName() wherever it only cares about the thing as a field, not as a config entity.

This issue still needs a change notice (or possibly several). I'll try to get to that soon.

tim.plunkett’s picture

Title: Needs change notice: Create a FieldDefinitionInterface and use it for formatters and widgets » Change notice: Create a FieldDefinitionInterface and use it for formatters and widgets
fago’s picture

yched’s picture

yched’s picture

Posted a patch in #2050801: Unify handling of default values between base and configurable fields, adds FieldDefinitionInterafce::getFieldDefaultValue()

yched’s picture

It seems there's a bug in the implementation of getFieldSetting() / getFieldSettings() in Field config entities : #2060003: Wrong precedence in Field::getFieldSetting[s]() when setting appears in both field and instance

yched’s picture

catch’s picture

effulgentsia’s picture

Status: Active » Needs review
yched’s picture

Title: Change notice: Create a FieldDefinitionInterface and use it for formatters and widgets » Create a FieldDefinitionInterface and use it for formatters and widgets
Status: Needs review » Fixed
Issue tags: -Needs change record

Works for me. Thanks @effulgentsia !

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

Anonymous’s picture

Issue summary: View changes

Updated summary for #111.

yched’s picture

klonos’s picture

Issue summary: View changes
Related issues: +#2134967: FieldDefinitionInterface should include a getTargetEntityTypeId()

FYI: we now have a section where we can add related issues as metadata :p