Disabling 'Display all values in the same row' shows all values in all rows

urgs

Totally agree that this sounds like a bug we have to fix.

This fixed it for me.
Although I'm not sure what they are trying to do there, in #1758616: Implement core handling for limiting displayed deltas they are actually talking about moving this logic to the field formatter so maybe this one should be marked as a duplicate instead.

Thank you for the patch!

Let's first see whether this breaks other things.

In general we really need tests to ensure that things don't break anymore in the future.

Added the test, first time I'm writing one of these, hopefully it's somewhat correct.

fixed, wrong issue id in filename

Here you go

Good point, added the second test and fixed some of the code formatting issues.

1. +++ b/core/modules/views/src/Tests/GroupRowsTest.php
   @@ -0,0 +1,98 @@
   + * Definition of Drupal\views\Tests\GroupRowsTest.
   ...
   +class GroupRowsTest extends ViewTestBase {
   

   I'd be great to move this at least into the handlers test namespace and prefix the class with Field so its more clear what this is about.

2. +++ b/core/modules/views/src/Tests/GroupRowsTest.php
   @@ -0,0 +1,98 @@
   +
   +  protected $node_type;
   +
   +  protected $field_name;
   +
   +  protected $field_storage;
   +
   +  protected $field;
   

   it would be great to document them + camelcase them :(

This fixes failures of #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency so let's bump this to critical and for itself this is pretty WTF that it doesn't work (so for itself I would argue this is major)

.

1. Moved to handler namespace and renamed class to "FieldGroupRowsTest"
2. I moved those variables to the setUp function, they are also described there.

This one should apply

This looks really fine!

I would have created just one test view and manipulated the checkbox manually in code, but having two exported views is fine.

Here are two left nitpicks!

1. +++ b/core/modules/views/src/Tests/Handler/FieldGroupRowsTest.php
   @@ -0,0 +1,90 @@
   + * Definition of Drupal\views\Tests\Handler\GroupRowsTest.
   

   Ensure that we point to the right classname in the documentation here :) (got confused for a while ... ). When you change this already use "Contains \Drupal...."

2. +++ b/core/modules/views/src/Tests/Handler/FieldGroupRowsTest.php
   @@ -0,0 +1,90 @@
   +/**
   + * Tests the "Display all values in the same row" setting.
   + *
   

   Can we put an @see \Drupal\views\Plugin\views\field\Field in here? This would help with understand where this setting is coming from.

3. +++ b/core/modules/views/src/Tests/Handler/FieldGroupRowsTest.php
   @@ -0,0 +1,90 @@
   +  function testGroupRows() {
   ...
   +  function testUngroupedRows() {
   

   you could change it to use "public ..."

Fixed those small issues.

Thank you!

+++ b/core/modules/views/src/Tests/Handler/FieldGroupRowsTest.php
@@ -0,0 +1,92 @@
+  /**
+   * Testing when "Display all values in the same row" is checked.
+   */
+  public function testGroupRows() {
+    $this->drupalGet('test-group-rows');
+    $result = $this->xpath('//div[contains(@class, "views-field-field-views-testing-group-")]/div');
+
+    $rendered_value = array();
+    foreach ($result as $row) {
+      $rendered_value[] = (string) $row[0];
+    }
+    $this->assertEqual(array('a, b, c'), $rendered_value);
+  }
+
+  /**
+   * Testing when "Display all values in the same row" is unchecked.
+   */
+  public function testUngroupedRows() {
+    $this->drupalGet('test-ungroup-rows');
+    $result = $this->xpath('//div[contains(@class, "views-field-field-views-testing-group-")]/div');
+    $rendered_value = array();
+    foreach ($result as $row) {
+      $rendered_value[] = (string) $row[0];
+    }
+    $this->assertEqual(array('a', 'b', 'c'), $rendered_value);
+  }

I think this would be a better test if there was only one view and one test method - but the test just changes the group_rows value. Also an assertion difference between array('a, b, c') and array('a', 'b', 'c') is not that easy to spot. Maybe adding an assertion about the number of rows would make the distinction easier to spot.

There is a same problem for drupal 7 views module, how could this patch be applied to the views version for drupal 7?

I removed the second view and changed the group_rows value in the test.
I also moved both scenarios in 1 test method and changed the way I'm handling assertions a bit.

1. +++ b/core/modules/views/src/Tests/Handler/FieldGroupRowsTest.php
   @@ -0,0 +1,95 @@
   +    // test grouped rows.
   ...
   +    // change the group_rows checkbox to false.
   ...
   +    // test ungrouped rows.
   

   Let's start with a upper character :(

2. +++ b/core/modules/views/src/Tests/Handler/FieldGroupRowsTest.php
   @@ -0,0 +1,95 @@
   +    $this->assertEqual($view->field[$this->fieldName]->advancedRender($view->result[0]), 'a, b, c');
   ...
   +    $this->assertEqual($view->field[$this->fieldName]->advancedRender($view->result[0]), 'a');
   +    $this->assertEqual($view->field[$this->fieldName]->advancedRender($view->result[1]), 'b');
   +    $this->assertEqual($view->field[$this->fieldName]->advancedRender($view->result[2]), 'c');
   

   This is indeed way more readable!

Start comments with uppercase

Thank you!

@geertvd
Feel free to include interdiffs (see https://www.drupal.org/documentation/git/interdiff ) in more core patches you do :)

That's turned out really nice - thanks @geertvd. Committed 38b7427 and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the summary.

diff --git a/core/modules/views/src/Plugin/views/field/Field.php b/core/modules/views/src/Plugin/views/field/Field.php
index c17a359..6788677 100644
--- a/core/modules/views/src/Plugin/views/field/Field.php
+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -783,7 +783,6 @@ public function getItems(ResultRow $values) {
    *
    * @param \Drupal\views\ResultRow $values
    *   The result row object containing the values.
-   *
    * @param \Drupal\Core\Entity\EntityInterface $entity
    *   The entity to be processed.
    *

On commit I removed the new line between the @param docs since it shouldn't be there.

Interesting that this bug has returned in Drupal 8.