Problem/Motivation

#2407801: Views generic field handler does not work with base fields added some code to \Drupal\views\Tests\ViewResultAssertionTrait::assertIdenticalResultsetHelper() which gets the value of a field directly from the entity contained in the view result, but it does not cast that value to a string like it is supposed to.

Proposed resolution

Cast those values to strings.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

CommentFileSizeAuthor
#8 interdiff-8.txt1.37 KBamateescu
#8 2922343-8.patch3.96 KBamateescu
#5 2922343-5.patch3.28 KBamateescu
#5 2922343-test-only.patch2.49 KBamateescu
#2 2922343.patch813 bytesamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
2922343.patch813 bytes

Very simple patch.

I'm not sure where we can add a test for this, so pinging @dawehner for help..

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Assigned: Unassigned » dawehner

Forgot to actually ping Daniel.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

We are already doing stringcasting in all the other cases, so +1 to add it here too!

I can't think of a really good place to add a test for that. The "best" place I was able to find is somewhere in \Drupal\Tests\views\Kernel\Handler\FieldFieldTest. Given that it might be worth writing a custom test class for \Drupal\views\Tests\ViewResultAssertionTrait.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
2922343-test-only.patch2.49 KB
2922343-5.patch3.28 KB

I finally figured out the problem! When we load entities from the storage, its field values are strings. However, if we change the values to be something else (e.g. integers or booleans) in a hook_entity_load() for example, that's when assertIdenticalResultsetHelper() breaks without the explicit string casting from #2.

The last submitted patch, 5: 2922343-test-only.patch, failed testing. View results

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

I've been having the same issue before on some rest issue. Saving and loading an entity isn't producing the exact same types sadly.

The tests looks as expected for me.

+++ b/core/modules/views/src/Tests/ViewResultAssertionTrait.php
@@ -96,7 +96,8 @@ protected function assertIdenticalResultsetHelper($view, $expected_result, $colu
-          $row[$expected_column] = $field->getValue($value, $column);
+          $field_value = $field->getValue($value, $column);
+          $row[$expected_column] = is_array($field_value) ? array_map('strval', $field_value) : (string) $field_value;

I think we should add an explanation here

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
2922343-8.patch3.96 KB
interdiff-8.txt1.37 KB

We were already explaining it a few lines above, just the code was not really doing what the comment was saying :)

Maybe it will be cleaner if we move the comment down a bit?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

+1. Thank you @amateescu!

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Assigned: dawehner » Unassigned

Yay, thanks for reviewing again @dawehner :) I forgot to unassign you after the initial feedback..

  • catch committed 892c0f3 on 8.5.x 
    Issue #2922343 by amateescu, dawehner: \Drupal\views\Tests\...

  • catch committed 4a3384a on 8.4.x 
    Issue #2922343 by amateescu, dawehner: \Drupal\views\Tests\...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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