Views "group by" on "Field API" fields broken [#2081533]

Problem/Motivation

core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php:

      $rkey = $this->definition['is revision'] ? 'FIELD_LOAD_REVISION' : 'FIELD_LOAD_CURRENT';
      // Go through the list and determine the actual column name from field api.
      foreach ($options as $column) {
        $name = $column;
        if (isset($this->field_info['storage_details']['sql'][$rkey][$this->table][$column])) {
          $name = $this->field_info['storage_details']['sql'][$rkey][$this->table][$column];
        }
       }

Original report:

I'm not sure what this code does, but this is most likely broken, 'storage_details' doesn't exist anymore after #1497374: Switch from Field-based storage to Entity-based storage

Proposed resolution

Remaining tasks

For now, ignore the patches on this issue and see if it is still a problem. There is no longer any mention of 'storage_details', as was originally reported. Test the following scenarios :

count by NID, groupby field
groupby field A, count field B

If this is no longer a problem, the issue can be safely closed out as not able to reproduce.

User interface changes

n/a

API changes

n/a

Comment	File	Size	Author
#19	views-group-scenario2.png	65.06 KB	epersonae2
#17	views-group-scenario1.png	64.59 KB	epersonae2
#14	2081533-test-start-do-not-expect-anything-at-all.patch	1.38 KB	dawehner
#14
#7	field_views_storage_details-2081533_7.patch	1.38 KB	yched
#7
#4	views.view_.test_field_group_by.yml_.txt	5.07 KB	yched
#2	views.view_.test_field_group_by.yml_.txt	5.15 KB	yched
#1	field_views_storage_details-2081533-1.patch	1.31 KB	yched
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

yched CreditAttribution: yched commented 4 September 2013 at 13:35

Title:	Field plugin for "Field API" fields still refers to 'storage_details'	» Views "group by" on "Field API" fields broken
Status:	Active	» Needs review

File	Size
field_views_storage_details-2081533-1.patch	1.31 KB

Patch.

What is weird though is that I can't reproduce an actual fail here.
- I set up a view that uses "group by" on a Field API field
("Format" section, click "settings"; in the "Style options" popup, select the field in "Grouping field Nr.1")
- The view works as expected in both HEAD & patch

That's because \Drupal\field\Plugin\views\field\Field::query() is always called with param $use_groupby == FALSE, so the code fixed here doesn't run anyway - yet the view works, results are grouped by values of the field.

What am I missing here ? Am I using the wrong views setup to test $use_groupby ?

Comment #2

yched CreditAttribution: yched commented 4 September 2013 at 19:55

File	Size
views.view_.test_field_group_by.yml_.txt	5.15 KB

Uploading the test view I'm using - groups on node "body" field.

Field name should be views.view.test_field_group_by.yml, obviously - d.o renamed it "for security reasons" (?).

Comment #3

dawehner

German

CreditAttribution: dawehner commented 4 September 2013 at 22:27

Issue tags:

+Needs tests

Just in general we should add test coverage for it, so something like /core/modules/views/lib/Drupal/views/Tests/QueryGroupByTest.php but for fieldapi fields.

Comment #4

yched CreditAttribution: yched commented 4 September 2013 at 23:15

File	Size
views.view_.test_field_group_by.yml_.txt	5.07 KB

As @dawehner pointed in IRC, I was looking at the wrong place in Views UI to set up aggregation.

This view generates a broken query in HEAD, and runs fine with the patch.
Although honestly, writing a test similar to QueryGroupByTest is a little bit beyond me right now :-/

Comment #5

dawehner

German

CreditAttribution: dawehner commented 4 September 2013 at 23:19

Sure sure, I just wanted to give myself some hints tomorrow, as it is so late that I won't remember everything. Maybe you should also consider to take a nap :)

Comment #6

yched CreditAttribution: yched commented 4 September 2013 at 23:50

LOL - yes, way past naptime indeed :-)

Comment #7

yched CreditAttribution: yched commented 5 September 2013 at 11:30

File	Size
field_views_storage_details-2081533_7.patch	1.38 KB

Reroll after #2079011: Move field.module constants related to storage to EntityStorageControllerInterface

Comment #8

swentel CreditAttribution: swentel commented 4 October 2013 at 09:37

Note sure if this is related, by process_entity() also has following code:

      foreach ($this->group_fields as $field_name => $column) {
        if (property_exists($values, $this->aliases[$column])) {
          $base_value[$field_name] = $values->{$this->aliases[$column]};
          if (isset($base_value[$field_name])) {
            $data = TRUE;
          }
        }
      }

$values doesn't even exist within the scope

Comment #9

swentel CreditAttribution: swentel commented 5 October 2013 at 21:58

note, also mentioned in #1866260: Undefined variable in Field:process_entity()

Comment #9.0

swentel CreditAttribution: swentel commented 5 October 2013 at 21:58

Issue summary:

View changes

Issue link

Comment #10

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran commented 19 January 2014 at 08:02

7: field_views_storage_details-2081533_7.patch queued for re-testing.

Comment #11

19 January 2014 at 09:34

Status:

Needs review

» Needs work

The last submitted patch, 7: field_views_storage_details-2081533_7.patch, failed testing.

Comment #12

dawehner

German

CreditAttribution: dawehner commented 20 January 2014 at 21:41

Priority:	Normal	» Critical
Issue summary:	View changes

Given that people rely on this feature I consider this as a critical.

Comment #13

catch

he/him

English

CreditAttribution: catch commented 21 January 2014 at 11:16

Priority:

Critical

» Major

Critical is data loss/fatal errors/systemic bugs, this is self-contained and doesn't harm anything else, but agreed on major rather than normal at least.

Comment #14

dawehner

German

CreditAttribution: dawehner commented 10 November 2014 at 21:19

File	Size
2081533-test-start-do-not-expect-anything-at-all.patch	1.38 KB

So we need to ensure that we test the following scenarious:

count by NID, groupby field
groupby field A, count field B

Here is something like a start for a test.

Comment #15

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC commented 14 May 2015 at 17:33

Issue summary:	View changes
Issue tags:		+Needs manual testing, +Novice

Comment #16

epersonae2 CreditAttribution: epersonae2 commented 15 May 2015 at 18:19

I'm trying out the proposed scenarios. Will report back shortly.

Comment #17

epersonae2 CreditAttribution: epersonae2 commented 15 May 2015 at 18:40

Issue summary:

View changes

File	Size
views-group-scenario1.png	64.59 KB

Grouping under Format works fine, but these scenarios sound more like Aggregation (under Advanced).

Scenario 1: Count NID, Group Results Together by Value on Title (Group column: Value) works with two nodes with the same title. See attached screenshot.

Going to test scenario 2 momentarily.

Comment #18

epersonae2 CreditAttribution: epersonae2 commented 15 May 2015 at 18:45

Issue summary:

View changes

Comment #19

epersonae2 CreditAttribution: epersonae2 commented 15 May 2015 at 18:47

File	Size
views-group-scenario2.png	65.06 KB

Tested scenario 2: Group by Content: Relationship (a text list field), Count Title. Looks like it works fine. See screenshot.

Comment #20

andriyun CreditAttribution: andriyun commented 30 August 2015 at 20:13

Issue tags:

-Needs manual testing

Manual test added. So... Lets remove proper tag.

Comment #21

geertvd CreditAttribution: geertvd at XIO commented 6 September 2015 at 09:54

Assigned:	Unassigned	» geertvd
Issue tags:	-Novice	+DUGBE0609

I think the novice part of this issue was the manual testing, so removing that tag.
I'm working on automated tests now.

Comment #22

geertvd CreditAttribution: geertvd at XIO commented 6 September 2015 at 11:49

Status:

Needs work

» Closed (fixed)

so this is not actually broken in HEAD anymore.
The code referenced in the IS is not there anymore, and I'm seeing some test coverage for this in \Drupal\views\Tests\QueryGroupByTest and \Drupal\views_ui\Tests\GroupByTest

So i think this can be closed since this has been fixed elsewhere (probably #2407801: Views generic field handler does not work with base fields).