Follow-up to #1292470: Convert user pictures to Image Field

Problem

  • Use-cases like Dummy Image or Gravatar module need to be able to inject "fake field values" for the user picture field.

Goal

  • Ensure that this is still possible.
  • Wouldn't mind if we directly move Gravatar into core as part of this exercise ;)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

last few posts in the aforemented issue suggest that gravatar can be a formatter on the user picture Field. If that does not work out, it would be its own field type. The new field type should set the 'picture' variable during preprocess just like the user picture Field does.

yannickoo’s picture

But the formatter needs a value. You cannot display a gravatar image based on an uploaded image.

moshe weitzman’s picture

Issue tags: -Regression

Formatters don't need a value - their hooks are called regardless. yched mentioned that at #1292470-243: Convert user pictures to Image Field. I also gave a backup plan in case these hooks don't work out.

Removing 'regression' tag since I don't see how there is a regression. gravatar can still do its job of showing a user picture without altering the templates.

tim.plunkett’s picture

Title: Ensure that use-cases like Gravatar still work after User picture field conversion » Ensure that formatters for fields with no values are still run
Issue tags: +Needs tests

Feel free to revert the title, but I think we just need some test coverage here.

sun’s picture

The other aspects of Gravatar module's use-case are actually:

- vertical extensibility: Technically, the Gravatar image formatter wants to extend Image module's image formatter - if the rendered user account has no custom/local image stored for it, then gravatar is checked and used. If there is a local image, it essentially wants to call parent::view() (or whatever the method is).

- horizontal extensibility: Technically, you could not only have the Gravatar module, but also other modules that are alternative "user picture providers" - the first provider who supplies an image wins. I quickly checked the module's code and it seems to cater for that situation.

Alan Evans’s picture

Working on a test for this ...

Alan Evans’s picture

Status: Active » Needs review
FileSize
3.39 KB

Would something along these lines fit what we're trying to test?

swentel’s picture

Hmm that looks pretty good to me, this will work fine for vertical extensibility.

I don't see horizontal extensibility in gravatar's code, it just implements hook_preprocess_user_picture unless I'm missing something completely - and it's not relevant here imo. We've got #1803064: Horizontal extensibility of Fields: introduce the concept of behavior plugins for that.

swentel’s picture

Issue tags: -Field system +Field API

tagging

swentel’s picture

Assigned: Unassigned » yched

So, this looks good to me IMO, unless I miss something. Will trigger re-test and assigning to yched for final call to RTBC.

swentel’s picture

yched’s picture

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/field/formatter/TestFieldEmptyFormatter.phpundefined
@@ -0,0 +1,59 @@
+    foreach ($items as $id => $item) {
+      if (empty($item)) {
+        // For fields with no value, just add the configured "empty" value.
+        $items[$id][0]['value'] = $this->getSetting('test_empty_string');
+      }
+    }

This looks odd. Shouldn't this be just if (empty($items)) without a foreach ?

Other than that, nitpicks :

+++ b/core/modules/field/lib/Drupal/field/Tests/DisplayApiTest.phpundefined
@@ -210,4 +210,28 @@ function testFieldViewValue() {
+   * Tests prepareView formatter hook still fires for empty values.

"Tests that the prepareView() formatter method..."
+ The test itself could use some comments as to what is being done and expected. Adding tests is good, but maintaining them during later API refactors and figuring out "ok, what's the logic in this test ?" is painful without comments :-).

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/field/formatter/TestFieldEmptyFormatter.phpundefined
@@ -0,0 +1,59 @@
+ * Definition of Drupal\field_test\Plugin\field\formatter\TestFieldEmptyFormatter.

Definition of \Drupal\...

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/field/formatter/TestFieldEmptyFormatter.phpundefined
@@ -0,0 +1,59 @@
+  /**
+   * Implements Drupal\field\Plugin\Type\Formatter\FormatterInterface::viewElements().

Should now be

/**
 * {inheritdoc]
 */
amateescu’s picture

Definition of \Drupal\...

That's actually: "Contains \Drupal\..." now :)

swentel’s picture

+++ b/core/modules/field/lib/Drupal/field/Tests/DisplayApiTest.phpundefined
@@ -210,4 +210,28 @@ function testFieldViewValue() {
+    $this->assertNoText($display['settings']['test_empty_string']);

Right, and now that I look at this, shouldn't this actually show text ?

yched’s picture

Assigned: yched » Unassigned

Unassigning now :-)

Alan Evans’s picture

Thanks for the reviews swentel and yched.

As it happens, no, at this point the field is not empty, so it shouldn't show the default "empty" text. The setUp method creates non-empty values for all of the fields. A couple of lines further down, the field values get explicitly nuked, and it's at that point that the default text needs to show.

I can see this reinforces yched's point about needing comments. Will address yched's review points shortly ...

Alan Evans’s picture

This looks odd. Shouldn't this be just if (empty($items)) without a foreach ?

I think what you get at this point if the field value is empty is an array containing an empty array, which does not in itself count as empty(), which was why I went with the foreach (which also preserves the same ID when adding in the dummy value). I will give it a try though to check and see what happens.

Alan Evans’s picture

Status: Needs review » Needs work

Yes, that was it. So, when the field is empty, $items typically contains:

Array
(
  [1] => Array()
)

The ID seems not unimportant also, so would prefer to preserve that.

I'll leave this as-is for now and do some of the other corrections (setting status to reflect that).

Alan Evans’s picture

Status: Needs work » Needs review
FileSize
3.76 KB
3.21 KB

Think I got all of those ...

Alan Evans’s picture

Assigned: Unassigned » Alan Evans

Status: Needs review » Needs work

The last submitted patch, drupal-test-empty-field-1851880-19.patch, failed testing.

Alan Evans’s picture

Well that looks like a fun new test fail, especially given that this patch only changes tests ... Will requeue for good luck.

Alan Evans’s picture

Status: Needs work » Needs review
Alan Evans’s picture

(Unable to get that specific test to fail locally FWIW)

yched’s picture

So, when the field is empty, $items typically contains:
Array
(
[1] => Array()
)

Not when testing on a loaded entity:
- Add a dsm($items); at the beginning of TextDefaultFormatter::viewElements()
- Create an article node, fill in just the title, leave the body empty
- When viewing the node, the dsm output shows that $items is array()

Your test changes the $entity by emptying the field, then saves the entity, but doesn't load it back before calling field_view_field(), that might be the difference.

However, following up on the steps above:
- If you edit your node and fill in a value for body
- Then at devel/php, execute :

$node = node_load($the_node_id);
$node->body[LANGUAGE_NOT_SPECIFIED] = array();
node_save($node);
field_view_field($node, 'body');

(which mimicks what your test does, only with a node instead of a test_entity),
the dsm() output sitll is array(), not array([the_node_id] => Array())

So something looks wrong, but it's not clear to me where...

smiletrl’s picture

@yched I think @Alan Evans has done the right thing here.

In your test, put dsm($items); in viewElements() and we get array(). That is exactly what this function works for. If we put dsm($items); in prepareView(), we will get array([the_node_id] => Array()) .

See interface FormatterInterface

   * @param array $items
   *   Array of field values for the entities, keyed by entity ID.

is for prepareView().

   * @param array $items
   *   Array of values for this field.

is for viewElements().

I did a test with class LinkFormatter, which confirmed above points. Also, in field_view_field

    // Invoke prepare_view steps if needed.
    if (empty($entity->_field_view_prepared)) {

      // some code.

      $formatter->prepareView(array($id => $entity), $display_langcode, $items_multi);
      $items = $items_multi[$id];
    }

    // Build the renderable array.
    $result = $formatter->view($entity, $display_langcode, $items);

When it comes to phase ->view() or ->viewElements(), $items has been extracted from phase ->prepareView() to be the child items.

However, the above snipt code leads to another concern for me. It seems tests added in this issue is to test this condition

  if (empty($entity->_field_view_prepared)) {

Because apparently, if this condition is met, prepareView() will be naturally executed, and then there's no need to add tests for this. We can also do anything we want in viewElements().

So, come back to original purpose of this issue, I guess it's good to include these empty field tests, but it's also nice to add tests for other field functions, like field_attach_view(), since this condition if (empty($entity->_field_view_prepared)) has been set/unset many times in other similar functions too. Just a suggestion:)

Otherwise patch at #19 looks RTBC to me.

yched’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.37 KB
3.78 KB

Oh doh, right, $items in prepareView() is an array of items for each entity. I should totally know that, sorry for adding confusion :-/.

It's just that the variable iterated on in the foreach shouldn't be named $item then, coz that's misleading.

- Renamed to $entity_items
- Moved prepareView() above viewElements(), to fit natural execution order.

Those are only minor adjustments, so I feel I can still RTBC this myself.

smiletrl’s picture

Some message for asserttions? Looks like it's a convention to add explanations message in assert:)

yched’s picture

Actually, the convention is rather to move away from custom assertion messages, as the default output makes it much easier to see what failed when something fails :-)
I.e which text was supposed to be in the page and is not, which strings were supposed to be equal and are not...

smiletrl’s picture

Yeah,that makes sense^^

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I expected this to be fixing some kind of a problem, but looks like it's just adding test coverage. I like test coverage!

#1803064: Horizontal extensibility of Fields: introduce the concept of behavior plugins was already cited as the horizontal corollary to this. I think we're good here.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

yched’s picture

LOL. Formatters do run on empty fields, but the output is still broken : #2083835: theme('field') displays no value on empty fields even if the formatter returned an output

yched’s picture

Issue summary: View changes

Updated issue summary.