Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Depends on #1969728: Implement Field API "field types" as TypedData Plugins
Instructions are on #2014671: [META] Convert all field types to plugins
Comment | File | Size | Author |
---|---|---|---|
#119 | interdiff-115-118.txt | 2.07 KB | smiletrl |
#118 | taxonomy_field_followup-2015687-118.patch | 6.6 KB | smiletrl |
#115 | taxo_field_followup-2015687-115.patch | 5.43 KB | yched |
#109 | interdiff.txt | 3.73 KB | amateescu |
#109 | convert_taxonomy_field-2015687-109.patch | 32.94 KB | amateescu |
Comments
Comment #1
swentel CreditAttribution: swentel commentedComment #2
swentel CreditAttribution: swentel commentedComment #3
pfrenssenComment #4
pfrenssenThis is blocked until #2015701: Convert field type to typed data plugin for entity reference module gets in.
Comment #5
amateescu CreditAttribution: amateescu commentedOne additional thing that we need to do in this conversion is to revert the taxonomy field item class from extending
\Drupal\field\Plugin\Type\FieldType\ConfigEntityReferenceItemBase
, and make them extend\Drupal\Core\Entity\Plugin\DataType\EntityReferenceItem
and implement\Drupal\field\Plugin\Type\FieldType\ConfigFieldItemInterface
instead.Comment #6
BerdirThat went in, so unpostponing.
Comment #7
smiletrl CreditAttribution: smiletrl commentedLet's do this!
This is just a first try anyway. It includes the duplicate bug fix for #1758622: Provide the options list of an entity field.
See #2015689-44: Convert field type to typed data plugin for options module . Either this issue or option field issue gets commited, the other issue needs a reroll for this fix.
Comment #9
jibranI think we can inject these.
Comment #10
smiletrl CreditAttribution: smiletrl commented@jibran, that was part of the plan too! See #2083937: Make typed data class dependency injectable..
Comment #11
smiletrl CreditAttribution: smiletrl commentedFix
Comment #12
swentel CreditAttribution: swentel commentedNitpick: could use an extra space between AllowValueInterface and the curly bracket
We're losing valuable comment/documentation. Wondering whether we should bring that back. Could be on the contraint or not, dunno.
Same here re: loss of documentation, not sure where exactly where to put it.
Looks fine other than that.
Comment #13
swentel CreditAttribution: swentel commentedOne more thing though, use $element instead of $form.
Comment #14
smiletrl CreditAttribution: smiletrl commentedThanks for your review!
Add flatten function to handle possible group arrays, provided by 'list_options_callback'.
Comment #16
smiletrl CreditAttribution: smiletrl commented#14: taxonomy_field_type-2015687-14.patch queued for re-testing.
Comment #17
effulgentsia CreditAttribution: effulgentsia commented#14 does that, but why? I agree that the current implementation of ConfigEntityReferenceItemBase isn't what we want, as it is still based on supporting legacy hooks, but don't we still want this class conceptually, and use it for things that can be made common to ConfigurableEntityReferenceItem and TaxonomyTermReferenceItem: for example, some of schema(), the AllowedValuesInterface implementations, and getConstraints()?
Comment #18
amateescu CreditAttribution: amateescu commentedBecause that's the job of #1847596: Remove Taxonomy term reference field in favor of Entity reference. I've been told by @Berdir and @fago in the patches that converted taxo_ref, file and image to field item classes that if the extending class has to *remove* some properties from
getPropertyDefinitions()
, there's not much point in subclassing. Also, because ER has a totally different architecture than taxo_ref, and, as you said, we could use only a little part of it.Comment #19
effulgentsia CreditAttribution: effulgentsia commentedWell, if #1847596: Remove Taxonomy term reference field in favor of Entity reference makes it, great, but if not due to UX issues, I still think it would be good for taxo_ref to get onto the same code architecture as entity_ref, and either subclass it or have the two subclass a common base class. But that doesn't need to hold up this issue, so carry on.
Comment #20
BerdirI think the situtation back then was a bit different, because the only thing the field item classes did was define getPropertyDefinition(), so ending up with almost the same amount of code to remove/alter stuff that we don't need was a bit weird.
If we can actually re-use code, then I think it's fine to subclass from entity reference (or, alternatively, move common code in a common base class that both extend from). But I think we shouldn't focus on that here but do what is the easiest way to convert it.
As an example, ImageItem now extends from FileItem but it didn't do that initially I think (That said, file still has code that is not used for images, like the description, so a common base class approach there would also be an option).
Comment #21
smiletrl CreditAttribution: smiletrl commentedI re-thought this a bit since #17. One thing missed at #14 is it has missed the consideration of ConfigEntityReferenceItemBase.php. So here comes a new patch, which is in terms of following rules:
Methods -- preSave() and isEmpty() are What ConfigurableEntityReferenceItem and TaxonomyTermReferenceItem have in common. So they should be put in a "Base Item" class.Whether this "base Item" class should implement ConfigFieldIteminterface depends upon whether ConfigurableEntityReferenceItem, TaxonomyTermReferenceItem and FileItem have something in common regarding three methods in ConfigFieldIteminterface. Thing is they have nothing in common here! It doesn't make sense to implement this interface directly. But, if this "Base Item" class doesn't implement this interface, i.e, ConfigFieldIteminterface, then the other three derivative classes have do this. So, this "Base Item" class should implement ConfigFieldItemBase, both saving a lot of duplicate code, and not enforced to "actually" implement that three methods of ConfigFieldIteminterface.Reuse the code. TaxonomyTermReferenceItem doesn't need instanceSettingsForm(), so it would be good that the "Base Item" class have this method covered, i.e, extends ConfigFieldItemBase.This "Base Item" class should be a base item for all reference field items. It doesn't make sense that the two of referenced items(ConfigurableEntityReferenceItem and TaxonomyTermReferenceItem) use this "Base Item", while the others(fileItem) doesn't. So, I can't find the necessary existence of 'ConfigEntityReferenceItemBase', just deleting this class. And use 'EntityReferenceItem' as the "Base Item".As @Berdir mentioned above, we need to consider the easist way to convert here. This patch could not be the "easist" way, but it makes sence to me:) And of course, there're other ways to do this.As for #1847596: Remove Taxonomy term reference field in favor of Entity reference, I'd love to remain taxonomy field, instead of deleting it.
Otherwise, it doesn't make sense to have discussions here, if it's really going to be deprecated:)
Comment #22
smiletrl CreditAttribution: smiletrl commentedOops, missed schema()
Comment #24
smiletrl CreditAttribution: smiletrl commentedForget #21 to #23. It's a disaster that I've forgotten that EntityReferenceItem is also used as base field.
Here comes a new patch, based on #14.
The idea is to put the two common methods used by ConfigurableEntityReferenceItem and TaxonomyTermReferenceItem into the base item class, i.e., ConfigEntityReferenceItemBase.
Comment #25
smiletrl CreditAttribution: smiletrl commented#24: taxonomy_field_type-2015687-24.patch queued for re-testing.
Comment #26
smiletrl CreditAttribution: smiletrl commentedOK, no more comments, just rtbc myself:) I believe this is an major one, since it's blocking #2014671: [META] Convert all field types to plugins.
Comment #27
BerdirI'd like fago to review the validation changes, I don't really understand the what and why there.
Comment #28
smiletrl CreditAttribution: smiletrl commentedThe validation change comes from #1758622: Provide the options list of an entity field.
The original purpose for that issue is to get rid of the procedure code hook_options_list(). The solution is to introduce an " AllowedValuesInterface with methods to get all possible options for a field". So, in the field widget level, it only needs $fieldItem->getPossibleOptions() to get option values for this FieldItem, e.g., taxonomy_term, list_text.
Since there's an option for user to choose, then we should ensure user's selected values to be within the options. So here comes the new validator AllowedValuesConstraintValidator. This validator is essentially the same as ChoiceValidator. The *creepy* part is it overrides $constraint->choice in it's validating process. This means, if we are going to use this validator, we can't/don't have to provide choice for that constraint. So I get this *weird* constraint code for the taxonomy_term field, as well as options field at #2015689: Convert field type to typed data plugin for options module .
I only need to make sure getSettableValues() of this FieldItem returns appropriate values to make sure AllowedValuesConstraintValidator::validate() get the correct choice there.
Why it works in this way? I guess one thing is to keep consistency for the whole purpose of this new interface, i.e., all options/choices should be returned by that four methods, instead of some other customized values. The other thing is to cover the following use case, i.e, when the "field item"(in fact, it's not a field item, see following for details) doesn't have such getContraints() function to provide 'choice' for constraint.
However, that issue also covers the usage for a primitve typedData, i.e, class FilterFormat. To make sure this new introduced validator work for this DataType, it adds the default constraint(i.e., AllowedValues) for all typedData at TypedDataManager::getConstraints
This is not correct, as the original purpose for this issue is to take care of hook_options_list() for FieldItem. And this line will force FieldItem to validate against this validator, while we only need to validate 'Properties' of FieldItem, e.g., value. So I made such change
I can't say this solution is a good one, but there may be some other solutions. For example, we add 'Allowed values' constraint for property of 'filter_format' at long_text FieldItem. In this way, we don't have provide such default constraint at typedDataManager at all.
Basically, this is the story for such validation changes:)
Comment #29
fagoUsually we go with is_subclass_of(), which works with interfaces as well - that should a bit clearer. However, I do not see why this should be restricted to primitives?
Why do we have to move that over?
Extends 80 cars.
ok, now I see why you did it remove for primitives. Well, while it makes your use-case work I do not think this is the right solution. The problem really is that the AllowedValuesInterface implementation does not work well for $items as the value is not the value of the item, it's the value of target_id. Elaborating on what we could do about that right now.
Comment #30
smiletrl CreditAttribution: smiletrl commentedOne noticable thing is the new validator process is *very* simplified as compared to the default taxonomy_field_validate(). It only checks the options value, not considering the connections among the 'vocabulary', 'terms' or 'Parent' thing.
Comment #31
fagook, after some discussions with berdir and daspeter, we came up with the following approach: Let's add a way for complex data to define its "main property" for which then the allowed values apply.
The underlying problem here is that we use a lot of $item data structures, while we mostly care about 1 of its values. We already ran into the problem in many situations that we do not have a good way to know which of the field item properties is the main ones. We started defaulting to 'value', then we got 'target_id' and so now we sometimes special check for entity-references or use the heuristic of going with the first defined property. So instead of all that, let's allow complex data to define its main property explicitly and use that.
However, putting it on complex data right now sucks a bit as we'd have to move the method entities and everywhere. So instead, the attached patch adds it only to field items right now - then, when we have fixed typed data to not force you to put all the methods on your objects you can move it over.
Attached patch just implements the suggestion (untested), but merging things should help you.
Comment #32
smiletrl CreditAttribution: smiletrl commentedThis is to keep class ConfigEntityReferenceItemBase clean and only contains common methods, i.e., presave() and isEmpty(), used in general reference fields(i.e., ConfigurableEntityReferenceItem and TaxonomyTermReferenceItem). Other methods inside this base item classes should either be deleted(e.g., legacy methods), or moved to ConfigurableEntityReferenceItem(at current Drupal HEAD, ConfigurableEntityReferenceItem is the only one extending the base item class. I want to make TaxonomyTermReferenceItem extend it too to make use of that two common methods).
Comment #33
smiletrl CreditAttribution: smiletrl commentedRe#31
This makes sense for the big picture. But in this context here, it solves the problem in a fragile way imo.
1). It handles complexData as a special case. What if we encounter a new type 'Typed Data', which can't be handled in current case, shall we add a new special case here:)?
2). It asserts ONLY ONE main property to be validated. If a contrib FieldItem needs to validate three(>1) equally important properties, which share the same options of this FieldItem class, then this code here won't allow them use this constraint in the same time. One might be picked up as the main property to use this constraint, the other two have to use the conventional way of getConstraints() to create constraints for them. This loses consistentcy and looks a bit wierd. I guess it's insane for getMainPropertyName() to return an array()^^
So, can we treat the 'AllowedValues' as the common validator? Like what mentioned at #28
This would avoid the $item main property issue. We don't need to add the default contraint in TypedDataManager::getConstraints for AllowedValuesInterface. And it's pretty clear that if we want to add constraint for any properties of a FieldItem/ComplextData as AllowedValues, we add the constraint for targeted property in getConstraints(). AllowedValuesConstraintValidator::validate() remains the same too.
Comment #34
smiletrl CreditAttribution: smiletrl commentedoops, filter_format is a DataType andi implement allowedValuesInterface itself. We cann't add it as normal constraint way. But still,if we need more than one properties to use this constraint, then #31doesn't work well. Any chance to use the old proposed way:)?
Comment #35
smiletrl CreditAttribution: smiletrl commentedI will add a patch based on #31 soon. Maybe I've over-thought this a bit:)
Comment #36
smiletrl CreditAttribution: smiletrl commentedNope, #31 still doesn't work.
For taxonomy_term, its validation depends upon the type of 'target_id'. If target_id is 'automcreate', then we should not have the validation.
I will try to remove the default constraints from typedDataManager and add individual constraints for each DataType or FieldItem.
Comment #37
smiletrl CreditAttribution: smiletrl commentedHere it is.
Comment #38
BerdirI think we got rid of the autocreate stuff for entity references, shouldn't be needed anymore with the computed entity property. See ConfigurableEntityReferenceItem::preSave().
Instead, it should be NULL and then the allowed values validation should be skipped. Not 100% sure how that works with the required validation, I guess that would have to be intelligent enough to see a new enitty reference as not empty. Which is exactly what ConfigEntityReferenceItemBase::isEmpty() does right now.
Comment #39
fagoAs berdir says, auto_create should be gone already - thus removing that extra edge-case should be fine.
ComplexData is one of the foundation typed data interface, therefor it's completely ok to "special case" on it imo. We won't add special case for other complex-data cases, as those would have to implement the interface accordingly for properly using typed data api.
Yes. If the field item provides an options list, I cannot see how this would be applicable to more than property. And yes, having the options list return an $item array as value does not work, as array-keys can only be primitives.
If you have multiple field item properties with multiple different allowed values, you cannot have a single allowed-values implementation for all of them on the field anyway. But you can perfectly implement it by defining per field-item-property classes each implementing the interface accordingly. Yes, that means you have provide some more classes, but I don't see that as a problem as it's not the 90% case but more the 1% case. The important point is that 1% can be still implemented, while the 90% case is most straight-forward to implement.
Comment #40
smiletrl CreditAttribution: smiletrl commentedYeah, this is the problem that validation needs to be skipped when target_id is NULL.
Yeah, auto_create could be gone, but when target_id is NULL, the enforced validation will probably fail. Will add a patch to see.
A proposal is to do something like this:
But this is obviously a bad thing.
Comment #41
smiletrl CreditAttribution: smiletrl commentedComment #42
smiletrl CreditAttribution: smiletrl commentedAs suggested in #40, skip the validation if main property, e.g, target_id, is NULL.
Comment #43
smiletrl CreditAttribution: smiletrl commentedHmm, looks like we need to go with #37?
While #42 passes, it doesn't look right to me.
Comment #44
Berdir#42: convert_taxonomy_term_field-2015687-42.patch queued for re-testing.
Comment #46
smiletrl CreditAttribution: smiletrl commentedIt' blocking other validation issues, so it could be critical.
Rerolled two patches.
Personally speaking, I'd favor patch #37. There could be other possibilities that we need to make the validation conditional.
1). Taxonomy term field here. We only validate field when 'target_id' is NULL.
2). A conditional field. For example, a field contains two elements, when user chooses 'pre-defined' for first element, we need to validate the other element for options list. When user chooses 'Custom', we don't need to validate the pre-defined option list.
Comment #47
fagoIt's doable, but there are multiple approaches. Whatever, it's doable but what's the best approach for us should probably discussed in its own issue. -> Opened #2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered.
I don't follow, why would we? Required validation should be intelligent enough to deal with isEmpty() of the field. If it isn't yet, I guess we should do that instead.
However, I don't see why #40 would be so evil - allowed-values for complex data can only be validated in a way like this. On the other side, I don't see why we would like to remove auto-validation for allowed values once they are defined?
Comment #48
smiletrl CreditAttribution: smiletrl commentedGoog point:)
Will add
isEmpty()
condition into validator to see.Yes, there're still approaches to deal with conditional fields. From my perspective, only wish it could have more flexibility. But yeah, it's a good pattern to define such auto-validation rule. So, no objection. Let's do this!
Comment #49
smiletrl CreditAttribution: smiletrl commentedComment #51
smiletrl CreditAttribution: smiletrl commented@fago I've left you a message at IRC, do u like patch at #46 -- the rerolled patch of #42?
Comment #52
fagoYeah, I still think the #46-42 variant is best.
Should use the $typed_data variable as well, as we've it defined now. (my bad)
That's the behaviour of the parent validation, so these lines should go away.
Comment #53
fagoComment #54
smiletrl CreditAttribution: smiletrl commentedIf they go alway, it will probably fail, just like #41. See reason at #40.
Here're two patches, one is with these lines, the other one has removed these lines(this one should probably fail).
Comment #55
smiletrl CreditAttribution: smiletrl commented@fago thanks for your guide! Ok, new patch. Update target_id's value to 'NULL' when field widget deals with 'auto-create', so parent validation call will recognize it and pass.
interdiff is for the remove-lines patch at #54.
Comment #57
smiletrl CreditAttribution: smiletrl commentedComment #59
smiletrl CreditAttribution: smiletrl commentedNope, it still needs to use the old lines, i.e.,
'target_id' is defined as 'integer' data type in EntityReferenceItem::getPropertyDefinitions to identity the term:tid:
It will fail its own primitive validation if we enfore target_id's value to NULL. However, this 'old lines' solution has these potential problems:
So, I think if we need to make sure "auto-validation for allowed values", I suggest to not use AllowedValuesInterface for TaxonomyTermReferenceItem here. We use the regular Choice validator. Will add a patch to see.
ListItem for options module doesn't have this issue. It could use this interface well.
Comment #60
smiletrl CreditAttribution: smiletrl commentedThis patch uses the old choices to add constraint.
Comment #62
smiletrl CreditAttribution: smiletrl commentedOops, we can't use 'Choice' directly..
Comment #63
fagoNo, NULL is allowed for all constraints unless the ones specifically checking for it, i.e. NotNULL, NotBlank. Null means no value is there and ignored by all the constraint validators.
As the check for
(empty($value))
seems to be necessary, there must be a non-NULL empty value, e.g. 0 or FALSE. I don't think a reference should have that though, so we should figure out where this comes from.Comment #64
smiletrl CreditAttribution: smiletrl commentedSee #55 -- interdiff https://drupal.org/files/interdiff-54-55-remove-lines.txt. '0' comes from auto-create. I tried to replace '0' with NULL inside taxonomy module for target_id. There might be some other problems/values I didn't find...
Comment #65
smiletrl CreditAttribution: smiletrl commentedOk, I see what's the problem.
Basicly, we have to use (empty($value)) check, and return TRUE directly if this condition is met, i.e.,
The possible $value for target_id could be 0, which '0' is created when taxonomy field uses auto-create widget. But this check is a bad practice imho, see reason at #59
Therefore, I tried to avoid the check here, i.e., deleting the check code here. In that case, I need to replace target_id's value '0' with NULL, so parent validation call will recognize it and pass it automatically. I have made some progress at #55 and #57. But they all failed.
The reason they failed is when $value is NULL, i.e, when taxonomy field uses auto-create widget, $constraint->choices(or the allowed values option) could be an empty array too. When user starts to create the very first term in allowed vocabulary for a taxonomy field with free-tagging/auto-create widget, there's no terms available in the 'allowed vocabulary', so $choice for the volidator is an empty array. In ChoiceValidator::validate, it will check the $constraint->choices firstly, before check $value. It finds choices is not set(an ampty array), and it throws an exception error.
In conclusion, I feel it's not appropriate to use AllowedValuesInterface for TaxonomyTermReferenceItem, but use the regular choice constraint, like #60 and #62 do. I will try to figure out what's troubling with that approach, which shouldn't be that difficult imho.
Comment #66
smiletrl CreditAttribution: smiletrl commentedOk, this patch is moving forward for #62.
Comment #67
smiletrl CreditAttribution: smiletrl commentedRegarding #65,a new idea. We could delete the check code and rewrite ChoiceValidator::validate() in current validator to check $value before chp
Comment #68
smiletrl CreditAttribution: smiletrl commentedbefore check choices. sorry in my phone. Will add a patch to see.
Comment #69
smiletrl CreditAttribution: smiletrl commentedThis patch is moving forward for #57, not using the
(empty($value))
check. It's trying to replace target_id's value from '0' to 'NULL', and change the check order in validate(), i.e., check $value firstly. When $value is 'null', it returns directly.Comment #71
smiletrl CreditAttribution: smiletrl commented#69: convert_taxonomy_term_field-2015687-69.patch queued for re-testing.
Comment #72
smiletrl CreditAttribution: smiletrl commentedSummary here:
Patch #66 doesn't make TaxonomyTermReferenceItem use AllowedValuesInterface, and it works fine.
Patch #69 allows TaxonomyTermReferenceItem use AllowedValuesInterface, and it rewirtes AllowedValuesConstraintValidator::validate() a bit, resulting in some duplicate code.
Which one should be adopted then? Patch #69 looks more consistent with the "allowed values" pattern though.
Comment #73
Berdir#69: convert_taxonomy_term_field-2015687-69.patch queued for re-testing.
Comment #75
smiletrl CreditAttribution: smiletrl commented@berdir, at this point, I'd like to wait for #2023563: Convert entity field types to the field_type plugin got in. I don't want to reroll all the time, unless you want this issue get in firstly:)
Comment #76
smiletrl CreditAttribution: smiletrl commented@berdir, at this point, I'd like to wait for #2023563: Convert entity field types to the field_type plugin got in. I don't want to reroll all the time, unless you want this issue get in firstly:)
Comment #77
smiletrl CreditAttribution: smiletrl commentedThis is a reroll for #69.
Comment #79
smiletrl CreditAttribution: smiletrl commentedreroll
Comment #80
smiletrl CreditAttribution: smiletrl commented@fago, maybe this is ready?
Comment #81
smiletrl CreditAttribution: smiletrl commentedThis needs a reroll. But my computer is blocked to reroll this issue for #2115145: Move field type plugins to Plugin/Field/FieldType~
Comment #82
BerdirRe-roll.
Comment #83
smiletrl CreditAttribution: smiletrl commentedThanks for the reroll!
Now I think this is ready. Let's do this asap.
Comment #84
smiletrl CreditAttribution: smiletrl commentedSorry, didn't mean to rtbc myself. Who wants to rtbc:)?
Comment #85
jibranExpand to 80 chars.
Second line is not correct see https://drupal.org/node/1354#todo
OO
Comment #86
smiletrl CreditAttribution: smiletrl commented@jibran thanks for review! although I didn't get your idea about 1) and 3)^
Updated the doc for a few 80 chars thing. Did I miss something from 1), and what do you mean by 'OO' at 3)?
Comment #87
BerdirChanging title.
Comment #88
jibranAccording to 1354
we can move 'methods' world to above line it will not cross the 80 chars limit in comments.
Same here of can be moved to above line.
No need for these changes all the comments are well in 80 chars limit.
Comment #89
jibranAbout #85.3 Why are we using wrapper function? We can use
\Drupal::entityManager()->getStorageController('taxonomy_vocabulary')->loadMultiple();
directly. If we can't inject it right now then we have add @todo with issue id. So OO mean object oriented :D. Sorry for the confusion and thanks for all the changes which you have to revert according to #88 ;).Comment #90
smiletrl CreditAttribution: smiletrl commentedThanks for clarify that:)
Well, there're 81 characters in these lines..
This is the recommended way. See taxonomy_vocabulary_load_multiple. I don't have a strong opinion regarding thing though:)
Comment #91
jibranThis is ready to fly.
Comment #92
fagoIt seems there are some unrelated changes in FieldItemInterface here? Those comments are <= 80chars for me also?
I think this comment could explain things better, if I got it right, maybe something like the following?:
// The parent implementation ignores not set values, but makes sure some choices are available first. However, we want to support empty choices for not set values, e.g. if a term reference field points to an empty vocabulary.
null would have to be uppercase, but usually we do !isset($value) here.
I know those have been there before, but do we still need them? Afaik they are only there temporarily - thus should not be noted here. Should probably be its own issue though.
Else, patch looks good to me!
Comment #93
fagoSome other remarks:
- Why is auto-creation limited to ConfigEntityReferenceItemBase, and not part of EntityReferenceItem? That's not necessary part of this issue, but as it already moves some code around here it seems related.
- Could we move taxonomy_allowed_values() to a method on the TaxonomyTermReferenceItem class? It seems to be used only there.
Comment #94
smiletrl CreditAttribution: smiletrl commentedThanks for your review!
Re #92:
1). Well, before this change, there're 81 chars in this line..
2). Exactly. Fixed.
3). fixed. I copied code from parent::validate directly before:)
4). Created #2117635: Clean up ConfigEntityReferenceItemBase::getPropertyDefinitions(). Add @todo in this patch.
Re #93:
1). Not sure about this. Is there any use case that a base entity reference field could auto-create new entity? For example, node's uid could be used to create a new user? Or just like term reference field, uid could be null, and we provide another interface to create new user based on certain parameters, e.g., username?
On the other hand, these base entity reference fields don't have widgets prepared for them (atm, maybe they will have in future), as they are not configurable. And auto-created new entity is achieved by auto-complete field widget. So EntityReferenceItem doesn't have auto-create functionality imo.
2). Fixed. But I doubt taxonomy_allowed_values() could be used at Views plugin. Not check that yet, as there're problems with views for options module. We could figure out that when we work at Views.
Comment #95
smiletrl CreditAttribution: smiletrl commentedThis part looks a bit ugly. Any suggestion to make it better?
I was about to use
return this->taxonomy_allowed_values($instance, $entity);
, but it kind of loses consistency compared with above code.Comment #96
smiletrl CreditAttribution: smiletrl commentedClean code a bit.
Comment #97
amateescu CreditAttribution: amateescu commentedMerging #2117635: Clean up ConfigEntityReferenceItemBase::getPropertyDefinitions() into this patch.
Comment #98
fagoMissing space after if
This should just call the method?
That's actually not true, as it's just the default implementation. If a module provides a custom function, it's wrong.
Should follow the usual, naming pattern, i.e. be camel cased - but could use a rename in general. E.g. getDefaultOptions() ?
Views should not use the function, but the defined interface anyway.
Comment #99
smiletrl CreditAttribution: smiletrl commentedHi, thanks for your review!
Copied from IRC, since your connection is out there.
-> regarding 3). what do you mean by "If a module provides a custom function, it's wrong."? I see this flattenoptions function has been used for options field for group options, so I think maybe taxonomy field's custom function may provide group array options too?
Comment #100
smiletrl CreditAttribution: smiletrl commentedHere's one patch to rename "taxonomy_allowed_values" to "getDefaultOptions".
A contrib module could possibly use FieldSetting's "options_list_callback" function to provide custom options, which may contain group array, i.e., nested array. That's why I add
flattenOptions(array $array)
. By default, options module has provided this flatten options function, so I think it's nice to have this group support for taxonomy field too?Comment #101
fagoWrong indentation.
@flatten-options: I see, makes sense. Maybe flatten options should be a publically available static helper on an options class somewhere? It seems weird to require one to redefine that? We can look at this in a follow-up though.
Method name should match its documentation - the docs explain in now way why this is default, instead they tell me it's just about the options what is wrong. (yes it was that way before, but by re-naming it we really have to fix the docs also)
Comment #102
smiletrl CreditAttribution: smiletrl commentedHmm, I didn't find this indentation:)
@flatten-options: add @todo
@getDefaultOptions: add doc to explain why it is default.
Comment #103
tim.plunkettThere is FormBuilder::flattenOptions(), are you sure that is not sufficient?
EDIT: Of course, #2120841: Convert form_options_flatten() to a method on FormBuilder didn't go in yet, so that's a complete lie :)
form_options_flatten() is what I meant.
Comment #104
smiletrl CreditAttribution: smiletrl commented@tim.plunkett thanks.
We could probably use \Drupal::formBuilder()->flattenOptions($array); after #2120841: Convert form_options_flatten() to a method on FormBuilder is in.
Comment #105
BerdirRe-roll after the uuid default value stuff went in. Might be possible to unify the list class with the base entity reference field item list class now, I haven't checked.
Comment #106
smiletrl CreditAttribution: smiletrl commentedLet's leave flatten issue at #2138803: Overhaul field items' flattenOption
Comment #107
catchComment #108
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #109
amateescu CreditAttribution: amateescu commentedJust a few cosmetic changes: a few doxygen updates weren't needed and the @todos introduced here needed a link.
Comment #110
amateescu CreditAttribution: amateescu commentedThis looks ready to go, my additions were minor.
Comment #111
alexpottCommitted c2f76e0 and pushed to 8.x. Thanks!
Comment #112
alexpottWe haven't done a change notice for the other conversions on #2014671: [META] Convert all field types to plugins. Sorry for the noise
Comment #113
alexpottComment #114
yched CreditAttribution: yched commentedKudos for driving this home, folks !
Just wondering: looks like we now have two TaxonomyTermReferenceItem classes ?
Drupal\taxonomy\Type\TaxonomyTermReferenceItem
Drupal\taxonomy\Plugin\field\field_type\TaxonomyTermReferenceItem
?
Comment #115
yched CreditAttribution: yched commentedQuick followup patch:
- Removes the leftover Drupal\taxonomy\Type\TaxonomyTermReferenceItem (was used by the old taxonomy_field_info() implementation that got removed here)
- Removes TaxonomyTermReferenceItem::getDefaultOptions() and inlines it into getSettableOptions()
Unless I'm missing something, that's just striclty internal logic, and we don't really want to make it available for the outside since it bypasses the 'options_list_callback' ?
And it feels weird / misleading to have a method receiving a $field_definition and $entity as params, while the Item object already has its own, feels wrong. If we want to have such a method, it should be a static. But as mentioned above, it doesn't seem like there's an actual use ?
- Minor stuff: indent, getEntity(), remove stale comment...
Comment #116
amateescu CreditAttribution: amateescu commentedOops, I think the leftover class was a bad merge on my part. The other changes are ok too.
Comment #117
yched CreditAttribution: yched commentedAlso, posted a patch in #2156337: merge ConfigEntityReferenceItemBase up into EntityReferenceItem, and fix inconsistencies
Comment #118
smiletrl CreditAttribution: smiletrl commentedThanks for all the work! And yes #115 makes sense.
Here's a patch to clean the code more, from #2138803: Overhaul field items' flattenOption.
Comment #119
smiletrl CreditAttribution: smiletrl commentedsorry, forget the interdiff.
Comment #120
alexpottI have not committed the followup.
I don't mean to be pain in the ... but I think we're abusing the followup process and completely messing with issue stats. Following up a fixed critical with a normal (and one that is not beta blocking) doesn't actually help up at all. Created #2159145: Clean up following conversion of taxonomy field to FieldType plugin.