CommentFileSizeAuthor
#119 interdiff-115-118.txt2.07 KBsmiletrl
#118 taxonomy_field_followup-2015687-118.patch6.6 KBsmiletrl
#115 taxo_field_followup-2015687-115.patch5.43 KByched
#109 interdiff.txt3.73 KBamateescu
#109 convert_taxonomy_field-2015687-109.patch32.94 KBamateescu
#108 convert_taxonomy_field-2015687-108.patch34.21 KBamateescu
#105 convert_taxonomy_field-2015687-105-interdiff.txt2.01 KBBerdir
#105 convert_taxonomy_field-2015687-105.patch35.09 KBBerdir
#102 convert_taxonomy_field-2015687-102.patch33.92 KBsmiletrl
#102 interdiff-100-102.txt1.25 KBsmiletrl
#100 convert_taxonomy_field_2015687-100.patch33.62 KBsmiletrl
#100 interdiff-97-100.txt1.62 KBsmiletrl
#97 convert_taxonomy_field-2015687-97.patch33.63 KBamateescu
#97 interdiff.txt1.02 KBamateescu
#96 convert_taxonomy_field-2015687-96.patch34.07 KBsmiletrl
#96 interdiff-94-96.txt2.1 KBsmiletrl
#94 convert_taxonomy_field-2015687-94.patch34.13 KBsmiletrl
#94 interdiff-90-94.txt5.91 KBsmiletrl
#90 convert_taxonomy_term_field-2015687-90.patch31.59 KBsmiletrl
#90 interdiff-85-90.txt1.62 KBsmiletrl
#86 convert_taxonomy_term_field-2015687-85.patch31.59 KBsmiletrl
#86 interdiff-83-85.txt2.06 KBsmiletrl
#83 convert_taxonomy_term_field-2015687-83.patch30.23 KBsmiletrl
#83 interdiff-82-83.txt683 bytessmiletrl
#82 convert_taxonomy_term_field-2015687-82.patch30.24 KBBerdir
#79 convert_taxonomy_term_field-2015687-79.patch30.57 KBsmiletrl
#77 convert_taxonomy_term_field-2015687-77.patch29.9 KBsmiletrl
#69 convert_taxonomy_term_field-2015687-69.patch31.84 KBsmiletrl
#69 interdiff-57-69.txt2.64 KBsmiletrl
#66 convert_taxonomy_term_field-2015687-66.patch29.16 KBsmiletrl
#66 interdiff-62-66.txt2.14 KBsmiletrl
#62 convert_taxonomy_term_field-2015687-62.patch27.84 KBsmiletrl
#62 interdiff-60-62.txt955 bytessmiletrl
#60 convert_taxonomy_term_field-2015687-59.patch27.83 KBsmiletrl
#60 interdiff-54-59.txt3.89 KBsmiletrl
#59 convert_taxonomy_term_field-2015687-58.patch28.01 KBsmiletrl
#59 interdiff-54-58.txt956 bytessmiletrl
#57 convert_taxonomy_term_field-2015687-57.patch29.84 KBsmiletrl
#57 interdiff-55-57.txt1.25 KBsmiletrl
#55 convert_taxonomy_term_field-2015687-55.patch28.59 KBsmiletrl
#55 interdiff-54-55-remove-lines.txt777 bytessmiletrl
#54 convert_taxonomy_term_field-2015687-52-remove-lines.patch27.84 KBsmiletrl
#54 interdiff-46-52-remove-lines.txt1.35 KBsmiletrl
#54 convert_taxonomy_term_field-2015687-52.patch27.97 KBsmiletrl
#54 interdiff-46-52.txt919 bytessmiletrl
#49 convert_taxonomy_term_field-2015687-49.patch27.9 KBsmiletrl
#49 interdiff-46-49.txt955 bytessmiletrl
#46 convert_taxonomy_term_field-2015687-37-reroll.patch25.3 KBsmiletrl
#46 convert_taxonomy_term_field-2015687-42-reroll.patch27.91 KBsmiletrl
#42 convert_taxonomy_term_field-2015687-42.patch27.84 KBsmiletrl
#42 interdiff-41-42.txt1.66 KBsmiletrl
#41 convert_taxonomy_term_field-2015687-41.patch27.69 KBsmiletrl
#41 interdiff-24-40.txt8.33 KBsmiletrl
#37 convert_taxonomy_term_field-2015687-37.patch25.19 KBsmiletrl
#37 interdiff-24-37.txt2.78 KBsmiletrl
#31 d8_allowed_values_field.patch4.44 KBfago
#21 taxonomy_field_type-2015687-21.patch29.72 KBsmiletrl
#21 interdiff-14-21.txt17.99 KBsmiletrl
#24 taxonomy_field_type-2015687-24.patch24.29 KBsmiletrl
#24 interdiff-14-24.txt11.11 KBsmiletrl
#22 taxonomy_field_type-2015687-22.patch28.5 KBsmiletrl
#22 interdiff-21-22.txt2.36 KBsmiletrl
#14 taxonomy_field_type-2015687-14.patch15.61 KBsmiletrl
#14 interdiff-11-14.txt4.9 KBsmiletrl
#11 taxonomy_field_type-2015687-11.patch15.3 KBsmiletrl
#11 interdiff-7-11.txt2.92 KBsmiletrl
#7 taxonomy_field_type-2015687-7.patch15.26 KBsmiletrl
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Title: Convert field type to data plugin for taxonomy module » Convert field type to typed data plugin for taxonomy module
swentel’s picture

Status: Postponed » Active
pfrenssen’s picture

Assigned: Unassigned » pfrenssen
pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Active » Postponed
amateescu’s picture

One 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.

Berdir’s picture

Status: Postponed » Active

That went in, so unpostponing.

smiletrl’s picture

Status: Active » Needs review
FileSize
15.26 KB

Let'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.

Status: Needs review » Needs work

The last submitted patch, taxonomy_field_type-2015687-7.patch, failed testing.

jibran’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/field_type/TaxonomyTermReferenceItem.php
@@ -0,0 +1,187 @@
+    $vocabularies = entity_load_multiple('taxonomy_vocabulary');
...
+    $constraint_manager = \Drupal::typedData()->getValidationConstraintManager();

I think we can inject these.

smiletrl’s picture

@jibran, that was part of the plan too! See #2083937: Make typed data class dependency injectable..

smiletrl’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
15.3 KB

Fix

swentel’s picture

  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/field_type/TaxonomyTermReferenceItem.php
    @@ -0,0 +1,187 @@
    +class TaxonomyTermReferenceItem extends EntityReferenceItem implements ConfigFieldItemInterface, AllowedValuesInterface{
    

    Nitpick: could use an extra space between AllowValueInterface and the curly bracket

  2. +++ b/core/modules/taxonomy/taxonomy.module
    @@ -898,80 +866,6 @@ function taxonomy_field_widget_info_alter(&$info) {
    - * Taxonomy field settings allow for either a single vocabulary ID, multiple
    - * vocabulary IDs, or sub-trees of a vocabulary to be specified as allowed
    - * values, although only the first of these is supported via the field UI.
    - * Confirm that terms entered as values meet at least one of these conditions.
    

    We're losing valuable comment/documentation. Wondering whether we should bring that back. Could be on the contraint or not, dunno.

  3. +++ b/core/modules/taxonomy/taxonomy.module
    @@ -1023,71 +917,6 @@ function taxonomy_autocomplete_validate($element, &$form_state) {
    -/**
    - * @defgroup taxonomy_index Taxonomy indexing
    - * @{
    - * Functions to maintain taxonomy indexing.
    - *
    

    Same here re: loss of documentation, not sure where exactly where to put it.

Looks fine other than that.

swentel’s picture

Status: Needs review » Needs work
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/field_type/TaxonomyTermReferenceItem.php
@@ -0,0 +1,187 @@
+    $form = array();
+    $form['#tree'] = TRUE;

One more thing though, use $element instead of $form.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
4.9 KB
15.61 KB

Thanks for your review!

Add flatten function to handle possible group arrays, provided by 'list_options_callback'.

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

The last submitted patch, taxonomy_field_type-2015687-14.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
Issue tags: +Field API
effulgentsia’s picture

One 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.

#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()?

amateescu’s picture

#14 does that, but why?

Because 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.

effulgentsia’s picture

Well, 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.

Berdir’s picture

I 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).

smiletrl’s picture

I 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:

  1. Methods -- preSave() and isEmpty() are What ConfigurableEntityReferenceItem and TaxonomyTermReferenceItem have in common. So they should be put in a "Base Item" class.
  2. 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.
  3. 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.
  4. 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.

  • Taxonomy field has different field settings and allows more flexible ways to create taxonomy fields.
  • The referenced entity -- 'Terms' are organized in hierarchy, not just some random ordered entities, e.g., nodes.

Otherwise, it doesn't make sense to have discussions here, if it's really going to be deprecated:)

smiletrl’s picture

Oops, missed schema()

Status: Needs review » Needs work

The last submitted patch, taxonomy_field_type-2015687-22.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
11.11 KB
24.29 KB

Forget #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.

smiletrl’s picture

smiletrl’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

OK, 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.

Berdir’s picture

Assigned: Unassigned » fago
Status: Reviewed & tested by the community » Needs review

I'd like fago to review the validation changes, I don't really understand the what and why there.

smiletrl’s picture

The 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 .

+    if (!empty($target_id) && $target_id != 'autocreate') {
+      $constraints[] = $constraint_manager->create('ComplexData', array(
+        'target_id' => array(
+           'AllowedValues' => array(
+             'message' => t('%name: illegal value.', array('%name' => $instance['label'])), // This line could be omitted too.
+           ),
+        ),
+      ));
+    }
+    return $constraints;

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

  // Check if the class provides allowed values.
  if (array_key_exists('Drupal\Core\TypedData\AllowedValuesInterface', class_implements($class))) {
    $constraints[] = $validation_manager->create('AllowedValues', array());
  }

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

-    if (array_key_exists('Drupal\Core\TypedData\AllowedValuesInterface', class_implements($class))) {
+    if (array_key_exists('Drupal\Core\TypedData\AllowedValuesInterface', class_implements($class)) && array_key_exists('Drupal\Core\TypedData\PrimitiveInterface', class_implements($class))) {
       $constraints[] = $validation_manager->create('AllowedValues', array());

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:)

fago’s picture

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -378,7 +378,7 @@ public function getConstraints($definition) {
-    if (array_key_exists('Drupal\Core\TypedData\AllowedValuesInterface', class_implements($class))) {
+    if (array_key_exists('Drupal\Core\TypedData\AllowedValuesInterface', class_implements($class)) && array_key_exists('Drupal\Core\TypedData\PrimitiveInterface', class_implements($class))) {

Usually 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?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceItem.php
@@ -45,6 +43,40 @@ class ConfigurableEntityReferenceItem extends ConfigEntityReferenceItemBase impl
    */
+  public function getPropertyDefinitions() {

Why do we have to move that over?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigEntityReferenceItemBase.php
@@ -8,79 +8,17 @@
+ * Extends the Core 'entity_reference_field' entity field item with common methods

Extends 80 cars.

+  public function getConstraints() {
+    $constraint_manager = \Drupal::typedData()->getValidationConstraintManager();
+    $constraints = parent::getConstraints();
+
+    $target_id = $this->get('target_id')->getValue();
+    $instance = $this->getFieldDefinition();
+    if (!empty($target_id) && $target_id != 'autocreate') {
+      $constraints[] = $constraint_manager->create('ComplexData', array(
+        'target_id' => array(
+           'AllowedValues' => array(

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.

smiletrl’s picture

One 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.

fago’s picture

ok, 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.

smiletrl’s picture

Why do we have to move that over?

This 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).

smiletrl’s picture

Re#31

+  public function getMainPropertyName() {
+    return 'target_id';
+  }

This makes sense for the big picture. But in this context here, it solves the problem in a fragile way imo.

+    if ($typed_data instanceof ComplexDataInterface) {
+      $name = $typed_data->getMainPropertyName();
+      if (!isset($name)) {
+        throw new \LogicException('Cannot validate allowed values for complex data without a main property.');
+      }
+      $value = $typed_data->get($name)->getValue();
+    }

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

For example, we add 'Allowed values' constraint for property of 'filter_format' at TextItemBase FieldItem. In this way, we don't have provide such default constraint at typedDataManager at all.

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.

smiletrl’s picture

oops, 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:)?

smiletrl’s picture

I will add a patch based on #31 soon. Maybe I've over-thought this a bit:)

smiletrl’s picture

Nope, #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.

+    if (!empty($target_id) && $target_id != 'autocreate') {
+      $constraints[] = $constraint_manager->create('ComplexData', array(
+        'target_id' => array(
+           'AllowedValues' => array(
+             'message' => t('%name: illegal value.', array('%name' => $instance['label'])),
+           ),
+        ),
+      ));
+    }
+    return $constraints;

I will try to remove the default constraints from typedDataManager and add individual constraints for each DataType or FieldItem.

smiletrl’s picture

Berdir’s picture

I 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.

fago’s picture

For taxonomy_term, its validation depends upon the type of 'target_id'. If target_id is 'automcreate', then we should not have the validation.

As berdir says, auto_create should be gone already - thus removing that extra edge-case should be fine.

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:)?

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.

2). It asserts ONLY ONE main property to be validated.

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.

smiletrl’s picture

Instead, it should be NULL and then the allowed values validation should be skipped.

Yeah, this is the problem that validation needs to be skipped when target_id is NULL.

As berdir says, auto_create should be gone already - thus removing that extra edge-case should be fine.

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:

+    // If the data is complex, we have to validate its main property.
+    if ($typed_data instanceof ComplexDataInterface) {
+      $name = $typed_data->getMainPropertyName();
+      if (!isset($name)) {
+        throw new \LogicException('Cannot validate allowed values for complex data without a main property.');
+      }
+      $value = $typed_data->get($name)->getValue();
        // If main property's value is NULL, we make the validation pass.
        if (empty($value)) {
          return TRUE;
        }

+    }

But this is obviously a bad thing.

smiletrl’s picture

smiletrl’s picture

As suggested in #40, skip the validation if main property, e.g, target_id, is NULL.

smiletrl’s picture

Hmm, looks like we need to go with #37?

While #42 passes, it doesn't look right to me.

Berdir’s picture

Issue tags: -Field API

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

The last submitted patch, convert_taxonomy_term_field-2015687-42.patch, failed testing.

smiletrl’s picture

Priority: Major » Critical
Status: Needs work » Needs review
FileSize
27.91 KB
25.3 KB

It' 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.

fago’s picture

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.

It'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.

1). Taxonomy term field here. We only validate field when 'target_id' is NULL.

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?

smiletrl’s picture

Required validation should be intelligent enough to deal with isEmpty() of the field

Goog point:)
Will add isEmpty() condition into validator to see.

On the other side, I don't see why we would like to remove auto-validation for allowed values once they are defined?

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!

smiletrl’s picture

Status: Needs review » Needs work

The last submitted patch, convert_taxonomy_term_field-2015687-49.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review

@fago I've left you a message at IRC, do u like patch at #46 -- the rerolled patch of #42?

fago’s picture

Yeah, I still think the #46-42 variant is best.

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
@@ -20,10 +21,25 @@ class AllowedValuesConstraintValidator extends ChoiceValidator {
       $allowed_values = $this->context->getMetadata()->getTypedData()->getSettableValues($account);

Should use the $typed_data variable as well, as we've it defined now. (my bad)

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
@@ -20,10 +21,25 @@ class AllowedValuesConstraintValidator extends ChoiceValidator {
+        // If main property's value is NULL, we make the validation pass.
+        if (empty($value)) {
+          return TRUE;

That's the behaviour of the parent validation, so these lines should go away.

fago’s picture

Assigned: fago » Unassigned
smiletrl’s picture

That's the behaviour of the parent validation, so these lines should go away.

If 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).

smiletrl’s picture

@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.

Status: Needs review » Needs work

The last submitted patch, convert_taxonomy_term_field-2015687-55.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
29.84 KB

Status: Needs review » Needs work

The last submitted patch, convert_taxonomy_term_field-2015687-57.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
956 bytes
28.01 KB

Nope, it still needs to use the old lines, i.e.,

+        // If main property's value is empty, e.g., FALSE, NULL, 0, we make
+        // the validation pass.
+        if (empty($value)) { // or  if ($value === 0) {
+          return TRUE; // or $value = NULL;
+        }

'target_id' is defined as 'integer' data type in EntityReferenceItem::getPropertyDefinitions to identity the term:tid:

      static::$propertyDefinitions[$key]['target_id'] = array(
        // @todo: Lookup the entity type's ID data type and use it here.
        'type' => 'integer',
        'label' => t('Entity ID'),
        'constraints' => array(
          'Range' => array('min' => 0),
        ),

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:

  1. When the main property's type is integer, then '0' could be one of the options too. Taxonomy term is an exception here.
  2. Main property's type is boolean, so 'FALSE' could be one of the option too.

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.

smiletrl’s picture

This patch uses the old choices to add constraint.

Status: Needs review » Needs work

The last submitted patch, convert_taxonomy_term_field-2015687-59.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
955 bytes
27.84 KB

Oops, we can't use 'Choice' directly..

fago’s picture

Status: Needs review » Needs work

It will fail its own primitive validation if we enfore target_id's value to NULL.

No, 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.

smiletrl’s picture

there must be a non-NULL empty value, e.g. 0

See #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...

smiletrl’s picture

Ok, 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.,

+        // If main property's value is empty, e.g., NULL, 0, we make
+        // the validation pass.
+        if (empty($value)) { // or  if ($value === 0) {
+          return TRUE;
+        }

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

1). When the main property's type is integer, then '0' could be an useful value to be validated, i.e. '0' can't be passed always. Taxonomy field's target_id is an exception here.

2). When main property's type is boolean, so 'FALSE' could be an useful value to be validated too. We can't always make "FALSE" pass.

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.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
2.14 KB
29.16 KB

Ok, this patch is moving forward for #62.

smiletrl’s picture

Regarding #65,a new idea. We could delete the check code and rewrite ChoiceValidator::validate() in current validator to check $value before chp

smiletrl’s picture

before check choices. sorry in my phone. Will add a patch to see.

smiletrl’s picture

This 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.

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

The last submitted patch, convert_taxonomy_term_field-2015687-69.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
Issue tags: +Field API
smiletrl’s picture

Summary 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.

Berdir’s picture

Issue tags: -Field API

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

The last submitted patch, convert_taxonomy_term_field-2015687-69.patch, failed testing.

smiletrl’s picture

@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:)

smiletrl’s picture

@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:)

smiletrl’s picture

Status: Needs work » Needs review
FileSize
29.9 KB

This is a reroll for #69.

Status: Needs review » Needs work

The last submitted patch, convert_taxonomy_term_field-2015687-77.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
30.57 KB

reroll

smiletrl’s picture

@fago, maybe this is ready?

smiletrl’s picture

This needs a reroll. But my computer is blocked to reroll this issue for #2115145: Move field type plugins to Plugin/Field/FieldType~

Berdir’s picture

smiletrl’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
683 bytes
30.23 KB

Thanks for the reroll!

Now I think this is ready. Let's do this asap.

smiletrl’s picture

Status: Reviewed & tested by the community » Needs review

Sorry, didn't mean to rtbc myself. Who wants to rtbc:)?

jibran’s picture

  1. +++ b/core/lib/Drupal/Core/Field/ConfigEntityReferenceItemBase.php
    @@ -8,85 +8,14 @@
    + * Extends the Core 'entity_reference' entity field item with common
    + * methods used in general configurable entity reference field.
    
    +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
    @@ -136,4 +136,18 @@ public function delete();
    +   * Some field items consist mainly of one main property, e.g. the value
    +   * of a text field or the @code target_id @endcode of an entity reference.
    +   * If the field item has no main property, the method returns NULL.
    

    Expand to 80 chars.

  2. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
    @@ -136,4 +136,18 @@ public function delete();
    +   * @todo: Move this to ComplexDataInterface once we improved Typed data to do
    +   * not enforce having all methods on the data objects.
    

    Second line is not correct see https://drupal.org/node/1354#todo

  3. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Field/FieldType/TaxonomyTermReferenceItem.php
    @@ -0,0 +1,165 @@
    +    $vocabularies = entity_load_multiple('taxonomy_vocabulary');
    

    OO

smiletrl’s picture

@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)?

Berdir’s picture

Title: Convert field type to typed data plugin for taxonomy module » Convert field type to FieldType plugin for taxonomy module

Changing title.

jibran’s picture

According to 1354

Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, with a few exceptions (noted in the Tag Reference below).
  1. +++ b/core/lib/Drupal/Core/Field/ConfigEntityReferenceItemBase.php
    @@ -8,85 +8,14 @@
    + * Extends the Core 'entity_reference' entity field item with common
    + * methods used in general configurable entity reference field.
    

    we can move 'methods' world to above line it will not cross the 80 chars limit in comments.

  2. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
    @@ -131,9 +132,23 @@ public function delete();
    +   * Some field items consist mainly of one main property, e.g. the value
    +   * of a text field or the @code target_id @endcode of an entity reference.
    

    Same here of can be moved to above line.

  3. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
    @@ -15,8 +15,9 @@
    - * When implementing this interface which extends Traversable, make sure to list
    - * IteratorAggregate or Iterator before this interface in the implements clause.
    + * When implementing this interface which extends Traversable, make sure to
    + * list IteratorAggregate or Iterator before this interface in the implements
    + * clause.
    
    @@ -122,8 +123,8 @@ public function update();
    -   * This method is called during the process of deleting an entity, just before
    -   * values are deleted from storage.
    +   * This method is called during the process of deleting an entity, just
    +   * before values are deleted from storage.
    
    @@ -131,8 +132,8 @@ public function delete();
    -   * revision, just before the field values are deleted from storage. It is only
    -   * called for entity types that support revisioning.
    +   * revision, just before the field values are deleted from storage. It is
    +   * only called for entity types that support revisioning.
    

    No need for these changes all the comments are well in 80 chars limit.

jibran’s picture

About #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 ;).

smiletrl’s picture

Thanks for clarify that:)

No need for these changes all the comments are well in 80 chars limit.

Well, there're 81 characters in these lines..

Why are we using wrapper function?

This is the recommended way. See taxonomy_vocabulary_load_multiple. I don't have a strong opinion regarding thing though:)

jibran’s picture

Status: Needs review » Reviewed & tested by the community

This is ready to fly.

fago’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
    @@ -15,8 +15,9 @@
    + * clause.
    

    It seems there are some unrelated changes in FieldItemInterface here? Those comments are <= 80chars for me also?

  2. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
    @@ -20,10 +21,27 @@ class AllowedValuesConstraintValidator extends ChoiceValidator {
    +    // this check here to make sure $value is checked before $constraint.
    

    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.

  3. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
    @@ -20,10 +21,27 @@ class AllowedValuesConstraintValidator extends ChoiceValidator {
    +    if ($value === null) {
    

    null would have to be uppercase, but usually we do !isset($value) here.

  4. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/ConfigurableEntityReferenceItem.php
    @@ -25,6 +28,47 @@ class ConfigurableEntityReferenceItem extends ConfigEntityReferenceItemBase impl
    +      static::$propertyDefinitions[$key]['label'] = array(
    +        'type' => 'string',
    +        'label' => t('Label (auto-create)'),
    +        'computed' => TRUE,
    +      );
    +      static::$propertyDefinitions[$key]['access'] = array(
    +        'type' => 'boolean',
    +        'label' => t('Access'),
    +        'computed' => TRUE,
    

    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!

fago’s picture

Some 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.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
5.91 KB
34.13 KB

Thanks 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.

smiletrl’s picture

+    $function = $this->getFieldSetting('options_list_callback');
+
+    if($function) {
+      return call_user_func_array($function, array($instance, $entity));
+    }
+    else {
+      return call_user_func_array(array($this, 'taxonomy_allowed_values'), array($instance, $entity));

This 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.

smiletrl’s picture

Clean code a bit.

amateescu’s picture

fago’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Field/FieldType/TaxonomyTermReferenceItem.php
    @@ -70,8 +72,14 @@ public function getSettableValues(AccountInterface $account = NULL) {
    +    if($function) {
    

    Missing space after if

  2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Field/FieldType/TaxonomyTermReferenceItem.php
    @@ -70,8 +72,14 @@ public function getSettableValues(AccountInterface $account = NULL) {
    +      return call_user_func_array(array($this, 'taxonomy_allowed_values'), array($instance, $entity));
    

    This should just call the method?

  3. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Field/FieldType/TaxonomyTermReferenceItem.php
    @@ -162,4 +170,29 @@ protected function flattenOptions(array $array) {
    +   * Returns the set of valid terms for a taxonomy field.
    

    That's actually not true, as it's just the default implementation. If a module provides a custom function, it's wrong.

  4. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Field/FieldType/TaxonomyTermReferenceItem.php
    @@ -162,4 +170,29 @@ protected function flattenOptions(array $array) {
    +  public function taxonomy_allowed_values(FieldDefinitionInterface $field_definition, EntityInterface $entity) {
    

    Should follow the usual, naming pattern, i.e. be camel cased - but could use a rename in general. E.g. getDefaultOptions() ?

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.

Views should not use the function, but the defined interface anyway.

smiletrl’s picture

Hi, 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?

smiletrl’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
33.62 KB

Here'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?

fago’s picture

Status: Needs review » Needs work
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Field/FieldType/TaxonomyTermReferenceItem.php
@@ -0,0 +1,192 @@
+   // Flatten options firstly, because Settable Options may contain group
+   // arrays.
+    $flatten_options = $this->flattenOptions($this->getSettableOptions($account));

Wrong 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.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Field/FieldType/TaxonomyTermReferenceItem.php
@@ -175,7 +175,7 @@ protected function flattenOptions(array $array) {
+  public function getDefaultOptions(FieldDefinitionInterface $field_definition, EntityInterface $entity) {

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)

smiletrl’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
33.92 KB

Wrong indentation.

Hmm, I didn't find this indentation:)

@flatten-options: add @todo

@getDefaultOptions: add doc to explain why it is default.

tim.plunkett’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Field/FieldType/TaxonomyTermReferenceItem.php
@@ -0,0 +1,198 @@
+  protected function flattenOptions(array $array) {

There 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.

smiletrl’s picture

@tim.plunkett thanks.
We could probably use \Drupal::formBuilder()->flattenOptions($array); after #2120841: Convert form_options_flatten() to a method on FormBuilder is in.

Berdir’s picture

Re-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.

smiletrl’s picture

catch’s picture

Issue tags: +beta blocker
amateescu’s picture

Rerolled.

amateescu’s picture

Just a few cosmetic changes: a few doxygen updates weren't needed and the @todos introduced here needed a link.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

This looks ready to go, my additions were minor.

alexpott’s picture

Title: Convert field type to FieldType plugin for taxonomy module » Change notice: Convert field type to FieldType plugin for taxonomy module
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed c2f76e0 and pushed to 8.x. Thanks!

alexpott’s picture

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

We haven't done a change notice for the other conversions on #2014671: [META] Convert all field types to plugins. Sorry for the noise

alexpott’s picture

Title: Change notice: Convert field type to FieldType plugin for taxonomy module » Convert field type to FieldType plugin for taxonomy module
yched’s picture

Kudos 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
?

yched’s picture

Title: Convert field type to FieldType plugin for taxonomy module » [Followup] Convert field type to FieldType plugin for taxonomy module
Priority: Critical » Normal
Status: Fixed » Needs review
FileSize
5.43 KB

Quick 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...

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Oops, I think the leftover class was a bad merge on my part. The other changes are ok too.

yched’s picture

smiletrl’s picture

Thanks for all the work! And yes #115 makes sense.

Here's a patch to clean the code more, from #2138803: Overhaul field items' flattenOption.

smiletrl’s picture

FileSize
2.07 KB

sorry, forget the interdiff.

alexpott’s picture

Title: [Followup] Convert field type to FieldType plugin for taxonomy module » Convert field type to FieldType plugin for taxonomy module
Priority: Normal » Critical
Status: Reviewed & tested by the community » Fixed

I 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.

Status: Fixed » Closed (fixed)

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