REST views: Views field is not displayed as token, if exclude flag is set and the display is RestExport

Clarifying.

Also, 8.0.x gets no more releases, only 8.1.x and newer do.

In a good world we would have tests to prove that this actually fixes the issue.

+++ b/core/modules/rest/src/Plugin/views/row/DataFieldRow.php
@@ -138,9 +138,7 @@ public function render($row) {
-      if (!empty($field->options['exclude'])) {

@@ -150,7 +148,9 @@ public function render($row) {
+      if ($field->options['exclude'] == FALSE) {

I think it would be nice to keep the !empty() style, because why changing it.

.

Just some small nitpicks, sorry for this small review.

1. +++ b/core/modules/rest/src/Tests/Views/ExcludedFieldTokenTest.php
   @@ -0,0 +1,116 @@
   +        'type'        => 'article',
   +        'title'       => 'Article test ' . $i,
   

   Nitpick: We don't use additional whitespace

2. +++ b/core/modules/rest/tests/modules/rest_test_views/test_views/views.view.test_excluded_field_token_display.yml
   @@ -0,0 +1,279 @@
   +uuid: 2fcb8077-82a9-4e89-96b6-d252a4aa9922
   

   Nitpick: we don't export the UUID here usually. Let's drop it.

Ran tests locally. Indeed fails without the changes. To show to committers that this is the case, I also uploaded a tests-only patch that should fail.

I only have nitpicks, so rather than posting a review for those, I just fixed them.

Took me some time to understand the test, but after having added some debug output, I am now convinced that this test is indeed testing the thing it should be testing, and that it matches the problem description from the IS.

Manually editing patches still is error prone. Hence the test-only patch in #13 failed.

Reuploading the test-only patch with the very first line deleted.

Drupal\Tests\views\Kernel\TestViewsTest
testDefaultConfig
fail: [Other] Line 30 of core/modules/views/tests/src/Kernel/TestViewsTest.php:

Apparently the included view is violating the Views config schema. Sadly, the output that DrupalCI provides is useless. And so is the output that run-tests.sh provides:

Drupal test run
---------------

Tests to be run:
  - \Drupal\Tests\views\Kernel\TestViewsTest

Test run started:
  Monday, September 12, 2016 - 19:05

Test summary
------------

Drupal\Tests\views\Kernel\TestViewsTest                        0 passes   1 fails                            

Test run duration: 1 sec

Detailed test results
---------------------


---- Drupal\Tests\views\Kernel\TestViewsTest ----


Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      TestViewsTest.php   30 Drupal\Tests\views\Kernel\TestViews

There is a huge reason I personally want to get rid of run-tests.sh and all kind of other simpletest stuff: The integration with phpunit is not trivial nor well executed.

Here is the test running with the proper test runner (phpunit):

$ ./vendor/bin/phpunit -c core core/modules/views/tests/src/Kernel/TestViewsTest.php
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

F

Time: 7.6 seconds, Memory: 6.75Mb

There was 1 failure:

1) Drupal\Tests\views\Kernel\TestViewsTest::testDefaultConfig
Schema key views.view.test_excluded_field_token_display:display.default.display_options.filters.status.value failed with: variable type is boolean but applied schema class is Drupal\Core\TypedData\Plugin\DataType\StringData

/Users/dawehner/www/d8/core/modules/config/src/Tests/SchemaCheckTestTrait.php:43
/Users/dawehner/www/d8/core/modules/views/tests/src/Kernel/TestViewsTest.php:52

FAILURES!
Tests: 1, Assertions: 47, Failures: 1.

A thousand times yes please.

Thanks, Daniel!

Here is a follow up to deal with that: #2799021: Ensure a failing PHPUnit test shows enough information via run-tests.sh

1. @benelori nice bug to find and the fix looks good

2. +++ b/core/modules/rest/src/Tests/Views/ExcludedFieldTokenTest.php
   @@ -0,0 +1,115 @@
   +  /**
   +   * @var \Drupal\user\Entity\User|false
   +   */
   +  protected $adminUser;
   ...
   +    $this->setAdminUser();
   ...
   +  /**
   +   * Creates an admin user.
   +   */
   +  protected function setAdminUser() {
   +    $this->adminUser = $this->drupalCreateUser(array(), NULL, TRUE);
   +  }
   

   Afaics this is all dead code.

3. +++ b/core/modules/rest/src/Tests/Views/ExcludedFieldTokenTest.php
   @@ -0,0 +1,115 @@
   +    $this->createTestNodes();
   ...
   +  /**
   +   * Creates test nodes for the exported view.
   +   */
   +  protected function createTestNodes() {
   +    for ($i = 1; $i <= 10; $i++) {
   +      Node::create([
   +        'type' => 'article',
   +        'title' => 'Article test ' . $i,
   +      ])->save();
   +    }
   +  }
   

   Making this a method doesn't really add to the understandability of the test... imo the node creation should just be inlined into the setUp().

4. +++ b/core/modules/rest/src/Tests/Views/ExcludedFieldTokenTest.php
   @@ -0,0 +1,115 @@
   +    $actual_json = $this->drupalGetWithFormat($this->view->getPath(), 'json');
   ...
   +    $expected = $this->getExpectedOutput();
   ...
   +  /**
   +   * Generates a JSON with the results of the executed view.
   +   *
   +   * @return string
   +   */
   +  private function getExpectedOutput() {
   +    $this->executeView($this->view);
   +    $expected = [];
   +    foreach ($this->view->result as $rowIndex => $row) {
   +      $expected_row = [];
   +      foreach ($this->view->field as $id => $field) {
   +        if (empty($field->options['exclude'])) {
   +          $expected_row[$id] = $this->view->getStyle()->getField($rowIndex, $id)->__toString();
   +        }
   +      }
   +      $expected[] = $expected_row;
   +    }
   +
   +    return json_encode($expected);
   +  }
   

   This looks super weird - if the expected JSON is not changing why wouldn't we just hardcode it. It looks like we're duplicating the logic under test in the test.

Making this a method doesn't really add to the understandability of the test... imo the node creation should just be inlined into the setUp().

I would disagree in case we would do more than those setup nodes, but there is not much more we actually do.

This looks super weird - if the expected JSON is not changing why wouldn't we just hardcode it. It looks like we're duplicating the logic under test in the test.

Good point. Its another of those instances where we copy logic we actually want to test in the test itself.

3 fails:

1. This fail is about
   

   +      filters:
   +        status:
   +          value: true
   

   because the failure says:

   Schema key views.view.test_excluded_field_token_display:display.default.display_options.filters.status.value failed with: variable type is boolean but applied schema class is Drupal\Core\TypedData\Plugin\DataType\StringData
   

   It looks like all other views in core have value: '1', not value: true.

2. One unrelated fail in Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest.

3. +++ b/core/modules/rest/src/Tests/Views/ExcludedFieldTokenTest.php
   @@ -0,0 +1,88 @@
   +      ['nothing' => 'Article test 10'],
   +      ['nothing' => 'Article test 9'],
   +      ['nothing' => 'Article test 8'],
   +      ['nothing' => 'Article test 7'],
   +      ['nothing' => 'Article test 6'],
   +      ['nothing' => 'Article test 5'],
   +      ['nothing' => 'Article test 4'],
   +      ['nothing' => 'Article test 3'],
   +      ['nothing' => 'Article test 2'],
   +      ['nothing' => 'Article test 1'],
   

   These need to be ordered inversely according to the test output.

There we go. I went with sorting by NID as that is certainly deterministic.

Yay!

+++ b/core/modules/rest/src/Tests/Views/ExcludedFieldTokenTest.php
@@ -0,0 +1,88 @@
+    'entity_test',
...
+    'field',

These modules aren't actually needed, can be removed on commit? Don't know what changes are acceptable for that...

Unrelated fail, see #2828045: Tests failing with unrelated „Translation: The French translation is not available”, waiting for that to go away before RTBCing again

Back to RTBC

Same unrelated fail again.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!