Problem/Motivation

With any fields using the views Field field handler, which is a lot after all the conversions to re-use field formatters, you cannot use the field row plugin and raw output. It will always be the rendered value.

Proposed resolution

Fix the DataFieldRow plugin so it works with fields using the Field handler too. The problem comes about as the Field handler never sets the field_alias property, which the DataFieldRow plugin checks for.

Remaining tasks

Tests

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip created an issue. See original summary.

damiankloip’s picture

FileSize
0 bytes
damiankloip’s picture

FileSize
1.61 KB

Damn, empty patch. Here is a patch just with a potential fix.

damiankloip’s picture

Status: Active » Needs review
FileSize
2.71 KB
4.32 KB

With some test coverage.

The last submitted patch, 4: 2574077-4-test-only-FAIL.patch, failed testing.

The last submitted patch, 4: 2574077-4-test-only-FAIL.patch, failed testing.

damiankloip’s picture

+++ b/core/modules/rest/src/Plugin/views/row/DataFieldRow.php
@@ -140,7 +140,7 @@ public function render($row) {
+      if (!empty($this->rawOutputOptions[$id])) {

I tihnk this change is ok, fields have changed sooo much since we added this originally. It doesn't make sense for the field handler with no actual field alias used, as the entity value is fetched directly.

dawehner’s picture

  1. +++ b/core/modules/rest/src/Plugin/views/row/DataFieldRow.php
    @@ -140,7 +140,7 @@ public function render($row) {
           // If this is not unknown and the raw output option has been set, just get
           // the raw value.
    -      if (($field->field_alias != 'unknown') && !empty($this->rawOutputOptions[$id])) {
    +      if (!empty($this->rawOutputOptions[$id])) {
    

    The document is outdated now, this is for sure. Is there a reason why we ever did that? IMHO this was a bad idea in the first place, this is for sure! Yeah that comment was horrible and pointless and all the time.

  2. +++ b/core/modules/views/src/Plugin/views/field/Field.php
    @@ -241,8 +241,11 @@ public function query($use_groupby = FALSE) {
    +      $table_mapping = $this->getTableMapping();
    +      $field_definition = $this->getFieldStorageDefinition();
    +
           foreach ($options as $column) {
    -        $fields[$column] = $this->getTableMapping()->getFieldColumnName($this->getFieldStorageDefinition(), $column);
    +        $fields[$column] = $table_mapping->getFieldColumnName($field_definition, $column);
           }
     
           $this->group_fields = $fields;
    

    ah so this doesn't change anything fundemental

damiankloip’s picture

Sorry, forgot about this, updated comment on the patch!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @damiankloip

charginghawk’s picture

This patched worked for me - thanks! There was an issue where if the value was an array, the output would simply be "Array". Maybe something should be thrown in, along the lines of...

      // If the raw output option has been set, just get the raw value.
      if (!empty($this->rawOutputOptions[$id])) {
        // Check whether value is array/object.
        if (is_array($field->getValue($row)) || is_object($field->getValue($row))) {
          $value = $row->_entity->$id;
        }
        else {
          $value = $field->sanitizeValue($field->getValue($row), 'xss_admin');
        }
      }

That'll give a serialized array output, instead of just "Array".

catch’s picture

Status: Reviewed & tested by the community » Needs review

If #11 is right look like we should keep going here and fix that case as well.

dawehner’s picture

Status: Needs review » Needs work

Well yeah the question is actually, what do you want to do in case there is an array/object, kinda tricky indeed.

damiankloip’s picture

If we just removed the unnecessary sanitizeValue() this would not be an issue.

dawehner’s picture

So what about just doing that then? Did anyone relied on that behaviour? Its rest, they should take care about escaping on the frontend code, if they really use REST for frontend stuff.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.47 KB
726 bytes

Yes, it makes no sense to me. It's ridiculous, not sure why it was there to start with (my fault I think).

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I guess we are fine that people have to escape when they output. It is simply the right thing to do.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I think we should add tests for the change in #16 both for an array value and a value with XSS.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.41 KB
7.18 KB
3.28 KB

Added some more coverage for these cases.

Status: Needs review » Needs work

The last submitted patch, 19: 2574077-19.patch, failed testing.

The last submitted patch, 19: 2574077-19-tests-only-FAIL.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
5.41 KB
7.18 KB
851 bytes

Whoops, added the assertion text in the wrong parens! Let's try that again.

The last submitted patch, 22: 2574077-22-tests-only-FAIL.patch, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The fix is super clean, this is for sure.

charginghawk’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.17 KB
87 bytes

I think there's a case for using $value = $row->_entity->$id; instead of $value = $field->getValue($row);.

In my use case, I have a normalizer plugin for a specific field type (see #2614292: Add normalizers for Field Collection Items (to enable REST/JSON API support)). Using the "Show: Entity" option, it gets routed through DataEntityRow, which just returns return $row->_entity;, and correctly outputs the field per my normalizer plugin.

However, if I use "Show: Fields" with this patch, it sidesteps my normalizer plugin completely. I think the ->getValue($row) departs from the serializing process.

So, I think the entity and field formatters should handle this logic the same way, and I'm submitting a patch for that.

Status: Needs review » Needs work

The last submitted patch, 25: 2574077-25.patch, failed testing.

charginghawk’s picture

Status: Needs work » Needs review
FileSize
7.17 KB

Patch failed to apply... :(

Let's try this again.

Status: Needs review » Needs work

The last submitted patch, 27: 2574077-27.patch, failed testing.

damiankloip’s picture

Sorry, a change like that almost certainly needs to be a follow up IMO. I think you are maybe getting confused about 'fields' in views. Not all of these will be a field API field or whatnot. This is about a views field, not necessarily a field on an entity. This could be any field defined in views data.

In that light the patch in #22 and the RTBC in #24 is good I think.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

Yeah fields make just more sense.

damiankloip’s picture

Just to be clear, RTBC is for patch in #22.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

@charginghawk - I agree with @damiankloip - let's explore #25 in a new issue.

Committed #22 569de54 and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed 8aeb8b9 on 8.1.x
    Issue #2574077 by damiankloip, dawehner: REST Export display cannot show...

  • alexpott committed 569de54 on
    Issue #2574077 by damiankloip, dawehner: REST Export display cannot show...
charginghawk’s picture

damiankloip’s picture

Thanks charginghawk!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.