Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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 ;)
Comment | File | Size | Author |
---|---|---|---|
#27 | drupal-test-empty-field-1851880-27.patch | 3.78 KB | yched |
#27 | interdiff.txt | 1.37 KB | yched |
#19 | interdiff.txt | 3.21 KB | Alan Evans |
#19 | drupal-test-empty-field-1851880-19.patch | 3.76 KB | Alan Evans |
#7 | drupal-test-empty-field-1851880.patch | 3.39 KB | Alan Evans |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedlast 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.
Comment #2
yannickooBut the formatter needs a value. You cannot display a gravatar image based on an uploaded image.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedFormatters 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.
Comment #4
tim.plunkettFeel free to revert the title, but I think we just need some test coverage here.
Comment #5
sunThe 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.
Comment #6
Alan Evans CreditAttribution: Alan Evans commentedWorking on a test for this ...
Comment #7
Alan Evans CreditAttribution: Alan Evans commentedWould something along these lines fit what we're trying to test?
Comment #8
swentel CreditAttribution: swentel commentedHmm 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.Comment #9
swentel CreditAttribution: swentel commentedtagging
Comment #10
swentel CreditAttribution: swentel commentedSo, this looks good to me IMO, unless I miss something. Will trigger re-test and assigning to yched for final call to RTBC.
Comment #11
swentel CreditAttribution: swentel commented#7: drupal-test-empty-field-1851880.patch queued for re-testing.
Comment #12
yched CreditAttribution: yched commentedThis looks odd. Shouldn't this be just if (empty($items)) without a foreach ?
Other than that, nitpicks :
"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 :-).
Definition of \Drupal\...
Should now be
Comment #13
amateescu CreditAttribution: amateescu commentedThat's actually: "Contains \Drupal\..." now :)
Comment #14
swentel CreditAttribution: swentel commentedRight, and now that I look at this, shouldn't this actually show text ?
Comment #15
yched CreditAttribution: yched commentedUnassigning now :-)
Comment #16
Alan Evans CreditAttribution: Alan Evans commentedThanks 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 ...
Comment #17
Alan Evans CreditAttribution: Alan Evans commentedI 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.
Comment #18
Alan Evans CreditAttribution: Alan Evans commentedYes, that was it. So, when the field is empty, $items typically contains:
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).
Comment #19
Alan Evans CreditAttribution: Alan Evans commentedThink I got all of those ...
Comment #20
Alan Evans CreditAttribution: Alan Evans commentedComment #22
Alan Evans CreditAttribution: Alan Evans commentedWell that looks like a fun new test fail, especially given that this patch only changes tests ... Will requeue for good luck.
Comment #23
Alan Evans CreditAttribution: Alan Evans commented#19: drupal-test-empty-field-1851880-19.patch queued for re-testing.
Comment #24
Alan Evans CreditAttribution: Alan Evans commented(Unable to get that specific test to fail locally FWIW)
Comment #25
yched CreditAttribution: yched commentedNot 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 :
(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...
Comment #26
smiletrl CreditAttribution: smiletrl commented@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
is for
prepareView()
.is for
viewElements()
.I did a test with
class LinkFormatter
, which confirmed above points. Also, infield_view_field
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
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.
Comment #27
yched CreditAttribution: yched commentedOh 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.
Comment #28
smiletrl CreditAttribution: smiletrl commentedSome message for asserttions? Looks like it's a convention to add explanations message in assert:)
Comment #29
yched CreditAttribution: yched commentedActually, 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...
Comment #30
smiletrl CreditAttribution: smiletrl commentedYeah,that makes sense^^
Comment #31
webchickI 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!
Comment #33
yched CreditAttribution: yched commentedLOL. 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
Comment #33.0
yched CreditAttribution: yched commentedUpdated issue summary.