Updated: Comment #N

Problem/Motivation

You get errors if you try to use any aggregation options with some handlers. E.g. it's computed, so we can't use an aggregation method in the query for a field that doesn't exist. Such as Drupal\views\Plugin\views\field\Counter

Proposed resolution

Override the usesGroupBy() method from HandlerBase on all handlers where using aggregation doesn't make sense

Remaining tasks

Find/discuss all handlers this should apply to

User interface changes

Removal of 'Aggregation settings' for some handlers

API changes

None

#2030453: Aggregate field data is not available as tokens for output rewriting

CommentFileSizeAuthor
#1 2080131.patch6.94 KBdamiankloip

Comments

damiankloip’s picture

Title: Add usesGroupBy for handlers that should not allow aggregation » Disable 'Aggregation settings' for Views handlers that should not support aggregation
Status: Active » Needs review
StatusFileSize
new6.94 KB

Let's start with these. For easy, here is a list of the handlers I have changed so far (Yes I'm lazy and this is just from the git diff):

- core/modules/comment/lib/Drupal/comment/Plugin/views/field/Comment.php
- core/modules/comment/lib/Drupal/comment/Plugin/views/field/NodeNewComments.php
- core/modules/contextual/lib/Drupal/contextual/Plugin/views/field/ContextualLinks.php
- core/modules/history/lib/Drupal/history/Plugin/views/field/HistoryUserTimestamp.php
- core/modules/history/lib/Drupal/history/Plugin/views/filter/HistoryUserTimestamp.php
- core/modules/node/lib/Drupal/node/Plugin/views/field/Link.php
- core/modules/user/lib/Drupal/user/Plugin/views/field/Link.php
- core/modules/views/lib/Drupal/views/Plugin/views/field/Counter.php
- core/modules/views/lib/Drupal/views/Plugin/views/field/Custom.php
- core/modules/views/lib/Drupal/views/Plugin/views/field/Links.php
- core/modules/views/lib/Drupal/views/Plugin/views/sort/Random.php

dawehner’s picture

+1 so far.

dawehner’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

+1 should actually be RTBC.

dawehner’s picture

1: 2080131.patch queued for re-testing.

webchick’s picture

Based on the sheer number of things opting out, should we instead make this an "opt-in" property? (maybe part of the annotation? not sure)

damiankloip’s picture

Hm, not sure about annotations as we currently use the pluginID annotation for these. Also this patch changes 11 or so handlers. We have many many more than that, so in the scheme of things its not that many. We could maybe look at adding this to views info like we do with a few other properties.

Also, not sure if that is a bit out of scope for this issue. Were just turning off aggregation options in the ui for some handlers here. We could change the mecvhanism , or at least discuss it in afollow up?

xano’s picture

1: 2080131.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7d42bc2 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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