Add an isComputed method to field handlers

I am in the process of re-rolling the patch so that it applies cleanly. At the same time I am checking it for issues and completeness.

There are two features of this patch that I have noted whilst re-rolling it:

The method isComputed() is public. Is this intentional, given that it is on the 8.0.x branch?

Various classes in core override query() without calling query() in the parent class. This removes the check for isComputed() in any of their children. I think this is okay. However, the classes that do this are:

• Drupal\user\Plugin\views\field\Link
• Drupal\taxonomy\Plugin\views\field\LinkEdit
• Drupal\node\Plugin\views\field\Path
• Drupal\comment\Plugin\views\field\StatisticsLastUpdated
• Drupal\node\Plugin\views\field\Link
• Drupal\search\Plugin\views\field\Score
• Drupal\comment\Plugin\views\field\NodeNewComments
• Drupal\comment\Plugin\views\field\StatisticsLastCommentName
• Drupal\user\Plugin\views\field\Permissions
• Drupal\user\Plugin\views\field\Roles
• Drupal\taxonomy\Plugin\views\field\TaxonomyIndexTid

Re-roll of patch in comment #1

Given that this is just an internal change, this should be fine! It makes it easier for contrib and custom modules.

+  public function isComputed() {
+    return isset($this->definition['computed']) ? $this->definition['computed'] : FALSE;
+  }

I don't understand why we introduce support for specifying this in the definition (i.e. in the annotation, right?) but then not actually use it but override the method each time. Seems like the base implementation could just return FALSE then, right? Can someone explain?

We use the definition pattern in case we have entries in hook_views_data() you would otherwise have not to
create a class. This is though not the case here, because you will always override render() for computed fields.

Nice observation tobias!

So that means this is "needs work", right? Or did I misinterpret your comment?

I just noticed a potential problem with a comment.

In the comments for FieldPluginBase::isComputed() it says:

"The default query behavior will be be invoked if this is TRUE."

Surely that should say FALSE. query() does nothing if isComputed() === TRUE

Uploaded a new patch (and interdiff) to address comments #9 - #13

Alright, let's fix that quickly

Feedback got fixed, thanks!

Sure.

Edit: Hehe, wanted to RTBC as well

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

Added

+++ b/core/modules/content_translation/src/Plugin/views/field/TranslationLink.php
@@ -72,16 +72,18 @@ protected function renderLink(EntityInterface $entity, ResultRow $values) {
diff --git a/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php b/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php

diff --git a/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php b/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php
index 684e4b7..87a5e3d 100644

index 684e4b7..87a5e3d 100644
--- a/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php

--- a/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php
+++ b/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php

+++ b/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php
+++ b/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php
@@ -134,6 +134,4 @@ public function render(ResultRow $values) {

@@ -134,6 +134,4 @@ public function render(ResultRow $values) {
     }
   }
 
-  public function query() { }
-
 }

Why does this not get an isComputed implementation?

Valid response!

Re-rolled and added isComputed in /core/modules/contextual/src/Plugin/views/field/ContextualLinks.php

+++ b/core/modules/comment/src/Plugin/views/field/Link.php
@@ -59,6 +59,14 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
+
+

Nitpicking, double newline

Plus, this needs tests. Probably just something along the lines of testClickSortable() in \Drupal\views\Tests\Handler\FieldUnitTest would be a good start.

Rerolled the patch as it would no longer apply.
Needs review.

+++ b/core/modules/views/src/Tests/Handler/FieldUnitTest.php
@@ -563,6 +563,19 @@ public function testClickSortable() {
+   * Tests the isComputed method.
+   */
+  public function testIsComputed() {
+    // Test that isComputed is FALSE by default.
+    $item = array(
+      'table' => 'views_test_data',
+      'field' => 'name',
+    );
+    $plugin = $this->container->get('plugin.manager.views.field')->getHandler($item);
+    $this->assertFALSE($plugin->isComputed(), 'FALSE as a default value is correct.');
+  }

Test should probably also assert that query() is not called on field handlers that have isComputed() === TRUE.

I'm also wondering if we should set isComputed = TRUE for the Broken field handler?

Test should probably also assert that query() is not called on field handlers that have isComputed() === TRUE.

Nvm, I talked to @Hjarnmastara, and since it's query()'s responsibility to check isComputed(), this might be hard to test.

Couple of minor tweaks

1. +++ b/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php
   @@ -96,6 +100,8 @@ public function preRender(&$values) {
   +   *
   +   * @return string
   

   unrelated change

2. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -779,7 +798,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +        '#options' => array(
   

   extra indent

3. +++ b/core/modules/views/src/Plugin/views/field/Links.php
   @@ -28,6 +28,13 @@ public function usesGroupBy() {
   +  /**
   +   * Overrides \Drupal\views\Plugin\views\field\FieldPluginBase::defineOptions().
   +   */
   

   Can just be {@inheritdoc}

Applied the minor tweaks.

Needed a reroll.

As per https://www.drupal.org/core/d8-allowed-changes : Provides new functionality or a new API: postpone it to 8.1.x.

So done

Lets open this back up for 8.1.x

IMHO this would be still really nice to have.

#2401953: Database exception for View with Combined field filter with fields with no query alias landed so this patch needs to address the code comment that issue added.

As per #45 updated the combine filter to use the isComputed method.

You missed to add it to the FieldHandlerInterface

Added the method to FieldHandlerInterface and moved the doc block there too. Makes sense right? Or not?

Updating patch for 8.2.x branch.
Re-rolled fresh patch as #48 wasn't being applied.

Added reroll tag. The issue summary could use some updates to reflect comments from @alexpott and current patch.

I'm working on this on DrupalCon Dublin mentored sprint, re-rolling a patch using this instructions: https://www.drupal.org/contributor-tasks/reroll.

I'm working on this on DrupalCon Dublin mentored sprint, re-rolling a patch using this instructions: https://www.drupal.org/contributor-tasks/reroll.

Patch is re-rolled , please have a look

Seems the patch in #57 accidentally removed the render() function from the Custom field plugin, causing the test failures. Added it back and fixed some whitespace issues that were introduced.

1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
   @@ -1072,4 +1072,10 @@ public function getValue(ResultRow $values, $field = NULL) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function isComputed() {
   +    // TODO: Implement isComputed() method.
   +  }
   

   Should this be a follow up or still be resolved as part of this patch?

2. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -131,12 +131,21 @@ protected function allowAdvancedRender() {
   +   * @inheritdoc
   

   Nitpick: Its {@inheritdoc} :(

3. +++ b/core/modules/views/src/Plugin/views/filter/Combine.php
   @@ -108,11 +108,8 @@ public function validate() {
   +        elseif ($fields[$id]->isComputed()) {
   +          // Combined filter breaks with computed fields.
              $errors[] = $this->t('Field %field set in %filter is not usable for this filter type. Combined field filter only works for simple fields.', array('%field' => $fields[$id]->adminLabel(), '%filter' => $this->adminLabel()));
            }
   

   This is really nice, IMHO

Feel free to set it back, but this TODO seems to be a bit dangerous.

Thanks @dawehner

Agreed that we should have a default implementation for isComputed, returning FALSE. Only concern is that right now, to "implement" a computed field, a plugin just overrides query(). By introducing isComputed() and having this return FALSE, we are essentially making those existing fields not computed. Might not be a big deal since the isComputed() method is only used in the Combine plugin for now?

Also fixed the {@inheritdoc}, and noticed a documentation error.

@mr.baileys nice work!

Only concern is that right now, to "implement" a computed field, a plugin just overrides query(). By introducing isComputed() and having this return FALSE, we are essentially making those existing fields not computed.

Well we go from 'we assume it isn't computed, and you have no way of telling us it is', to 'we assume it isn't computed, but you can tell us it is'. So I don't think we will be introducing any new false assumptions.

See #2695207: Text (plain, long) field is not click sortable and does not appear in combine fields filter as an example of where the current attempt at assumption goes wrong.

Re-rolled for short array syntax and \Drupal\views\Plugin\views\field\Field deprecation.

+++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -267,4 +267,18 @@ public function getRenderTokens($item);
+  public function isComputed();

Adding this to the interface is a BC break (I know I did it myself in #48, the things we learn), anything implementing FieldHandlerInterface but not extending FieldPluginBase will break. I doubt this happens in the wild, but technically it's a BC break. Or are interfaces always internal (since any change to them will always lead to a BC break)?

Also we need some additional test coverage for the introduced 'if's

1. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -138,12 +138,21 @@ protected function allowAdvancedRender() {
   +    // Computed fields have no business with the query.
   +    if (!$this->isComputed()) {
   +      $this->ensureMyTable();
   +      // Add the field.
   +      $params = $this->options['group_type'] != 'group' ? ['function' => $this->options['group_type']] : [];
   +      $this->field_alias = $this->query->addField($this->tableAlias, $this->realField, NULL, $params);
   +      $this->addAdditionalFields();
   +    }
   

   Needs test coverage

2. +++ b/core/modules/views/src/Plugin/views/filter/Combine.php
   @@ -34,9 +34,9 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +        // Don't allow computed fields. Computed fields will break the Combine
   +        // filter.
   +        if (!$field->isComputed()) {
              $options[$name] = $field->adminLabel(TRUE);
            }
   

   Needs test coverage

3. +++ b/core/modules/views/src/Plugin/views/filter/Combine.php
   @@ -109,11 +109,8 @@ public function validate() {
   +        elseif ($fields[$id]->isComputed()) {
   +          // Combined filter breaks with computed fields.
              $errors[] = $this->t('Field %field set in %filter is not usable for this filter type. Combined field filter only works for simple fields.', ['%field' => $fields[$id]->adminLabel(), '%filter' => $this->adminLabel()]);
            }
   

   Needs test coverage.

@Lendude: https://www.drupal.org/core/d8-bc-policy

(undocumented): If an API carries neither of these distinctions, we treat it as "we'll try very hard not to break it, but we may. These will receive a release notes mention.

Interfaces follow a similar pattern as above with respect to @api, @internal, or neither. However, in case of neither tag, the interface is treated as an API for callers but not for implementers.

Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.
However, we reserve the ability to add methods to these interfaces in minor releases to support new features.

TLDR: It should be safe to add it in a minor release. NW for tests.

+++ b/core/modules/views/src/Plugin/views/field/EntityField.php
@@ -1093,6 +1093,13 @@ public function getValue(ResultRow $values, $field = NULL) {
   /**
    * {@inheritdoc}
    */
+  public function isComputed() {
+    return FALSE;
+  }

#2852067: Add support for rendering computed fields to the "field" views field handler went and did "Update the EntityField plugin to support computed fields.", so can't just return FALSE.

Only patch reroll to contribute to this cause.

Needs testing for each of the different handler with a computed field.

Re-roll patch created and fixed test, Please review.

As @daffie pointed out in #75 we need tests for all the changed handlers that isComputed() does indeed return FALSE, and we need test coverage for the change in query().

@Lendude

need tests for all the changed handlers that isComputed() does indeed return FALSE

I've added test cases for few field handlers. Can you please confirm if the approach is correct?

we need test coverage for the change in query().

I am not sure how we can test it.
One test case I saw in FieldKernelTest.php where we are testing query using testQuery() method.

However, in our case I want to test that whenever $this->isComputed() is true, no further code gets related to
$this->ensureMyTable(); gets executed.
Any idea how can test this one?

@mohit_aghera one way to check your tests is to upload a tests-only patch with the full one. So the tests only patch will fail and regular ones passes then you know it was a good test.

Moving to needs work based on your other questions though

Rerolling #82 for 10.2

Ignore last patch; let's try this again

Third time is a charm: re-roll for 10.2