Updated: Comment #75
Problem/Motivation/Current situation
Vision: Unify base and configurable fields and their handling.
For historical reasons, base field types (e.g. StringItem, IntegerItem, EntityReferenceItem) are directly exposed as Typed Data (with ID's like string_field, integer_field, entity_reference_field).
Configurable fields were ported to the @FieldType Plugin type. They use plugin ID's like text, email, entity_reference. They are then also exposed as typed data plugins as derivatives and are prefixed with field_item:, so e.g. field_item:text, field_item:entity_reference.
Proposed resolution
To be able to instantiate all field types using the field type plugin manager, base fields need to be exposed as @FieldType plugins too. To keep them apart, they have a configurable = TRUE/FALSE flag, that indicates if they *can* be used to create configurable fields, but they can also be used as base fields. Classes that want to support configurable fields need to implement ConfigFieldIteminterface (and extend from the corresponding base class), they also automatically get the corresponding ConfigFieldItemList list class.
This is the what this patch does, All *Item.php classes are moved from \Drupal\Core\Entity\Plugin\DataType to ..Plugin\field\field_type.
That results in a number of smaller and bigger problems:
- We get naming conflicts because. Notably the email and entity_reference field types currently have a simple version that's in Core and have modules that provide widgets/formatters and so on. The patch solves that problem in two different ways:
-- entity_reference.module alters the field type definition and replaces it using it's own implementation. This means that enabling entity_reference.module results in a different runtime class being used for base fields like $node->uid.
-- Same for EmailItem/email.module.
- We need to prevent non-configurable fields from showing up in the field UI. This patch adds a new method, getConfigurableDefinitions(), to the field type plugin manager to make those easily accessible.
- Base field names change from *_field to field_item:*. To avoid having to change ~10 baseFieldDefinition() implementation, this patches introduces a simply mapping for those names. This is temporary until #2047229: Make use of classes for entity field and data definitions lands, which will replace those magic naming patterns with speaking methods like $definition->getFieldType(). The mapping layer means that we don't have to change those methods twice.
- The fact that the configurable entity reference implementation is used resulted in some interesting test fails because they considered references to uid as a new entity and tried to auto-save it. This has been prevented by overriding isNew() for users and return FALSE for uid 0, which is a correct fix anyway.
- CommentItem changes: > It has field properties defined as entity_reference_field. This is completely bogus, a field item can't have field types as properties. It only works because we never instantiate it thanks to the lazy-loading logic in field items.
- Last, validation constraint messages are currently not dynamic, so configurable field type classes create a dynamic constraint that allows them to add a validation message during runtime. The result is that we suddently have two validation fails when an e-mail is too long in a test. This will be unified in https://drupal.org/node/2012690. It's not a new behavior, it would have already happened for configurable fields, now it also happens for base fields.
Remaining tasks
See follow-up issues references above.
We also need to continue with the approach discussed in Prague, to use the field type plugin manager for instantiating field item classes and decouble field item classes from TypedDataInterface. This is a blocker for additional work on that.
User interface changes
None.
API changes
As outlined above, the plugin ID's change.
Related Issues
Original report by @fago
#1969728: Implement Field API "field types" as TypedData Plugins defines a new field_type plugin, once it's in we can convert entity field types over to make use of it. However for that, we need to make field.module respect the "configurable" flag such that it does not deal with non-configurable fields.
Comment | File | Size | Author |
---|---|---|---|
#78 | convert_entity_field_type-77.patch | 48.13 KB | Berdir |
#78 | convert_entity_field_type-77-interdiff.txt | 976 bytes | Berdir |
#76 | convert_entity_field_type-75.patch | 48.1 KB | Berdir |
#73 | convert_entity_field_type-73.patch | 48.42 KB | Berdir |
#73 | convert_entity_field_type-73-interdiff.txt | 7.99 KB | Berdir |
Comments
Comment #1
fagoComment #2
fagoComment #3
fagook, here is a first patch doing the general changes necessary + converting the entity language field as a first test.
Comment #4
fagoPrevious patch was missing a git commit, attached is the right one - sry.
Note that this is not nice, but should be fixed automatically with #2041423: Rely on 'provider' instead of 'module' for Field plugin types.
Comment #5
yched CreditAttribution: yched commentedCould we move the array_filter logic in field_info_field_types() to a getConfigurableTypes() (or something) method in the manager ?
Comment #6
fagoGood point, but it's not getting a type plugin (instance) but a definition - so I guess that should be reflected in the naming? Or maybe, we just extend getDefinitions with an optional $configurable filter?
Then, I guess field_info_field_types() should be deprecated in favour of using the manager directly?
Comment #7
yched CreditAttribution: yched commentedBuild on getDefinitions() : yup, makes sense.
field_info_*_types() functions are totally up for deprecation, yes.
Comment #8
yched CreditAttribution: yched commentedPatch in #2047983: Mark field_info_*_types() / field_info_*_settings() functions as deprecated.
Comment #9
smiletrl CreditAttribution: smiletrl commentedRerolled patch at #4. Also applied changes for last comments.
hmm, should this be converted to
public function getConstraints()
for item class too?Also, maybe add doc like
Overrides \Drupal\Core\Plugin\DefaultPluginManager::getDefinitions().
forgetDefinitions($configurable = NULL)
?Next step is to convert all other item datatype under namespace
Drupal\Core\Entity\Plugin\DataType
? Are these item class used by other systems, not only used as base fields?Comment #11
yched CreditAttribution: yched commentedCross-posting #2052135: Determine how to deal with field types used in base fields in core entities
Comment #12
yched CreditAttribution: yched commentedIs there a way this can let us get rid of "FieldItem classes are exposed as FieldType plugins, then derived as DataType plugins, and instantiated through TypedDataManager" ?
As we talked about in Portland,
- this is really confusing (FieldItem::getPluginId() returns the id as a DataType rather than the id defined at the top of the class)
- this means duplicated static caches in FieldTypePluginManager & TypedDataManager
Comment #13
smiletrl CreditAttribution: smiletrl commentedHmm, not sure about this:
(FieldItem::getPluginId() returns the id as a DataType rather than the id defined at the top of the class)
Did a test:
Looks like FieldItem::getPluginId() has returned "id defined at the top of the class" when it comes to configurable fields. As for base fields atm, it should return id as DataType?
I guess duplicate caches comes from a field_type plugin cached/managed by two independent managers, i.e. FieldTypePluginManager & TypedDataManager at the same time. How about we make FieldTypePluginManager extends TypedDataManager, insead of DefaultPluginManager. So the two plugin managers could share the same cache backend?
Comment #14
yched CreditAttribution: yched commenteddsm($plugin_id); // Output => field_item:text_with_summary
That's what I'm talking about, 'field_item:text_with_summary' instead of 'text_with_summary'. The other example for integer_field is because "base field types" are currently not exposed as FieldType plugins, which exactly what this patch is about.
Comment #15
smiletrl CreditAttribution: smiletrl commented@yched, yeah, right, I see your point.
I think concerns at #12 deserves its own issue...
Anyway, two ideas here:
1). FieldTypePluginManager extends TypedDataManager to share the same cacheback. When FieldTypePluginManager needs to get plugin definition(s), it could filter the defintions(start with 'field_item') returned by TypedDataManager. This probably solves the duplicate cache issue, but TypedDataManager is still responsible for instantiate field items.
2). Override ItemList's createItem method for configfield. Here's a little patch to show this. It will definitely fail:)
Thing is TypedDataManager is probably also responsible for instantiating ConfigField too, or even entity itself?
Comment #16
smiletrl CreditAttribution: smiletrl commentedComment #18
yched CreditAttribution: yched commentedFieldTypePluginManager extends TypedDataManager would still lead to duplicate static cache of plugin definitions. The moment two separate manager services are exposed, two separate objects are created, each with its own 'cache' property.
Comment #19
yched CreditAttribution: yched commentedNote regarding the current patch:
A "configurable" field type already has to implement ConfigFieldItemInterface. So rather than requiring an additional explicit 'configurable' = TRUE property in the annotation, couldn't we automatically derive this from whether the class implements ConfigFieldItemInterface ?
Comment #20
BerdirRe-roll, no other changes yet.
Comment #22
BerdirRemoved a weird merge conflict.
Comment #23
BerdirOk, major update. Moving all field types over and adding bc layer to baseFieldDefinitions() that translates the type names, so that we don't have to change them all.
This seems to be working pretty fine and is an important preparation for using the field type plugin manager for field items.
One major problem is that we now have conflicts, e.g. entity_reference is defined twice, and we have things like date/datetime. Some could be merged (like email, which is weird anyway) but we need to rename one of the entity references...
Comment #25
BerdirAs an example, started to merge the emailitem classes together but failing due to the missing FieldDefinition, so postponed on #1994140: Unify entity field access and Field API access.
Comment #26
BerdirFalling back for base fields implemented, email field tests passed fine. Others are going to be tougher :)
Comment #28
BerdirFixing comment related tests.
Main problem at this point is all the entity reference classes, need to make sense of that stuff somehow.
Comment #30
BerdirFixing various tests, CommentItem shouldn't contain entity_reference_field subproperties, attempt to replace the used class for entity reference fields instead of defining separately. Seems to be working surprisingly well (just one weird test fail in the UI from what I can see). We'll be able to clean the class hierarchies up there later on and might be able to move more into the base class (I'd like to avoid another base class in between but we need to convert taxonomy first).
Comment #31
BerdirWrong patch.
Comment #33
BerdirKilled custom list_class definitions. This should hopefully fix those fails.
Comment #35
Berdir#33: d8_entity_field_type-2023563-33.patch queued for re-testing.
Comment #37
BerdirInteresting. Fixed the notices in field_help(), and the altering of the entity reference field item caused the auto-create/save to be triggered for uid 0. Added two separate fixes there, both should fix it but I think both makes sense on their own.
This is the kind of surprises we could get with this approach. Thoughts about it? I think it's probably the most controversal part about this issue.
The main problem is that the behavior of entity reference fields change based on whether entity_reference.module is enabled or not. Conceptually, that's not too different from what e.g. date.module does (altering the node create form).
Talked with @amateescu about the configurable property on in the @FieldType definition, this is not a problem as base field definitions are accessed through FieldDefinition, which has a hardcoded return FALSE in isFieldConfigurable(). The flag on that level is just that it's possible to create configurable fields with that type, e.g. it will show up in the UI.
Adding sprint tag and raising to major, this is necessary to continue with our typed data plans (Create field item classes through the field type plugin manager)
Comment #39
amateescu CreditAttribution: amateescu commentedSmall review of the code:
Extra empty line.
Boolean -> bool. The function paramater should default to FALSE instead of NULL.
And how about this text: "(optional) Whether to return only configurable field types. Defaults to FALSE."
Shouldn't this be uppercase? Same for all occurences..
Is this necessary because comment.module does not depend on entity_reference? Also, 'type' should be kept above 'label'.
"the" :)
About the entity reference field, I think I'm starting to like this approach.. at least it's better than having two different field type names.
What I'm still a bit worried about is: what does this mean for taxonomy/file/image reference field types which also extend the base entity reference field type (through ConfigEntityReferenceItemBase)? I think the answer is "nothing changes", but I'd still like a confirmation :)
Comment #40
fagoGreat work!
Should we do it that way even in the generic implementation? (Maybe as follow-up). The ER-change kind of makes the assumption already that only a NULL id means the id is unset.
Maybe the comment should just say that core components may be in there and are just skipped as they do not have help.
Maybe the comment should just say that core components may be in there and are just skipped as they do not have help.
Maybe say cannot be created via the UI, as configurable fields could be created via config also which does not necessarily map to "programmatically".
bool|null instead of Boolean? Also, it should mentioned default behavior.
Great to see that working, but could we stay with the old data types in the array for now?
This would simplify the work of the BC-layer in the class-based definitions patch as it would have to deal with only one case.
Should we do it that way even in the generic implementation? (Maybe as follow-up). The ER-change kind of makes the assumption already that only a NULL id means the id is unset.
Comment #41
BerdirThanks for the reviews!
@amatescu:
1. Fixed.
2. Fixed.
3. As discussed, CommentItem is a field item, it shouldn't have field item properties. I have no idea why this works :) Let's create a field item with lists of entities :p
4. Fixed.
Yes, for now, nothing changes. I'd like to reduce the hierarchy level that we have there, but not here and not until all of them are converted to @FieldType plugins.
@fago:
Update Dreditor! ;) Also looks like various points of your review are duplicated.
1. Moved it up, let's see if this breaks something.
2. Updated comment.
3. Not touching the configurable documentation. Right below is no_ui, which is specifically a configurable field that does not have a UI. We might unify them later on, but not now.
4. Yep, went with @amateescu's suggestion.
5. See the first 3.
Also fixed the tests. EntityFieldTest was trivial, had to go back to check !$target_id instead !== NULL as apparently target_id is 0 on auto-create, not sure who sets it. Anyway, in combination with the isNew() change, this should be fine. UserValidationTest is interested, the e-mail validation is just fine, the only problem is that we get two violations now in case of an invalid e-mail. It passes with the change I made, not sure if we can go on like this, I know there is an issue to fix that weirdness with translatable/dynamic violation strings..
Comment #43
BerdirInteresting, so making that the default didn't work. I'd suggest to consider that in a separate issue.
Comment #44
fagoFinally, done ;-)
Weird, but yeah - sounds good.
I was not suggesting to touch configurable documentation, but I think that this change is not necessarily true:
field types that are configurable, can not only be created programmatically but via config also. That's why I suggested to go with "cannot be created via the UI" instead of "can only be created programmatically.
Comment #45
BerdirI know what you meant, but "cannot be created via the UI" is exactly how the no_ui flag is documented below. And they're not the same. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
Comment #46
smiletrl CreditAttribution: smiletrl commentedThis is really impressive work!
Here's part review. Will check left later..
Let's remove the redundant one, i.e,
Annotation\Plugin
:)Maybe something like this to save code?
Maybe configurable = FALSE, and for other field types?
Translation has been deleted for all new converted Basic field_types? Maye we still need them?
And we don't need Drupal\Core\Entity\Annotation\FieldType?
During the converting process, some field types (boolean, email, float, integer, language, string, uri, uuid) get new id without '_field' suffix. Will that confuse the $propertyDefinitions's property type?
Like this 'boolean' field type, both its id and property's type are called 'boolean'. I sense property's type means DataType, instead of FieldType. But it might be better to reserve the suffix '_field' suffix for FieldType to distinguish it from DataType imo.
This doesn't look good:( Core will depend on field module. It's not a good infrastructure to me:) kind of regression.
I'd suggest to keep this email field at email module. Also, since it's configurable, I think we need to keep the old doc(and default widget), insead of " the 'email' entity field type.".
Any particular reason to not use entity reference field here?
Comment #47
BerdirThanks for the review. As I told @fago, numbered bullet point help to reference to your review, dreditor does that automatically now.
- Removed use.
- Simplified getDefinitions()
- Changed to uppercase FALSE, already discussed that above but forgot to apply.
- Annotation classes no longer need to be use'd, they just work.
- The BooleanItem has field type "boolean" and data type "field_item:boolean", Boolean has just data type "boolean". "field_item:boolean_field" would be repetitive, as would field type "boolean_field". That's like saying node is "node_entity". Also note that this is an intermediate step, #2047229: Make use of classes for entity field and data definitions will result in typed objects for the field definitions, and they will have a getType (or DataType()) method and a getFieldType().
- field is a required module. All of them are defined as a field/field_type plugin (that is defined core but uses the field namespace). That said, this, too, is a intermediate step. We in the process of moving the field plugin types to Core\Entity, together with all interfaces that are relevant. I think this is fine. email.module currently continues to provide the widget and formatter and does add that in an alter hook. As with entity_reference, we could add the configurable flag there, not sure. We should probably decide on a case by case base which modules still makes sense, I'd be happy to let email go but many others, more complicated ones might still make sense. We'll see.
- Everyone points that out, see #41.3 (the first).
Comment #48
BerdirOh, I understand why entity_reference_field worked. Because it's never used ;) Nothing ever did $node->comment_field->get('cid'), so it never even looked at the type but relied on the lazy-loading things to just write and get it from $this->values. It could be type = 'banana' and would work just as well ;)
Comment #49
smiletrl CreditAttribution: smiletrl commented1).
Thanks for clarifying that:) But does that still result in duplicate data cache? TypedDataManager and FieldTypeManager will still cache one field_item in the same time? I thought we are solving this problem in this issue?
2).
I'm not sure how $configurable and $no_ui work together.
I think we only need one? Maybe we still don't reach the consensus that a configurable field should be available at UI, while a non-configurable field should not. But the problem is we get all field types, including those base field types, available at field ui now. We have two date, integer, boolean, float fields etc, at field ui:)
Update:
And Email field type, even email module is not enabled, and I just tried to add
configurable = FALSE
to EmailItem annotation, it still shows in the field ui. In fact, it should not, becaue it doesn't have formatter and widget prepared for it.Comment #50
yched CreditAttribution: yched commentedno_ui was added in D7 for the case of a contrib module willing to include field types with some specific business logic, for which "add new ones to your heart's content" is not wanted, but benefitting on the flexibility of field API (configurable widgets & formatters). It does look like it somewhat duplicates "not configurable". I.e, those use cases would probably define "non configurable" field types now, and define the fields as base fields ?
Comment #51
smiletrl CreditAttribution: smiletrl commentedMore thinking..
1).
I feel it's not necessary here. ConfigFieldItemList should only contain config FieldItem, not base field items. If $instances[$this->getName()] may doesn't exist, then I will start to think maybe something went wrong:)
2).
Some lines might be avoided.
3). Regarding #2), any benifits of implementing it this way, i.e., use
$info['entity_reference']['class']
, instead of exposing ConfigurableEntityReferenceItem to FieldType directly? I thought one new independent FieldType might be more clearer?4).
I found this rule applies everywhere:) See Node::baseFieldDefinitions. Those properties types remains the old way in this patch, but they work well! Of course, something might be broken, but we didn't notice it from Tests inside Drual HEAD now.
Anyway, some have been updated, like
Assume they actually work(because currently, they don't work, i.e., type could be anything^^), I feel we should use
path
directly, instead offield_item:path
. One benifit of this issue I could see, is trying to distinguish FieldType from DataType. When we need to do, maybegetPropertyInstance
for Entity(this function is used at ContentEntityBase::getTranslatedField()), e.g., node, user, we could use something like,FieldTypeManager::createInstance()
orFieldTypeManager::create()
to instantiate these properties, instead of\Drupal::typedData()->getPropertyInstance()
.Comment #52
Berdir- Yes, this still results in duplicated caches, that will not change. What we will change is which plugin manager instantiates field item objects. that will change but not here. But we need to change this so that all field item classes are field type plugins to make that possible in a next step.
- Yes, the fact that base fields show up there is simply a bug. We extend the method to allow to return only configurable field types but we don't use that yet. We obviously need to, then they will go away. I'd prefer a getConfigurableDefinitions() or something instead of an optional argument, though. But that's an easy change, will change that on the next re-roll.
- No, no_ui and configurable are *not* the same. no_ui can still exist as configurable fields with a field config and field instance config to back them, create storage and so on. Yes, we might at some point be able to have pluggable storage for non-configurable fields too, but we don't have that now. So we can't unify them *yet*.
#51
1. It is necessary, the comment explains why. Once something can have configurable fields but is also used for base fields, we need to support both. As the @todo explains there, this is another temporary thing until we can inject them the right definition object from the plugin manager. Then they won't have to worry about it and will just do a return $this->definition.
2. This happens *after* processDefinition(), we need to specify the class. I tried ;) The constraint is duplicated, true.
3. Naming, then they'd need a separate name. entity_reference_base for the base one? entity_reference_configurable for the configurable? The first seems weird when you have to define it as a base field, the other one would mean that we have to change all code that references it somehow. That's why I went with extending it, but as mentioned above, this is the most controversial part but it seems like the best option to me.
4. No, they are very much not the same. The type of base fields is important, but there's a simple preg_replace() in EntityManager that converts *_field to field_item:*. Converting them here would make the patch twice as big and they will change anyway again in #2047229: Make use of classes for entity field and data definitions (to setFieldType('something') or so), so this is much easier. However, that BC layer only applies to base fields, that's why path has to change. And yes, path will have to change again (in the mentioned issue or the one that will switch to use the field type plugin manager, or both). And then it will change to just 'path'. So, yes, we came to the same conclusion as you in Prague, but we need to do it step be step.
Comment #53
BerdirMarking needs work for the configurable thing, will work on that later today.
Comment #54
smiletrl CreditAttribution: smiletrl commentedLooks like adding no_ui = TRUE works. When one class needs to serve both base field and configurable field, no_ui need to be updated too. See interdiff. Also, find a code stye thing
Need one more space for new line:)
I feel current name works fine, i.e., EntityReferenceItem and configurableEntityReferenceItem. But whatever, current code works well too:)
Comment #55
BerdirInstead of adding it manually, we could also add it in processDefinition() if configurable == FALSE. But I think we should just be specific about which kind of fields we want in the field UI's. Attached patch changes most calls that make sense to getConfigurableDefinitions().
Also made the email field item not configurable by default as we don't have a widget/formatter, similar to entity_reference.
About the naming, it's not the class name that's the problem, it's the plugin id. Right now, we have entity_reference_field for the base field and field_item:entity_reference for the other one, now they're both the same, just like e-mail. So we would have to rename one of them.
Also added some test coverage for this, the test patch should fail now. While writing these tests, I also noticed that the existing check for the no_ui flag there was broken as the selector didn't match the select anymore. The usual problem with negative assertions. It also has the same problem as #2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0, so fixed that too.
Comment #56
fagoPatch looks good to me. Some questions/remarks:
- Which email constraints get how doubled?
- ER fields as part of a comment item - sounds a bit like field collection to me ;-)
- Still not sure about the UI flag, let's try to discuss on IRC
Comment #57
BerdirOk, no_ui thing was a communication problem. We agreed on removing the second sentence, nothing useful/correct in there.
Added the link to an issue that should help with the @todo in the user validation test. The only problem there is that email item wants to have a better error message but itself only validates a subset of being a valid e-mail.
Comment #58
fagoThanks, this should be good to go then.
Comment #59
smiletrl CreditAttribution: smiletrl commentedGood work!
getConfigurableDefinitions()
is a good solution to handle the no_ui stuff.1. re#52
Regarding this, I still feel it's a bit wierd. No matter an item class is used by both base field and configurable field or not, ConfigFieldItemList should ALWAYS contain a list of configurable item class(even this item might be used for base field too, but it should still be configurable, otherwise we get big problems here I think.)
The scenario as you mentioned perhaps happens the other way round, i.e, for FieldItemList, it needs to support configurable
item class too. Or if there's a smiliar situation for FieldItemList as ConfigFieldItemList here, it's worthy to add support for configurable item class, because those "base field item class" could also be "configurable item class".
But in my first thinking, they should work independently in specific context, i.e., when it comes to ConfigFieldItemList, we only need to treat the item class as configurable item class; when it comes to FieldItemList, we only need to treat the item class as "base field item class". Am I right?:)
2.
Since EmailItem isn't configurable here, then we should probably dismiss ConfigFieldItemBase here. I'd suggest we
use the same trick as for entity_reference. In email_field_info_alter, add a line $info['email']['class'] = 'ConfigurableEmailItem', and then make
ConfigurableEmailItem extend EmailItem implement ConfigFieldItemInterface
. In this way, we still reuse the code, and only expose one 'field_item:email' to Drupal.3.
While this is deprecated, I guess we still need to update doc for this function if we update code like this. On the other
hand, I'm not sure we really need this update here.
4.
This line should probably be 'Non-configurable field type...'
otherwise, this patch looks good to go:)
Comment #60
Berdir1. I don't see why this is a problem. ConfigFieldItem(List) support configurable fields that are backed by a field instance. We can't have a dynamic class hierarchy, all fields that want to support that need to support both. I don't see a problem with that. And this code is temporary. They will just get a $definition object that implements FieldDefinitionInterface and is either FieldDefinition (base field) or a FieldInstance (configurable field). It is even possible that we will merge the additional methods of the config variants together into the normal ones.
2. This doesn't change anything. the replacement is permanent. As soon as entity_reference.module is enabled, *all* entity_reference fields, base field or not, use the extended version that supports configurable fields. The same would be the case for email. For entity_reference, the separation makes sense, because it has tons of features that are not relevant when used as a base field. For email, there are currently two simple methods, getConstraints() will go away when we solve the validation message problem and schema() method move up anyway. Also, that class would have to implement the method of ConfigurableFieldItemInterface itself instead of being able to rely on a base implementation.
3. Good point, I'm not sure. The direct replacement is getConfigurableDefinition() but we will probably remove this function before 8.0, so I don't think it's that important.
4. Yes, it should be. I thought I wrote that...
Comment #61
BerdirFixed 3. and 4.
Comment #62
smiletrl CreditAttribution: smiletrl commentedOk, since no body else argues against with 1) and 2), I'm fine with it too... Let's do this.
Comment #63
Berdir#61: d8_entity_field_type-2023563-61.patch queued for re-testing.
Comment #65
BerdirRe-roll. Tiny conflict in FieldInfoTest.
Comment #66
effulgentsia CreditAttribution: effulgentsia commentedThis patch looks awesome. Just some minor questions/feedback. Setting to "needs review" for #3 below. The other stuff can be follow up, unless a patch is being rerolled anyway.
Wouldn't this be more logical as $this->getDefinitions(), in case a getDefinitions() method is later added to this class?
A comment here explaining why EmailItem extends ConfigFieldItemBase even though it's configurable = FALSE would be helpful.
Why?
Would be good to add a comment here as to why this class doesn't have any annotation, despite being in the Plugin directory and being described as a "Plugin implementation ...".
Not this issue's fault, but $field_module seems like a terrible name for this local variable. Can we fix that while we're in here?
Comment #66.0
effulgentsia CreditAttribution: effulgentsia commentedUpdated issue summary.
Comment #67
Berdirre 3: Hah, you're like the fifth person to point that out. The old code was bogus, this is a field item, it can't contain other field items inside it :)
Added this to the issue summary:
See #48 why it worked.
The other points are totally valid, will re-roll tomorrow with some documentation improvements.
Comment #68
effulgentsia CreditAttribution: effulgentsia commentedThanks, back to RTBC then. If you reroll for the other stuff before commit, great. But if a committer wants to commit this before then, also great.
Comment #69
Berdir1. Change, left-over of this being an override of getDefinitions()
2. Added a comment.
4. Yeah. Moved out of the plugin directory and updated the documentation, better like this?
5. $plugin?
Comment #71
smiletrl CreditAttribution: smiletrl commentedThis might fix the error.
Comment #72
alexpottA couple of minors.
Not sure that this new comment to explain why EmailItem extends ConfigFieldItemBase makes sense.
Also I'm wondering why we are using two different patterns here - one for email and one for entity_reference where we swap out the entire class to make it configurable. Compare
email_field_info_alter
vsentity_reference_field_info_alter
Unnecessary as far as I can see.
Comment #73
BerdirOk, as discussed, here's an implementation that uses the same approach for email and extends the base email item class.
Somehow I thought the overhead would be bigger but we just need to implement two additional methods. This makes the user validation fix unecessary, the same problem will still happen when email.module is enabled but that's no longer visible in the test.
Off-topic: Anyone knows what the weird widget handling in email.module is supposed to do? Why does it use the text version when text.module is enabled (which it always is, so it never uses it's own?). I kept it because that's how it works right now but it makes zero sense.
Comment #74
Berdir#73: convert_entity_field_type-73.patch queued for re-testing.
Comment #75.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #76
BerdirRe-roll, updated issue summary. Who wants to RTBC? :)
Comment #77
smiletrl CreditAttribution: smiletrl commentedRe #73 interdiff
1.
FieldInterface seems unnecessary.
2.
I feel this constraint should be kept in EmailItem. As entity field type, EmailItem also needs to validate the length constraint.
3.
Wrong file definition:)
Comment #78
Berdir1. Fixed.
2. No, we don't. The length validation is only a small subset of validating an e-mail, we already have full e-mail validation by using type email for the ->value property. The constraint is only there because that pattern was added to all configurable fields, so that they contain the field name. The current actual patch doesn't have to touch that at all anymore, which I think is the best solution for us here.
3. Fixed.
Comment #79
fagoI looked at the changes since #69.
I could see the email field type class become a single one later one, while ER-field probably won't though. Anyway, taking the same approach for both field types seems to be a reasonable thing to do at least for now.
-> Recent changes look good to me, so back to RTBC.
Comment #80
alexpottCommitted 0f8b4e7 and pushed to 8.x. Thanks!
Comment #81
smiletrl CreditAttribution: smiletrl commentedCreate a record https://drupal.org/node/2111927.
As many changes in this patch are supposed to be temporary, it only specifies the difference/association between base field type and configurable field type.
Comment #82
BerdirI think we should extend and improve https://drupal.org/node/2064123 instead, that change notice doesn't make much sense on it's own. Will try to work on it tonight.
Comment #83
BerdirOk, updated https://drupal.org/node/1806650/revisions/view/2831231/2877845 and https://drupal.org/node/2064123/revisions/view/2862015/2877871.
Also updated the change notice by @smiletrl to clarify and reference the other two change notices: https://drupal.org/node/2111927/revisions/view/2876689/2877885
There will be at least two additional issues (changing baseFieldDefinitions and field definition structures to classes) and using the field type plugin manager to create field item classes, I'll continue to update those change notices as we go on.
Closing, thanks for all the reviews, see you all in #2047229: Make use of classes for entity field and data definitions!
Comment #84.0
(not verified) CreditAttribution: commentedUpdated issue summary
Comment #85
Berdir