Taxonomy aggregation of name fields

Let's role a patch against 8.x-3.x

don't hate me

The fix looks fine, but agree we probably need tests :)

Rerolled.

I tested this issue. The issue have the solution in the comment #1809128-1: Taxonomy aggregation of name fields and the test in the comment #1809128-4: Taxonomy aggregation of name fields. If I apply the test without solution the test pass fine. If I apply the patch in the comment 1 and create two taxonomy with (vocabulary1: test1, test2, vocabulary2: test1, test2) I get in the result (Agregation 1) and the link to term is correct. This is expected behavior?. Patches makes sense?.

NOTE: The reroll only contains the code from comment #1809128-4: Taxonomy aggregation of name fields (without comment #1809128-1: Taxonomy aggregation of name fields) and pass all tests.

Now i remember, the test itself doesn't fail, because the actual bug is much more complicated.

You would have to create a aggregation query and group by the name field to see see initial report.
Anyway i think the test in #6 would be helpful as it tests the functionality of the handler in general.

I attach a new patch. This patch contain the test for see if work fine the aggregation.

Sorry. The new patch

Great work!

There are just some small things.

+++ b/core/modules/views/lib/Drupal/views/Tests/Taxonomy/TaxonomyFieldTest.phpundefined
@@ -0,0 +1,96 @@
+    // Disable the link
...
+    //Check that terms[0] and term[1] is aggregated correctly and the count is 2

Should have a point at the end.

+++ b/core/modules/views/lib/Drupal/views/Tests/Taxonomy/TaxonomyFieldTest.phpundefined
@@ -0,0 +1,96 @@
+    debug($output);
...
+    debug($output);
...
+    debug($output);

Better remove all this debug calls first :)

+++ b/core/modules/views/lib/Drupal/views/Tests/Taxonomy/TaxonomyFieldTest.phpundefined
@@ -0,0 +1,96 @@
+    $view = views_get_view('test_taxonomy_field_aggregation_wo_link');

To be able to save some test views you could change the setting (whether it is linked or not), see the code after "Disable the link".

+++ b/core/modules/views/tests/views_test_config/config/views.view.test_taxonomy_field.ymlundefined
@@ -0,0 +1,53 @@
+uuid: 7dca114c-419f-46e1-9405-05415ba4939c

+++ b/core/modules/views/tests/views_test_config/config/views.view.test_taxonomy_field_aggregation.ymlundefined
@@ -0,0 +1,165 @@
+uuid: d2a34445-6850-4bde-9800-da39bf731ce6

+++ b/core/modules/views/tests/views_test_config/config/views.view.test_taxonomy_field_aggregation_wo_link.ymlundefined
@@ -0,0 +1,165 @@
+uuid: d2a34445-6850-4bde-9811-da39bf632ce6

We decided to remove the uuid from test views, sure you can't know that. Please remove UUID from all of the test views.

I attach new patch with all changes.

Thanks for all your hard work!

$view->field['name']->additional_fields = array();

Oh i see the init method does actually set the additional_fields.

As this issue is actually about the behavior of the init() method we should not just override it's values but set the option before. One way to do this would be to use $view->storage->display['default']['display_options']['fields']['name']['link_to_taxonomy'] = FALSE; and then call $view->initHandlers();

I guess it would be helpful to explain that in a short comment.

dawehner thanks for the information. I attach a new patch.

It is getting near!

+++ b/core/modules/views/lib/Drupal/views/Tests/Taxonomy/TaxonomyFieldTest.phpundefined
@@ -0,0 +1,97 @@
+    $view->field['name']->options['link_to_taxonomy'] = FALSE;

You should probably use the same code as below for the disabling of the link.

+++ b/core/modules/views/lib/Drupal/views/Tests/Taxonomy/TaxonomyFieldTest.phpundefined
@@ -0,0 +1,97 @@
+    $view->storage->display['default']['display_options']['fields']['name']['link_to_taxonomy'] = FALSE;

For easier code readability an empty line here would help.

+++ b/core/modules/views/lib/Drupal/views/Tests/Taxonomy/TaxonomyFieldTest.phpundefined
@@ -0,0 +1,97 @@
+    $view->field['name']->options['link_to_taxonomy'] = FALSE;

You just don't need this anymore. Maybe move the "Disable the link" comment above

+++ b/core/modules/views/lib/Drupal/views/Tests/Taxonomy/TaxonomyFieldTest.phpundefined
@@ -0,0 +1,97 @@
+    $this->assertTrue(strpos($output, '<span class="field-content">2</span>') !== FALSE, "The field name with aggregation work fine"); ¶

Just a space at the end. Additional you could improve the assertion message with "Check that the terms are aggregated correctly". Maybe you could also test the actual query result of the view. There is also a helper method for that $this->assertIdenticalResultSet

Ok. New patch with the changes and with other assert for test Result.

New patch with problem fixed. Another thing I've noticed. If I add a sort by name or by tid is replayed the same error. Is this behavior correct?

Regrads.

Forgot my last comment. I attach new patch. In my local passed all test.

Fixed some small nitpicks which aren't worth to let you rerole the patch.

@dawehner you had one error with one . :D

Rerolled the patch.

@h4rrydog
You seems to have a lot of parts missing.

Sorry... my fault. Here's the reroll...

Everything from 24 to 27 can be ignored :) It's simply the wrong patch.

Forgot the interdiff

#28: drupal-1809128-28.patch queued for re-testing.

Another piece of rerole.

#31: drupal-1809128-31.patch queued for re-testing.