REST views: remove "Hide If Empty" option (Field Level)

Hi Anishnirmal,

I also faced the same issue and I have created patch for that.

Thanks,
Saranya.P

       // Omit excluded fields from the rendered output.
-      if (empty($field->options['exclude'])) {
-        $output[$this->getFieldKeyAlias($id)] = $value;
+       if (empty($field->options['exclude'])) {
+        if(!empty($field->options['hide_empty'])) { // hide if empty
+          if(!empty($value) ) {
+            $output[$this->getFieldKeyAlias($id)] = $value;
+          }
+        }
+        else { // show all fields
+          $output[$this->getFieldKeyAlias($id)] = $value;
+        }
       }
     }

Manually reviewed the patch and it doesn't seems to follow the Drupal Coding standards
Could you please have a look at it.

See the discussion on the existing code here: #2606548: \Drupal\rest\Plugin\views\row\DataFieldRow::render should take into account the 'exclude' flag. I'm not getting into that discussion again, especially since the security implications aren't there for this instance.

Hiding a field when empty sounds like really weird behaviour in a REST service to me, but if we offer the option, we should respect it. So in that sense we should fix it, but in my opinion it would be a better fix to remove the option to do so.

Hi @Dinesh18

Thanks for find coding standards issue, i remove all these errors.

Rename patch please review

Thanks! However, to prove that this is indeed solving the problem, this needs tests.

The patch in #9 appears to be the same as #7, regardless it still doesn't meet with coding standards. I have also simplified the logic.

@Wim Leers Is there already a test for this class that we can add to? Or does this require its own new test file?

Hi Jo Fitzgerald,
Thanks for review
I changed as per #4 comments
Well done you do the great job. :)

\Drupal\comment\Tests\Views\CommentRestExportTest is the only functional Views REST export test that I can find. So that's the only place you might be able to add test coverage to. Yes, REST + Views has barely any test coverage :(

Needs work for tests.

Running into the same problem, that the view option "Hide if empty" is ignored. When applying the patch from #11, we discovered, that a simple !empty($value) will only work with raw output but not with rendered values, e.g. images (the direct lines before the patch allow either a $field->getValue() or a $field->advancedRender()). One solution could be to rely always on the raw output, therefore instead of !empty($value) asking for !empty($field->getValue($row)). I'm not sure, if this is a nice solution or a bad workaround. Any opinons on this?

       // Omit excluded fields from the rendered output.
       if (empty($field->options['exclude'])) {
-        $output[$this->getFieldKeyAlias($id)] = $value;
+        if (empty($field->options['hide_empty']) || !empty($field->getValue($row))) {
+          $output[$this->getFieldKeyAlias($id)] = $value;
+        }
       }
     }

#16 That change indeed helped hiding image link when there is no image, without using raw value (which always prints nid where image is attached).

We had the same problem as #16 points out. I've updated the patch from #11 with their suggestion. It does make sense to me to check the raw value instead of a rendered one.

Checking for empty() alone makes it so integer 0 or string "0" are hidden too which should not be the default case. This version of the patch checks for NULL or empty string instead.

I had problems with #11 and also #23 after upgrading to Drupal 9 from Drupal 8.
Have attached the version thats currently working for me.

Working with the custom preprocessor the raw integer value could have the following content:
[0 => 0], this should be empty.
Check if !empty() is not enough.

I have added a validation using array_filter if the raw output is an array.

Working with the custom preprocessor the raw integer value could have the following content:
[0 => 0], this should be empty.
Check if !empty() is not enough.

I have added a validation using array_filter if the raw output is an array.

#25 has a typo, use this instead.

Working with the custom preprocessor the raw integer value could have the following content:
[0 => 0], this should be empty.
Check if !empty() is not enough.

I have added a validation using array_filter if the raw output is an array.

#26 has a typo (again), use this instead.

Attached patch using https://api.drupal.org/api/drupal/core%21modules%21views%21src%21Plugin%... to check if the value is empy:

Adding:

// Omit 'Hide if empty' fields from the rendered output for empty values.
      if ($field->options['hide_empty'] && $field->isValueEmpty($value, $field->options['empty_zero'])) {
        continue;
      }

right before the check for // Omit excluded fields from the rendered output.

Attached patch with a test included:

/**
   * Tests the display of an "hide if empty" body field.
   */
  public function testHideIfEmptyBodyDisplay() {
    $actual_json = $this->drupalGet($this->view->getPath(), ['query' => ['_format' => 'json']]);
    $this->assertSession()->statusCodeEquals(200);
    $expected = [
      ['title' => 'Article Title 10', 'body' => "<p>Article test 10</p>\n"],
      ['title' => 'Article Title 9'],
      ['title' => 'Article Title 8', 'body' => "<p>Article test 8</p>\n"],
      ['title' => 'Article Title 7'],
      ['title' => 'Article Title 6', 'body' => "<p>Article test 6</p>\n"],
      ['title' => 'Article Title 5'],
      ['title' => 'Article Title 4', 'body' => "<p>Article test 4</p>\n"],
      ['title' => 'Article Title 3'],
      ['title' => 'Article Title 2', 'body' => "<p>Article test 2</p>\n"],
      ['title' => 'Article Title 1'],
    ];

    $expected_json = json_encode($expected, JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_QUOT | JSON_HEX_AMP | JSON_UNESCAPED_UNICODE);
    $this->assertSame($expected_json, $actual_json);
  }

To run it:

$ cd core
$ ../vendor/bin/phpunit --filter testHideIfEmptyBodyDisplay modules/rest/tests/src/Functional/Views/HideIfEmptyFieldTest.php

Looks good to me.

+++ b/core/modules/rest/tests/src/Functional/Views/HideIfEmptyFieldTest.php
@@ -0,0 +1,108 @@
+  protected $profile = 'standard';

There's no need to explicitly install the standard profile. The testing profile should be sufficient for it.

Thanks Naveen, I'm using the standard profile because I need to have a body field in the article content type, also we may need extra tests to check: field_image and field_tags from the same content type and all those fields are provided by default on standard profile.

Testing #36 and it works. Attaching screenshots.

For the committer can we have tests-only patch also to make sure it's working.

Patch is the same as #36 but with a tests-only patch also.

since I didn't make any change just was checking the testing I think I can mark this as RTBC

The Issue Summary here is incomplete. Write an issue summary for an existing issue has guidance. The changes should include the comment in #5 because it questions if this should be done at all. Adding tag.

in #37, it is said that 'this looks good' There is no indication if that means a patch review was done or simply that the patch works as expected. This needs a code review as well.

My comments below do not constitute a full code review.

1. +++ b/core/modules/rest/tests/src/Functional/Views/HideIfEmptyFieldTest.php
   @@ -0,0 +1,108 @@
   + * @see \Drupal\rest\Plugin\views\display\RestExport
   + * @see \Drupal\rest\Plugin\views\row\DataFieldRow
   

   I don't recall seeing @see lines in a test before. Is the intention to be an @covers or something else?

2. +++ b/core/modules/rest/tests/src/Functional/Views/HideIfEmptyFieldTest.php
   @@ -0,0 +1,108 @@
   +   * @var \Drupal\views\ViewExecutable
   

   Missing one line description.

3. +++ b/core/modules/rest/tests/src/Functional/Views/HideIfEmptyFieldTest.php
   @@ -0,0 +1,108 @@
   +   * The profile to install as a basis for testing.
   

   This comment is not adding any useful information. Let's remove it.

Should this not be in the views component?

+++ b/core/modules/rest/tests/src/Functional/Views/HideIfEmptyFieldTest.php
@@ -0,0 +1,108 @@
+  protected $profile = 'standard';

Also, the standard profile having some fields you need isn't a good enough reason to use that, the overhead of using standard vs testing profile is massive, please just add the fields and content types you need in the test, since this also makes it much clearer what is being tested.

Still not convinced we shouldn't just remove the option, but this might not be that easy for just Rest view fields. So to answer my own feedback in #5: this is probably the best way to do this.

Updated IS summary.

Also addressed #44 1-3. and #45

Should this not be in the views component?

Could see it there or here. Not sure.

Thanks! Looks good to me.
I see the concerns in #38, #44, and #45 has been addressed

I feel that #5 has never been properly answered. I think we need a use-case here were this makes sense. At the moment we run the risk of breaking people's assumptions because atm empty values are being added to the REST output.

I think if no use-case can be found then I think we should remove the functionality and maintain the behaviour. It's less risky to existing sites.

I agree with @Lendude and @alexpott here, removing the functionality for rest maintains bc and is less risky. It would also be a very weird behavior to only sometimes include the field in the same response.

So like this?

Moving to 9.5.x

Used a special file naming because this patch is following a different approach

Change record created https://www.drupal.org/node/3313620
Also added it to the release notes (not sure if this is needed for this issue).

Latest patch still needs review and so does the change record/release note. Re-worded the title a bit.

+++ b/core/modules/rest/rest.module
@@ -34,3 +34,16 @@ function rest_help($route_name, RouteMatchInterface $route_match) {
+function rest_form_views_ui_config_item_form_alter(&$form, &$form_state, $form_id) {
+  if ($form_id == "views_ui_config_item_form") {

No need to check the $form_id in a form ID specific hook, it should only get called when the form ID is that form ID.

This seems way too generic though. Won't it remove the option for all Views fields when you enable the Rest module? We need to specifically target the form then it is being used in a Rest display (not a clue how to do that from the top of my head, sorry).

You may remove the option by default for backwards compatibility, but please make the option to hide fields into a setting. My use case is that I have a bunch of different content types with different fields. I use the grouping patch from here https://www.drupal.org/project/drupal/issues/3251937 to group the fields by content type. In this case it doesn't make sense to show empty fields for those content types that don't have the fields.

I think this issue still needs an IS update. The title of the issue has been updated to reflect that the approach in the patches is to remove the "Hide if empty" option, however the Proposed resolution is still talking about the expected behavior when the option is checked, which is the inverse of removing the option entirely.