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.added as point to the follow-up #2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property
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 ;-) - 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
Comment | File | Size | Author |
---|---|---|---|
#184 | interdiff-172-181.txt | 33.2 KB | effulgentsia |
#181 | d8_field_types.patch | 314.09 KB | fago |
#180 | d8_field_types.patch.interdiff.txt | 15.49 KB | fago |
#172 | d8_field_types-1969728-172.patch | 304.74 KB | effulgentsia |
#172 | interdiff.txt | 1.39 KB | effulgentsia |
Comments
Comment #1
yched CreditAttribution: yched commentedAt 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.
Comment #2
fagoWe've the following "hooks" already covered by the entity field classes:
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?
Comment #3
yched CreditAttribution: yched commentedThe 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 ?
Comment #4
fagoI'd think so.
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.
Comment #5
fagoJust 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.
Comment #6
yched CreditAttribution: yched commentedSo 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 ?
Comment #7
fagoI'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!
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.
Comment #8
Berdir#1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory is supposed to fix that, needs a review.
Comment #9
yched CreditAttribution: yched commentedForgot 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...
Comment #10
yched CreditAttribution: yched commentedMore generally : do EntityNG data types have a notion of 'settings' of their own right now ?
Comment #11
yched CreditAttribution: yched commentedStill 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 :-)
Comment #12
yched CreditAttribution: yched commentedWhich 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.
Comment #13
andypostSuppose better to get rid of "duplicate" hooks
Then
And finally find a way for settings, load and prepare
Comment #14
fagoI think so as well, yes.
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?
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.
Yes, there is a 'settings' key with custom-content per class in the definition.
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.
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.
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).
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.
Comment #15
yched CreditAttribution: yched commentedFYI: working on the "TypedData-based" plan laid out above - not as fast as I wished though...
Comment #16
yched CreditAttribution: yched commentedRetitling
Comment #17
yched CreditAttribution: yched commentedInitial 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.
Comment #18
yched CreditAttribution: yched commentedPushed 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)
Comment #18.0
(not verified) CreditAttribution: commentedadded missing hooks
Comment #20
swentel CreditAttribution: swentel commentedLet Drupal install - see interdiff. Merged HEAD as well in sandbox.
Comment #22
swentel CreditAttribution: swentel commentedHrm, works locally though :/
- edit - minimal works, standard fails
Comment #23
fago:( @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.
Comment #24
swentel CreditAttribution: swentel commentedThis should install
Comment #25
yched CreditAttribution: yched commentedA 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.
Comment #26
yched CreditAttribution: yched commentedHeh, crosspost with #24 :-)
I think the fix is more along the lines of #25 though.
Comment #27
swentel CreditAttribution: swentel commented@yched of course, just wanted to see how the testbot behaves :)
Comment #28
swentel CreditAttribution: swentel commentedSo yeah, clearing the typed data definition works fine.
Also fixes a couple of 1000 notices.
Comment #30
yched CreditAttribution: yched commentedOpened #1992138: Helper issue for "field types as TypedData Plugins" to go wild with the tesbot without polluting this issue too much.
Comment #30.0
yched CreditAttribution: yched commentedUpdated for the current approach
Comment #31
yched CreditAttribution: yched commentedNote: branched validation work in the separate field-types-as-datatypes-1969728-validate branch in the sandbox
Comment #32
yched CreditAttribution: yched commentedNote: 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 :-/
Comment #33
plach[wrong issue]
Comment #34
yched CreditAttribution: yched commentedNote: the current patch in #1992138: Helper issue for "field types as TypedData Plugins" now actually triggers TypedData validation, which shows a couple issues with our current implicit constraints. Opened:
#2012662: Constraints on 'target_id' / 'tid' properties break autocomplete if applied (major)
#2012682: Constraints in getPropertyDefinitions generate violations without a delta (major)
#2012690: [Meta] Add useful information to 'type' constraints error messages
Comment #34.0
yched CreditAttribution: yched commentedlink to the testbot helper issue.
Comment #34.1
yched CreditAttribution: yched commentedadd 1st followup
Comment #34.2
yched CreditAttribution: yched commentedtypo
Comment #35
yched CreditAttribution: yched commentedNot 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.
Comment #36
yched CreditAttribution: yched commentedPosted an initial patch summary in the OP.
Comment #37
yched CreditAttribution: yched commentedSo 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()
Comment #38
yched CreditAttribution: yched commentedOne 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... :-/
Comment #39
yched CreditAttribution: yched commentedOr, 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)
Comment #40
yched CreditAttribution: yched commentedReroll
Comment #40.0
yched CreditAttribution: yched commentedcurrent state of the patch
Comment #41
yched CreditAttribution: yched commentedUpdated 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.
Comment #41.0
yched CreditAttribution: yched commentedAdded a related issue.
Comment #42
fagohm, 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.
Comment #43
yched CreditAttribution: yched commented@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.
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.
Agreed, but I'm not sure I feel comfortable going there in this patch...
Comment #43.0
yched CreditAttribution: yched commentedreflect progress
Comment #44
yched CreditAttribution: yched commentedStarting on prepare_view().
If someone is interested in implementing the custom ConfigurableFieldType annotation class, I could use help on that :-)
Comment #45
swentel CreditAttribution: swentel commented@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 :)
Comment #46
yched CreditAttribution: yched commented@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 ! :-)
Comment #46.0
yched CreditAttribution: yched commentedupdate dependent issues
Comment #47
yched CreditAttribution: yched commentedAdded #1986802: rename PluginInspectionInterface::getDefinition() to getPluginDefinition() as a blocker in the OP.
Comment #48
fago>@fago: this should not affect the regular load case,..
I see - then that should be fine! Thanks for explaining!
Comment #49
swentel CreditAttribution: swentel commented@yched re: annotation class see http://drupalcode.org/sandbox/yched/1736366.git/commitdiff/e60ea5613f70c...
Comment #50
swentel CreditAttribution: swentel commentedApparently 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...
Comment #51
swentel CreditAttribution: swentel commentedWe 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
Comment #52
yched CreditAttribution: yched commented@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.
Comment #53
yched CreditAttribution: yched commentedre: 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.
Comment #54
swentel CreditAttribution: swentel commentedre: 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()
Comment #54.0
swentel CreditAttribution: swentel commentedadded related issue
Comment #54.1
swentel CreditAttribution: swentel commentedUpdated issue summary.
Comment #55
yched CreditAttribution: yched commentedNote @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.
Comment #55.0
yched CreditAttribution: yched commentedRemove committed blocker
Comment #56
yched CreditAttribution: yched commentedRerolled + updated with @swentel's custom annotation class.
Comment #56.0
yched CreditAttribution: yched commentedadd blocker issue
Comment #57
yched CreditAttribution: yched commentedIncludes @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)
Comment #57.0
yched CreditAttribution: yched commentedremove done TODO
Comment #57.1
yched CreditAttribution: yched commentedRework the "related patches" section
Comment #58
yched CreditAttribution: yched commentedCreated #2018707: Do we still need to support hook_field_prepare_translation() ? to discuss the fate of hook_field_prepare_translation()
Comment #59
fagoOpend the somehow related issue #2019055: Switch from field-level language fallback to entity-level language fallback.
Comment #60
yched CreditAttribution: yched commentedRenamed 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
Comment #60.0
yched CreditAttribution: yched commentedadd related issue
Comment #61
yched CreditAttribution: yched commentedAs 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 ;-)
Comment #62
swentel CreditAttribution: swentel commentedCommitted some nitpicks, spaces and typo's on the branch.
I'm not sure exactly what that 'the/ key()' means - or I'm going mad, that's possible too :)
Comment #63
plachAll 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?
Comment #64
yched CreditAttribution: yched commentedChanges:
- 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()
Comment #65
yched CreditAttribution: yched commented@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.
Totally not the issue for this, but then maybe 'und' is not a great key name ?
Comment #66
plachMaybe 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.
Comment #67
yched CreditAttribution: yched commentedAh, upgrades :-/...
Thanks for testing !
Comment #68
yched CreditAttribution: yched commented@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 ???)
Comment #69
plachThe attached interdiff does the trick ;)
Comment #71
yched CreditAttribution: yched commentedOh, nice :-) Committed, will be in the next patch. Thanks !
Comment #72
yched CreditAttribution: yched commentedReroll (includes @plach's #69)
Comment #72.0
yched CreditAttribution: yched commentedupdate class names
Comment #74
yched CreditAttribution: yched commentedSo 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)
Comment #75
yched CreditAttribution: yched commentedSo, this leaves the fact that invokeFieldItemPrepareCache() currently iterates on languages with:
$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 ?
Comment #76
yched CreditAttribution: yched commentedFWIW: 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 :-)
Comment #78
yched CreditAttribution: yched commented#74: field-types-as-datatypes-1969728-74.patch queued for re-testing.
Comment #78.0
yched CreditAttribution: yched commentedRemove committed blockers
Comment #79
yched CreditAttribution: yched commentedAdded #75 as a "TODO" in the issue summary
Comment #80
andypost@yched #75 and the similar trouble we could get for comments with calling preview of parent entity - this needs better implementation
Comment #81
yched CreditAttribution: yched commented@andypost: hmm - doesn't seem related ?
Comment #82
Berdir@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.
Comment #83
plachWhat @Berdir said
(as always ;)
Comment #84
yched CreditAttribution: yched commented@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 ?
Comment #85
plach@yched:
This is the new code:
It should be quite faster. We could even optimize it a bit more, I think.
I think this should be fair :)
Comment #86
fagohm, it should be possible to work over getTranslationLanguages() to not issue instantiations of all the objects. I can have a look at that.
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:
Comment #87
fagoNote: Looking into validation issues now.
Comment #88
yched CreditAttribution: yched commented@plach / @fago: thanks !
I removed the @todos about getTranslationLanguages()
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.
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 ?
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 ?
Comment #88.0
yched CreditAttribution: yched commentedAdded a todo
Comment #88.1
swentel CreditAttribution: swentel commentedUpdated issue summary.
Comment #89
yched CreditAttribution: yched commentedAlso : #68 (location of invokeFieldMethod() / invokeFieldMethodMultiple() / invokeFieldItemPrepareCache()) still needs feedback :-)
(added it in the TODOs in the issue summary)
Comment #90
fagoSounds good to me.
Sure - that's great, just trying to agree upon possible follow-up tasks before you leave ;-)
Oh, indeed. Let's see how this evolves then...
Yep, that's fine with me. This mega patch should not be hold up by naming discussions anyway, renames can be totally follow-ups.
ah, sry I was about to comment it anyway, but forgot. So:
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.
Comment #91
yched CreditAttribution: yched commented@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 :-)
Comment #92
fagoAre 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.)
Comment #93
yched CreditAttribution: yched commentedYes 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...)
Comment #94
yched CreditAttribution: yched commentedMoved FieldInvoke*() to EntityStorageControllerBase + EntityStorageControllerInterface
Leaving at "do not test", because the poor bot already has troubles keeping up in the helper issue...
Comment #95
yched CreditAttribution: yched commentedMoved to the current state of #2018323: Make TypedData implement PluginInspectionInterface.
Comment #96
yched CreditAttribution: yched commented- 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:'
Comment #96.0
yched CreditAttribution: yched commentedadjust TODOs
Comment #97.0
yched CreditAttribution: yched commentedupdated related patches
Comment #98
yched CreditAttribution: yched commentedAnd 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.
Comment #99
fagoThe 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".
Comment #100
yched CreditAttribution: yched commentedMerged 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():
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.
Comment #102
yched CreditAttribution: yched commented- Fix incomplete revert of the PluginInspectionInterface stuff
- Removes _field_invoke_multiple() / _field_invoke_multiple_default(), not used anymore - yay !
Comment #104
yched CreditAttribution: yched commented#102: field-types-as-datatypes-1969728-102.patch queued for re-testing.
Comment #105
yched CreditAttribution: yched commentedre: 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.
Comment #107
yched CreditAttribution: yched commentedTemptative reroll after #1893772: Move entity-type specific storage logic into entity classes and #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets being tested in #1992138-141: Helper issue for "field types as TypedData Plugins" for now
Comment #108
yched CreditAttribution: yched commentedReroll 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.
Comment #108.0
yched CreditAttribution: yched commentedupdate TODO
Comment #108.1
yched CreditAttribution: yched commentedupdate status
Comment #109
yched CreditAttribution: yched commentedSo, 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 :-(.
Comment #110
fagoComment is outdated now.
Comment #111
fagoI take a look on those.
Comment #111.0
fagotypo
Comment #112
yched CreditAttribution: yched commentedAdded 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 ? ;-)
Comment #113
yched CreditAttribution: yched commented#110 - indeed, will fix in the next patch
#111 - thanks ;-)
Comment #114
yched CreditAttribution: yched commentedOK, 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:
Surprised that we don't have more fails, actually :-p
I'm afraid that's all for today...
Comment #116
fagoYes, 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.
Comment #117
yched CreditAttribution: yched commentedSo 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 ?
Comment #118
yched CreditAttribution: yched commentedLooked 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.
Comment #120
yched CreditAttribution: yched commentedWhoops, wrong diff command for the interdiff.
@fago - one question regarding
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 ?
Comment #121
yched CreditAttribution: yched commentedReroll after #2018731: Move field_has_data() to Field::hasData()
Comment #121.0
yched CreditAttribution: yched commentednew todo
Comment #123
fagoI 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 ;-)
Comment #124
fago#121: field-types-as-datatypes-1969728-121.patch queued for re-testing.
Comment #126
yched CreditAttribution: yched commentedForgot 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:
- invokeFieldItemPrepareCache:
- invokeFieldMethodMultiple():
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)
Comment #128
fagohm, it should not matter as the BC-decorator just forwards the call to the NG-entity anyway.
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.
Comment #129
yched CreditAttribution: yched commentedGee, HEAD moves fast...
Comment #130
yched CreditAttribution: yched commented@fago - yep, weird...
Comment #132
amateescu CreditAttribution: amateescu commented#129: field-types-as-datatypes-1969728-129.patch queued for re-testing.
Bot was out of disk space..
Comment #134
yched CreditAttribution: yched commentedOh 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.
Comment #135
amateescu CreditAttribution: amateescu commentedTook me 5 runs in another issue :P
Comment #136
effulgentsia CreditAttribution: effulgentsia commented#2012682: Constraints in getPropertyDefinitions generate violations without a delta landed, so this just removes those hunks.
Comment #137
effulgentsia CreditAttribution: effulgentsia commentedSorry, 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.
Comment #138
yched CreditAttribution: yched commentedre @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.
Comment #139
yched CreditAttribution: yched commentedre #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".
Comment #140
yched CreditAttribution: yched commentedReroll / updated after #1903346: Establish a new DefaultPluginManager to encapsulate best practices
Comment #141
yched CreditAttribution: yched commentedActually, #139 is a reminder that those getInstance() methods should be part of interfaces...
--> updated ConfigFieldItemInterface, added ConfigFieldInterface
Comment #142
yched CreditAttribution: yched commentedOops, ConfigField::getInstance() phpdoc should be an {@inheritdoc} too now
Just fixed that, no need to re-trigger tests.
Comment #143
yched CreditAttribution: yched commentedPeople in there might be interested in this : #2021817: Make widgets / formatters work on EntityNG Field value objects. :-)
Comment #144
yched CreditAttribution: yched commentedAs 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 :-)
Comment #145
plach:)
Comment #145.0
plachno more test fails !
Comment #145.1
yched CreditAttribution: yched commentedUpdate TODOs & blockers
Comment #146
yched CreditAttribution: yched commentedI'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...)
Comment #146.0
yched CreditAttribution: yched commentedcommit credits
Comment #147.0
(not verified) CreditAttribution: commentedlast update
Comment #148
yched CreditAttribution: yched commentedHah - never say die...
#2018323: Make TypedData implement PluginInspectionInterface got in, reroll.
Comment #149
Gábor HojtsyWhere 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.
Comment #150
fago@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.
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.
Comment #151
Gábor HojtsyIt 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.
Comment #152
fagoThat'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...?
Comment #153
fagoIt 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!
Comment #154
yched CreditAttribution: yched commentedFYI, 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
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
Comment #155
effulgentsia CreditAttribution: effulgentsia commentedI 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.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.Comment #156
fagoYes, let's do that. (I think we discussed that already at some point? :)
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?
Comment #157
effulgentsia CreditAttribution: effulgentsia commentedGreat! Do you want me to roll the patch, or do you have it?
Comment #158
fagoI take a stab on it.
Comment #158.0
fagomore specific
Comment #158.1
fagoadded follow-ups
Comment #159
fagoPosted 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
Comment #160
andypostSuppose this issue would help to solve #1751210: Convert URL alias form element into a field and field widget and make spart team happy with #1553358: Figure out a new implementation for url aliases
Comment #160.0
fagoanother follow-up
Comment #161
fagoSo thanks to effulgentsia this should be green again.
Interdiffs since the latest patch (#148) - besides re-rolls:
Comment #163
effulgentsia CreditAttribution: effulgentsia commentedSame 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.
Comment #164
andypostSuppose this change should get Gabor's approval because in #1966298: Introduce menu link bundles per menus we discussed that "fieldable" should not affect translation
Not sure that "fieldability" should affect translation. This was removed for menu_link entity that none-fieldable by default but needs translation
Comment #165
fago:-( 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.
Comment #166
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
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 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 ?
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...)
Comment #167
effulgentsia CreditAttribution: effulgentsia commentedPart 1 of my review. Up to field.module. Mostly nits so far, nothing that can't be done in a follow up.
Seems like the "have a single value" part of the comment is now obsolete.
Here and elsewhere, s/instanciate/instantiate/.
Let's make sure we have an issue filed for this.
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'.
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.
s/"configurable field"/field type/
'Range' is incorrect. Couldn't this be changed to {@inheritdoc}?
Reading from the cache isn't bypassed. Only writing to it is.
Comment #168
effulgentsia CreditAttribution: effulgentsia commentedPart 2. Up to field_ui.module.
Not crazy about the name "handler" for this. Do we even need this property? Is it used anywhere?
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.
In copying this from the old entity_reference_field_is_empty(), we lost a useful comment: "// Allow auto-create entities.".
Why does this interface need to extend that one? Can't it be standalone, and implementations can implement both?
Looks like these tests were removed, but not replaced with any equivalent. Is that ok?
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]
Comment #169
effulgentsia CreditAttribution: effulgentsia commentedAnd the 3rd and final part.
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?
On the flip side, why is this in the base class rather than the TextWithSummaryItem class?
Should this constraint be keyed on 'summary' rather than 'value'?
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?
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?
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?
Comment #170
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll for #1981314: Drop procedural usage of fields in field module.
Comment #172
effulgentsia CreditAttribution: effulgentsia commentedFixed the test failure.
Comment #174
effulgentsia CreditAttribution: effulgentsia commented#172: d8_field_types-1969728-172.patch queued for re-testing.
Comment #176
effulgentsia CreditAttribution: effulgentsia commented#172: d8_field_types-1969728-172.patch queued for re-testing.
Comment #178
effulgentsia CreditAttribution: effulgentsia commented#172: d8_field_types-1969728-172.patch queued for re-testing.
Comment #179
effulgentsia CreditAttribution: effulgentsia commentedFrom 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.
Comment #179.0
effulgentsia CreditAttribution: effulgentsia commentedUpdated issue summary.
Comment #179.1
fagoadded translation bug issue
Comment #179.2
fagoremoved obsolete related patch
Comment #180
fagoI'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:
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
Not sure about this one.
If this makes it in before #2018707 gets resolved, then - yes I think so.
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.
Comment #180.0
fagoanother follow-up
Comment #181
fagook, 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.
Comment #181.0
fagohandle caching
Comment #181.1
fagoupdated summary and added follow-up
Comment #182
fagoUpdated 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.
Comment #183
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #184
effulgentsia CreditAttribution: effulgentsia commentedFor clarity, here's the total interdiff from #172 to #181.
Comment #185
effulgentsia CreditAttribution: effulgentsia commentedChanges 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.
Comment #186
alexpottOkay the followups are filed and the patch looks great...
Committed a2c2367 and pushed to 8.x. Thanks!
Comment #187
clemens.tolboomJust a note as we do not have head2head: I got this fatal while pulling on an active site.
That's logical as this issue remove the class. I'm curious how to solve this particular problem.
Comment #188
aspilicious CreditAttribution: aspilicious commentedReinstall, or run update.php to clear the caches.
Comment #189
effulgentsia CreditAttribution: effulgentsia commentedIf 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.Comment #190
effulgentsia CreditAttribution: effulgentsia commentedAnother follow up: #2028175: Finish the removal of non-formatter's prepareView()
Comment #191
fagoAnd another one: #2028403: Mark field_attach_* functions as deprecated
Comment #192
clemens.tolboom@effulgentsia and @effulgentsia tnx. I'd never thought about running update.php to clear caches. Clearing files/php is in my normal workflow.
Comment #193
andypostPlease 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
Comment #194
swentel CreditAttribution: swentel commentedThere 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.
Comment #195
fagoOpened another follow-up: #2031203: Configurable fields should use applyDefaultValue()
Comment #196
swentel CreditAttribution: swentel commentedSo, why did we put FieldDataTypeDerivative.php in core/lib/entity and not in core/modules/field ?
Comment #197
fagoBecause of #2023563: Convert entity field types to the field_type plugin - it's not supposed to be config-field-only.
Comment #198
yched CreditAttribution: yched commentedNote : opened #2051177: Make existing ConfigFieldItem subclasses usable by base fields - needs feedback :-)
Comment #199
klausiDraft change notice created: https://drupal.org/node/2064123
What else do we want to cover there?
Comment #200
andypost@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
Comment #201
yched CreditAttribution: yched commentedThanks @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.
Comment #202
jibranUpdating title.
Comment #203
yched CreditAttribution: yched commentedOf interest to the people here: #2074547: Remove ConfigFieldItemInterface::getInstance()
Comment #205
yched CreditAttribution: yched commentedOops - #2086129: Leftover text_field_schema()
Comment #205.0
yched CreditAttribution: yched commentedUpdated credited people
Comment #206
joachim CreditAttribution: joachim commentedThe patch for this issue seems to have removed field_attach_validate(), but the change record doesn't mention it.
Comment #207
andypostThere 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