Problem/Motivation

Unable to add list field types in views

In #1592632: Merge List field types into Options module, we moved List module to Options module. However, it looks like some traces of the former List module code remained in Field module. I don't know if we need to replace it with anything, but I think the current code is both broken and not being called from anywhere.

Proposed resolution

Patch the Views Module by allowing it to include list field types by:
-Creating new field plugins for rendering options list fields
-Creating a new FieldSettingsTrait to allow views plugins to easily fetch settings for a given field

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
None. Instructions

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#145 interdiff.txt2.89 KBdawehner
#145 2012130-145.patch39.9 KBdawehner
#141 interdiff.txt789 bytesdawehner
#141 2012130-141.patch39.78 KBdawehner
#139 interdiff.txt3.39 KBdawehner
#139 2012130-139.patch39.76 KBdawehner
#137 interdiff.txt2.2 KBdawehner
#137 2012130-137.patch39.79 KBdawehner
#132 Screenshot 2015-01-23 13.41.22.png41.43 KBdashaforbes
#132 Screenshot 2015-01-23 13.41.04.png20.68 KBdashaforbes
#130 2012130-list-views.129.patch39.39 KBlarowlan
#130 interdiff.txt603 byteslarowlan
#127 2012130-list-views.127.patch39.53 KBlarowlan
#127 interdiff.txt4.68 KBlarowlan
#124 interdiff.txt11.78 KBdawehner
#124 2012130-124.patch39.21 KBdawehner
#122 interdiff.txt2.11 KBdawehner
#122 2012130-122.patch32.46 KBdawehner
#120 2012130-120.patch32.56 KBpcambra
#114 interdiff.txt3.47 KBdawehner
#114 2012130-114.patch33.11 KBdawehner
#112 2012130-112.patch33.03 KBdawehner
#112 interdiff.txt13.71 KBdawehner
#108 interdiff-2012130-108.txt26.71 KBdamiankloip
#108 2012130-108.patch33.04 KBdamiankloip
#101 2012130-101.patch36.36 KBdamiankloip
#101 interdiff-2012130-101.txt13.67 KBdamiankloip
#96 interdiff-2012130-96.txt730 bytesdamiankloip
#96 2012130-96.patch30.97 KBdamiankloip
#90 2012130-89.patch1.17 KBmgifford
#82 interdiff-2012130-82.txt8.55 KBdamiankloip
#82 2012130-82.patch31.11 KBdamiankloip
#73 vdc-2012130-73.patch31.79 KBpcambra
#70 vdc-2012130-70.patch31.79 KBwebflo
#68 vdc-2012130-68.patch31.75 KBwebflo
#67 vdc-2012130-67.patch30.31 KBwebflo
#62 vdc-2012130.patch40.98 KBdawehner
#57 vdc-2012130-57.patch40.9 KBdawehner
#57 interdiff.txt3.74 KBdawehner
#55 views_integration_list_field-2012130-55.patch40.8 KBsmiletrl
#55 interdiff-53-55.txt4.86 KBsmiletrl
#53 views_integration_list_field-2012130-52.patch40.48 KBsmiletrl
#53 interdiff-50-52.txt8 KBsmiletrl
#51 views_integration_list_field-2012130-50.patch31.49 KBsmiletrl
#51 interdiff-47-50.txt12.87 KBsmiletrl
#48 views_integration_list_field-2012130-47.patch31.49 KBsmiletrl
#48 interdiff-46-47.txt541 bytessmiletrl
#46 views_integration_list_field-2012130-46.patch31.49 KBsmiletrl
#46 interdiff-37-46.txt1.73 KBsmiletrl
#38 views_integration_list_field-2012130-37.patch31.51 KBsmiletrl
#38 interdiff-34-37.txt9.55 KBsmiletrl
#34 views_integration_list_field-2012130-34.patch31.34 KBsmiletrl
#34 interdiff-24-34.txt18.1 KBsmiletrl
#32 views_integration_list_field-2012130-32.patch31.34 KBsmiletrl
#32 interdiff-24-32.txt18.1 KBsmiletrl
#25 views_integration_list_field-2012130-24.patch13.24 KBsmiletrl
#25 interdiff-22-24.txt6.7 KBsmiletrl
#23 views_integration_list_field-2012130-22.patch11.91 KBsmiletrl
#23 interdiff-13-22.txt5.98 KBsmiletrl
#13 views_integration_list_field-2012130-10.patch16.19 KBsmiletrl
#13 interdiff-3-10.txt13.73 KBsmiletrl
#9 views_integration_list_field-2012130-3.patch17.19 KBsmiletrl
field-list-removal-cleanup.patch6.81 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

smiletrl’s picture

In field.module function field_help($path, $arg) It mentions List module is required. Probably it needs deleted too:)

This patch removes two views arguments and one filter. It doesn't smell right to me, because I can't find the replacement at other places, including options module.

On the other hand, I tried to create 2 list fields (List--integet and List--text) and use the to fields as contextual filters. I thought that two arguments should have some impact on this. But it didn't. Everything looks the same after I applied this patch. I didn't find the settings at the two arguments, e.g.,

-    parent::buildOptionsForm($form, $form_state);
-
-    $form['summary']['human'] = array(
-      '#title' => t('Display list value as human readable'),
-      '#type' => 'checkbox',
-      '#default_value' => $this->options['summary']['human'],
-      '#states' => array(
-        'visible' => array(
-          ':input[name="options[default_action]"]' => array('value' => 'summary'),
-        ),
-      ),
-    );

I didn't find this checkbox setting for both two fields I just created.

So my question is in what context which argument will be used? Ididn't find the answer, even for this Date argument It is obviously used for time-field, but I can't see the context in code that this argument should be used.

So is this patch. I can't tell how/when/where the three plugins should be used. So I can't tell should they be removed.

smiletrl’s picture

Issue tags: +VDC-integration

Adding tag

yched’s picture

The blame annotations on function list_field_views_data($field) { (merlinofchaos 2009-05-17 "Add the 7.x-3.x Views branch") seem to indicate that this comes straight from the direct merge of the D7 views code that was done at the beginning of the "VDC / Views in core" effort.

list.module has been removed/merge into options.module in july, so probably before the VDC work started.

So this most probably never worked in D8 - but it's still something that's *meant* to be there...
Filters on List fields need to be able to provide a list of available options for the admin UI (or exposed filters).
Less sure about what the custom argument handlers do - at least they provide the text replacement in the view title when the argument is provided ('option_1' --> "The human label for option_1"

yched’s picture

Component: field system » options.module

Also, I guess the list_field_views_data() function currently lies directly in field.views.inc rather than in options.module (and the associated handler classes live under field.module's lib folder) - as opposed to, say, image fields views integration being provided by image.module, is precisely because list.module had disappeared when the views folks imported the code.

@effulgentsia: so this currently never called.
The right fix would be to move that function & the classes over to options.module, rename to options_field_views_data().
But then it would be active again.
What do you think is easier on #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets ? "unbreak" this now, or leave it broken and care about it after #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets ?

yched’s picture

Title: Remove traces of List module » Views integration for "list" field types is broken

More accurate title.

effulgentsia’s picture

Title: Views integration for "list" field types is broken » Regression: Views integration for "list" field types is broken
Category: task » bug
Priority: Normal » Critical

I don't plan on working on this until after #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets, but if someone else gets this fixed before then, then I'll update that issue to work with it.

If this is a regression from something that works in D7 Views, this probably needs to be a critical bug. Unless it's considered a minor enough thing that it can be deprioritized, but I'll leave that call to others.

effulgentsia’s picture

Status: Needs review » Active

Oh, and setting status to "active", since the original patch isn't remotely the right solution :)

smiletrl’s picture

Assigned: Unassigned » smiletrl

@yched has got the point -- Perhaps list_field_views_data should be renamed and moved to options module, which explains why the three views plugins not working. But in D7, they work well.

@effulgentsia I will try to fix this according to instructions at #3 and #4, if this makes your life easier:)

smiletrl’s picture

Status: Active » Needs review
FileSize
17.19 KB

Ok, this patch works well for me locally. Let's see if it pleases bot.

yched’s picture

Thanks @smiletrl !
Code looks reasonable. I'll let @effulgentsia chime in whether it makes things easier to fix this now or later.

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/FieldList.php
@@ -0,0 +1,76 @@
+class FieldList extends Numeric {
+++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/ListString.phpundefined
@@ -0,0 +1,78 @@
+class ListString extends String {

FieldList / ListString - not too consistent. That was already the case, but maybe we should fix that while we're here ?

What about:
NumberListField extends Numeric
TextListField extends String
ListField extends ManyToOne ?

+++ b/core/modules/options/options.views.incundefined
@@ -0,0 +1,35 @@
+ * @ingroup views_module_handlers

Shouldn't there be a matching @endgroup then ?

+++ b/core/modules/views/lib/Drupal/views/ManyToOneHelper.phpundefined
@@ -309,7 +316,7 @@ function add_filter() {
-          $this->query->add_where_expression(0, "$field $operator($placeholder)", array($placeholder => $value));
+          $this->handler->query->add_where_expression(0, "$field $operator($placeholder)", array($placeholder => $value));
         }

Does this really belong here ?
If the previous code is incorrect, then it looks like other existing filters are currently broken ?

smiletrl’s picture

Thanks for review!

1) Yeah, we could do better with name consistency. But should we add module name prefix, like what taxonomy module does? like
OptionsNumberListField extends Numeric
OptionsTextListField extends String
OptionsListField extends ManyToOne ?

I hope the length wouldn't be that large:)

2) Although '@endgroup' sounds reasonable, it's not in the list https://drupal.org/node/1354:( I did a " grep -r '@endgroup' ." It returned nothing. @Jthorson in irc isn't sure about this either, but @amateescu is pretty sure the answer is NO:)

3). The previous code will report a bug for this kind of list fileds, it will say 'query' is not a defined property for this object, and 'add_where_expression' is trying to work on non-object. Of course, the views is broken too. I tried taxonomy field's filter to test this code, because its field filter also extends "ManyToOne". It reports the same error.

Take a whole picture of this class, ' $this->query->add_where_expression' looks more like a typo, because '$this->handler->query->add_where_expression' has been used everywhere:) I see $handler is used as a dynamic property of this class, it's not optimized? So I declare it as a property in the first place.

But when I test taxonomy field with views, I find another bug? See #2017829: "Show hierarchy in dropdown" doesn't work for taxonomy field views filter.

yched’s picture

Not sure prefixing the class name with the module name is required. What do the other "custom" views handlers to ?

@endgroup - ok, sorry about that, I learned something :-)

$this->query->add_where_expression:
Yup, it's a bit tedious because you can't really test the (re)introduced 'list fields" filter handler without fixing this bug too, but it really looks like a fix that would deserve its own issue...

smiletrl’s picture

Ok, I'm trying to split that views helper issue to #2017939: Views' helper ManyToOneHelper has a typo?, since taxonomy term field reports the issue too:)

This issue will focus on the list field thing. Rename these classes according to #2012130-10: Regression: Views integration for "list" field types is broken.

Status: Needs review » Needs work
Issue tags: -VDC-integration

The last submitted patch, views_integration_list_field-2012130-10.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
smiletrl’s picture

dawehner’s picture

Issue tags: +Needs tests

This is the proper way to go. In general please make a patch with using git diff 8.x -M, because this really allow the patch to decrease,
and people get not distracted by missing proper docs in all the views files.

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/NumberListField.phpundefined
@@ -0,0 +1,76 @@
+ * Definition of Drupal\options\Plugin\views\argument\NumberListField.

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/TextListField.phpundefined
@@ -0,0 +1,78 @@
+ * Definition of Drupal\options\Plugin\views\argument\TextListField.

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/filter/ListField.phpundefined
@@ -0,0 +1,27 @@
+ * Definition of Drupal\field\Plugin\views\filter\ListField.
...
+  public function getValueOptions() {

Should be Contains "\" ...

+++ b/core/modules/options/options.views.incundefined
@@ -0,0 +1,35 @@
+ * Provide Views data and handlers for options.module.

Let's remove "and handlers" as it is not longer true (sorry that is d6 comment code.

smiletrl’s picture

ok now thing is a bit tricky for me. Ignore views test for now,

See

+/**
+ * Filter handler which uses list-fields as options.
+ *
+ * @ingroup views_filter_handlers
+ *
+ * @PluginID("list_field")
+ */
+class ListField extends ManyToOne {
+
+  public function getValueOptions() {
+    $field = field_info_field($this->definition['field_name']);
+    $this->value_options = options_allowed_values($field);
+  }

This is views filter plugin for list field. As for options_allowed_values, it requires $field_definition and $entity to get value. So we need to adjust this function here.

For $field_definition, I think we could use field instance(this could depend on bundle too!) for this field. For $entity, we need to get every quried entity object for this views. Is that even possible in views, dynamic options? Now I understand why @effulgentsia wants to push field_definition into core firstly:)

Any idea to proceed?

smiletrl’s picture

For $field_definition from #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets, shall field_definition be independent from entity bundle? I think it should be, since it's called field-definition, instead of field_instance_definition:)

But, how to get a field_definition for a specific field name in this case, e.g., 'field_test_me'? The only way I could think of, is get a field instance of this field, then, I could use $field_definition = $instance to do what I want, which will depends on bundle again.

As for every queried entity object, e.g, every node this views is querying, here's a compromise way. If this field uses dynamic allowed value options, i.e.,
$function = $field_definition->getFieldSetting('allowed_values_function'); exists, we get back to the regular filter. otherwise, we still use the options here.

I'd like to add reference of dynamic allowed options example here

smiletrl’s picture

Also, while options field supports dynamic allowed values, there's no way for users to add dynamic allowed_values_function from field UI. I'm thinking can we add some settings, so user could add such a function from field edit ui, without have to create such field from code?

smiletrl’s picture

Open this #2037217: Should we need dynamic options in options_allowed_values()?.

Also, some discussion in IRC

[23:04] dawehner_: smiletrl: we could just pass in the entity type and bundle as string to be honest to the API
[23:04] swentel: smiletrl: I don't see the flaw tbh - again, not following completely

[23:12] smiletrl: dawehner_: as you said, we could pass a fake entity type and bundle, this could work for most cases. One thing is, a field could show in different entiy type and bundle, we could see this from view context now. The other thing is pontentially, the options depends on each entity. when that happens, error will crashes views cruely...
[23:12] dawehner_: smiletrl: one major problem is: we don't have the bundle available
[23:12] dawehner_: smiletrl: all we have is the name of the field + the entity type
[23:12] dawehner_: smiletrl: but i always thought that options are defined by field not per instance
[23:13] dawehner_: smiletrl: the problem is that a filter potentially filters on different bundles
[23:13] tizzo-afk is now known as tizzo
[23:13] dawehner_: smiletrl: and the field has an instance on different bundles

Technically speaking, yeah, no way to accurately tell which bundle this context is in, since there could be bundle filter in views too. At this point, as @dawehner said, we could dismiss this dynamic options feature?

Want to see what @amateescu, @swentel @yched thinks:)

smiletrl’s picture

Ok, here's the idea of how to proceed.

function options_field_views_data($field) {

  $data = field_views_field_default_views_data($field);
  // $field is instance of \Drupal\field\Plugin\Core\Entity\Field;
  // So $field is instance of FieldDefinitionInterface $field_definition too.
  $function = $field->getFieldSetting('allowed_values_function');
  // If this field makes use of dynamic allowed options, we ignore the views setting.
  if(!empty($function)) {
    return $data;
  }
  foreach ($data as $table_name => $table_data) {
    foreach ($table_data as $field_name => $field_data) {
      if (isset($field_data['filter']) && $field_name != 'delta') {
        $data[$table_name][$field_name]['filter']['id'] = 'list_field';
      }
      if (isset($field_data['argument']) && $field_name != 'delta') {
        if ($field['type'] == 'list_text') {
          $data[$table_name][$field_name]['argument']['id'] = 'text_list_field';
        }
        else {
          $data[$table_name][$field_name]['argument']['id'] = 'number_list_field';
        }
      }
    }
  }
  return $data;
}

/**
 * A new function to return "regular" options field's allowed values.
 * "regular" options field means it doesn't make use of dynamic allowed values.
 */
function options_regular_allowed_values($field_name) {
  // This function is necessary imho, because in most cases, options fields
  // are created via default field UI, and these options fields are just
  // regular options fields.
  // Personally speaking, this function is convenient for programmer to
  // get regular options field's allowed values from a simply field name.

  // function to get it's field object Drupal\field\Plugin\Core\Entity\field $field.
  // Here we could use service to instantiate it.
  $field = some_function_get_field_object($field_name);
  $function = $field->getFieldSetting('allowed_values_function');
  // We still checks it here.
  if(!empty($function)) {
    throw exception("You are trying to get dynamic options fields' allowed values in a regular way.");
  }
  return $field->getFieldSetting('allowed_values');
}

function some_function_get_field_object($field_name) {
  // Drupal service to instantiate field object.
}

class ListField extends ManyToOne {

  public function getValueOptions() {
  dsm($this->definition);
    $field = field_info_field($this->definition['field_name']);
    $this->value_options = options_regular_allowed_values($field_name);
  }

}

This approach will ignore special options fields, which makes use of dynamic allowed values, for views.

If an option field makes use dynamic option fields, there could be no way to pre-define options for views, because we can't predict what will happen inside dynamic allowed function, imho.

smiletrl’s picture

Ignore views test for now.

This patch is trying to fix options_allowed_values issue. It's designed according to comment #22.

dawehner’s picture

This is a great start so far! Here are some nitpicks and comments to the actual important code.

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/filter/ListField.phpundefined
@@ -0,0 +1,26 @@
+ * Definition of \Drupal\field\Plugin\views\filter\ListField.

nitpick: Files should be started with "Contains \..."

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/filter/ListField.phpundefined
@@ -0,0 +1,26 @@
+  public function getValueOptions() {

nitpick: this should have a @inheritdoc on there.

+++ b/core/modules/options/options.moduleundefined
@@ -271,6 +271,56 @@ function options_allowed_values(FieldDefinitionInterface $field_definition, Enti
+function options_regular_allowed_values($field_name) {
...
+    $field = entity_load('field_entity', $field_name);

I am wondering whether similar to the other function this should already receive a Field entity.

+++ b/core/modules/options/options.moduleundefined
@@ -271,6 +271,56 @@ function options_allowed_values(FieldDefinitionInterface $field_definition, Enti
+      throw new FieldException("Attempt to get special list fields' allowed values in regular way.");

THis exception is not "use"d in the file, so it will fail really if it fails.

+++ b/core/modules/options/options.moduleundefined
@@ -271,6 +271,56 @@ function options_allowed_values(FieldDefinitionInterface $field_definition, Enti
+    // If $cacheable is FALSE, then the allowed values are not statically
+    // cached. See options_test_dynamic_values_callback() for an example of
...
+    $cacheable = TRUE;
...
+    if ($cacheable) {

Let's skip cacheable, as in this case the values are not dynamic.

+++ b/core/modules/options/options.views.incundefined
@@ -0,0 +1,41 @@
+  $function = $field->getFieldSetting('allowed_values_function');
+  // If this field makes use of dynamic allowed options, we ignore the views setting.
+  if(!empty($function)) {
+    return $data;
+  }

If it uses dynamic values we could at least provide a pure string based filter/field

smiletrl’s picture

Thanks for your review!

Ok, new patch according to #24.

As for

If it uses dynamic values we could at least provide a pure string based filter/field

This will fallback to default views filter for string. Right now in Drupal core HEAD, this is the case. Our patch will provide the ManyToOne options for views filter and arguments, provided the options list field is regular field.

Status: Needs review » Needs work
Issue tags: -Needs tests, -VDC-integration

The last submitted patch, views_integration_list_field-2012130-24.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +VDC-integration
dawehner’s picture

Thanks!

+++ b/core/modules/field/field.moduleundefined
@@ -139,7 +139,7 @@ function field_help($path, $arg) {
-      $output .= '<dd>' . t('The Field module provides the infrastructure for fields and field attachment; the field types and input widgets themselves are provided by additional modules. Some of the modules are required; the optional modules can be enabled from the <a href="@modules">Modules administration page</a>. Drupal core includes the following field type modules: Number (required), Text (required), List (required), Taxonomy (optional), Image (optional), and File (optional); the required Options module provides input widgets for other field modules. Additional fields and widgets may be provided by contributed modules, which you can find in the <a href="@contrib">contributed module section of Drupal.org</a>. Currently enabled field and input widget modules:', array('@modules' => url('admin/modules'), '@contrib' => 'http://drupal.org/project/modules', '@options' => url('admin/help/options')));
+      $output .= '<dd>' . t('The Field module provides the infrastructure for fields and field attachment; the field types and input widgets themselves are provided by additional modules. Some of the modules are required; the optional modules can be enabled from the <a href="@modules">Modules administration page</a>. Drupal core includes the following field type modules: Number (required), Text (required), Options (required), Taxonomy (optional), Image (optional), and File (optional); the required Options module provides input widgets for other field modules. Additional fields and widgets may be provided by contributed modules, which you can find in the <a href="@contrib">contributed module section of Drupal.org</a>. Currently enabled field and input widget modules:', array('@modules' => url('admin/modules'), '@contrib' => 'http://drupal.org/project/modules', '@options' => url('admin/help/options')));

This change seems to be optional and kind of out of scope, but for sure it make sense to replace list with option here as well.

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/NumberListField.phpundefined
@@ -30,13 +30,13 @@ class FieldList extends Numeric {
+    $field = entity_load('field_entity', $this->definition['field_name']);

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/TextListField.phpundefined
@@ -30,15 +30,18 @@ class ListString extends String {
+    $field = entity_load('field_entity', $this->definition['field_name']);

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/filter/ListField.phpundefined
@@ -0,0 +1,30 @@
+    $field = entity_load('field_entity', $this->definition['field_name']);

You could also inject the field_entity storage controller via the create() method to be able to load the field entity.

smiletrl’s picture

Hmm, if use entity_create, or storage,

$field = Drupal::entityManager()->getStorageController($entity_type)->create($values);

We need to provide other information of this field(Id, and type are required), not just a field name. I guess simply loading the existing field is okay:)

dawehner’s picture

I meant something like this:

class EmailAction extends ConfigurableActionBase implements ContainerFactoryPluginInterface {

  /**
   * The token service.
   *
   * @var \Drupal\Core\Utility\Token
   */
  protected $token;

  /**
   * The user storage controller.
   *
   * @var \Drupal\Core\Entity\EntityStorageControllerInterface
   */
  protected $storageController;

  /**
   * Constructs a EmailAction object.
   *
   * @param array $configuration
   *   A configuration array containing information about the plugin instance.
   * @param string $plugin_id
   *   The plugin ID for the plugin instance.
   * @param array $plugin_definition
   *   The plugin implementation definition.
   * @param \Drupal\Core\Utility\Token $token
   *   The token service.
   * @param \Drupal\Core\Entity\EntityManager $entity_manager
   *   The entity manager.
   */
  public function __construct(array $configuration, $plugin_id, array $plugin_definition, Token $token, EntityManager $entity_manager) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);

    $this->token = $token;
    $this->storageController = $entity_manager->getStorageController('user');
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, array $plugin_definition) {
    return new static($configuration, $plugin_id, $plugin_definition,
      $container->get('token'),
      $container->get('plugin.manager.entity')
    );
  }

}
smiletrl’s picture

Thanks for the head-sup!

Hmmm, as I understand, this 'create' method is usually used inside a class to inject objects. In our case here, I'm not sure where to use this feature. Where can I inject the storage controller? Or you want me to create a new class?

smiletrl’s picture

Add test for options views integration. This is just a start, should fail:)

It only counts the regular options field for now. Perhaps we need to add tests for special options list field.

Status: Needs review » Needs work

The last submitted patch, views_integration_list_field-2012130-32.patch, failed testing.

smiletrl’s picture

smiletrl’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, views_integration_list_field-2012130-34.patch, failed testing.

dawehner’s picture

I really like the amount of test coverage which is added by that patch.

+++ b/core/modules/options/lib/Drupal/options/Tests/views/OptionsListArgument.phpundefined
@@ -0,0 +1,46 @@
+class OptionsListArgument extends OptionsTestBase {

Oh cool, there is a base test class!

+++ b/core/modules/options/lib/Drupal/options/Tests/views/OptionsTestBase.phpundefined
@@ -0,0 +1,129 @@
+ * Contains \Drupal\taxonomy\Tests\Views\OptionsTestBase.
...
+/**
+ * Base class for all taxonomy tests.
+ */

This comment is not right anymore.

+++ b/core/modules/options/lib/Drupal/options/Tests/views/OptionsTestBase.phpundefined
@@ -0,0 +1,129 @@
+      if ($this->field_name == 'field_test_list_text') {
+        entity_create('field_entity', array(
+          'field_name' => $this->field_name,
+          'type' => 'list_integer',
+          'cardinality' => 1,
+          'settings' => array(
+            'allowed_values' => array(
+              array(
+                $this->field_values[0] => $this->field_values[0],
+                $this->field_values[1] => $this->field_values[1],
+              ),
+            ),
+          ),
+        ))->save();
+      }
+      else {
+        entity_create('field_entity', array(
+          'field_name' => $this->field_name,
+          'type' => 'list_integer',
+          'cardinality' => 1,
+          'settings' => array(
+            'allowed_values' => array(
+              array(
+                $this->field_values[0],
+                $this->field_values[1],
+              ),
+            ),
+          ),
+        ))->save();

We could just skip this if else and setup the two entities before.

+++ b/core/modules/options/lib/Drupal/options/Tests/views/OptionsListArgument.phpundefined
@@ -0,0 +1,46 @@
+class OptionsListArgument extends OptionsTestBase {

Test class names should always end with "...Test".

+++ b/core/modules/options/lib/Drupal/options/Tests/views/OptionsListArgument.phpundefined
@@ -0,0 +1,46 @@
+class OptionsListArgument extends OptionsTestBase {

Oh cool, there is a base test class!

+++ b/core/modules/options/lib/Drupal/options/Tests/views/OptionsTestBase.phpundefined
@@ -0,0 +1,129 @@
+ * Contains \Drupal\taxonomy\Tests\Views\OptionsTestBase.
...
+/**
+ * Base class for all taxonomy tests.
+ */

This comment is not right anymore.

+++ b/core/modules/options/lib/Drupal/options/Tests/views/OptionsTestBase.phpundefined
@@ -0,0 +1,129 @@
+      if ($this->field_name == 'field_test_list_text') {
+        entity_create('field_entity', array(
+          'field_name' => $this->field_name,
+          'type' => 'list_integer',
+          'cardinality' => 1,
+          'settings' => array(
+            'allowed_values' => array(
+              array(
+                $this->field_values[0] => $this->field_values[0],
+                $this->field_values[1] => $this->field_values[1],
+              ),
+            ),
+          ),
+        ))->save();
+      }
+      else {
+        entity_create('field_entity', array(
+          'field_name' => $this->field_name,
+          'type' => 'list_integer',
+          'cardinality' => 1,
+          'settings' => array(
+            'allowed_values' => array(
+              array(
+                $this->field_values[0],
+                $this->field_values[1],
+              ),
+            ),
+          ),
+        ))->save();

We could just skip this if else and setup the two entities before.

smiletrl’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +VDC-integration
FileSize
9.55 KB
31.51 KB

Thanks for your review!

Ok new patch. This patch may pass, because it passes locally.

However, there're problems with this test OptionsListArgumentTest, but I can't figure out the problem.

This problem is this test should execute views $view = views_get_view('test_options_list_argument');, but it's verbose message of the execute view says different thing:

Executed view: SELECT node_field_data.title AS node_field_data_title, node.nid AS nid, node_field_data.created AS node_field_data_created
FROM 
{node} node
INNER JOIN {node_field_data} node_field_data ON node.nid = node_field_data.nid
WHERE (( (node_field_data.status = :db_condition_placeholder_0) AND (node_field_data.type IN  (:db_condition_placeholder_1)) ))
ORDER BY node_field_data_created DESC
LIMIT 5 OFFSET 0

This is exactly what it executes at test OptionsListFilterTest. It should be something like this:

SELECT field_data_field_test_list_integer.field_test_list_integer_value AS field_data_field_test_list_integer_field_test_list_integer_v, COUNT(node.nid) AS num_records, MIN(node.nid) AS nid
FROM 
{node} node
INNER JOIN {node_field_data} node_field_data ON node.nid = node_field_data.nid
LEFT JOIN {field_data_field_test_list_integer} field_data_field_test_list_integer ON node.nid = field_data_field_test_list_integer.entity_id AND (field_data_field_test_list_integer.entity_type = 'node' AND field_data_field_test_list_integer.deleted = '0')
WHERE (( (node_field_data.status = '1') AND (node_field_data.type IN  ('article')) ))
GROUP BY field_data_field_test_list_integer_field_test_list_integer_v
ORDER BY field_data_field_test_list_integer_field_test_list_integer_v ASC
LIMIT 5 OFFSET 0

This query is copied from test_options_list_argument views's displayed sql query.

Any ideas?

Issue tags: -Needs tests, -VDC-integration

The last submitted patch, views_integration_list_field-2012130-37.patch, failed testing.

smiletrl’s picture

Status: Needs review » Needs work

The last submitted patch, views_integration_list_field-2012130-37.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +VDC-integration

The last submitted patch, views_integration_list_field-2012130-37.patch, failed testing.

catch’s picture

+++ b/core/modules/options/lib/Drupal/options/Tests/views/OptionsListArgumentTest.phpundefined
@@ -0,0 +1,51 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\options\Tests\Views\OptionsListArgumentTest.
+ */
+
+namespace Drupal\options\Tests\Views;
+
+/**
+ * Tests the representative node relationship for terms.
+ */

Isn't this missing a use? Assume that's why the test fails.

+++ b/core/modules/options/lib/Drupal/options/Tests/views/OptionsTestBase.phpundefined
@@ -0,0 +1,123 @@
+ * @file
+ * Contains \Drupal\taxonomy\Tests\Views\OptionsTestBase.

Nothing to do with taxonomy.

dawehner’s picture

@smiletrl

You have to pass in the arguments in the $this->executeView call.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
31.49 KB

Thanks for your review!

@catch, I just found I used capital characters in namespace declaration for options base test, which probably leads to the problem:)

@dawehner, hmm, the first view -- views.view.test_options_list_filter.yml is a regular view, no contextual filters provided, do we need arguments for this view? As for the other view views.view.test_options_list_argument.yml, this view uses argument. But options argument class -- TextListField and NumberListField only add a summary options in views summary settings:'Display list value as human readable', which requires no arguments provided to take effect. So I think this view doesn't need arguments too?

Status: Needs review » Needs work

The last submitted patch, views_integration_list_field-2012130-46.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
541 bytes
31.49 KB

ok, fix contain declaration. This is quite wizard, because the three tests share same namespace and it works locally.

If this fails gain, I shall add "use" as @catch said.

Status: Needs review » Needs work

The last submitted patch, views_integration_list_field-2012130-47.patch, failed testing.

dawehner’s picture

So you are using a mac, i guess this should solve the problem.

+++ b/core/modules/options/lib/Drupal/options/Tests/views/OptionsTestBase.php
+namespace Drupal\options\Tests\Views;

See views vs. Views

smiletrl’s picture

Status: Needs work » Needs review
FileSize
12.87 KB
31.49 KB

@dawehner Great point!

I didn't expect namespace needs to match the real file directory:(

dawehner’s picture

Issue tags: -Needs tests

Just in general, this is an impressive piece of work. Just have a look at the amount of

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/NumberListField.phpundefined
@@ -30,13 +30,13 @@ class FieldList extends Numeric {
+    $field = entity_load('field_entity', $this->definition['field_name']);

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/TextListField.phpundefined
@@ -30,15 +30,18 @@ class ListString extends String {
+    $field = entity_load('field_entity', $this->definition['field_name']);

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/filter/ListField.phpundefined
@@ -0,0 +1,30 @@
+    $field = entity_load('field_entity', $this->definition['field_name']);

You should better inject the field_entity storage controller using the create method.

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/filter/ListField.phpundefined
@@ -0,0 +1,30 @@
+  public function getValueOptions() {
...
+    $this->value_options = options_regular_allowed_values($field);

These kind of code should check whether ->value_options is already set.

+++ b/core/modules/options/options.moduleundefined
@@ -271,6 +271,43 @@ function options_allowed_values(FieldDefinitionInterface $field_definition, Enti
+      throw new \Drupal\field\FieldException("Attempt to get special list fields' allowed values in regular way.");

I think you should add "Use Drupal\field\FieldException" at the top.

smiletrl’s picture

Thanks for review!

New patch according to #52. Some questions though.
1).

  /**
   * The field storage controller.
   *
   * @var \Drupal\Core\Entity\EntityStorageControllerInterface
   */
  protected $storageController;

Which storage interface should be used to declare var, since FieldStorageController implements both EntityControllerInterface, EntityStorageControllerInterface? Is there any preference?

2). Compared to use Drupal::entityManager()->getStorageController($entity_type); directly, current code in patch inject entityManager. What's different here?

Does 'Drupal::' means a Drupal current active container, but code could provide another/alternative container to provide a different entityManger? Kind of like drupal database connection, default connection is provided in settings.php, but code could provide alternative connection to connect to other databases.

3). The argument views test doesn't test what I want here, but I don't know how to do this in views.

The main function these two options field arguments have provided is the 'Summary' -- t('Display list value as human readable'). So I want to check whether executed result from views should contain options allowed values, not keys, e.g., should contain 'man' and 'woman', instead of key '1' and '2' for NumberListField.

But what I could get only contains node-id, not the pure text from views result. The test will pass, but passes in strange way. This is the verbose message for both tests:

Returned data set: Array
(
    [0] => Array
        (
            [nid] => 1
        )

    [1] => Array
        (
            [nid] => 2
        )

)


Expected: Array
(
    [0] => Array
        (
            [nid] => 1
        )

    [1] => Array
        (
            [nid] => 2
        )

)

I should separate test from fix to show whether the test really checks fix here.

dawehner’s picture

Which storage interface should be used to declare var, since FieldStorageController implements both EntityControllerInterface, EntityStorageControllerInterface? Is there any preference?

You could argue that the deepest interface which has the methods you need, for example EntityStorageControllerInterface, if we just the load method.

mpared to use Drupal::entityManager()->getStorageController($entity_type); directly, current code in patch inject entityManager. What's different here?

Does 'Drupal::' means a Drupal current active container, but code could provide another/alternative container to provide a different entityManger? Kind of like drupal database connection, default connection is provided in settings.php, but code could provide alternative connection to connect to other databases.

The main point here is once you inject all the dependencies into an object, it is somehow self-documentation how to reuse the code. At the same time this allows you to write unit tests, as the object itself works together with its dependencies but independent from the other rest of the outside world.

3). The argument views test doesn't test what I want here, but I don't know how to do this in views.

So what you would do is to create a new view using the summary functionality in views and then ensure that the result of that special configuration is as expected. If you configure it, the result will differ.

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/NumberListField.phpundefined
@@ -0,0 +1,115 @@
+      return check_plain($value);

+++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/TextListField.phpundefined
@@ -0,0 +1,119 @@
+      return $this->caseTransform(check_plain($value), $this->options['case']);

Let's use String::checkPlain

+++ b/core/modules/options/lib/Drupal/options/Tests/Views/OptionsListArgumentTest.phpundefined
@@ -0,0 +1,51 @@
+/**
+ * Tests options list argument for views.
+ */
+class OptionsListArgumentTest extends OptionsTestBase {

+++ b/core/modules/options/lib/Drupal/options/Tests/Views/OptionsListFilterTest.phpundefined
@@ -0,0 +1,51 @@
+/**
+ * Tests options list filter for views.
+ */
+class OptionsListFilterTest extends OptionsTestBase {

As most of the views tests we are testing basically the functionality of a single class, so I really like to have a @see \Drupal\...\PathToClass, so it is really easy to figure out where to look at.

smiletrl’s picture

Thanks for your help!
This patch has applied some changes at #54.

However, this patch doesn't work for views. Options filter and argument is broken. I guess the reason is because of change mentioned at #52 to apply create method to inject container and storagecontroller. The way to instantiate these views plugins doesn't invoke create/inject way?

Status: Needs review » Needs work

The last submitted patch, views_integration_list_field-2012130-55.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.74 KB
40.9 KB

This would fix it.

catch’s picture

Issue tags: -VDC-integration

#57: vdc-2012130-57.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC-integration

The last submitted patch, vdc-2012130-57.patch, failed testing.

smiletrl’s picture

There're still bugs there for the latest patch, which may involve the existing code of options field and taxonomy field. I'd love to wait untill options field and taxonomy field has been converted into the new FieldType plugin and then start to work at this.

I'll try to fix the two issue firstly, see:
#2015689: Convert field type to typed data plugin for options module and #2015687: Convert field type to FieldType plugin for taxonomy module

Of course, others are free to reassign this issue, if anyone would like to pick this issue too:)

jibran’s picture

Issue summary: View changes
Issue tags: +VDC, +Needs reroll

Needs reroll

dawehner’s picture

FileSize
40.98 KB

Rerolled ...

dawehner’s picture

Status: Needs work » Needs review

.

Status: Needs review » Needs work

The last submitted patch, 62: vdc-2012130.patch, failed testing.

smiletrl’s picture

This patch intrduces

+  public function getValueOptions() {
+    $field = $this->storageController->load($this->definition['field_name']);
+    if (!isset($this->value_options)) {
+      $this->value_options = options_regular_allowed_values($field);
+    }
+  }
+

and especially this funtion options_regular_allowed_values().

It's interesting that taxonomy field doesn't need to consider this situation(I don't know actually how this is achieved^). It only searches the db to get all the tids inside the choosen vocabulary for $value_option. So I think maybe we could use the same trick here?

We could simply check whether list field uses custom callback. If it has, we return the NULL for value options. Otherwise, we use the common function to return the available options. In this way, we don't need this function options_regular_allowed_values(). This may relate with the converted new option fields issue.

star-szr’s picture

I can't speak to #65 but this could use another reroll it looks like.

webflo’s picture

FileSize
30.31 KB

Rerolled ...

webflo’s picture

Status: Needs work » Needs review
FileSize
31.75 KB

Doh, the last patch was broken. (core/modules/options/options.views.inc was missing)

Status: Needs review » Needs work

The last submitted patch, 68: vdc-2012130-68.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
31.79 KB

This should work. Updated the tests to field_config and field_instance_config.

Status: Needs review » Needs work

The last submitted patch, 70: vdc-2012130-70.patch, failed testing.

YesCT’s picture

Assigned: smiletrl » Unassigned
Issue tags: -Needs reroll

@smiletrl it's been a while. :) if you want to work on those fails, just comment back here. otherwise I think this is open for anyone.

patch applies, removing needs reroll tag

I viewed the test results. Fails (not random as they are in classes this patch adds) are:

CollapsedDrupal\options\Tests\Views\OptionsListArgumentTest	
Identical result set.	Other	OptionsListArgumentTest.php	51	Drupal\options\Tests\Views\OptionsListArgumentTest->testViewsTestOptionsListArgument()	
CollapsedDrupal\options\Tests\Views\OptionsListFilterTest	
Identical result set.	Other	OptionsListFilterTest.php	51	Drupal\options\Tests\Views\OptionsListFilterTest->testViewsTestOptionsListFilter()
pcambra’s picture

Status: Needs work » Needs review
FileSize
31.79 KB

This needed re-roll after some changes in core, should be green but not sure if something else might be needed

Status: Needs review » Needs work

The last submitted patch, 73: vdc-2012130-73.patch, failed testing.

pcambra’s picture

Just a side note, tests pass locally both UI and command line

martin107’s picture

Hmm ... I can confirm #75 ... at least tests pass for me in local browser testing... so retesting.

martin107’s picture

Status: Needs work » Needs review

73: vdc-2012130-73.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 73: vdc-2012130-73.patch, failed testing.

martin107’s picture

restest results ... unchanged... ( I am on a MAMP stack - just for information )
php 5.5.3

looks like testbot was PHP 5.4

damiankloip’s picture

Status: Needs work » Needs review

73: vdc-2012130-73.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 73: vdc-2012130-73.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
31.11 KB
8.55 KB

Here are a few changes, quite a few things were broken with changes to core :) Alot of work still needs to be done.

The new tests were yielding broken handlers due to an exception thrown when trying to load the 'field_entity' entity type, which does not exist anymore. This should fix that at least. The tests should still fail (or actually pass). The arguments are not affecting the queries. The tests should also be checking that maybe just one of those entities is in the results, not both? or a combination with more node created for the tests.

We have a problem here:

//$this->allowed_values = options_regular_allowed_values($field);

options.module now just has options_allowed_values() which wants a field entity, and an entity to work out allowed values for. This is not really possibly for do for arguments and filters...

Status: Needs review » Needs work

The last submitted patch, 82: 2012130-82.patch, failed testing.

dawehner’s picture

martin107’s picture

Issue tags: +Needs reroll
damiankloip’s picture

Status: Needs work » Postponed

This is really postponed on the issue opened in #84.

mgifford’s picture

xjm’s picture

Issue tags: +Triaged D8 critical
YesCT’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update

noted why this was postponed in the issue summary, but other details there need to be filled in.

mgifford’s picture

FileSize
1.17 KB

this patch was going to need to get re-rolled anyways...

catch’s picture

Title: Regression: Views integration for "list" field types is broken » [PP-1] Regression: Views integration for "list" field types is broken
xjm’s picture

Title: [PP-1] Regression: Views integration for "list" field types is broken » Regression: Views integration for "list" field types is broken
Issue summary: View changes
Status: Postponed » Needs work
dawehner’s picture

Issue tags: +Needs tests

No matter what we do here, but we need test coverage here, so we don't break that again.

mgifford’s picture

Status: Needs work » Needs review

Checking #90 to see if it busts anything. Still needs new tests though.

damiankloip’s picture

mgifford, did you see patches preview to #90, that 'reroll' is just a hunk of that patch. I'm confused. 31kb down to 1kb :)

damiankloip’s picture

Issue tags: -Needs reroll, -Needs tests
FileSize
30.97 KB
730 bytes

Ok, rerolled #82, which has all the work and tests in. That is an 8 month reroll, so expect something to go wrong :p

dawehner’s picture

this patch was going to need to get re-rolled anyways...

HA

  1. +++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/NumberListField.php
    @@ -0,0 +1,121 @@
    +  public function buildOptionsForm(&$form, &$form_state) {
    +    parent::buildOptionsForm($form, $form_state);
    

    FormState thing

  2. +++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/NumberListField.php
    @@ -0,0 +1,121 @@
    +      return field_filter_xss($this->allowed_values[$value]);
    

    That thing doesn't exist anymore

  3. +++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/TextListField.php
    @@ -0,0 +1,124 @@
    + * Argument handler for list field to show the human readable name in the
    + * summary.
    

    This is why I hate our 80chars limit.

  4. +++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/TextListField.php
    @@ -0,0 +1,124 @@
    +    $field = $this->storageController->load($this->definition['field_name']);
    +    $this->allowed_values = options_regular_allowed_values($field);
    

    Let's use protected $allowedValues here, it is a little bit more sane.

  5. +++ b/core/modules/options/lib/Drupal/options/Plugin/views/argument/TextListField.php
    @@ -0,0 +1,124 @@
    +    $options['summary']['contains']['human'] = array('default' => FALSE, 'bool' => TRUE);
    

    No bool needed anymore.

yched’s picture

  1. +++ b/core/modules/options/src/Plugin/views/filter/ListField.php
    @@ -0,0 +1,74 @@
    +      $field_definition = $this->storageController->load($this->definition['entity_type'] . '.' . $this->definition['field_name']);
    +      $this->value_options = $field_definition->getSetting('allowed_values');
    

    - This should work on a FieldStorageDefinition rather than a FieldConfig
    - It should call options_allowed_values() rather than reading the 'allowed_values' setting.

  2. +++ b/core/modules/options/src/Plugin/views/argument/NumberListField.php
    @@ -0,0 +1,121 @@
    +    $this->allowed_values = options_allowed_values($field);
    

    Same here, this should be a FieldStorageDefinition

  3. +++ b/core/modules/options/src/Plugin/views/argument/TextListField.php
    @@ -0,0 +1,124 @@
    +    $this->allowed_values = options_regular_allowed_values($field);
    

    And same here - but not sure why this one uses options_regular_allowed_values() (which doesn't seem to exist ?)

Also - maybe that's not for this issue, but the pattern of reading the Field / FieldStorage definitions from config is a little weird, and very much hardcodes the various handlers to only work with configurable fields.

Handlers for Field API fields are about a FieldStorageDefinition, we should try to inject it, or at least read it from EntityManager::getFieldStorageDefinitions(), rather than pulling them from *config*.

It's true that currently field_views_data() only ever uses those handlers for configurable fields, but the less the handlers themselves know that, and the more they work with FieldStorageDefinitionInterface wherever it comes from, the better for the "use Field API handler for base fields" issues ?

The last submitted patch, 90: 2012130-89.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 96: 2012130-96.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
13.67 KB
36.36 KB

yched, dawehner, how about this? We make a trait with some field helper methods on (that the field Field plugin is using already) as reuse that in our args and filters?

Made some of those other changes too from points in #97. Haven't looked at the tests yet.

Status: Needs review » Needs work

The last submitted patch, 101: 2012130-101.patch, failed testing.

yched’s picture

Yep, I think a trait makes tons of sense :-)

A couple adjustments then :

  1. +++ b/core/modules/field/src/Views/FieldStorageViewsTrait.php
    @@ -0,0 +1,54 @@
    +   * A field storage definition turned into a field definition, so it can be
    +   * used with widgets and formatters. See
    +   * BaseFieldDefinition::createFromFieldStorageDefinition().
    

    Not fully sure, but wouldn't that be more helpful in the phpdoc of getFieldDefinition() ?

  2. +++ b/core/modules/field/src/Views/FieldStorageViewsTrait.php
    @@ -0,0 +1,54 @@
    +   * The field config.
    ...
    +  protected $fieldStorageConfig;
    

    "The field storage definition"
    $fieldStorageDefinition

  3. +++ b/core/modules/field/src/Views/FieldStorageViewsTrait.php
    @@ -0,0 +1,54 @@
    +   * Gets the field configuration.
    ...
    +  protected function getFieldStorageConfig() {
    

    "Gets the field storage definition"
    getFieldStorageDefinition()

  4. +++ b/core/modules/options/src/Plugin/views/argument/NumberListField.php
    @@ -74,13 +80,13 @@ public static function create(ContainerInterface $container, array $configuratio
    +    $field = $this->getFieldStorageConfig();
    +    $this->allowedValues = options_allowed_values($field);
    
    +++ b/core/modules/options/src/Plugin/views/argument/TextListField.php
    @@ -73,8 +78,8 @@ public static function create(ContainerInterface $container, array $configuratio
    +    $field = $this->getFieldStorageConfig();
    +    $this->allowedValues = options_allowed_values($field);
    
    +++ b/core/modules/options/src/Plugin/views/filter/ListField.php
    @@ -63,12 +64,12 @@ public static function create(ContainerInterface $container, array $configuratio
    +    $field = $this->getFieldStorageConfig();
    +    // Set valueOptions here so getValueOptions() will just return it.
    +    $this->valueOptions = options_allowed_values($field);
    

    s/$field/$field_storage/

  5. +++ b/core/modules/options/src/Plugin/views/filter/ListField.php
    @@ -45,13 +46,13 @@ class ListField extends ManyToOne {
    -    $this->storageController = $entity_manager->getStorageController('field_config');
    +    $this->storageController = $entity_manager->getStorage('field_config');
    

    Looks like this is not needed anymore ?

dawehner’s picture

  1. +++ b/core/modules/options/src/Plugin/views/argument/NumberListField.php
    @@ -0,0 +1,127 @@
    +  use FieldStorageViewsTrait;
    

    Agreed a trait is helpful here.

  2. +++ b/core/modules/options/src/Plugin/views/argument/NumberListField.php
    @@ -0,0 +1,127 @@
    +      '#title' => t('Display list value as human readable'),
    

    Let's always use $this->t()

  3. +++ b/core/modules/options/tests/options_test_views/options_test_views.info.yml
    @@ -0,0 +1,10 @@
    +hidden: true
    

    afaik we don't need hidden anymore, because the extension listing will not find it anyway

  4. +++ b/core/modules/options/tests/options_test_views/test_views/views.view.test_options_list_argument.yml
    @@ -0,0 +1,200 @@
    +status: '1'
    

    Can you try to recast all those entries?

  5. +++ b/core/modules/options/tests/options_test_views/test_views/views.view.test_options_list_argument.yml
    @@ -0,0 +1,200 @@
    +      filters:
    +        status:
    

    We have to add entity_type and entity_field to all those new views configs.

  6. +++ b/core/modules/options/tests/options_test_views/test_views/views.view.test_options_list_filter.yml
    @@ -0,0 +1,203 @@
    +base_field: nid
    +base_table: node
    

    In an ideal world we would use the entity_test instead of node integration.

dawehner’s picture

Issue tags: +entity storage

.

damiankloip’s picture

Assigned: Unassigned » damiankloip
Issue tags: -entity storage

Rerolling this with all that great feedback.

damiankloip’s picture

Issue tags: +entity storage
damiankloip’s picture

FileSize
33.04 KB
26.71 KB

Updated some config typing and a couple of other things, should be less than 123 fails.

dawehner’s picture

Status: Needs work » Needs review

Let's see whether @damiankloip lies

Status: Needs review » Needs work

The last submitted patch, 108: 2012130-108.patch, failed testing.

jibran’s picture

+++ b/core/modules/options/src/Tests/Views/OptionsListArgumentTest.php
@@ -0,0 +1,52 @@
+namespace Drupal\options\Tests\Views;
+use Drupal\views\Views;

+++ b/core/modules/options/src/Tests/Views/OptionsListFilterTest.php
@@ -0,0 +1,52 @@
+namespace Drupal\options\Tests\Views;
+use Drupal\views\Views;

space missing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.71 KB
33.03 KB

Just fixed stuff here and there.

Status: Needs review » Needs work

The last submitted patch, 112: 2012130-112.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 114: 2012130-114.patch, failed testing.

vladan.me’s picture

Status: Needs work » Needs review

Hm, I have tested locally, didn't get a failure, let's try again?

vladan.me queued 114: 2012130-114.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 114: 2012130-114.patch, failed testing.

star-szr’s picture

I was going to run the tests myself but this no longer applies, non-trivial (to me) conflict with #2407801: Views generic field handler does not work with base fields.

pcambra’s picture

Status: Needs work » Needs review
FileSize
32.56 KB

Here's a reroll

Status: Needs review » Needs work

The last submitted patch, 120: 2012130-120.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
32.46 KB
2.11 KB

Let's fix it.

larowlan’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/field/src/Views/FieldStorageViewsTrait.php
    @@ -0,0 +1,63 @@
    +      $field_storage_definitions = \Drupal::entityManager()->getFieldStorageDefinitions($this->definition['entity_type']);
    

    Should we put this behind a getEntityManager() method and add a setEntityManager() for unit-testing sake and consistency with other similar traits - or is that overkill?

  2. +++ b/core/modules/options/options.views.inc
    @@ -0,0 +1,42 @@
    +  if(!empty($function)) {
    

    nitpick : missing a space

  3. +++ b/core/modules/options/options.views.inc
    @@ -0,0 +1,42 @@
    +        if ($field->type == 'list_text') {
    

    I thought this was renamed to list_string - I can't find any reference to list_text in core anymore.

  4. +++ b/core/modules/options/src/Tests/Views/OptionsTestBase.php
    @@ -0,0 +1,113 @@
    +      'type' => 'list_string',
    

    Yes it seems so

dawehner’s picture

Status: Needs work » Needs review
FileSize
39.21 KB
11.78 KB

Thank you for your review!

Let's do that as well as use VIewUnitTestBase instead.

jibran’s picture

I think this is ready.

+++ b/core/modules/options/options.views.inc
@@ -0,0 +1,42 @@
+  // If this field makes use of dynamic allowed options, we ignore the views setting.

more then 80 chars.

larowlan’s picture

Issue tags: +Critical Office Hours

Thanks, will try and get someone to look at the issue summary and manual tests during today's critical office hours

larowlan’s picture

Some nits and one bug
Fixed #125 whilst at it

after manual testing and issue summary update, I think this is ready too

larowlan’s picture

+++ b/core/modules/options/src/Plugin/views/argument/StringListField.php
@@ -13,6 +13,7 @@
+use Drupal\Component\Utility\String as StringUtility;

@@ -83,7 +84,7 @@ public function summaryName($data) {
+      return $this->caseTransform(StringUtility::checkPlain($value), $this->options['case']);

This was the bug, the rest were nits

Status: Needs review » Needs work

The last submitted patch, 127: 2012130-list-views.127.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
603 bytes
39.39 KB

thought that might happen :)

dawehner’s picture

Yeah this is stupidity of PHP 5.4

dashaforbes’s picture

Manually tested as follows:

- Added a list(string) field and a list(text) field to the Article content type

- Created two sample Articles

- Created a view showing the Articles

View:

View Fields:

larowlan’s picture

Just an issue summary update and I think we're there - any takers?

dashaforbes’s picture

Issue summary: View changes
dashaforbes’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Moving to RTBC as per comments 125, 127 and 133

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/field/src/Views/FieldStorageViewsTrait.php
@@ -0,0 +1,77 @@
+   *   The field definition used by this handler.

+++ b/core/modules/options/options.views.inc
@@ -0,0 +1,43 @@
+          $data[$table_name][$field_name]['argument']['id'] = 'string_list_field';

Missing schema and, therefore, I think test coverage of StringListField.

dawehner’s picture

Status: Needs work » Needs review
FileSize
39.79 KB
2.2 KB

Mh fair, let's work on that

yched’s picture

Nitpick, not a blocker : the name FieldStorageViewsTrait is a bit vague, and the trait is not "just" about "field storage".
This is generic support methods for field API based views handlers, and for use by several kinds of handlers : fields, arguments, filters... So maybe FieldAPIHandlerTrait ? I know @dawehner and I have different, er, views :-p on class naming, so feel free to ignore...

+++ b/core/modules/options/src/Plugin/views/argument/NumberListField.php
@@ -0,0 +1,91 @@
+  public function summaryName($data) {
...
+      return $this->fieldFilterXss($this->allowedValues[$value]);

+++ b/core/modules/options/src/Plugin/views/argument/StringListField.php
@@ -0,0 +1,91 @@
+  public function summaryName($data) {
...
+      return $this->caseTransform($this->fieldFilterXss($this->allowedValues[$value]), $this->options['case']);

It looks like this is the only difference between StringListField and NumberListField.
- couldn't those two inherit from a common one for all the rest of the code ?
- not sure why there is a difference, actually, since both those lines attempt to display a string label, independently of the underlying (string or number) value ?

dawehner’s picture

Assigned: damiankloip » Unassigned
FileSize
39.76 KB
3.39 KB

Nitpick, not a blocker : the name FieldStorageViewsTrait is a bit vague, and the trait is not "just" about "field storage".
This is generic support methods for field API based views handlers, and for use by several kinds of handlers : fields, arguments, filters... So maybe FieldAPIHandlerTrait ? I know @dawehner and I have different, er, views :-p on class naming, so feel free to ignore...

That name is alright for me, ... there simply has been bad examples out there which copied basically the entire namespace into the classname.

It looks like this is the only difference between StringListField and NumberListField.

Well, you missed one tiny little difference:

class NumberListField extends Numeric {

vs.

class StringListField extends String {

... all this functionality here is inherited from String vs. Numeric, the only common thing is that both are argument plugins, this is all.

yched’s picture

NumberListField extends Numeric / StringListField extends String

Self-slap. Right, of course :-)

Yet, this remark from #138 is still valid IMO :

+++ b/core/modules/options/src/Plugin/views/argument/NumberListField.php
@@ -0,0 +1,91 @@
+  public function summaryName($data) {
...
+      return $this->fieldFilterXss($this->allowedValues[$value]);
+++ b/core/modules/options/src/Plugin/views/argument/StringListField.php
@@ -0,0 +1,91 @@
+  public function summaryName($data) {
...
+      return $this->caseTransform($this->fieldFilterXss($this->allowedValues[$value]), $this->options['case']);

both those lines attempt to display a string label, independently of the underlying (string or number) value

$this->allowedValues[$value] is a "human label" string. So I don't get why one would do a CaseTransform on the *label*, and not the other ? The labels, unlike the values, are the "same kind of stuff" (just strings for display) on both StringList and [Numeric]List field types.

dawehner’s picture

FileSize
39.78 KB
789 bytes

Fair point. Here though, we just want to caseTransform the human label.

yched’s picture

@dawehner: thanks :-)

Then, "two classes duplicating the exact same methods because they each have their own specific parent class to inherit from" is the textbook case for "should we make that a trait ?"

dawehner’s picture

Well, the implementation is still not the same ... As for the key case

yched’s picture

Right, I should have looked closer, sorry for that.
Down to just nitpicks then.

  1. +++ b/core/modules/field/src/Views/FieldAPIHandlerTrait.php
    @@ -0,0 +1,77 @@
    +   * The field definition to use.
    ...
    +  protected $fieldDefinition;
    ...
    +   * The field storage definition.
    ...
    +  protected $fieldStorageDefinition;
    

    No preference between "to use" or not "to use", but consistency++ ?

  2. +++ b/core/modules/field/src/Views/FieldAPIHandlerTrait.php
    @@ -0,0 +1,77 @@
    +   * Gets the field definition.
    +   *
    +   * A field storage definition turned into a field definition, so it can be
    +   * used with widgets and formatters.
    ...
    +  protected function getFieldDefinition() {
    

    That's a good thing to explain, IMO for clarity we could turn this into an actual sentence with a verb, and maybe expand a bit, since this is the crux of the Views / Field API issue :-)

    Proposal :
    A View works on an entity type across bundles, and thus only has access to field storage definitions. In order to be able to use widgets and formatters, we create a generic field definition out of that storage definition.

  3. +++ b/core/modules/field/src/Views/FieldAPIHandlerTrait.php
    @@ -0,0 +1,77 @@
    +   * Gets the field configuration.
    ...
    +  protected function getFieldStorageDefinition() {
    

    "Gets the field storage definition" ?

  4. +++ b/core/modules/options/src/Plugin/views/argument/StringListField.php
    @@ -0,0 +1,91 @@
    +    $field = $this->getFieldStorageDefinition();
    +    $this->allowedValues = options_allowed_values($field);
    

    Minor, but for accuracy and consistency with the same code in NumberListField :
    s/$field/$field_storage

  5. +++ b/core/modules/options/src/Plugin/views/argument/NumberListField.php
    @@ -0,0 +1,91 @@
    +    // If the list element has a human readable name show it,
    

    Likewise, s/,/./

dawehner’s picture

FileSize
39.9 KB
2.89 KB

Thank you for your continuous reviews!

No preference between "to use" or not "to use", but consistency++ ?

Yeah, let's just drop it.

That's a good thing to explain, IMO for clarity we could turn this into an actual sentence with a verb, and maybe expand a bit, since this is the crux of the Views / Field API issue :-)

I c&pasted your text.

"Gets the field storage definition" ?

fixed

Removed the caseTransform method again, because there is no way to configure it at the moment and so kinda pointless

Minor, but for accuracy and consistency with the same code in NumberListField :
s/$field/$field_storage

Fixed.

Alright, next one :)

yched’s picture

Thanks @dawehner !

From latest interdiff:

+++ b/core/modules/options/src/Plugin/views/argument/NumberListField.php
@@ -78,9 +78,9 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-      return $this->caseTransform($this->fieldFilterXss($this->allowedValues[$value]));
+      return $this->fieldFilterXss($this->allowedValues[$value]);

Unintentionally reverts #141 ?

Other than that, this looks RTBC to me ?

dawehner’s picture

Unintentionally reverts #141 ?

... nope, caseTransform just makes sense when you can configure the case ... but we don't expose that option for numeric handlers, and based upon that, calling it, doesn't makes sense, IMHO.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Hm. That gets us back to #141 - NumberListFild is a numeric handler, but it displays a string (the human label). Exposing the regular display options for that string could make sense IMO, but I can't really say I care that much :-)

Doesn't have to block the critical from getting fixed either. Let's get this in !

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 6ab60f7 and pushed to 8.0.x. Thanks!

  • alexpott committed 6ab60f7 on 8.0.x
    Issue #2012130 by smiletrl, dawehner, damiankloip, larowlan, webflo,...
damiankloip’s picture

Nice, thanks for picking this up guys!! I slacked off.

dawehner’s picture

@damiankloip
So glad that you succeed in slacking :P

Status: Fixed » Closed (fixed)

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