Commit credits so far : yched, fago, plach, effulgentsia, swentel, amateescu
(hope I didn't forget anyone)

- Work takes place in the 'field-types-as-datatypes-1969728' branch in the sandbox.
- #1992138: Helper issue for "field types as TypedData Plugins" is a testbot playground helper issue.

Overview

This patch moves the "field type" API from "pseudo hooks" to plugins / OO classes.
Most of all, and according to the plan summarized in #1949932-1: [META] Unify entity fields and field API, this patches unifies Field API "field types" and Entity NG "typed data": the "field type" plugin classes, implementing the behavior of the field type previously held in the various hook_field_[op]() implementations, are the *same* classes as TypedData's "field item" classes (that all field types currently need to implement anyway already).
In short, the logic about the field type (which is mostly about describing / massaging the values) is held in methods on the "field value" objects.

- solves DX WTFs of currently having to write on top of two entirely different APIs and data structures to provide a new field type
- stops the constant back and forth converstions of "field values" between TypedData and dumb arrays when going in and out of the Field API functions.
- brings field-level validation to REST entity writes.

Patch summary

- Introduces a new "configurable field type" plugin type (ConfigFieldTypePluginManager).
Discovery is annotations based.
The "configurable field types" are added as "data type" plugins through a derivative (field_data_type_info() / ConfigFieldDataTypeDerivative)

- The old hooks are migrated as such:
hook_field_info() -> Plugin annotations
hook_field_schema() -> ConfigFieldItemInterface::schema() (static method)
hook_field_settings_form() -> ConfigFieldItemInterface::settingsForm()
hook_field_instance_settings_form() -> ConfigFieldItemInterface::instanceSettingsForm()
hook_field_is_empty() -> ComplexDataInterface::isEmpty() (already exists)
hook_field_presave() -> FieldItemInterface::preSave()
hook_field_insert() -> FieldItemInterface::insert()
hook_field_update() -> FieldItemInterface::update()
hook_field_delete() -> FieldItemInterface::delete()
hook_field_delete_revision() -> FieldItemInterface::deleteRevision()
hook_field_validate() -> TypedDataInterface::getConstraints() (field validation moves on top of the existing Entity validation w/ Symfony constraints)
hook_field_load() -> PrepareCacheInterface (extends ConfigFieldItemInterface) ::prepareCache()
hook_field_prepare_view() -> removed, in favour of prepareView() on Formatters.
hook_field_prepare_translation() -> Dropped - was only related to the old (node-only) translation model. Also see #2018707: Do we still need to support hook_field_prepare_translation() ?

- Patch migrates the three "text" field types, and provides a "legacy" BC layer that keeps the other core field types working:
FieldTypePluginManager's discovery includes a LegacyFieldTypeDiscoveryDecorator for the field types still exposed in hook_field_info().
LegacyConfigField / LegacyConfigFieldItem plugin classes proxies method calls to the old callbacks.

TODO

- TextProcessed::setValue() raises an exception when something tries to set the computed 'processed" property of a FieldItem. - That's exactly what invokeFieldMethod() & friends do when they need to work on BC entities, though.
So for now I left that "throw new ReadOnlyException" line commented out :-/
added as point to the follow-up #2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property
- @fago left a "@todo: Pass $violation->arrayPropertyPath as property path." in WidgetBase, but I'm not sure what he meant ;-) - created follow-up #2027059: Improve the documentation of WidgetBase::errorElement() for mapping violation property paths to form elements

Related patches / blockers

Patch includes:
- a small subpart of #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest, to fix testNestedFieldForm() / field_test_entity_nested_form() by having it run on the NG entity_test rather than test_entity - no biggie if this gets committed as part of this patch IMO

Followups

#2014671: [META] Convert all field types to plugins
#2017711: Create FieldTypePluginManager::getOptions()
#2019601: Rename config Field / FieldInstance structures ?
#2023465: Allow validation constraints to add in message replacements
#2023473: Merge FieldItem::insert() and FieldItem::update() on fields into preSave()
#2023563: Convert entity field types to the field_type plugin
#2026315: Getting a translated field value sometimes fails
#2026323: Add field invocation helpers and test them
#2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property
#2027059: Improve the documentation of WidgetBase::errorElement() for mapping violation property paths to form elements

CommentFileSizeAuthor
#184 interdiff-172-181.txt33.2 KBeffulgentsia
#181 d8_field_types.patch314.09 KBfago
#180 d8_field_types.patch.interdiff.txt15.49 KBfago
#172 d8_field_types-1969728-172.patch304.74 KBeffulgentsia
#172 interdiff.txt1.39 KBeffulgentsia
#170 d8_field_types-1969728-170.patch304.67 KBeffulgentsia
#163 d8_field_types-1969728-163.patch304.66 KBeffulgentsia
#161 d8_field_types.patch305.51 KBfago
#146 field-types-as-datatypes-1969728-146.patch304.9 KByched
#146 interdiff.txt22.91 KByched
#148 field-types-as-datatypes-1969728-148.patch303.77 KByched
#144 field-types-as-datatypes-1969728-144.patch295.82 KByched
#144 interdiff.txt25.57 KByched
#142 field-types-as-datatypes-1969728-142-do-not-test.patch283.66 KByched
#142 interdiff.txt591 bytesyched
#141 field-types-as-datatypes-1969728-141.patch283.75 KByched
#141 interdiff.txt2.79 KByched
#140 field-types-as-datatypes-1969728-140.patch282.84 KByched
#140 interdiff.txt3.85 KByched
#136 field-types-as-datatypes-1969728-136.patch281.99 KBeffulgentsia
#129 field-types-as-datatypes-1969728-129.patch284.43 KByched
#126 field-types-as-datatypes-1969728-126.patch284.42 KByched
#126 interdiff.txt3.22 KByched
#121 field-types-as-datatypes-1969728-121.patch284.46 KByched
#118 field-types-as-datatypes-1969728-118.patch284.8 KByched
#118 interdiff.txt0 bytesyched
#108 field-types-as-datatypes-1969728-108.patch283 KByched
#108 interdiff.txt6.19 KByched
#108 interdiff_2.txt2.8 KByched
#102 field-types-as-datatypes-1969728-102.patch280.54 KByched
#102 interdiff.txt15.72 KByched
#100 field-types-as-datatypes-1969728-100.patch265.83 KByched
#100 interdiff.txt1.77 KByched
#98 field-types-as-datatypes-1969728-97.patch263.94 KByched
#98 interdiff.txt21.33 KByched
#96 field-types-as-datatypes-1969728-96.patch254.69 KByched
#96 interdiff.txt10 KByched
#94 field-types-as-datatypes-1969728-94-do-not-test.patch257.59 KByched
#94 interdiff.txt11.79 KByched
#95 field-types-as-datatypes-1969728-95-do-not-test.patch252.59 KByched
#95 interdiff.txt8.83 KByched
#74 field-types-as-datatypes-1969728-74.patch257.04 KByched
#74 interdiff.txt2.44 KByched
#72 field-types-as-datatypes-1969728-72.patch280.42 KByched
#69 interdiff.txt1.47 KBplach
#65 field-types-as-datatypes-1969728-65.patch280.51 KByched
#65 interdiff.txt1.36 KByched
#64 field-types-as-datatypes-1969728-64.patch280.33 KByched
#64 interdiff.txt3.67 KByched
#60 field-types-as-datatypes-1969728-60.patch280.31 KByched
#57 field-types-as-datatypes-1969728-58.patch280.04 KByched
#56 field-types-as-datatypes-1969728-56-do-not-test.patch267.42 KByched
#41 field-types-as-datatypes-1969728-41-do-not-test.patch268.69 KByched
#40 field-types-as-datatypes-1969728-40-do-not-test.patch267.64 KByched
#35 field-types-as-datatypes-1969728-35-do-not-test.patch267.94 KByched
#28 field-types-as-datatypes-1969728-28.patch139.43 KBswentel
#28 interdiff.txt3.8 KBswentel
#24 field-types-as-datatypes-1969728-24.patch139.55 KBswentel
#24 interdiff.txt1.3 KBswentel
#20 field-types-as-datatypes-1969728-20.patch139.05 KBswentel
#20 interdiff.txt978 bytesswentel
#17 field-types-as-datatypes-1969728-18.patch139.31 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Title: Move field types to classes » Move field types to classes / Plugins

At least let's mention "Plugins" in the title, even though which plugin type it's going in to be in the end is not fully clear yet.

fago’s picture

We've the following "hooks" already covered by the entity field classes:

  • hook_field_is_empty()
  • hook_field_validate() would be already covered by typed data validation, so instead of implementing this I think we should migrate this over. Maybe, we could do that in a pre-requisite issue first?

The following would be reasonable addition to the current FieldInterface I think.
hook_field_presave()
hook_field_insert()
hook_field_update()
hook_field_delete()
hook_field_delete_revision()
hook_field_schema() (could be removed if we manage to generate it from metadata later)
hook_field_prepare_translation() (should be removed probably)

While initially I've been thinking about putting them in a "ConfigurableFieldInterface" I see no reason why we shouldn't put them directly in the entity API's FieldInterface - let's better avoid unnecessary divergences.

Instead of hook_field_info() I'd see us having our own field plugin type + a single derivate "data type" plugin implementation for all field types that adds in field types as data types automatically.

What's left then is

hook_field_load() (acts on multiple entities)
hook_field_prepare_view() (acts on multiple entities)

which, could go in an optional "multiple handler" as discussed during the hangout - as in most cases they are not needed anway?

yched’s picture

The moment we start putting those methods on FieldInterface, the question becomes : how do we invoke them ?

Roughly, in current HEAD, hook_field_[op]() are invoked within field_attach_[op](), that does :
"invoke [field_type_module]_field_[op]() on all languages on all configurable fields on the bundle (field_info_instances($entity_type, $bundle)"

If those live on FieldInterface independantly of the notion of configurable or hardcoded "field", then the field_attach_[op]() functions go away, and we put this "foreach (field, language) {invoke $field->[op]()}" logic at the places in Entity code that currently call field_attach_[op]() ?

Also, regarding "putting those methods on FieldInterface":
It's not clear to me how would they be overriden / implemented by data type plugins then ? I thought we wanted "being a field type" to simply consist of providing one plugin class, that would be an enhanced version of the current 'field item class' (IntegerItem, TextItem...). But those currently extend FieldItemBase, not Field / FieldInterface ?

fago’s picture

If those live on FieldInterface independantly of the notion of configurable or hardcoded "field", then the field_attach_[op]() functions go away, and we put this "foreach (field, language) {invoke $field->[op]()}" logic at the places in Entity code that currently call field_attach_[op]() ?

I'd think so.

But those currently extend FieldItemBase, not Field / FieldInterface ?

Right. So for invoking the behaviour I think it makes most sense to have them on field level, not field item level. I could see us forwarding preSave/insert/update/schema.. calls to the individual items. So then, we could have a default Field class implementation which mostly forwards and people can care about implementing their stuff in the item class. While if you really need to, you could customize the Field class also.

fago’s picture

Just ran over a use-case for having all the CRUD-callbacks in the general field interface - the path alias field of terms/nodes. Regardless whether that's configurable or not, something like that should be doable without having to off-load the logic in regular entity-CRUD hooks. See http://drupal.org/node/1818560#comment-7306192.

yched’s picture

So this would mean adding the methods in both FieldInterface and FieldItemInterface. Sucks a bit for phpdoc duplication, but right...

So currently hook_field_[op]() receive $items (the array of multiple values). This forces them to iterate in it themselves, but performs better in cases where the callback needs to perform db requests or the like : one request for all values vs. one request per value.

So let me check if I got things right :
- We add base methods in Field, they contain the current code in field_default_[op](), and do foreach ($item) {proxy to FieldItem::op()}
- you override the methods in your MyDataTypeItem (extends FieldItemBase - 'class' entry in the data_type plugin definition) and do the item-per-item version there.
- If you rather need to operate on all items as a whole, you declare a MyDataTypeField (extends Field - 'list class' entry in the data_type plugin definition) and override the method here, so that it operates on all items instead of proxying to the methods of the FieldItem.
*But*: you just want to override the foreach() part of the parent, we still need the "field_default_[op]()" part to run.
So I guess this requires some additional splitting of the parent methods in Field.

Side note: data type plugins are currently still hook-info based ? Is there a plan to move those to annotations ?

fago’s picture

*But*: you just want to override the foreach() part of the parent, we still need the "field_default_[op]()" part to run. So I guess this requires some additional splitting of the parent methods in Field.

I'm a bit confused by the "field_default_[op]()" thing. It's either that we have per-plugin callbacks - thus no default-op, or a default op for a given op, right? Or are there cases where field API has both?

As default-op functionality is something not being field type specific it should be implemented generally, thus either built in in the entity API or contributed by field module I think.

Looking at the code I see only 2 default callbacks left. field_default_validate() can be replaced by entity validation, field_default_prepare_translation() should go away anyway - so I do not see a problem here. So I guess we could just leave them as they are now and work on removing them in their own issues?

Apart from the default-thing #6 sounds good to me!

Side note: data type plugins are currently still hook-info based ? Is there a plan to move those to annotations ?

Definitely. It's just that we had to implement then below Core\Entity and there is no solution for placing the plugins properly below Core\Entity with annotations yet. Anyway, I guess we should start moving that to annotations sooner rather than later.

Berdir’s picture

yched’s picture

Forgot in the list of current "field type hooks" :
hook_field_settings_form()
hook_field_instance_settings_form().
Added them to the OP.

Not sure where those would go. They are used when configuring the field definition, this is not about field values, and there is no entity or field value objects to work with at this point. Thus they are difficult to put on Fieldinterface...

yched’s picture

More generally : do EntityNG data types have a notion of 'settings' of their own right now ?

yched’s picture

Still navigating this and trying to figure out how this can be laid out.

Topic of the moment:
hook_field_info(), hook_data_type_info(), and is the result : '"they just merge" ?

There's not a 1:1 relationship between field API "field types" and 'field item classes' right now:
'text' and "text_long' both use TextItem, 'list_integer' and 'number_integer' both use IntegerItem...
AFAICT, that's because we're in an intermediate step where field types "point to" FieldItem classes but the two are still different things, the latter being just how the former is "seen" by EntityNG.
If we move to "a field type *is* a FieldItem class", then two different types mean two separate classes, right ?

Then, amongst all the classes that implement FieldItemInterface, there seem to be those that are intended to be used for "configurable fields" and the other ones. Existing classes like StringItem, LanguageItem, UriItem... are currently only used for base properties, we don't want to show them in Field UI's "add new", right ? There is an EntityReferenceItem for $node->uid, and a ConfigurableEntityReferenceItem used to back entity_reference Field API fields. We obviously don't want to present both in the UI.

So, no 1:1 relationship between "Configurable field" types (i.e. former Field API field types) and 'field item classes' anyway.

"Configurable field" types seem a bit "special" :
- They need to be recognized somehow as the only ones that are listed in Field UI's "add new".
Where would this be denoted :
A boolean property in the hook_data_type_info() entries ?
Or they're exposed in a different hook (i.e. they are a different plugin type) ?
- They need to provide some settingsForm() / instanceSettingsForm() methods somehow. Static methods on the FieldItem class ?
So does this mean there should be a ConfigurableFieldItemInterface that extends FieldItemInterface ?
- They need to define a "schema" for their values (in the sense of the current hook_field_schema(), because unlike base fields, their storage is not created by the entity type. Not sure whether this should go in extra entries in their getPropertyDefinitions(), or in a new, dedicated method.
- Not sure yet to which extent they can work without accessing the split $field and $instance definition structures that will remain the base of the "configurable field" config storage. #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets works on a minimal $definition format that could be suitable for widgets & formatters, but field types are a different story.

As you see, I'm still a bit foggy on this :-)

yched’s picture

Not sure yet to which extent [FieldItem classes for "configurable field" types] can work without accessing the split $field and $instance definition structures that will remain the base of the "configurable field" config storage. #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets works on a minimal $definition format that could be suitable for widgets & formatters, but field types are a different story.

Which would in turn mean that there would be two separate sets of FieldItem classes:
- those that can be used for configurable fields,
- and those that can be used for base properties,
which is probably not what we aimed for.

andypost’s picture

Suppose better to get rid of "duplicate" hooks

  • hook_field_is_empty()
  • hook_field_validate()

Then

  • hook_field_presave()
  • hook_field_insert()
  • hook_field_update()
  • hook_field_delete()
  • hook_field_delete_revision()

And finally find a way for settings, load and prepare

fago’s picture

If we move to "a field type *is* a FieldItem class", then two different types mean two separate classes, right ?

I think so as well, yes.

So, no 1:1 relationship between "Configurable field" types (i.e. former Field API field types) and 'field item classes' anyway.

Yes, we probably want to keep some FieldItem classes non-configurable like UUID field, at least initially. So we'd need a way to to tell field API that a certain field should be exposed for configuration. E.g., we could support a 'hidden' flag in the plugin info?

hook_field_info(), hook_data_type_info(), and is the result : '"they just merge" ?

I'd suggest doing a separate plugin type that allows registering "configurable field types" separately + a derivative plugin that exposes every configurable field types as "data type" also. This would solve the problem of "what should we show in field UI" screens also.

More generally : do EntityNG data types have a notion of 'settings' of their own right now ?

Yes, there is a 'settings' key with custom-content per class in the definition.

- Not sure yet to which extent they can work without accessing the split $field and $instance definition structures that will remain the base of the "configurable field" config storage. #1950632: Make formatters and widgets work on nonconfigurable fields works on a minimal $definition format that could be suitable for widgets & formatters, but field types are a different story.

I'd assume that the field API $field structure could move towards keeping just an entity field definition. $instance would stay separately to store instance specific settings, which get added in at the entity field definition's method getDefinition() and or might be accessed directly by field UI/API.

- They need to define a "schema" for their values (in the sense of the current hook_field_schema(), because unlike base fields, their storage is not created by the entity type. Not sure whether this should go in extra entries in their getPropertyDefinitions(), or in a new, dedicated method.

I'd prefer a new method here, as that would make it easier to replace the method with something that generates the schema based upon the definition. I think we should move that method to the Entity field interface as with #1497374: Switch from Field-based storage to Entity-based storage it would be needed by the entity storage and not by field API any more.

- They need to provide some settingsForm() / instanceSettingsForm() methods somehow. Static methods on the FieldItem class ? So does this mean there should be a ConfigurableFieldItemInterface that extends FieldItemInterface ?

- They need to provide some settingsForm() / instanceSettingsForm() methods somehow. Static methods on the FieldItem class ? So does this mean there should be a ConfigurableFieldItemInterface that extends FieldItemInterface ?

Good question. What we do here e.g. for conditions is that we extend from FormInterface and re-use that for the configuration form also. E.g. see the NodeType class/condition. Obviously that won't work with fields that require an instance and field settings form. But still, couldnt we just add settingsForm() / instanceSettingsForm() methods based as non-static methods based on a ConfigurableFieldItemInterface ? We'd have to be able to instantiate the plugin with the provided default settings, but that shouldn't be a problem and is what we do also when adding entities (entity add form receives empty $entity). From a typed data API perspective it's perfectly fine to instantiate a data object without values as that's what we do when we want to work with the metadata only. Thus, I don't think those methods have to be static.

Thus, the plugin could just configure it internally by writing the respective settings, while field settings could directly go into $this->definition['settings'] and be extracted from there when field API wants to save it. Instance settings are stored separately so I'd better keep them separated from start - e.g. they could be written in the instance config entity ($this->instance ?). I guess a getter like $field->getInstance() would make sense also, which only works if the field has enough context (i.e. the parent is set).

Which would in turn mean that there would be two separate sets of FieldItem classes:
- those that can be used for configurable fields,
- and those that can be used for base properties,
which is probably not what we aimed for.

I think that's ok as long as the non-configurable field items are what is required by various other systems like rendering and forms. ConfigurableFieldItems are just handy for users to configure field storage via the UI, while for widgets and formatters it doesn't matter whether the field were configurable or not.

yched’s picture

FYI: working on the "TypedData-based" plan laid out above - not as fast as I wished though...

yched’s picture

Title: Move field types to classes / Plugins » Implement Field API "field types" as TypedData Plugins

Retitling

yched’s picture

Initial patch. Very rough yet, posting to have a first testbot feedback.
More detailed comments/questions later on.

In short:
- Patch does as suggested in #14 : "configurable field type" plugins are discovered through a dedicated, annotations-based 'field type' plugin type (FieldTypePluginManager), and added as "data types" plugins through the Drupal/field/Plugin/DataType/ConfigurableFieldDataType derivative.
We'll discuss that, I don't think this is the right approach in the end.

- The presave(), delete(), ... methods are invoked from EntityStorageController (final location & shape of the corresponding code is still TBD). The multilingual logic is very probably totally incorrect.

- Class names are totally non-settled

- Converts the three "text" field types. The other core field types are handled through BC classes (LegacyField / LegacyFieldItem), that route method calls to the former callbacks.

- All of the old "field type" callbacks listed in the OP are handled, except:
hook_field_validate() - for now I have no clue how we want to plug that to the Entity validation flow and Symfony validation API.
hook_field_load() - acts on multiple entities
hook_field_prepare_view() - acts on multiple entities
hook_field_prepare_translation() - probably on its way out, but still called in current HEAD
Those are still called through the corresponding field_attach_*() functions.

yched’s picture

Status: Active » Needs review

Pushed the code to the 'field-types-as-datatypes-1969728' branch in the sandbox
(and removed the previous 'field-plugins-field-type-1969728' branch, that contained my initial, non DataType based work)

Anonymous’s picture

Issue summary: View changes

added missing hooks

swentel’s picture

Let Drupal install - see interdiff. Merged HEAD as well in sandbox.

swentel’s picture

Status: Needs review » Needs work

Hrm, works locally though :/

- edit - minimal works, standard fails

fago’s picture

:( @fail

The patch looks already pretty good already! I think it would ease doing #1497374: Switch from Field-based storage to Entity-based storage quite a lot also.

swentel’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
139.55 KB

This should install

yched’s picture

Status: Needs review » Needs work

A brief debug seemed to imply that the problem is that nothing clears the plugin definition caches when a module gets enabled. field_info_cache_clear() should probably call clearCachedDefinitions() on the FieldTypePluginManager (and maybe also on the TypedDataManager - having two separate plugin types & discoveries sucks IMO).

Can't roll another patch myself before tomorrow, though.

yched’s picture

Heh, crosspost with #24 :-)
I think the fix is more along the lines of #25 though.

swentel’s picture

@yched of course, just wanted to see how the testbot behaves :)

swentel’s picture

Status: Needs work » Needs review
FileSize
3.8 KB
139.43 KB

So yeah, clearing the typed data definition works fine.
Also fixes a couple of 1000 notices.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-1969728-28.patch, failed testing.

yched’s picture

Opened #1992138: Helper issue for "field types as TypedData Plugins" to go wild with the tesbot without polluting this issue too much.

yched’s picture

Issue summary: View changes

Updated for the current approach

yched’s picture

Note: branched validation work in the separate field-types-as-datatypes-1969728-validate branch in the sandbox

yched’s picture

Note: Patch in #1992138-40: Helper issue for "field types as TypedData Plugins" is down to "1 fail, 4 exceptions"
(well that's with explicitly commenting out some tests for stuff that's not ported yet :-), mostly validation)

As I wrote over there, I won't be able to push this further for the next couple days, help would be welcome for the remaining tests:
- EntityReferenceAutoCreateTest: probably needs someone familiar with entity_ref autocreate widget
- EntityTranslationSyncImageTest: not sure how to deal with that, FieldTranslationSynchronizer::synchronizeItems() is pretty tied to dealing with field values as an array keyed by language, probably needs some refactoring :-/

plach’s picture

[wrong issue]

yched’s picture

yched’s picture

Issue summary: View changes

link to the testbot helper issue.

yched’s picture

Issue summary: View changes

add 1st followup

yched’s picture

Issue summary: View changes

typo

yched’s picture

Status: Needs work » Needs review
FileSize
267.94 KB

Not fully done yet, but it's probably safer to start collecting reviews.
I'll try to update the OP with a summary of the current status.

In short:
- hook_field_attach_prepare_view() not done yet
- hook_field_attach_prepare_translation() not done yet (that's the D6 translation model, likely to go away entirely anyway ?)

There are still known fails, so I'm not running the bot here so that patch sticks to "needs review".
See #1992138-96: Helper issue for "field types as TypedData Plugins" for the corresponding test runs.

yched’s picture

Posted an initial patch summary in the OP.

yched’s picture

So there's an issue with field purge...
Data purging relies on loading data for deleted fields, and running prepareCache() and delete() on them. Current patch lets the caller of field_attach_load(), invokeFieldItemPrepareCache() and invokeFieldMethod() inject an arbitrary $instance to indicate "run on *this* field.
But that's not enough since $entity->get($field_name) fails because the field, being "deleted", is not part of $entity->getPropertyDefinitions()

yched’s picture

One solution would be to change field storage's hook_field_storage_field_load() so that they return the loaded Field values instead of adding them to the $entities.
Then field purge could call the storage load directly, and do its job on the Field value object directly, instead of going through the "regular" invokeFieldItemPrepareCache() and invokeFieldMethod(), that work on $entity->get($field_name) and thus rely on the "official" list of getPropertyDefinitions().

This organization would make much sense IMO, since EntityNG basically means "you can't put arbitrary stuff in $entity and access it through the regular ->get() syntax". So we need a way to work on Field values directly, instead of having them already tied to entities.
"Your job, as a field values loader, is to generate Field value objects, not to mess with entities".

But:
- this would add even more to the scope of the patch, that is nearing 300k already.
- this seriously collides with the field/entity storage refactors that are in progress in other issues

Wondering if we could pitch the idea of disabling field purge for now and come back to it when the various issues have settled... :-/

yched’s picture

Or, other approach: find a way to somehow inject stuff in getPropertyDefinitions() at run time...
The problem of getPropertyDefinitions() being strictly limited to the official set of fields that currently exist in the active config is that it makes it hard / impossible to do stuff:
- with fields that do not exist in config - possibly as in "do not exist yet" (like "being created in a multistep UI" - see #1963340: Change field UI so that adding a field is a separate task),
- with "slightly modified versions of the field definition" (eg you sometimes want to work with a field as if it wasn't required, or multi-valued)

yched’s picture

yched’s picture

Issue summary: View changes

current state of the patch

yched’s picture

Updated patch:
- fixes the last validation fails
- workaround for the problems mentioned above about field purge - bypass $entity->get($field_name), work on raw values read from $entity->{$field_name} (see field_attach_load() / field_purge_data())

No easy interdiff, includes a reroll.

yched’s picture

Issue summary: View changes

Added a related issue.

fago’s picture

hm, I think this work-a-round is quite problematic as it cripples performance for the regular load case, right? It does not only instantiate the object, it even does it in a way it's not running through prototyping and cannot be used later on any more.

So maybe, we can split that field_attach_load() in a separate helper dedicated for purging? Alternatively, what we'd really need here is probably the possibility to inject different field definitions. Maybe, just add a setter to that and save that alternative code path?
Once everything is properly injected, we could do that also by decorating the EntityManager and adjusting the return of getFieldDefinitions(), not sure this is really better than expose this setter.

yched’s picture

@fago: this should not affect the regular load case, the "special" treatment is only applied when field_attach_load() is called with an $options['instance'] specified, which is only to support field_purge_batch(). If $options['instance'] is not set, the overhead is just a couple "if (isset())"s.

That's not very different from current HEAD, where field_attach_load() can receive $options['field_id']. The difference is:
- the caller has to fetch and inject the $instance himself, rather than having field_attach_load() know about "deleted fields" and where they are stored. We've been wanting to do that for a while.
- the code to call prepareCache() is a bit more involved in the case where $options['instance'] is present. But it only runs in the case of field purge.

maybe, we can split that field_attach_load() in a separate helper dedicated for purging ?

I did consider bypassing field_attach_load() and calling the field_storage load hook + prepareCache() directly from field_purge_batch(), thus moving complexity in the specific area that needs it without cluttering the "main" code.
But the problem is the (mostly useless but) fairly complex hook sequence invoked in field_attach_load() right now (hook_field_storage_pre_load(), hook_field_storage_load(), prepareCache(), hook_field_attach_load())
AFAIK the "field / entity storage" issue intends to simplify that quite a bit.
At this point, this option could be reconsidered, but doing so right now would mean a bit more code duplication than I think is reasonable.

Alternatively, what we'd really need here is probably the possibility to inject different field definitions

Agreed, but I'm not sure I feel comfortable going there in this patch...

yched’s picture

Issue summary: View changes

reflect progress

yched’s picture

Starting on prepare_view().
If someone is interested in implementing the custom ConfigurableFieldType annotation class, I could use help on that :-)

swentel’s picture

@yched you mean like the dedicated annotation classes like we have right now in core - like say InPlaceEditor ?
I can try that tonight after I played with converting options module :)

yched’s picture

@swentel: yup - not all existing plugin types have their own annotation class right now, but I think the policy is that newly added plugin types should come with one.
Thanks ! :-)

yched’s picture

Issue summary: View changes

update dependent issues

yched’s picture

fago’s picture

>@fago: this should not affect the regular load case,..

I see - then that should be fine! Thanks for explaining!

swentel’s picture

swentel’s picture

Apparently there's also a default_token_formatter - datetime uses it - added it to the the annotation class as well, see http://drupalcode.org/sandbox/yched/1736366.git/commitdiff/48d90c847a88d...

swentel’s picture

We should probably add a getOptions() method on the FieldTypePluginManager so we are consistent with the Widget and Formatter plugin manager, see also https://drupal.org/node/2017627

yched’s picture

@swentel: yay, thanks !

re: default_token_formatter
Hmm... This has never been part of the official hook_field_info().
Just like annotations using the generic @Plugin can accept annotation entries that are not officially exposed in the annotation class (since Plugin exposes no properties of its own), I don't think we *need* to officially add that key to the CFieldType annotation class. Actual annotations in plugin classes can still include custom properties, they will be parsed all the same ?
(that seems important to support properties about contrib modules anyway)

Additionally, this 'default_token_formatter' entry currently in datetime_field_info() doesn't seem actually used anywhere in the code anyway ? Looks like a leftover from the more feature rich D7 contrib date module.

yched’s picture

re: FieldTypePluginManager::getOptions()
Sounds like a good idea, but do you think we should also change Field UI to actually use it in this same patch ?
If not, then maybe that would be an addition for the issue that actually uses it in Field UI ?
No strong opinion here.

swentel’s picture

re: default_token_formatter: ha, I hadn't actually checked whether it was used or not. I'll verify and remove it in case it's not used. I'll add comment in the Annotation that modules can 'define' their own annotations through hook_field_info_alter() then.

re: getOptions(); created a follow up, see #2017711: Create FieldTypePluginManager::getOptions()

swentel’s picture

Issue summary: View changes

added related issue

swentel’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

Note @fago, @Berdir: I extracted a sub-part of the patch to a separate issue at #2018323: Make TypedData implement PluginInspectionInterface (and added the issue to the "blockers" section in the OP.

yched’s picture

Issue summary: View changes

Remove committed blocker

yched’s picture

Rerolled + updated with @swentel's custom annotation class.

yched’s picture

Issue summary: View changes

add blocker issue

yched’s picture

Includes @plach's fixes from #2013837: Rewrite field sync on top of NG entities, we should be green now !

(but hook_field_prepare_view() is still @todo)

yched’s picture

Issue summary: View changes

remove done TODO

yched’s picture

Issue summary: View changes

Rework the "related patches" section

yched’s picture

Created #2018707: Do we still need to support hook_field_prepare_translation() ? to discuss the fate of hook_field_prepare_translation()

fago’s picture

yched’s picture

Renamed a bunch of classes:

FieldTypePluginManager -> ConfigFieldTypePluginManager
CFieldType -> ConfigFieldType (annotation class)
CFieldDataTypeDerivative -> ConfigFieldDataTypeDerivative
CField -> ConfigField
LegacyCField -> LegacyConfigField
CFieldItemInterface -> ConfigFieldItemInterface
CFieldItemBase -> ConfigFieldItemBase
LegacyCFieldItem -> LegacyConfigFieldItem
CTextItemBase -> ConfigTextItemBase
CTextItem -> ConfigTextItem
CTextLongItem -> ConfigTextLongItem
CTextWithSummaryItem -> ConfigTextWithSummaryItem

yched’s picture

Issue summary: View changes

add related issue

yched’s picture

As a side note, I'm going afk (or at least away from a coding env) in 4-ish days.
I'm moving forward on the still missing prepareView() here, but now would be a good time to enter a review phase ;-)

swentel’s picture

Committed some nitpicks, spaces and typo's on the branch.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -480,6 +484,20 @@ public function getSchema() {
+    // some other use cases rely on identifying the first column with the/ key()

I'm not sure exactly what that 'the/ key()' means - or I'm going mad, that's possible too :)

plach’s picture

+++ b/core/modules/translation_entity/translation_entity.admin.inc
@@ -589,20 +589,21 @@ function translation_entity_translatable_batch($translatable, $field_name, &$con
+//    field_attach_presave($entity);
+//    field_attach_update($entity);

All the code surrounding these lines is going to be no longer needed once we are are done with field NG. AAMOF we will store orginal field values under the 'und' language no matter if they are translatable or not. But I guess we are not there yet. Is this passing tests because we have no coverage or is there some magic around I couldn't spot?

yched’s picture

Changes:
- ConfigurableEntityReferenceItem::__construct() needs to be kept in sync with the one from ConfigFieldItemBase, since ConfigurableEntityReferenceItem extends EntityReferenceItem rather than ConfigFieldItemBase.
- forgot a call to hook_field_load() in node_preview()

yched’s picture

@swentel #62: Thanks for the fixes !
"with the/ key()" : lol, this sounds mysterious, doesn't it ?
Fixed in the branch.

@plach #63: Nope, that sounds like missing test coverage around whatever this function is doing ;-)
The code in there is a bit beyond me, but I fixed to what looks like the correct way now.
See interdiff.

we will store orginal field values under the 'und' language

Totally not the issue for this, but then maybe 'und' is not a great key name ?

plach’s picture

Totally not the issue for this, but then maybe 'und' is not a great key name ?

Maybe it's not ideal but it's the one that will cause less troubles with the upgrade path. However we have a nice constant to mask it: Language::LANGCODE_DEFAULT ;)

I'll manually test this and report.

yched’s picture

Ah, upgrades :-/...

Thanks for testing !

yched’s picture

@all, with special poke to @fago / @Berdir:

So we have InvokeFieldMethod(), invokeFieldItemPrepareCacheMethod() + I'm adding a 3rd, invokeFieldMethodMultiple() in my local code for prepareView().

They currently live in DatabaseStorageController() because they started off as part of invokeHook() before moving to standalone methods. But:
- they currently aren't in any interface
- they don't really have anything to do with storage, either conceptually or code-wise (they only use $this in their code branches supporting EntityBC, to access getFieldDefinitions())
- they are replacements for the _field_invoke[_multiple]() functions from field.attach.inc (except they run on all fields, base + config)

What do you think would be a good class + interface for those ?
Entity ? EntityManager ?
We don't want them on config entities, do we ? (do we ???)

plach’s picture

FileSize
1.47 KB

The attached interdiff does the trick ;)

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-1969728-65.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

Oh, nice :-) Committed, will be in the next patch. Thanks !

yched’s picture

Reroll (includes @plach's #69)

yched’s picture

Issue summary: View changes

update class names

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-1969728-72.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
2.44 KB
257.04 KB

So those new fails appeared after adding the call to invokeFieldItemPrepareCache() in node_preview().
Basically, entering new terms (more than one...) in the "autocomplete / autocreate" widget for taxo fields and hitting "Preview" fails.

Spent hours in there, it's a hell combination of:
- invokeFieldItemPrepareCache() calls getTranslationLanguages(), which initializes the Field objects from the raw values (more on that later). This places the autocreated 'entity' in $node->fields, and *removes* it from the the raw values in $node->values. Note : those entity are new, they have no id(), no way to retrieve them.
- then field_attach_prepare_view() and its calls to field_language(), _field_invoke_multiple(), field_invoke_method_multiple(), relies on the BC decorators / BC syntax to access field values, which at some point *deletes* the entries in $node->fields, and thus we have lost the 'entity'.
For some reason, it does work when there is only one new entity, but breaks when there are more.

No clue how to fix this :-(.

In the end, however, I think it is actually reasonable to *not* call invokeFieldItemPrepareCache() on preview.
We called hook_field_load() in preview in D7 because the field values that are about to be displayed come out of raw form values instead of data storage. If the formatters needed something that was added in hook_field_load(), we needed that one to run.
But since then, it's been established that hook_field_load() / prepareCache() is not the right place to load additional stuff, and actually in core just datetime fields use them right now.

So I think it's reasonable to let the contract be "displayed values might not have run through prepareCache()". If at display time you need something in that is set in prepareCache. be prepared to populate it also in prepareView() if it's missing.

- On form submit, the whole code flow (validate, presave, insert / update) has always dealt with those "items out of form values", that haven't run through prepareCache() either.
- The new name of the method, "prepareCache()", makes it a bit weird to call during preview...

Attached patch does that (also, rerolled after a couple dependency patches have been committed)

yched’s picture

So, this leaves the fact that invokeFieldItemPrepareCache() currently iterates on languages with:

foreach (array_keys($entity->getTranslationLanguages()) as $langcode) {

$entity->getTranslationLanguages() calls $this->getProperties(), which creates all the Field objects - this nullifies the efforts invokeFieldItemPrepareCache() makes to avoid creating them unless stricly needed :-)

Is there another way I can gat all the langcodes I need to iterate on, without instanciating the Field objects ?

yched’s picture

FWIW: I also tried removing the 'computed' flag in the definition of the 'entity' property in TaxoFieldItem (not that I have any clue what the effect would be, nor if that's a sick idea to begin with - just trying...).
Not that many fails, see #1992138-125: Helper issue for "field types as TypedData Plugins"...

At any rate - I welcome feedback / advice / proposals on this topic, but for now I'm leaving this and going with #74 (don't call prepareCache() in node_preview())

#75 is a problem, though. Feedback welcome on this too :-)

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

The last submitted patch, field-types-as-datatypes-1969728-74.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
Issue tags: +entity API, +Field API
yched’s picture

Issue summary: View changes

Remove committed blockers

yched’s picture

Added #75 as a "TODO" in the issue summary

andypost’s picture

@yched #75 and the similar trouble we could get for comments with calling preview of parent entity - this needs better implementation

yched’s picture

@andypost: hmm - doesn't seem related ?

Berdir’s picture

@yched: Re getTranslationLanguages(). I think the work @plach is doing in #1810370: Entity Translation API improvements introduces the knowledge of which translations are available, including adding/removing translations methods/hooks) on the entity level and I think that will eliminate the need for the slow getProperties() call but I haven't looked at the code in detail.

plach’s picture

What @Berdir said
(as always ;)

yched’s picture

@Berdir, @plach: Thanks for the heads up. Sounds great :-).

So this means that we can leave that call to getTranslationLanguages() in invokeFieldItemPrepareCache() here ? It ruins the optimizations for now, but at some point it won't ?

Also, current patch has a "// @todo getTranslationLanguages() seems like a potential perf drag ?" in InvokeFieldMethod()
(InvokeFieldMethod() can be called 3-4 times during the load / save cycle of an entity)
Can I get rid of the @todo and assume the next version will fix this ?

plach’s picture

@yched:

It ruins the optimizations for now, but at some point it won't ?

This is the new code:

<?php
  public function getTranslationLanguages($include_default = TRUE, $include_removed = FALSE) {
    if (!$include_removed) {
      $translations = array_filter($this->translations, function($translation) { return $translation['status']; });
    }
    else {
      $translations = $this->translations;
    }

    unset($translations[Language::LANGCODE_DEFAULT]);

    if ($include_default) {
      $langcode = $this->get('langcode')->language->langcode;
      $translations[$langcode] = TRUE;
    }

    // Now load language objects based upon translation langcodes.
    return array_intersect_key(language_list(Language::STATE_ALL), $translations);
  }
?>

It should be quite faster. We could even optimize it a bit more, I think.

Can I get rid of the @todo and assume the next version will fix this ?

I think this should be fair :)

fago’s picture

hm, it should be possible to work over getTranslationLanguages() to not issue instantiations of all the objects. I can have a look at that.

So I think it's reasonable to let the contract be "displayed values might not have run through prepareCache()". If at display time you need something in that is set in prepareCache. be prepared to populate it also in prepareView() if it's missing.

Sounds reasonable + I've never been happy with code relying on that safe_value key... Code show not fail if some sort of "cache" is missing, so +1 on that.

I also look over the whole patch, and I overall like what I see. Really great work!

So here are some comments:

  • I must say I dislike the ConfigField naming convention. I'd prefer the long but more explicit ConfigurableField. That's just my personal preference though and I won't start derailing this on names :D
  • ConfigFieldItemBase gets the instance during construction. Can we lazy-load that instead + user setterInjection? And make getFieldInstance() public?
  • Can we remove insert() and update() on fields in favour of preSave() ? $entity->isNew() tells you whether it's an update anyway there? Could be a follow-up.
fago’s picture

Note: Looking into validation issues now.

yched’s picture

@plach / @fago: thanks !

I removed the @todos about getTranslationLanguages()

I must say I dislike the ConfigField naming convention. I'd prefer the long but more explicit ConfigurableField. That's just my personal preference though and I won't start derailing this on names :D

Heh, I hoped it would pass, but I kind of expected that it would raise eyebrows ;-)
I'd suggest we keep those for now and align with the outcome of #2019601: Rename config Field / FieldInstance structures ?. This means I might not be able to do the rename myself before I go afk.. - but well, it's a rename ;-). The impacted classes are listed in #60.

ConfigFieldItemBase gets the instance during construction. Can we lazy-load that instead + user setterInjection? And make getFieldInstance() public?

I'd rather not go there if I can avoid it :-). I think this will be a non-issue when what we receive in __construct() is a FieldDefinitionInterface object ?

Can we remove insert() and update() on fields in favour of preSave() ? $entity->isNew() tells you whether it's an update anyway there? Could be a follow-up.

I'm fine with that, but, same as above, I don't think I'll be able to do it before I leave :-(. My priority right now is finishing prepareView () and the related test fails...
Should be a relatively easy refactor though. The only thing is that ATM it's not completely easy to go up to the $entity when you're in a Field / FieldItem method. So maybe a getEntity() helper ?

yched’s picture

Issue summary: View changes

Added a todo

swentel’s picture

Issue summary: View changes

Updated issue summary.

yched’s picture

Also : #68 (location of invokeFieldMethod() / invokeFieldMethodMultiple() / invokeFieldItemPrepareCache()) still needs feedback :-)
(added it in the TODOs in the issue summary)

fago’s picture

Should be a relatively easy refactor though. The only thing is that ATM it's not completely easy to go up to the $entity when you're in a Field / FieldItem method. So maybe a getEntity() helper ?

Sounds good to me.

I'm fine with that, but, same as above, I don't think I'll be able to do it before I leave :-(. My priority right now is finishing prepareView () and the related test fails...

Sure - that's great, just trying to agree upon possible follow-up tasks before you leave ;-)

I'd rather not go there if I can avoid it :-). I think this will be a non-issue when what we receive in __construct() is a FieldDefinitionInterface object ?

Oh, indeed. Let's see how this evolves then...

I'd suggest we keep those for now and align with the outcome of #2019601: Rename config Field / FieldInstance structures ?. This means I might not be able to do the rename myself before I go afk.. - but well, it's a rename ;-). The impacted classes are listed in #60.

Yep, that's fine with me. This mega patch should not be hold up by naming discussions anyway, renames can be totally follow-ups.

Also : #68 (location of invokeFieldMethod() / invokeFieldMethodMultiple() / invokeFieldItemPrepareCache()) still needs feedback :-)

ah, sry I was about to comment it anyway, but forgot. So:

What do you think would be a good class + interface for those ?
Entity ? EntityManager ?
We don't want them on config entities, do we ? (do we ???)

My initial take was that this should be moved to the new EntityStorageControllerBase, as needless separation between config-storage and db-storage resulted in pain previously. So yep, I'd put them on config entities also - as discussed we want to align those with EntityNG except for __get().

However, you are right in that this isn't really about storage, but a general way to invoke hooks on top of entities. Right now, it are just storage hooks, but it would be reasonable to use the same methods for e.g. invoking hook_entity_view. So yes, I think the entity manager would be a good location, but I'd leave that to a follow-up also and just move it to the storage base for now.
Problem is that the EM is right now not entity-type specific, what results in needless code-changes imho.

yched’s picture

@fago: thanks !

I'll move the methods to the storage base (and to the StorageControllerinterface as well, then, right ?)

One thing I forgot about preSave() / insert() / update()
- Currently preSave() is called before a new entity is saved and receives an id(). insert() is called after, and some current implementations rely on the fact that the entity will have an id() by then
- So if we merge the three into one single method, it needs to be called after the entity is saved (i.e at the place we currently call insert() or update()), so that the entity has is an id()
- so Field/FieldItem::preSave() would be pre-"the field gets saved", not pre-"the entity gets saved". Still makes sense I guess, but a bit subtle :-)

fago’s picture

(and to the StorageControllerinterface as well, then, right ?)

Are those used outside of the controllers? If not, I'd keep them out for now. (So adding somewhere else would be easier later on I hope.)

yched’s picture

Are those used outside of the controllers?

Yes they are. Currently In field_attach_load() (ok, still storage related) and field_attach_prepare_view() (in my work in progress code, not posted here yet, but I will merge pretty soon).
As you pointed earlier, they are totally intended to be generic iterators, not only for storage-related methods (view flow, form flow...)

yched’s picture

Moved FieldInvoke*() to EntityStorageControllerBase + EntityStorageControllerInterface

Leaving at "do not test", because the poor bot already has troubles keeping up in the helper issue...

yched’s picture

yched’s picture

- Merged cleanups by fago around validation (includes #2012682: Constraints in getPropertyDefinitions generate violations without a delta)
- changed the derivative prefix from 'field_type:' to 'configurable_field_type:'

yched’s picture

Issue summary: View changes

adjust TODOs

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-1969728-96.patch, failed testing.

yched’s picture

Issue summary: View changes

updated related patches

yched’s picture

Status: Needs work » Needs review
FileSize
21.33 KB
263.94 KB

And here's the current state of the prepareView() work...

Still a couple fails - notably, the "taxonomy autocreate on node preview" fails are back - as stated above, we don't call prepareCache() in node_preview() anymore, but this is triggered by something else...

I could really use some help on those :-)
Steps to reproduce:
- go to node/add/article
- enter a title, and *two* random tags (e.g 'one, two' - they must not exist in the 'Tags' vocabulary so far)
- hit "Preview"
--> Call to a member function label() on a non-object in taxonomy/.../LinkFormatter.php on line 40

in TaxonomyFormatterBase::prepareView(), in the $items, the first item has an 'entity', but not the second.

fago’s picture

The interim improvement (before plach's work goes in) for getTranslationLanguages() is green as well :-) - see #1992138-137: Helper issue for "field types as TypedData Plugins".

yched’s picture

Merged in @fago's optimizations on the current getTranslationLanguages().
Thanks a lot :-)

Forgot to add:
In the prepareView stuff I've been seeing tests where the following code in invokeFieldMethodMultiple would raise exceptions in get():

      $definitions = $this->getFieldDefinitions(array(
        'EntityType' => $this->entityType,
        'Bundle' => $bundle,
      ));
      foreach ($definitions as $field_name => $definition) {
        $items = $entity->getTranslation($langcode)->get($field_name);
      }

The current patch hides those with a try / catch, but this seems weird.
The exceptions didn't occur if I replaced with just $entity->get($field_name), skipping the translation.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-1969728-100.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
15.72 KB
280.54 KB

- Fix incomplete revert of the PluginInspectionInterface stuff
- Removes _field_invoke_multiple() / _field_invoke_multiple_default(), not used anymore - yay !

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

The last submitted patch, field-types-as-datatypes-1969728-102.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
Issue tags: +entity API, +Field API
yched’s picture

re: my #98 and the fails on taxo autocreate on preview:

FWIW, file and image fields are affected by what looks like a similar problem (although it's not what is causing the fails in those modules, I'm on those):
- Add a file field to a node type,
- upload *two* files to a node
- on viewing the node, when reaching the formatter, the second $item has no 'entity' entry.

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-1969728-102.patch, failed testing.

yched’s picture

Reroll is fine.

Updated patch:
- Simplifies the signature of prepareView()
- Fixes the FileFieldDisplayTest fails
What happens here is that file_field_prepare_view() *removes* the items that are supposed to stay hidden. For some reason, those items removed in the BC/array shape of the $items stick and pop up again when reaching the formatter, and thus are displayed while they shouldn't be.
The thing, though, is that purely dropping items from $entity for the rest of the cycle is a sick thing to do in prepare_view()...
I opened #2020677: file_field_prepare_view() should not delete items to fix this, and patch includes just the parts needed to stay green here.

Two interdiffs, since there is a reroll in between.

yched’s picture

Issue summary: View changes

update TODO

yched’s picture

Issue summary: View changes

update status

yched’s picture

So, I'm starting to be short on time here :-/

Remaining fails:
- ImageFieldDefaultImagesTest:
I couldn't really look into them so far, will try to tonight or tomorrow
- PagePreviewTest / TermTest:
Those are the "taxo autocreate fails on preview" described in #98 / #105. They really beat me for now :-(.

fago’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.php
@@ -223,7 +223,7 @@ public function invokeFieldMethodMultiple($method, array $entities, $langcode) {
         // @todo do we really need to pass the $definition here ?
-        $type_definition['class']::$method($entities_items, $definition);

Comment is outdated now.

fago’s picture

- PagePreviewTest / TermTest:
Those are the "taxo autocreate fails on preview" described in #98 / #105. They really beat me for now :-(.

I take a look on those.

fago’s picture

Issue summary: View changes

typo

yched’s picture

Added a TODO in the issue summary: "report the phpdoc of field.api.php's hook_field_info() into the ConfigFieldType annotation class, and ditch hook_field_info() docs completely"
Easy - any takers ? ;-)

yched’s picture

#110 - indeed, will fix in the next patch
#111 - thanks ;-)

yched’s picture

OK, dug a bit more in ImageFieldDefaultImagesTest, this is funny / worrying :-)

The thing is that when a field is shared across different node types, Field (value) objects are not recreated from scratch, but shared *across node types*.
ConfigField::__construct() is called only once for a given field name ('body').
This is a problem since ConfigField (and ConfigFieldItem) store their own $instance definition.

So if an article node is created first, then a page node, $page_node->get('body') is a ConfigField whose ->instance is the $instance of 'body' on article nodes, and thus the instance settings are not the right ones :-)

Simple steps to reproduce:
- Edit ConfigField.php and make the $instance property public
- Create one article node (say, nid 1), and one page node (nid 2)
- Execute the following:

$node = node_load(1); // <-- This is an article
dsm('node : ' . $node->type); // <-- "node : article"
$field = $node->get('body');
dsm('instance: ' . $field->instance->bundle); // <-- "instance : article" - fine...

$node = node_load(2); // <-- This is a page
dsm('node : ' . $node->type); // <-- "node : page"
$field = $node->get('body');
dsm('instance: ' . $field->instance->bundle); // <-- "instance : article" - Doh !

Surprised that we don't have more fails, actually :-p

I'm afraid that's all for today...

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-1969728-108.patch, failed testing.

fago’s picture

ConfigField::__construct() is called only once for a given field name ('body').
This is a problem since ConfigField (and ConfigFieldItem) store their own $instance definition.

Yes, they are prototyped, so construct does not run. Problem is during clone, the instance is not re-newed and cannot be easily. Thus, I migrated code to use a lazy-retrieval approach for $this->instance as discussed above - even if turns out unnecessary in future, having an explicit getter for that is a good thing anyway I think.

yched’s picture

So the problem is that right now Field / FieldItem __construct() receive and store a $(field_)definition array that, in EntityNG's formulation for base fields, is the same across all bundles (base fields have no 'instance' variants). So the current way where a Field object is created once and reused for other bundles, all with the same Field::$definition property, works.

But the above is, like, really not true for config fields, and does not hold if we want the $defiinition received in the __construct() to be the FieldDefinitionInterface object (i.e, for config fields, the $instance object). There, $definition really must be different per bundle.

So we can move to a post-creation getter for the $instance right now, it works as long as $instance and $definition are different things. But IMO after #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets it would be sad to be forced to keep a separate array $definition structure in addition to the FieldDefinitionInterface objects ?

yched’s picture

Looked at @fago's fixes better, they work and totally make sense.
#117 still stands IMO (constructor injection of $definition / $instance as a FieldDefinitionInterface object), but we're not there yet.

At any rate, @fago did an amazing run and fixed the remaining fails - we're green !
- Fixed BC-mode letting 'entity' property of references vanish.
- Potentail fix for getTranslation() issue.
- Use getInstance() method to lazy-load and initialize field instances

Will do a pass on the remaining TODOs tomorrow.

The last submitted patch, field-types-as-datatypes-1969728-118.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

Whoops, wrong diff command for the interdiff.

@fago - one question regarding

             try {
-              $entities_items[$id] = $entity->getTranslation($langcode)->get($field_name);
+              $entities_items[$id] = $entity->getNGEntity()->getTranslation($langcode)->get($field_name);
             }

You added ->getNGEntity() to try to get rid of the try/catch, but in the end had to reintroduce the try/catch. Do we keep the ->getNGEntity() then ?

yched’s picture

yched’s picture

Issue summary: View changes

new todo

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-1969728-121.patch, failed testing.

fago’s picture

Status: Needs review » Needs work

You added ->getNGEntity() to try to get rid of the try/catch, but in the end had to reintroduce the try/catch. Do we keep the ->getNGEntity() then ?

I think we can go with the NG-entity as it already checks for that, and not with the BC-decorator. I realized the method should be the same for the BC decorated - as it's just forwarded, but if accessed directly it wouldn't be the case. So it confused me as it was before ;-)

fago’s picture

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

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

The last submitted patch, field-types-as-datatypes-1969728-121.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
3.22 KB
284.42 KB

Forgot one piece in the reroll in #121 - duplicated code sucks.
(+ sync other methods, + fix 80 chars wrap, + remove unneeded "use" statements)

@fago: sorry, I didn't fully parse #123 ;-)
Do we keep $entity->getNGEntity()->getTranslation() in invokeFieldMethodMultiple(), or do we come back to $entity->getTranslation() ? The other two invokeField*() methods use $entity->getTranslation() directly and seem to work fine (and no need for try / catch there).

FWIW, what those three methods do is:

- invokeFieldMethod:

$translation = $entity->getTranslation($langcode);
foreach ($translation as $field_name => $field) {

- invokeFieldItemPrepareCache:

$translation = $entity->getTranslation($langcode);
foreach ($translation->getPropertyDefinitions() as $property => $definition) {
  if (we really need to create the Field) {
    $field = $translation->get($property);
  }

- invokeFieldMethodMultiple():

$definitions = $this->getFieldDefinitions(array(
  'EntityType' => $this->entityType,
  'Bundle' => $bundle,
));
foreach ($definitions as $field_name => $definition) {
  // This sometimes fails and needs to be wrapped in a try / catch.
  $field = $entity->getNGEntity()->getTranslation($langcode)->get($field_name);
     // or $entity->getTranslation($langcode)->get($field_name); - no visible difference ?

The ->get() sometimes fails in that last case because Translation::getPropertyDefinitions() return an empty array - but that same method works fine in the second code ...
In other words, I'm fine with keeping the try / catch, but something escapes me here :-)
(also, try / throw $exception / catch is costly CPU-wise, so it might be best if we could find a better safeguard)

The last submitted patch, field-types-as-datatypes-1969728-126.patch, failed testing.

fago’s picture

The ->get() sometimes fails in that last case because Translation::getPropertyDefinitions() return an empty array - but that same method works fine in the second code ...

hm, it should not matter as the BC-decorator just forwards the call to the NG-entity anyway.

@fago: sorry, I didn't fully parse #123 ;-)

sry, it had a syntax error ;-) - updated it. I'd vote for going with the NG-entity variant as it is less confusing, but it really should not matter.

yched’s picture

Status: Needs work » Needs review
FileSize
284.43 KB

Gee, HEAD moves fast...

yched’s picture

@fago - yep, weird...

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

The last submitted patch, field-types-as-datatypes-1969728-129.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review

#129: field-types-as-datatypes-1969728-129.patch queued for re-testing.

Bot was out of disk space..

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-1969728-129.patch, failed testing.

yched’s picture

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

Oh come on now...
How many runs does it take to get green after you've stated "we're green !" ?

#129: field-types-as-datatypes-1969728-129.patch queued for re-testing.

amateescu’s picture

Took me 5 runs in another issue :P

effulgentsia’s picture

effulgentsia’s picture

Sorry, coming late to the party here. Just scanned through the patch for the first time. It's looking really good so far.

One question: is there a reason for ConfigFieldTypePluginManager to be prefixed with "Config"? Same question for the ConfigFieldType annotation, and all of the text.module field type plugin classes.

My thought here is that while it makes sense to have a concept of "configurable fields", I don't get the logic of "configurable field types" being treated this separately from "field types". My goal is that we'll eventually have both $node->title and $node->body using a text.module field type. text.module shouldn't really care that the latter is configurable and the former isn't. I do see the logic in a ConfigFieldItemInterface and a ConfigFieldItemBase, and in TextItemBase extending it, since it provides settings forms. I just don't see the logic in that warranting it to be named ConfigTextItemBase. In other words, I think field types should just be field types, and it's an implementation decision as to whether they provide settings forms. I'm also fine if we want to punt this to a follow up, but I think all I'm asking for is to rename the above classes.

yched’s picture

re @eff #139:
- The "other" field types are currently munged together in hook_data_type_info(), along with all other data types that are not about fields (primitives, other stuff it seems)
- Field UI needs to be able to list "just configurable field types"
- The "data type" plugin is still hook-based currently, and this can't fly for us: we need to be able to get to a FieldItem::schema() for fields created in updates, and hook-based discoveries now trigger exceptions during updates :-).

So right now, "configurable field types" are exposed as a separate, annotations based, plugin type, and they are "added" to the general list of "data types" plugins through a derivative (see field_data_type_info()).

I tried to get back to having config field types be exposed with all the rest of the data types at some point, but I hit a wall - unfortunately I can't remember exactly what is was :-(. That was even before item 3 above became effective, and item 3 is a blocker in itself.
I also didn't want to widen the scope of the patch with moving existing (non-configurable) "field types" in the new system.

In Portland with @fago we mentioned the possibility that maybe the "typed data" plugin type, that currently munges very different animals (field types, primitive types...), with no mechanism right now to ask "give me only the list of those of this kind", should be split in separate plugin types. But I don't know what the plans are on this regard.

yched’s picture

re #137, part II :-)

As to the question of specifically removing the "Config" prefix in ConfigTextItemBase and its child classes:
Right now "configurable field type" classes rely on the fact that they can access the FieldInstanceInterface $instance object that defines them (available at $this->getInstance()). And they do expect an $instance.
So they can't be used as the class for a non-configurable field.

Now that #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets is in, this might change if/when the $definition received in the __construct() directly is a FieldDefinitionInterface object (i.e, the $instance when the class is used for a "configurable field") - but as #116 / #117 show, it might not be that simple :-).
Then, some field types / FieldItem classes (and that's probably the case for text fields) might be able to work with any "FieldDefinitionInterface", though I guess some might not and need a FieldInstanceInterface specifically.
Bottomline is : we have FieldDefinitionInterface objects right now, but they are not yet leveraged by TypedData.

So, ConfigTextItem --> TextItem:
would make sense later on, but right now those classes are very much tied to being used for "configurable fields with an $instance definition".

yched’s picture

yched’s picture

Actually, #139 is a reminder that those getInstance() methods should be part of interfaces...
--> updated ConfigFieldItemInterface, added ConfigFieldInterface

yched’s picture

Oops, ConfigField::getInstance() phpdoc should be an {@inheritdoc} too now
Just fixed that, no need to re-trigger tests.

yched’s picture

People in there might be interested in this : #2021817: Make widgets / formatters work on EntityNG Field value objects. :-)

yched’s picture

As per #2018707: Do we still need to support hook_field_prepare_translation() ?, removed support for hook_field_prepare_translation().
+ removes the old and ugly _field_invoke() iterator - my highlight of the day :-)

plach’s picture

:)

plach’s picture

Issue summary: View changes

no more test fails !

yched’s picture

Issue summary: View changes

Update TODOs & blockers

yched’s picture

I'm nearing the end of my intervention here...

- Removed some testing code I left here and there
- Took care of the last pending TODO in the issue summary (port the doc of hook_field_info(), then remove it and references to it)
- Did a last pass on the in-patch @todos
Most of them are "Remove when all entity types are NG" / "Remove when all field types are converted"

Some @todos left :
- TextProcessed::setValue() raises an exception when something tries to set the computed 'processed" property of a FieldItem.
That's exactly what invokeFieldMethod() & friends do when they need to work on BC entities, though.
So for now I left that "throw new ReadOnlyException" line commented out :-/
- @fago left a "@todo: Pass $violation->arrayPropertyPath as property path." in WidgetBase, but I'm not sure what he meant ;-)

I'll copy those to the issue summary.

Thanks a *ton* to the many people that helped and unblocked me at various stages here :-)
@fago, @plach, @Berdir, @swentel, @amateescu... awesome team work !
I'll leave it to you guys to drive this home (heh, fingers crossed...)

yched’s picture

Issue summary: View changes

commit credits

Status: Needs review » Needs work

The last submitted patch, field-types-as-datatypes-1969728-146.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

last update

yched’s picture

Status: Needs work » Needs review
FileSize
303.77 KB
Gábor Hojtsy’s picture

Where is the prepopulation added back in this patch? The conclusion as far as I understood in #2018707: Do we still need to support hook_field_prepare_translation() ? was that the hook is not needed but the feature should be reproduced in some other way, or you introduce a regression.

fago’s picture

@prepopulation: I don't know how entity translation deals with that, but afaik it has no such hook. If there is a regression, it's in moving from content translation to entity translation in general.

- @fago left a "@todo: Pass $violation->arrayPropertyPath as property path." in WidgetBase, but I'm not sure what he meant ;-)

Thing is, that the violations should tell us already what's the error property, so I think we should pass the relative property path as $sub_property_path to the method, so the implementation can easily re-use. Also, we can provide a default implementation which just uses assumes there is a form element for the given property, so just need to override it if e.g. you have a multiple value widget or something else special.
If that works for you I'd be happy to take care of that, could be another issue though.

Gábor Hojtsy’s picture

It does not seem likely that we can remove node-copy translation module from core (ala profile module in Drupal 7). Crippling that functionality is IMHO still a regression.

fago’s picture

My thought here is that while it makes sense to have a concept of "configurable fields", I don't get the logic of "configurable field types" being treated this separately from "field types".

That's a fair point. I agree that we should try to avoid that distinction, however I see that difficult as well as the current implementations lookup settings in their $instance, what then wouldn't be available... Still, we could already allow to use the same plugin-type/annotation for providing both kind of fields + just have ConfigurableFieldInterface be an optional, additional interface.

Once implementations can start to rely on the shared definition, I think it's doable.

I'd see that work as
- pass in the field base definition in the constructor only (this is going to be prototyped, so it must be something that's shared)
- then, have a setIntance() method which allows setting the more specific definition + set the more instance-definition later on. When exactly, is the key question though - probably in setContext().

I think we should work on both issues later on, so we can move on with this biggie asap. So I guess it's time to start creating all relevant follow-up issues...?

fago’s picture

It does not seem likely that we can remove node-copy translation module from core (ala profile module in Drupal 7). Crippling that functionality is IMHO still a regression.

It can be still done, as noted in #2018707: Do we still need to support hook_field_prepare_translation() ? - however can we have that discussion in the issue we opened for it? :-) Thanks!

yched’s picture

FYI, I have left my coding env, will check the queues but can't touch code anymore.

re @fago #150 / violations: yup, works for me.

#152

Still, we could already allow to use the same plugin-type/annotation for providing both kind of fields + just have ConfigurableFieldInterface be an optional, additional interface

OK, but we need something more than just the interface for field UI to know if it should list a field type in its "add new field" UI, because checking the interfaces means loading the classes. has to be a flag in the definition

effulgentsia’s picture

OK, but we need something more than just the interface for field UI to know if it should list a field type in its "add new field" UI, because checking the interfaces means loading the classes. has to be a flag in the definition

I recommend changing the @ConfigFieldType annotation to @FieldType, and including a 'configurable' = TRUE|FALSE key within it. I'd be ok with that being a follow up though.

+++ b/core/modules/text/lib/Drupal/text/Plugin/field/field_type/ConfigTextItem.php
@@ -0,0 +1,104 @@
+class ConfigTextItem extends ConfigTextItemBase {

My concern though is that we're renaming TextItem to ConfigTextItem (and similarly for the other text.module classes) in this patch, and that that will get propagated to all the other types after this initial patch lands. I don't see that as necessary. The class is already in the Plugin/field/field_type namespace, which means whatever that means in terms of configurability, plus it implements the interfaces needed by field.module, and has the annotations it needs. So, I think the prefix is unnecessary, and needlessly reinforces a mental distinction we'll try to undo later.

fago’s picture

I recommend changing the @ConfigFieldType annotation to @FieldType, and including a 'configurable' = TRUE|FALSE key within it. I'd be ok with that being a follow up though.

Yes, let's do that. (I think we discussed that already at some point? :)

So, I think the prefix is unnecessary, and needlessly reinforces a mental distinction we'll try to undo later.

Indeed, having the interface/flag is enough. So let's remove Config prefix from the actual implementation but keep it on the base-class + interface?

effulgentsia’s picture

Great! Do you want me to roll the patch, or do you have it?

fago’s picture

I take a stab on it.

fago’s picture

Issue summary: View changes

more specific

fago’s picture

Issue summary: View changes

added follow-ups

fago’s picture

Posted a patch over at #1992138-153: Helper issue for "field types as TypedData Plugins" - let's see whether bots can give it green light first (it basically works for me locally).

This patch/interdiff renames those text classes, moves the manager to the entity core component and defines the configurable flag. It does not make field api respect the flag nor use the the new plugin type for existing entity field types yet though - I think that would be fine to handle in a follow-up.

Also I created the following follow-up issues and added it to the summary:
#2023465: Allow validation constraints to add in message replacements
#2023473: Merge FieldItem::insert() and FieldItem::update() on fields into preSave()
#2023563: Convert entity field types to the field_type plugin

Besides that, I think we should mark all non-storage related field API attachers as deprecated. I realized we have no entity preprocessing yet, so I guess we should that first before we mark this one as deprecated. Opened: #2023571: Support preprocessing in EntityViewBuilder

andypost’s picture

fago’s picture

Issue summary: View changes

another follow-up

fago’s picture

Status: Needs work » Needs review
FileSize
305.51 KB

So thanks to effulgentsia this should be green again.
Interdiffs since the latest patch (#148) - besides re-rolls:

Status: Needs review » Needs work

The last submitted patch, d8_field_types.patch, failed testing.

effulgentsia’s picture

Assigned: yched » Unassigned
FileSize
304.66 KB

Same patch as in #1992138-186: Helper issue for "field types as TypedData Plugins". It's green, so copying it to this issue, for hopefully close to final reviews, and a soon RTBC.

#1992138-187: Helper issue for "field types as TypedData Plugins" shows a very disappointing performance regression.

andypost’s picture

Suppose this change should get Gabor's approval because in #1966298: Introduce menu link bundles per menus we discussed that "fieldable" should not affect translation

+++ b/core/modules/translation_entity/translation_entity.module
@@ -862,10 +862,11 @@ function translation_entity_field_info_alter(&$info) {
-function translation_entity_field_attach_presave(EntityInterface $entity) {
-  if ($entity->isTranslatable()) {
+function translation_entity_entity_presave(EntityInterface $entity) {
+  $entity_info = $entity->entityInfo();
+  if ($entity->isTranslatable() && !empty($entity_info['fieldable'])) {

Not sure that "fieldability" should affect translation. This was removed for menu_link entity that none-fieldable by default but needs translation

fago’s picture

#1992138-187: Helper issue for "field types as TypedData Plugins" shows a very disappointing performance regression.

:-( I guess we should do the interface check for prepare-view also, as for prepareCache - such that we do not instantiate unneeded objects. Also, I'll see whether there is more stuff that could be optimised -I guess we are instantiating new $item objects at some places, which we should be able to optimise with NG-entities in place.

effulgentsia’s picture

This issue comment is (paraphrased slightly) from yched. I'm posting it, because his only internet access right now is a phone that apparently has trouble posting to d.o. issues with this many comments already.

Not sure that "fieldability" should affect translation. This was removed for menu_link entity that none-fieldable by default but needs translation

That hunk results in no functional change introduced by this patch. Within DatabaseStorageController(NG)::invokeHook(), the old field_attach_*() functions were already restricted to only fieldable entities. Changes to what we want translation_entity.module doing to nonfieldable entities should be handled in a separate issue.

I guess we should do the interface check for prepare-view also, as for prepareCache - such that we do not instantiate unneeded objects.

I remember we mentioned doing that in Portland, but couldn't remember the reasoning. Not sure we gain much with not generating Field value objects for prepareView() but generating them for view()... If those objects are too expensive to generate in the most common cycles (load / view)... well, when and why would we use them ?

#1992138-187: Helper issue for "field types as TypedData Plugins" shows a very disappointing performance regression.

It would be good to check the impact without the various BC and legacy bridges, I.e.
- on a non BC entity (nodes are still a bit mixed; maybe a taxo term or an entity_test)
- with field types that have been fully ported and don't rely on ConfigLegacy*. Thats only text fields atm... (but those don't have a prepare view step atm, so, short of converting one of the field types that do, there's no good way to check prepareView...)

effulgentsia’s picture

Part 1 of my review. Up to field.module. Mostly nits so far, nothing that can't be done in a follow up.

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -126,9 +126,9 @@ public function &__get($name) {
         // This will work with all defined properties that have a single value.
         // We need to ensure the key doesn't matter. Mostly it's 'value' but
-        // e.g. EntityReferenceItem uses target_id.
-        if (isset($this->decorated->values[$name][Language::LANGCODE_DEFAULT][0]) && count($this->decorated->values[$name][Language::LANGCODE_DEFAULT][0]) == 1) {
-          return $this->decorated->values[$name][Language::LANGCODE_DEFAULT][0][key($this->decorated->values[$name][Language::LANGCODE_DEFAULT][0])];
+        // e.g. EntityReferenceItem uses target_id - so just take the first one.
+        if (isset($this->decorated->values[$name][Language::LANGCODE_DEFAULT][0]) && is_array($this->decorated->values[$name][Language::LANGCODE_DEFAULT][0])) {
+          return $this->decorated->values[$name][Language::LANGCODE_DEFAULT][0][current(array_keys($this->decorated->values[$name][Language::LANGCODE_DEFAULT][0]))];

Seems like the "have a single value" part of the comment is now obsolete.

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -241,13 +240,49 @@ protected function actions(array $form, array &$form_state) {
+      // instanciate NG items objects manually.

Here and elsewhere, s/instanciate/instantiate/.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.php
@@ -137,4 +137,159 @@ protected function cacheSet($entities) {
+            // @todo $entity->getTranslation()->get($name) sometimes fails,
+            // because Entity\Translation::getPropertyDefinitions() is empty() ??
+            try {
+              $entities_items[$id] = $entity->getNGEntity()->getTranslation($langcode)->get($field_name);
+            }
+            catch (\InvalidArgumentException $e) {
+              break;
+            }

Let's make sure we have an issue filed for this.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.php
@@ -137,4 +137,159 @@ protected function cacheSet($entities) {
+            if (!empty($definition['configurable'])) {

I'm not clear why this check is only there for the BC condition, but doesn't really matter, since there's a separate todo about respecting 'configurable'.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EmailItem.php
@@ -7,12 +7,12 @@
 namespace Drupal\Core\Entity\Field\Type;
 
-use Drupal\Core\Entity\Field\FieldItemBase;
+use Drupal\field\Plugin\field\field_type\LegacyConfigFieldItem;

Not ideal for something in Drupal\Core to depend on something in a module, but we can move the needed things from field module to Drupal\Core in a follow up. field.info.yml lists field.module as required, so this doesn't present a functional problem at the moment.

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/FieldDataTypeDerivative.php
@@ -0,0 +1,51 @@
+ * Provides data type plugins for each existing "configurable field" plugin.

s/"configurable field"/field type/

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/CountConstraint.php
@@ -0,0 +1,37 @@
+   * Overrides Range::validatedBy().

'Range' is incorrect. Couldn't this be changed to {@inheritdoc}?

+++ b/core/modules/field/field.attach.inc
@@ -872,28 +559,30 @@ function field_attach_form(EntityInterface $entity, &$form, &$form_state, $langc
+ *     corresponding field will be loaded, and the cache is bypassed. This

Reading from the cache isn't bypassed. Only writing to it is.

effulgentsia’s picture

Part 2. Up to field_ui.module.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.php
@@ -196,6 +194,13 @@ class Field extends ConfigEntityBase implements FieldInterface {
+   * The field type handler.
+   *
+   * @var \Drupal\field\Plugin\Type\FieldType\ConfigFieldItemInterface
+   */
+  protected $handler;

Not crazy about the name "handler" for this. Do we even need this property? Is it used anywhere?

diff --git a/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigEntityReferenceItemBase.php b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigEntityReferenceItemBase.php
new file mode 100644
index 0000000..1dadc77

As per earlier comments, I'd like us to rename all the new classes that have the abbreviated word "Config". Especially this one, given that the next word is "Entity". But, I also agree with earlier comments that we should punt that to a follow up, so as not to slow this issue down on a bikeshed.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigEntityReferenceItemBase.php
@@ -0,0 +1,169 @@
+    $target_id = $this->get('target_id')->getValue();
+    if (!empty($target_id) && is_numeric($target_id)) {
+      return FALSE;
+    }
+    if (empty($target_id) && ($entity = $this->get('entity')->getValue()) && $entity->isNew()) {
+      return FALSE;
+    }

In copying this from the old entity_reference_field_is_empty(), we lost a useful comment: "// Allow auto-create entities.".

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/PrepareCacheInterface.php
@@ -0,0 +1,30 @@
+interface PrepareCacheInterface extends ConfigFieldItemInterface {

Why does this interface need to extend that one? Can't it be standalone, and implementations can implement both?

+++ b/core/modules/field/lib/Drupal/field/Tests/TranslationTest.php
@@ -101,117 +101,6 @@ function testFieldAvailableLanguages() {
-   * Test the multilanguage logic of _field_invoke().
-   */
-  function testFieldInvoke() {
...
-   * Test the multilanguage logic of _field_invoke_multiple().
-   */
-  function testFieldInvokeMultiple() {

Looks like these tests were removed, but not replaced with any equivalent. Is that ok?

+++ b/core/modules/field_sql_storage/field_sql_storage.module
@@ -416,7 +416,8 @@ function field_sql_storage_field_storage_load($entity_type, $entities, $age, $fi
+          // Unserialize the value if specified in the column schema.
+          $item[$column] = (!empty($attributes['serialize'])) ? unserialize($row->$column_name) : $row->$column_name;
@@ -496,7 +497,10 @@ function field_sql_storage_field_storage_write(EntityInterface $entity, $op, $fi
+          // Serialize the value if specified in the column schema.
+          $record[$column_name] = (!empty($attributes['serialize'])) ? serialize($value) : $value;

This looks logical, but why is it made necessary by this patch? [Edit: never mind, I see that we're removing custom (un)serialization logic from link.module as part of this patch]

effulgentsia’s picture

And the 3rd and final part.

+++ b/core/modules/text/lib/Drupal/text/Plugin/field/field_type/TextItem.php
@@ -0,0 +1,104 @@
+class TextItem extends TextItemBase {

There's a bit of duplicated code between the classes that extend TextItemBase. Code that wasn't duplicated within the old procedural pseudo-hooks being replaced. Perhaps we should add more to the base class?

+++ b/core/modules/text/lib/Drupal/text/Plugin/field/field_type/TextItemBase.php
@@ -0,0 +1,109 @@
+      if ($this->getType() == 'field_item:text_with_summary') {
+        $this->set('safe_summary', text_sanitize($this->getInstance()->settings['text_processing'], $langcode, $itemBC, 'summary'));
+      }

On the flip side, why is this in the base class rather than the TextWithSummaryItem class?

+++ b/core/modules/text/lib/Drupal/text/Plugin/field/field_type/TextWithSummaryItem.php
@@ -0,0 +1,153 @@
+      $constraints[] = $constraint_manager->create('ComplexData', array(
+        'value' => array(
+          'Length' => array(
+            'max' => $max_length,
+            'maxMessage' => t('%name: the summary may not be longer than @max characters.', array('%name' => $this->getInstance()->label, '@max' => $max_length)),
+          )
+        ),
+      ));

Should this constraint be keyed on 'summary' rather than 'value'?

+++ b/core/modules/text/lib/Drupal/text/Plugin/field/widget/TextareaWithSummaryWidget.php
@@ -57,18 +58,8 @@ function formElement(array $items, $delta, array $element, $langcode, array &$fo
-  public function errorElement(array $element, array $error, array $form, array &$form_state) {
-    switch ($error['error']) {
-      case 'text_summary_max_length':
-        $error_element = $element['summary'];
-        break;
-
-      default:
-        $error_element = $element;
-        break;
-    }
-
-    return $error_element;
+  public function errorElement(array $element, ConstraintViolationInterface $violation, array $form, array &$form_state) {
+    return $element[$violation->arrayPropertyPath[0]];
   }

If I read this right, then in the 'default' case, the new code returns $element['value'] where the old code returned $element. Is that a desired change?

+++ /dev/null
@@ -1,136 +0,0 @@
-/**
- * Tests text field translation.
- */
-class TextTranslationTest extends WebTestBase {

Per #151, does removing this here require us to raise the priority of #2018707: Do we still need to support hook_field_prepare_translation() ? to critical?

+++ b/core/modules/text/text.module
@@ -217,11 +59,15 @@ function text_field_is_empty($item, $field_type) {
+
+  // Optimize by opting out for the trivial 'empty string' case.
+  if ($item[$column] == '') {
+    return '';
+  }

Why is this here? If we're making the assumption that check_markup() always returns an empty string if given an empty string, then shouldn't we add this code into check_markup() rather than into here?

effulgentsia’s picture

Status: Needs review » Needs work

The last submitted patch, d8_field_types-1969728-170.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
304.74 KB

Fixed the test failure.

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

The last submitted patch, d8_field_types-1969728-172.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review

#172: d8_field_types-1969728-172.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, d8_field_types-1969728-172.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review

#172: d8_field_types-1969728-172.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, d8_field_types-1969728-172.patch, failed testing.

effulgentsia’s picture

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

#172: d8_field_types-1969728-172.patch queued for re-testing.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

From my review in #167 - #169, I consider only two things to be potentially commit-blocking for this initial issue:
- That the performance regression shown in #1992138-187: Helper issue for "field types as TypedData Plugins" is awful.
- That we're knowingly introducing a D7 to D8 functionality regression with respect to translation, and removing the corresponding tests, without a clear answer as to whether that functionality needs to be restored in core, and if so, how. See #2018707: Do we still need to support hook_field_prepare_translation() ?.

However,
- It's likely that the performance regression is due to temporary BC layers and other EntityNG things we already have critical issues open for, and therefore, need to optimize anyway.
- #1810370: Entity Translation API improvements may solve, partially solve, or change the nature of the solution, for the translation question.

So, given that we're so close to API freeze, and there's other work that depends on or is affected by this going in, I'm RTBC'ing it, to let a core committer start the process of reviewing this rather large patch, and decide on whether to commit or not.

effulgentsia’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

Issue summary: View changes

added translation bug issue

fago’s picture

Issue summary: View changes

removed obsolete related patch

fago’s picture

I've been looking a bit into performance issues and made already considerable improvements over at #1992138: Helper issue for "field types as TypedData Plugins".

Also, I've addressed reviews of #167-#169 except for re-naming classes (follow-up) and improving text-field item's code separation.

Some comments on those:

-  function testFieldInvoke() {
...
-   * Test the multilanguage logic of _field_invoke_multiple().
-   */
-  function testFieldInvokeMultiple() {

Looks like these tests were removed, but not replaced with any equivalent. Is that ok?

I don't know really, but comments say they test multi-lingual logic of those functions - however that's work differently with EntityNG now. Thus, I guess they test only BC-layer and are therefore obsolete. We should probably add new, public field-invoke helpers and test those. Opened #2026323: Add field invocation helpers and test them

If I read this right, then in the 'default' case, the new code returns $element['value'] where the old code returned $element. Is that a desired change?

Not sure about this one.

Per #151, does removing this here require us to raise the priority of #2018707: Do we still need to support hook_field_prepare_translation() ? to critical?

If this makes it in before #2018707 gets resolved, then - yes I think so.

@@ -217,11 +59,15 @@ function text_field_is_empty($item, $field_type) {
+
+  // Optimize by opting out for the trivial 'empty string' case.
+  if ($item[$column] == '') {

Why is this here? If we're making the assumption that check_markup() always returns an empty string if given an empty string, then shouldn't we add this code into check_markup() rather than into here?

I think the previous comment was wrong. Even if it's optimizing check_markup() will need this logic in isEmpty() such that an empty text correctly leads to an empty field item, i.e. the item being not saved or rendered.

Attached is the interdiff for addressing the reviews. I'll post a new patch including those improvements here once test bot asserts everything's fine.

fago’s picture

Issue summary: View changes

another follow-up

fago’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
314.09 KB

ok, here is an updated patch.

It contains three interdiffs:
- the changes above
- diff2 - performance improvements by introducing a local cache for EntityNG::language()
- diff3 - avoid creating field item objects by making prepareView() optional
- diff 4 - forgot to adapt picture image formatter

Actually - the third one does not make prepareView(), but *removes* it. Details on that:

When try to make it optional I noticed that the two only implementations in core are for image + file fields - all others are using formatters. Image+file fields use it for pre-loading the referenced entities, while ER does exactly the same it does it in formatter prepareView in a base class. So looking at that, I think it makes a lot of sense to do it the ER-field way when common prepareView logic is needed: Provide a formatter base class for it.Thus, instead of adding the interface to conditionally have prepareView logic in the field class - attached patch moves that into according formatter-base classes, what makes a lot of sense to me.

Given all that change I ran performance tests on a page rendering 300 comments. While the patch from #172 resulted in performance regression of 38% (!!) on that page, this patch is actual an performance improvement of -4,96%. Memory stays about the same now.

8.x without patch
Page execution time was 5620.71 ms. Memory used at: devel_boot()=7.2 MB, devel_shutdown()=47.8 MB, PHP peak=55.75 MB.
Patch from #172
Page execution time was 7768.74 ms. Memory used at: devel_boot()=6.77 MB, devel_shutdown()=53.41 MB, PHP peak=61.5 MB. +38%
This patch
Page execution time was 5341.74 ms. Memory used at: devel_boot()=7.27 MB, devel_shutdown()=48.91 MB, PHP peak=56.25 MB. -4,96%

Please review the changes.

fago’s picture

Issue summary: View changes

handle caching

fago’s picture

Issue summary: View changes

updated summary and added follow-up

fago’s picture

Updated summary (no todo left) and added the following follow-ups:
#2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property
#2027059: Improve the documentation of WidgetBase::errorElement() for mapping violation property paths to form elements

Also, performance troubles are solved, yched +1ed moving the field prepare_view() callback to formatter base classes at #1992138-201: Helper issue for "field types as TypedData Plugins". Then, this blocks not only all the follow-up work and field-API improvements, but also important typed clean-up issues like #1867856: Use annotation discovery for data type plugins.

effulgentsia’s picture

I just did a spot check benchmark of #181 by running ab -c1 -n20 until the median number stabilized (caches warmed up).

Fresh standard profile install, created a node with 14 comments. Per #1992138-202: Helper issue for "field types as TypedData Plugins", I tried it with user pictures enabled and disabled.

HEAD:
default settings (comment_user_picture enabled): 545ms
comment_user_picture disabled: 485ms

Patch:
default settings (comment_user_picture enabled): 511ms
comment_user_picture disabled: 475ms

So indeed, it seems the latest patch completely removes the performance regression and actually results in a small improvement.

effulgentsia’s picture

FileSize
33.2 KB

For clarity, here's the total interdiff from #172 to #181.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Changes look good to me, so back to RTBC. The performance concern from #179 looks resolved. The translation API regression still remains, but IMO, even if critical, shouldn't block commit, because we can figure out what to do there after #1810370: Entity Translation API improvements also lands.

alexpott’s picture

Title: Implement Field API "field types" as TypedData Plugins » Change notice: Implement Field API "field types" as TypedData Plugins
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Okay the followups are filed and the patch looks great...

Committed a2c2367 and pushed to 8.x. Thanks!

clemens.tolboom’s picture

Just a note as we do not have head2head: I got this fatal while pulling on an active site.

PHP Fatal error:  Class '\\Drupal\\text\\Type\\TextSummaryItem' not found in /Users/clemens/Sites/drupal/d8/www/core/lib/Drupal/Core/TypedData/TypedDataFactory.php on line 60

That's logical as this issue remove the class. I'm curious how to solve this particular problem.

aspilicious’s picture

Reinstall, or run update.php to clear the caches.

effulgentsia’s picture

If update.php doesn't work (i.e., is where the error gets thrown), you can sudo rm -rf sites/default/files/php and truncate the {cache} and {cache_*} tables manually.

effulgentsia’s picture

fago’s picture

clemens.tolboom’s picture

@effulgentsia and @effulgentsia tnx. I'd never thought about running update.php to clear caches. Clearing files/php is in my normal workflow.

andypost’s picture

Please make a change notice, I need to re-implement #731724: Convert comment settings into a field to make them work with CMI and non-node entities on top of the issue

swentel’s picture

There are some instructions on #2014671: [META] Convert all field types to plugins - and also a link to a branch with some commits in it already (the only change is ConfigFieldBaseItem instead of CFieldItemBase). Text is also converted already.

fago’s picture

swentel’s picture

So, why did we put FieldDataTypeDerivative.php in core/lib/entity and not in core/modules/field ?

fago’s picture

Because of #2023563: Convert entity field types to the field_type plugin - it's not supposed to be config-field-only.

yched’s picture

klausi’s picture

Status: Active » Needs review

Draft change notice created: https://drupal.org/node/2064123

What else do we want to cover there?

andypost’s picture

@klausi suppose better to format functions with table and also point argument changes (suppose there's some)
Also #2051157-11: pass $has_data as a param to ConfigFieldItem::settingsForm() should be added too

yched’s picture

Priority: Critical » Major
Status: Needs review » Fixed
Issue tags: -Needs change record

Thanks @klausi, and sorry for the delay.
Added a couple things in https://drupal.org/node/2064123, and refactored it to be more similar to the other change notices about formatters as plugins & widgets as plugins.

jibran’s picture

Title: Change notice: Implement Field API "field types" as TypedData Plugins » Implement Field API "field types" as TypedData Plugins

Updating title.

yched’s picture

Status: Fixed » Closed (fixed)

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

yched’s picture

yched’s picture

Issue summary: View changes

Updated credited people

joachim’s picture

The patch for this issue seems to have removed field_attach_validate(), but the change record doesn't mention it.

andypost’s picture

There was added arrayPropertyPath property to each violation object which caused deprecation warning on PHP 8.2, now it needs to get rid of it in #3298731: Using ConstraintViolation::$arrayPropertyPath bugs on PHP 8.2