Posted by sun on November 27, 2012 at 3:50pm
20 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | field system |
| Category: | task |
| Priority: | major |
| Assigned: | Alan Evans |
| Status: | closed (fixed) |
| Issue tags: | Field API, Needs tests |
Issue Summary
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 ;)
Comments
#1
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.
#2
But the formatter needs a value. You cannot display a gravatar image based on an uploaded image.
#3
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.
#4
Feel free to revert the title, but I think we just need some test coverage here.
#5
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.
#6
Working on a test for this ...
#7
Would something along these lines fit what we're trying to test?
#8
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_pictureunless 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.#9
tagging
#10
So, this looks good to me IMO, unless I miss something. Will trigger re-test and assigning to yched for final call to RTBC.
#11
#7: drupal-test-empty-field-1851880.patch queued for re-testing.
#12
+++ 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]
*/
#13
That's actually: "Contains \Drupal\..." now :)
#14
+++ 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 ?
#15
Unassigning now :-)
#16
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 ...
#17
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.
#18
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).
#19
Think I got all of those ...
#20
#21
The last submitted patch, drupal-test-empty-field-1851880-19.patch, failed testing.
#22
Well that looks like a fun new test fail, especially given that this patch only changes tests ... Will requeue for good luck.
#23
#19: drupal-test-empty-field-1851880-19.patch queued for re-testing.
#24
(Unable to get that specific test to fail locally FWIW)
#25
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...
#26
@yched I think @Alan Evans has done the right thing here.
In your test, put
dsm($items);inviewElements()and we getarray(). That is exactly what this function works for. If we putdsm($items);inprepareView(), we will getarray([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, infield_view_field<?php
// 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
<?phpif (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 inviewElements().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 conditionif (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.
#27
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.
#28
Some message for asserttions? Looks like it's a convention to add explanations message in assert:)
#29
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...
#30
Yeah,that makes sense^^
#31
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!
#32
Automatically closed -- issue fixed for 2 weeks with no activity.