We currently have too much custom code related to user pictures when really they should be a field on the user. Let's investigate converting this over to an actual image or file field and how we can handle it when displaying a user's picture elsewhere in core.
Comment | File | Size | Author |
---|---|---|---|
#232 | core.user-picture-field-1292470-232.patch | 1.49 KB | ParisLiakos |
#230 | core.user-picture-field-1292470-230.patch | 962 bytes | ParisLiakos |
#225 | user-picture-field-1292470-225.patch | 85.13 KB | Berdir |
#225 | user-picture-field-1292470-225-interdiff.txt | 2.2 KB | Berdir |
#221 | user-picture-field-1292470-221.patch | 84.57 KB | Berdir |
Comments
Comment #1
aaron CreditAttribution: aaron commentedsubscribe. this allows lolcat videos for user pics w00t
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedGreat goal. I see one sticky issue - see CommentController::buildQuery. That has us manually JOINING to user table to get the picture column (which contains a file ID). That column would no longer be present after this patch.
How do we enrich the comments with author pictures and preserve good performance? Do we dare do a user_load_multiple() on all the comment authors? If so, when do we do that?
Comment #3
catchThere's entity_prepare_view(), this exists specifically for doing multiple loads of related entities during rendering more or less - i.e. taxonomy terms on node listings.
If we can remove that join altogether that'd be great. Needs profiling to see how expensive it is. Hopefully we can get render caching of entities in at some point then we'd potentially need to load neither the comments nor the users and just pull a string per comment from cache_get_multiple() on cache hits, that'd be in the region of a 40% improvement on some pages but of course doesn't exist yet.
There are memory issues in search indexing (where it tries to load every single comment into memory), which is definitely going to be made worse if we load users too. That needs its own fix but will be more important to get right if it happens like this.
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commented26 files changed, 153 insertions(+), 586 deletions(-)
Mission accomplished (infamous last words).
Comment #5
Dave ReidSome things that need to be discussed:
1. How this works for anonymous users (which would fall back to the default user picture)? Currently this seems like it would make Gravatar support impossible?
Comment #7
Lars Toomre CreditAttribution: Lars Toomre commented@Moshe - Thanks very much for this start... A couple of thoughts:
1) I would encourage that we use a variable with a default of 'thumbnail' for the default style. There is no need for a UX to set this, but it allows programmers to set an image style without needing to generate another image style in preprocess or process hooks to override.
2) My immediate reaction was that this should be attached to a profile entity. Hence, I do not think we should put off for long the profile entity discussion.
3) For my clients at least, losing the default picture when none uploaded is no small matter. This is a regression from current experience and would makes a column of user comments with user images enabled quite un-uniform.
4) In my opinion, waiting to write the upgrade code makes this a 'needs work' until that important task is completed as well.
Thanks again for starting this.
Comment #8
Lars Toomre CreditAttribution: Lars Toomre commentedcross-posting
Comment #9
sunFirst of all, I love this patch. We should really have done so for D7 already, it perfectly makes sense :)
I reviewed the patch in #4 in detail, trying to focus on non-obvious things.
Upfront: After reviewing the patch, I had the impression that I'm missing something "big" in terms of non-obvious consequences, so I'd really appreciate it if others could spare some time to think about that, too.
1) The previous code didn't even try to render a picture if the theme opted out. In terms of performance, I guess it would be beneficial to retain that conditional logic (but it would definitely be nice to not squeeze it onto a single line, as you've already started).
2) Let's already create a follow-up issue to add a "picture_style" theme variable or so to these theme functions, so the 80% use-case of overriding becomes a no-brainer.
3) Speaking of overrides, it probably makes sense to wrap this entire rendering code into a
!isset($variables['picture'])
condition, so we don't even attempt to re-render the picture if it's already there.Fatal error: Undefined function user_pictures_enabled().
Separate issue: The fatal error isn't mentioned once in the test results. So either system_theme_settings() isn't tested at all, or we don't properly catch fatal errors anymore.
Informational only:
We are removing the "direct" low-level usage of some APIs with this patch, which unintentionally may have served as "regression testing".
In particular,
- usage of the managed_file API outside of a field, and deeply integrated into an entity property, and used in forms, but also in entity views, and also in mock/partial rendering
- custom storage paths for managed_file files
- default files for where no file was saved
- implicitly, usage of the file usage API in a fairly custom context (not limited to but including user registration, which is a very special case on its own)
- direct usage of Image module's image style API for arbitrary image files
- custom validation of file uploads (in terms of dimensions and file size)
This should not hold up this patch, but it's essential for us to know exactly what kind of "hidden" integration functionality we're actually removing. That is, because I believe our "unit" tests in these fields are anything but solid.
Please excuse my harsh words, but:
Perhaps it's only me, but I do have a serious problem with deferring the update path for this patch to a - at this point - "vaporware" drupal-to-drupal major upgrade migration process that many dream of, but for which no code essentially exists in core.
That migration process will also run into many, various critical problems and challenges we weren't able to solve for D7 already (in particular with regard to pluggable field storage engines). The intended new code layer will come with its own problems and additionally will have to solve the existing ones, which is - realistically - unlikely to happen unless someone starts a dedicated major initiative for that (now), so we have sufficient time to implement and test the idea as an end-user-facing operation.
Until that happens, I'm heavily opposed to the idea of ditching and forgetting about regular module updates/upgrades.
In short: IMO we need a module update that converts the entity property data into field data and removes the schema column afterwards. Should actually be fairly simple to do.
Minor:
1) Should be /**.
2) Second sentence should be split into the description.
3) Superfluous blank phpDoc line at the end.
isset() ?
/**
Creative!
I wonder whether this 1) is reliable, and 2) makes sense from A) a site builder perspective and B) an architectural core perspective.
On 1) and B):
False-positives?
But also, false-negatives? (if cache flushing/info rebuilding remains to be as fragile and overloaded as it is today)
Major/critical follow-up issue for token support in image field alt attributes?
I guess we'd also no longer be storing user pictures as /files/pictures/picture-[user:uid].ext, but then again, I never really understood the purpose and benefit of doing that.
Not sure what this refers to.
NACK on removing this test case.
Our high-level tests (like this one) are asserting high-level user-land expectations, and regardless of how we store, manage, or handle user pictures internally, these expectations are still the same.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedThis version adds a new view mode on user entity called 'compact' which just shows the user picture. This is the view mode used on node/comment views. The advantage is that we now have a UI for changing image style and also a UI where admins can add additional fields to be shown (first/last name, hometown, memberSince, etc.).
The Doxygen above user_picture_enabled() explains how gravatar and such should provide alternate user pictures.
Addresses a lot of Sun's review:
1. Done
2. No longer needed. See above.
3. Not needed since template_preprocess_[node|comment] run early in preprocess chain. modules and themes run later so they have no chance to preempt us. Their later position is good since they can tweak as needed. Note that field_view_field() does not render() anything. It prepares the render() array which is relatively cheaper.
4. Fatal error fixed. Simpletest used to report these problems for sure. I saw a verbose() with a this failure so I know that unit tests execute this code.
5. Minor items fixed
6. I think that alt attribute had pretty obvious text. I'd go for a normal feature request here.
7. Yeah, looks like we have to write a hook_update_n() for this. I hope to get to it.
8. I agree that we should keep some testing here. I plan to add back just a check for the user picture when viewing node/comment. The add/edit/delete/render is all tested by Image field.
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedchx points out that I need
user_view($comment->account, 'compact');
instead of field_view_field().Also, I misspoke earlier. Image Field already has a 'default image' feature which is working well for user picture. We are not losing that feature. We do need someone to contribute a suitable image. Usually this is a grey headshot with a blank face. Would be nice to do that with a subtle pointed head ala druplicon.
There is now doxygen above user_picture_enabled() which briefly describes how alternative user picture modules can provide images to user/comment/profile renderings.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedLets get more feedback from humans and test bot ... As stated, upgrade path is still outstanding.
Comment #14
chx CreditAttribution: chx commentedThis is the original druplicon.svg just cut down brutally to be nothing more than an outline (edit: ie. I have hand edited the svg file and removed the other paths and the colors so the outline is guaranteed to be byte-by-byte identical). We could save it as a png and include as default. convert av1.svg -transparent white -resize x85 av1.png saves it as a transparent background 74x85 png for example.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedtry to satisfy bot patch apply. no changes.
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedRe-upload per bizarre PIFT msg.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedFix test fail, I hope.
Comment #20
moshe weitzman CreditAttribution: moshe weitzman commentedThe only remaining test failure is due to a too long table being created by simpletest. It has a very long prefix for some reason. It looks like it is double prefixed. The error starts with
PDOException: SQLSTATE[42000]: Syntax error or access violation: 1103 Incorrect table name 'simpletest959239simpletest736321field_revision_field_user_picture': CREATE TABLE {field_revision_field_user_picture}
...Comment #21
moshe weitzman CreditAttribution: moshe weitzman commentedAdded back a test that assures that the preprocess functions are fetching the user rendering and populating the picture variable. The accuracy of the rendering is tested elsewhere by view modes and image fields.
Still needs migration.
Comment #22
moshe weitzman CreditAttribution: moshe weitzman commentedAdds update function to migrate to Field API. Now ready for review.
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedFixed a unit test fail.
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedTest bot is ignoring this latest patch? Any ideas?
Comment #27
andypostThis hunk should be in batch for sites with tremendous user amount, also this update misses a part of variables migration.
Suppose there should be 2 update functions:
8000) Add user_picture field if it was enebled (possible this field should be added by default)
8001) Migrate images in batch
This settings should be migrated to image field settings
Suppose this name should be user_picture so lives in user's module namespace
Defaults should be set from old variables
26 days to next Drupal core point release.
Comment #28
moshe weitzman CreditAttribution: moshe weitzman commentedAttached patch keeps up with HEAD and breaks up into two update functions as suggested. Still needs batch update. I'm not so inclined to migrate those variables since Field API already has good defaults for this stuff. It just doesn't seem important. Would be happy if someone else took that part. CNR for test bot.
Comment #30
moshe weitzman CreditAttribution: moshe weitzman commentedperhaps someone else can finish this
Comment #31
sunI'll try to have a stab at this in the next days, unless someone beats me to it.
Comment #32
sunFirst of all, a non-trivial re-roll against latest HEAD.
Comment #34
sunSome more clean-ups and fixes. But no effin' idea what's going on in that SimpleTest test case.
Comment #36
sunResolves the ImageAdminStylesUnitTest test failures.
The remaining test failures (except UserPictureTestCase) are still highly mysterious to me. Although tests seem to be able to install and even upgrade Drupal, there must still be some very ugly bootstrap/subsystem dependency conflict/problem with this patch.
Comment #38
sunRe-rolled against latest HEAD.
Comment #39
Deciphered CreditAttribution: Deciphered commentedMarking as 'Need review' to get patch tested.
Comment #41
Dave ReidComment #42
Dries CreditAttribution: Dries commentedTagging. I like.
Comment #43
catchI'm just about to commit #1361228: Make the user entity a classed object which had to move all this nastiness into the user controller. Would be good to revive this.
Comment #44
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRerolled. Uploading to see how it performs.
Edit: Careful when basing work on this: I accidantely reverted some of the user entity conversion.
Comment #45
BerdirIf it's only used by profile.module, doesn't that mean that it's not used anymore at all? :)
Hah, that just got added less than 24h hours ago ;)
$edit is dead, this can now simply add the value to $account and save it.
This also means that we require the .module files I guess, probably no easy way around that.
Oh, you re-added user_save() as well ;)
Comment #46
Niklas Fiekas CreditAttribution: Niklas Fiekas commented.profile
rules used anywhere, so dumped them.- 'picture' => array('picture'),
Yeah. Now it's gone ;)
Comment #48
sunWrong rebase/merge? Doesn't seem to be related to this patch, unless I'm missing something.
Resolved some test failures.
Comment #50
sunUpdated UserPictureTestCase accordingly. Still contains one commented out test, which verified that previous user pictures supported external URLs as default images. (?)
Comment #51
sunFinal.
Comment #52
sunFixed in attached patch.
To be resolved in a separate issue.
That's a test for image fields, which may or may not exist already. Irrelevant for this issue.
Removed this test.
Comment #53
Dave ReidShould use db_query_range?
Could be simplified using field_info_instance()?
-25 days to next Drupal core point release.
Comment #54
sunPrevent a division by zero in the update function.
Comment #55
sunIncorporated @Dave Reid's suggestions. Thanks!
Comment #57
sunwhoopsie - endless loop in that update function ;)
Comment #58
moshe weitzman CreditAttribution: moshe weitzman commentedI looked at this one more time and I'm quite pleased with where it is now. imo this is rtbc.
Comment #59
Dave ReidAm I alone in feeling like this touches a lot of stuff and that it could use a bit more actual click-through testing and review rather than just patch-level?
Comment #60
xjmAgreed; we'll make this a priority for office hours in the upcoming week if others don't manually test it first.
Comment #61
sunI actually think that it makes more sense to commit this monster now and follow up with separate issues.
I tested the functionality manually in Stark and it works.
However, needless to say that we don't really have a clean concept for adjusting the user interface for these kind of "built-in default fields attached to entities, but which can be configured like any other field, so they may or may not exist and look and behave as expected." -- I actually think there's a lot of follow-up work to do; not only on the field widget in the user account form, but also regarding the presentation within the user account view page as well as when embedded into other entities. But in the end, I don't see why that should hold up this initial conversion patch, which primarily deals with the required storage and code and API-level adjustments.
Comment #62
BerdirHm, not sure how far this should go, but I think we should attempt to have a more less nice looking UI in our core themes otherwise we end up with critical/major follow-up tasks which we should avoid :)
Comment #63
kbasarab CreditAttribution: kbasarab commentedTesting of user profile photo to fields:
Installed patch from #57 ran update.php
Went to user profile edit and selected photo to upload, Shows new image
Checked that photos show on new article node and comment:
Disabled User pictures in posts and user pictures in comments in admin/appearance/settings/bartik photos immediately disappeared as expected
Tried showing node via in Seven theme and all works there as well.
Disabled user pictures in posts and user pictures in comments on Seven theme and photos went away as expected.
Added new image style and set it to preview style on manage fields:
admin/config/people/accounts/fields
Changed display to new image style and all worked as well:
admin/config/people/accounts/display
Screenshots attached.
Need to circle back and read through more of the patch to test more advanced situations.
Comment #64
andypostAdded migration of variables, not sure about using defualt 'medium' imagestyle
Also we need migrate tests
Comment #66
sunRe-rolled against HEAD. And:
For whatever reason, the $account->save() in the update did not work at all. After debugging for one hour I gave up.
But since there was a @todo about not using API functions (that are calling hooks) in the module update either way, rewrote it entirely to execute direct database queries.
Doing so made me also realize that this entire functionality requires Image module now (which is optional). Previously, user pictures worked without Image module, and it was only leveraged to format user pictures with a certain image style.
Now, requiring Image module for user pictures is totally OK. However, the upgrade path gets a bit bumpy due to that. In particular with regard to the question: "How do we upgrade if Image module is not enabled?"
Image module requires File module...
For now, my answer to that is: {users}.picture is deleted. Nothing is migrated. If you want to migrate user pictures, enable Image module before you upgrade.
I perfectly realize that this is suboptimal and debatable. :)
Additionally, I fixed the test failures from #64 and cleaned up the update code that was added since then.
Unfortunately, HEAD changed too much since #64, so interdiff yielded too many bogus results.
Comment #68
sunFixed those remaining exceptions.
Comment #69
kbasarab CreditAttribution: kbasarab commentedSynced Git to head and ran core/update.php. Ran 2 DB updates and both succeeded.
Ran the same basic tests I did in #63 and all still worked.
Looks like Patch 68 is more for migration though so I'll try and work on testing that some this week as well.
Comment #70
sun#68: drupal8.user-picture-field.68.patch queued for re-testing.
Comment #72
August1914 CreditAttribution: August1914 commentedRe-rolled, no functional test, just to get it to apply.
Comment #73
sun#72 is a clean re-roll. Some form properties have been removed in the account settings form, which don't not to be accounted for elsewhere.
Comment #74
tim.plunkettLets see what the bot thinks!
Comment #75
kendramyers CreditAttribution: kendramyers commentedassigned to myself to test manually
Comment #76
sun#72: drupal8.user-picture-field.69.patch queued for re-testing.
Comment #78
tim.plunkettRerolled, and setting back to RTBC as sun asked.
Comment #80
tim.plunkettI guess #1401558: Remove the usage handling logic from file_delete() broke this somehow. Didn't dig in.
Comment #81
BerdirYes, you probably simply need to add a block like in that issue over there that sets the timestamp back and call cron to have the file deleted. Or check the status instead of it being deleted.
Comment #82
webchickSince this removes user-picture.tpl.php and associated preprocess functions in favour of general field API stuff, tagging as theme system cleanup.
Comment #83
tim.plunkettOkay, borrowed the same process from #1401558: Remove the usage handling logic from file_delete().
Comment #85
marcingy CreditAttribution: marcingy commented#83: drupal-1292470-83.patch queued for re-testing.
Comment #86
marcingy CreditAttribution: marcingy commentedLocally no fails
Comment #87
moshe weitzman CreditAttribution: moshe weitzman commentedThanks Tim. Back to RTBC.
Comment #88
Eric_A CreditAttribution: Eric_A commentedThere was a code style fix in a commit from a week ago: http://drupalcode.org/project/drupal.git/commitdiff/467a825 (#1358944: Misused @ingroup commands)
Comment #89
Dave ReidSo, as far as I can tell, if you have user pictures enabled, but you don't have image module enabled, things just go boom and everyone loses their user pictures? Has anyone manually tested this patch with different configurations of user pictures being enabled/not, image module enabled/not?
Comment #90
Dave ReidComment #91
Dave ReidAlso wondering if the field should be created with the machine name 'user_picture' and not 'field_user_picture' so that it wouldn't conflict with an existing field that has been created. What happens when you have a field already named 'field_user_picture' and you run the updates?
Comment #92
yched CreditAttribution: yched commented'user_picture' ++, 'field_user_picture' --
the 'field_' prefix is only for UI-added fields. Not needed nor relevant here.
Module-added fields should be prefixed by the module name, that's a common practice across any machine names to avoid clashes (and, IIRC, is even explicitly mentioned in the docs in the case of field names). 'body' was deemed an acceptable exception to keep $node->body, that was in everyone's fingers pre-D7.
So user_picture it should be. There still is the possibility of a clash with an existing 'user_picture' field defined by a 'user_picture' module - but, hey, http://drupal.org/project/user_picture belongs to @Dave Reid ! :-)
Comment #93
catchApart from Dave Reid's comment that there's no attempt to enable image module here, you can't use module_exists() in update function, image module might be installed but disabled during the upgrade for example. Help is needed on #1134088: Properly document update-api functions.
Same problem.
Also there's no new data here in the upgrade test database dumpes, but it feels unlikely that the existing database dump already has user pictures in it.
Comment #94
Dave ReidNote that this patch will conflict hard with #1361226: Make the file entity a classed object which is critical so it would be good if that issue could land before this one.
In summary:
- Change the machine name of the user picture field from 'field_user_picture' to 'user_picture'
- Upgrade needs to work even if image module is disabled. We cannot have loss of functionality on upgrade with user pictures.
- Ensure that the upgrade database dump actually has user pictures in it.
Comment #95
tim.plunkettI was going to wait until the file entity patch was in, but because the comment and user tests went PSR-0, it had to be rerolled anyway, and this should be an easier reroll the next time.
I fixed the "addtogroup updates-7.x-to-8.x" thing, and swapped out field_user_picture for user_picture, but I didn't get to updating the dump, or addressing the "we need to enable a module but we can't and tim didn't read the other issue yet" part.
Marking CNR just to make sure I didn't mess anything up, but I'll postpone it afterward until that other issue is in.
The interdiff is against the straight reroll I did of #83, it only shows changes I made since then, and not the moving around due to PSR-0 test changes.
Comment #96
tim.plunkettOkay, rerolled after #1361226: Make the file entity a classed object.
Still to be done:
Since I have no idea how to do the first, I'm unassigning from myself.
Comment #97
tim.plunkettWhoops.
Comment #98
Manuel Garcia CreditAttribution: Manuel Garcia commentedpatch applies fine, but going to update.php gives you:
I went ahead and renamed the update functions to 8003 and 8004, ran the update script and everything looks fine. I attach the updated patch so that others can test.
Two notes:
Comment #99
cosmicdreams CreditAttribution: cosmicdreams commentedLooks pretty solid to me. RTBC
Comment #100
tim.plunkettStill CNW for the two points in #96
Comment #101
tim.plunkettThis patch assumes that #1619898: Convert upgrade tests to PSR-0 is committed.
I've uploaded an interdiff, the test only combined with the PSR-0 patch, the full patch combined with the PSR-0 patch, and the actual patch that should be used.
Comment #102
tim.plunkettOkay, that went in. Uploading both with and with upgrade functions again.
Comment #103
tim.plunkettOh, forgot to mention. This makes no attempt to address
"Upgrade needs to work even if image module is disabled."
And I'm not sure how to deal with that.
Comment #104
BerdirWhat about simply forcefully enable image.module? To upgrade the data we need it. In case you don't want pictures, you can always remove the field and disable the module?
Comment #105
tim.plunkettMoshe suggested update_check_requirements, I'm going to add something along those lines now.
Comment #106
Dave ReidHow does that work for people upgrading core? If people are upgrading core and have dropped in d7 but it can't upgrade, how do they enable a module when the site itself is in d6/7 limbo?
Comment #107
tim.plunkettOkay, here's a stab at that.
There is a lot of discussion in IRC about just calling module_enable, but catch said we couldn't in #93. Not sure what to do.
Comment #108
tim.plunkettcatch pointed out update_module_enable(), which still has some potential issues (#1239370: Module updates of new modules in core are not executed), but it exists for this purpose.
Comment #110
aspilicious CreditAttribution: aspilicious commentedArgument 1 passed to update_module_enable() must be an array, string given, called in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/user/user.install on line 444 and defined
Should be an easy fix ;)
Comment #111
tim.plunkettDuh. Thanks aspilicious.
Comment #113
tim.plunkettSo the awesome new update_module_enable doesn't account for dependencies. So added checks for file as well.
Comment #114
tim.plunkettYay!
+215 lines of upgrades and upgrade tests.
+299, -692 lines of actual changes.
And now it just needs some real manual testing.
Comment #115
aspilicious CreditAttribution: aspilicious commentedSo because we type hint our functions out upgrade function fails... SO every function we use like that in an upgrade path we can't type hint...
*sigh*
Comment #116
aspilicious CreditAttribution: aspilicious commentedaspilicious: well, yes. you can't rely on api functions in the upgrade path, that's not specific to file entities
berdir, ok good to now :)
aspilicious: you either need to write direct sql queries or create a upgrade-safe helper function
aspilicious: see for example the versioned field functions that are used in the 7.x upgrade path
Have fun ;)
Comment #117
BerdirNote that the names in #116 are reversed.
Comment #118
xjmOho, timely. We're working on the following documentation patch in office hours: #1134088: Properly document update-api functions
Comment #119
sun#1496534: Convert account settings to configuration system will be massively conflicting with this patch. I'd really like to see this patch here land, so the config conversion can just simply remove these settings instead.
Re-rolled against HEAD. (and moving into a platform branch, since this is a huge thing to re-roll)
Comment #120
aspilicious CreditAttribution: aspilicious commentedSo for this to go in, we should rip out all the api functions in the update and switch them to custom update functions.
Comment #121
sunI do not see why that needs to be done here. #1348162: Add update_variable_*() (or a follow-up issue to that) will be able to simply replace these calls with whatever new functions there might be. There's no reason to block this patch on that.
Comment #122
sunSince an "interdiff impossible; taking evasive action", I manually compared the current patch against the original #72, and reviewed it once more in-depth.
Why do we remove the file usage references for user pictures? Shouldn't we update them instead?
I guess the files are still marked as permanent, so system_cron() won't delete them -- but still, this looks bogus to me?
Additionally, file_usage_delete() can no longer be used; it requires a classed File object in the meantime.
Lastly - this is interesting - User module never declared/registered a file usage for the default user picture. ;) So the file usage for the default image does not seem to require an update, since the created image field (hopefully) cares for that already (not verified).
In attached patch:
896c93a Removed duplicate enabling of Image/File modules in user picture updates.
f037620 Fixed file usage for user pictures is deleted instead of updated.
Comment #123
sunGood. Given that we even have upgrade tests now, I think this is RTBC. However, I authored major parts of this patch, so I'm not really eligible to set that status.
Comment #124
moshe weitzman CreditAttribution: moshe weitzman commentedThis is really nice work. Let's commit this please. Resist the nitpicking!
Comment #125
aspilicious CreditAttribution: aspilicious commentedI hate to do this, the upgrade path doesn't work (properly).
ps: image/file module were disabled for this experiment
1) The default image is not converted, I even can't find it inside the directory structure anymore very strange. Maybe I did something wrong but probably not.
2) A custom image is converted but contains this url: "sites/default/files/styles/medium/public/pictures/picture-1-1343657337.jpg"
The image is located in "sites\default\files\pictures\picture-1-1343657337.jpg"
The files/styles direcotry is empty so I guess the styles need to be recreated after upgrade
3) the url says "medium" and if I'm not mistaken the default in drupal7 was thumbnail.
Comment #126
tim.plunkettI want to get this done.
Comment #127
tim.plunkettFirst, here's a reroll. #1499596: Introduce a basic entity form controller did a number on this patch, hopefully this still passes tests. Now onto manual testing
Comment #128
tim.plunkettOkay, so @aspilicious was right, the upgrade path doesn't work.
default_image needs file ID, not a string. This doesn't fix it all the way, but exposed the failure.
We'll need to add a file to actually migrate, the one in drupal-7.user_picture.database.php isn't going to work.
Comment #130
tim.plunkettI really wanted to see this one through, but I can't deal with upgrade path issues this month.
Comment #131
moshe weitzman CreditAttribution: moshe weitzman commentedAnyone available to finish up the upgrade path? Please please.
Comment #132
larowlantagging
Comment #133
pcambraJust rerolling to see how it looks to the bot now
Comment #134
pcambraMissed new files.
Comment #135
Dave ReidRestoring tags.
Comment #136
larowlangroan, sorry about the tags
Comment #138
pcambraHere's a new reroll:
Setting to CNR to see what Testbot things, it will fail somewhere as #128, but less than #134. I'll try to get Tim on irc to clarify some steps forward.
Comment #140
pcambraJust opened #1806970: Add schema version to image module for minimal databases for fixing the extra upgrade tests errors on minimal databases
Comment #141
pcambraAnother reroll and a first try of using a local image
Comment #143
Eric_A CreditAttribution: Eric_A commentedUsing file_save_data() within a hook_update_N() implementation must be a no go, no...?
Comment #144
pcambraNot sure, normally you already have the folder locally so maybe the creation won't be necessary?
Anyways, here's another try.
Comment #146
dagmarRe-rolled. Also, probaly the tests were failing because the progress was never incremented.
Comment #148
moshe weitzman CreditAttribution: moshe weitzman commentedOnly 2 fails. Go pcambra and dagmar!
Comment #149
BerdirOk, the reason for those two fails is this:
If image.module is not enabled, it's enabled using update_module_enable(). That function explicitly marks the schema version as 0 so that any updates will run. They don't run in the same batch, so that's why we are left with remaining update functions.
I'm actually not quite sure why it works like that. Because image/file aren't new modules, I'm not sure if we shouldn't just use module_enable(). That however, exposed another upgrade bug, it explodes because file_managed already exists. I think that's a real bug because it could also happen if you try to manually enable file.module after you upgraded from 7.x. Not sure how to deal with this.
Comment #150
chx CreditAttribution: chx commentedSchema version needs to be set separately if it was installed but disabled in d7 (you catch it during the {system} table copy) and if it was not even installed in d7 (it was not present during {system} table copy).
Comment #151
BerdirOk, so the way I understand update_module_enable() is that it is explicitly meant for enabling new modules that contain tables which were previously in another module, e.g. system.
That is however not the case for image/file, so using that function given the way it currently works seems wrong.
However, as mentioned before, the problem that we have here is that a table was moved from system.module to file.module, so even manually enabling the file module after upgrading from 7.x to 8.x will fail. badly.
Any suggestions on how to deal with that? I guess we actually need to install file.module in a system_update_X() and this update function will then just have to depend on that one. But I'm not sure how we need to properly handle those update functions from image.module (and possibly those from file.module if we add some later), or if those modules would actually *do* stuff in hook_install() or add additional tables. Installation an old version of the module and then run them? Probably, but it breaks our current UpgradePathTestBase class, so we need to improve that. The same will actually happen as soon as any of our current "enabled-by-update_module_enable()"-module adds an update function.
So a way to do that and run them all at once would be to add additional batch operations while we're running them. Not sure if that is possible, maybe #1797526: Make batch a wrapper around the queue system would allow it?
Oh so many problems ;)
Comment #152
catchIt was added as a partial fix for #1239370: Module updates of new modules in core are not executed iirc - I think that issue at least partly covers the problems you're discussing here.
In terms of having updates left over that need to run, I'm not massively concerned about that, between Drupal 6/7 we had the 'abort' flag added which would mean re-running update.php after an otherwise successful upgrade - at least that issue should not block this one (we've had that since field module was added to 7.x and it's still not fixed).
Comment #153
catchdouble post.
Comment #154
BerdirOk, I think we can work around these problems with this approach.
- Added an update function that enables file.module with update_module_enable() to avoid the error. file.module currently has no update functions or hook_enable()/install() stuff, so that's fine for the moment.
- Changed the update_module_enable() for image.module to a normal image.module because we want to enable it as a normal, already existing module in the current version, without running it's update functions.
This means the the actual problem here ( conflict between UpgradePathTestBase and update_module_enable() on a module with update functions) is going to bite someone else. We should probably open a follow-up issue for that or deal with it in #1239370: Module updates of new modules in core are not executed.
The upgrade path tests should pass now, confused by those exceptions, I'll look into them if they still happen.
Comment #156
BerdirOk, this should fix the exceptions. Need to ask someone from the fields team if this is right. Could also just add the weight to the field display definition for the user picture.
Comment #158
BerdirInteresting timing ;)
Removed the views integration for user.picture.
At least now we can say that the patch removes more code than is being added, despite upgrade path and upgrade path tests :)
Comment #159
BerdirHm, didn't reroll the branch, let's try that again.
Comment #160
swentel CreditAttribution: swentel commentedBerdir asked me to quickly check whether the exception fix for the 'weight' key is ok (see #156).
_field_write_instance
in field.crud.inc also has a check to see if the weight property is set or not, so this kind of makes it consistent. In the end, it doesn't really matter I guess as long as it works fine, unless yched has really big problems with this, this is ready to fly for me (well at least that part, not going to push the RTBC button for the patch as whole as I haven't skimmed that).Comment #161
yched CreditAttribution: yched commentedThat _update_7000_field_create_instance() hunk is fine by me as well.
Comment #162
Lars Toomre CreditAttribution: Lars Toomre commentedThere are some documentation issues in this patch that need to be addressed. Is now the point that one should roll such a enhanced patch as a part of our d8 process?
Comment #163
moshe weitzman CreditAttribution: moshe weitzman commentedI reviewed the code and it is is looking awesome. Has upgrade path and even upgrade tests. If someone provides a patch for Lars' docs issues, that would be great. Until that patch arrives, this patch is RTBC.
Comment #164
ParisLiakos CreditAttribution: ParisLiakos commentedawesomeness! good job Berdir:D
Comment #165
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @moshe weitzman for the RTBC. I do not want to hold up the patch process commit process. I also want to make sure that we meet appropriate standards. Hence, I asked the question.
I am often in doubt about when to ask if a patch needs a nit picky review from a documentation perspective.
Comment #166
Berdir@moshe: Yay!
@Lars Toomre: Feel free to do a documentation review, just make sure to clarify that a post-commit cleanup is fine as well for that. Then it doesn't hold it up from being commited and if someone finds time to re-roll the patch and fix those issues before commit, even better.
Comment #167
Lars Toomre CreditAttribution: Lars Toomre commentedAs an aside, please appreciate that my git skills are at a rather 'low level'.
Given that aside, when attempting to apply this patch locally, I received a rejected hunk in the user.module file.
I am unclear what further steps I should take.
Comment #168
Berdir#159: user-picture-field-1292470-159.patch queued for re-testing.
Comment #170
ParisLiakos CreditAttribution: ParisLiakos commentedi think thats why http://drupalcode.org/project/drupal.git/commit/4365f50
Comment #171
BerdirRe-rolled. Let's see if that was all or some other code needs to be updated.
Comment #172
aspilicious CreditAttribution: aspilicious commentedOnce it is rerolled I want to manually test this. I'll make some time for it as I'm the one who put it backs to needs work weeks ago. I just want to be sure we covered everything in our upgrade tests.
Someone else can do it too.
Best testing scenario.
1) Setup a D7 site
2) Add a default user picture
3) Create a user with the default picture
4) Create a user wit a custom picture
5) Disable image and file module
------------------------------------------
6) Replace the D7 code with D8 code
------------------------------------------
7) Run update.php
8) Verify everything is ok
Comment #173
xjm@Lars Toomre, @Berdir: http://xjm.drupalgardens.com/review-guide#oct-2012-update
As far as I can figure you won't go wrong using that strategy. A docs cleanup reroll with an interdiff is acceptable even when an issue is RTBC.
Comment #174
xjmOpps, xpost.
Comment #175
BerdirI actually think that was a nested crosspost and it should be needs review to be manually tested.
Comment #176
aspilicious CreditAttribution: aspilicious commentedI failed to do the tests. When trying to upgrade I get a fatal with a very informative message in my apache error logs...
[Tue Oct 23 20:31:40 2012] [error] [client 127.0.0.1] PHP Fatal error: Exception thrown without a stack frame in Unknown on line 0
Someone else should try to upgrade... I probably did something totally wrong...
Comment #177
BerdirCan confirm. I did get a bit further, but our upgrade path is completely hosed at this point.
There's a whole bunch of problems, starting with the fact that the drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE) call right at the beginning of update_prepare_d8_bootstrap() calls config(), then it breaks because the config directories are missing. To create these directories, we need to load module.inc and cache.inc and pseudo-enable system.module so move all that code up there. That allows to run through that but then it dies with a fatal error during running update functions. I gave up at that point :)
All that has nothing to do with this issue however. So my suggestion would be to get this in and repeat the manual test later on.
Comment #178
moshe weitzman CreditAttribution: moshe weitzman commentedAgree with fixing generic upgrade issues in separate issue as per #177.
Comment #179
catchIs there an issue for the hosed upgrade path? Shame upgrade path tests aren't catching the failures.
Comment #180
BTMash CreditAttribution: BTMash commentedI've created an issue for the upgrade path being hosed at #1821312: Manual upgrade path from 7.x to 8.x loses fixed module_list() inexplicably. and set it to 'major' for now. Feel free to change it as needed.
Comment #181
webchickThis patch is a lovely bunch of loveliness!
I tried it out manually too, and all works as expected. The only thing I found confusing is that when I ran the upgrade path from 8.x to 8.x + this patch, the display settings got messed up:
This doesn't happen in a fresh install, so is probably some missed setting in the upgrade path. Can we fix that real quick before commit?
Comment #182
sunCan we please handle that in a critical follow-up instead:
#1821756: "Member for" appears for each comment below user picture after updating to latest HEAD
Thanks.
Comment #183
sun#171: user-picture-field-1292470-171.patch queued for re-testing.
Comment #184
xjmWhy does a normal task get to spawn a critical bug? -1 on not fixing it before commit.
Comment #185
webchickYeah, I don't get that either. Let's just fix it here, where everyone who worked on the upgrade path for this patch already is.
Comment #186
BerdirTurns out that set-back was quite useful, did some 8.x -> 8.x upgrade tests myself and noticed a whole bunch of issues which should now be fixed:
- Force member_for visibility to FALSE for compact view mode. Currently quite hardcoded, I'm not sure if we should consider a loop there, also considering there might be fields and so on on th user.
- If there are more than 20 users, the upgrade path failed because it tried to re-add the field data row for the same users. added a db_update() to set the picture of the migrated users to 0.
- Also filling the field_revision table. While users are (currently) not revisionable, I think this makes sense for consistency and is what field_sql_storage would do.
- Used updated_variable_*() functions.
Comment #187
Berdir#186: user-picture-field-1292470-186.patch queued for re-testing.
Comment #189
BerdirRe-rolled, changes:
- Updated update function numbers.
- Moved prepare hook implementations to the corresponding render controllers.
- Updated for the entity as plugin changes.
#1778178: Convert comments to the new Entity Field API currently also messes with how pictures and other account information is handled on comments, either issue will need to be re-rolled and adopt the changes of the other one. Not sure which commit order is better. This one might be harder to re-roll but the other one is more important to get in right now.
Comment #191
BerdirFixed the annotion, no other changes.
Comment #192
aspilicious CreditAttribution: aspilicious commentedAssigning to me so I can test the upgrade path again :)
Comment #194
aspilicious CreditAttribution: aspilicious commentedStill not able to upgrade...
Getting closer, ;)
An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: http://localhost/drupal7/core/update.php?op=selection&token=a-Ak5kPHeezEQEtUX1lJsvBLtIdyiW-4UBrzrpLg2do&id=2&op=do_nojs&op=do StatusText: OK ResponseText: Fatal error: Class name must be a valid object or a string in C:\xampp\htdocs\drupal7\core\includes\entity.inc on line 265
<strong>The update process was aborted prematurely while running update #8010 in user.module</strong>. All errors have been logged. You may need to check the watchdog database table manually.
Watchdog contains *nothing*
And when going to the frontpage
Fatal error: Call to a member function fetchCol() on a non-object in C:\xampp\htdocs\drupal7\core\modules\node\node.module on line 2382
Comment #195
Berdir#191: user-picture-field-1292470-191.patch queued for re-testing.
Comment #197
BerdirThese test failures don't really look related?
Triggering a re-test.
Comment #198
Berdir#191: user-picture-field-1292470-191.patch queued for re-testing.
Comment #200
fagoWith the new entity field API I think the user_attach_accounts() helper logic should be directly moved into the field logic, so that $entity->uid->entity gets you the proper user entity, even if anonymous. We could already handle that by providing a custom class for the entity property that adds in this logic - or we wait for a proper fix of having different $user entities or anonymous users as well...
I did not have a close look at the patch, but I fear the approach used by user_attach_accounts() won't work for comments right now, as they also store additional information like the homepage for anonymous users. That should be probably moved to the user entity or reworked otherwise, but that's probably it's own issue... Meanwhile, a comment specific helper should do it. So for now I think it would be best to with the comment specific helper introduced by #1778178: Convert comments to the new Entity Field API for comments, and user the general one else.
Comment #201
moshe weitzman CreditAttribution: moshe weitzman commentedI don't really follow the last comment by @fago. What exactly needs to be done here (versus what is noce to have)?
Comment #202
ParisLiakos CreditAttribution: ParisLiakos commentedBut the revision table name is
field_revision_user_picture
?Also i don't see the reason you assign those names to a variable and not directly use them in db_insert.
Besides the above and that system_update_N function has to be incremented one more value, i tried the upgrade manually and it worked:D good job!
Comment #203
BerdirThanks, sounds like the table name is wrong, not sure why it didn't fail.
@moshe_work: I hope that the CommentNG issue will get in soon, this will need to be re-rolled after that, as they solve the missing picture problem in a different way.
Comment #204
dawehnerSee #1836204: Test failure in Drupal\system\Tests\Menu\BreadcrumbTest for the random test failure.
Comment #205
BerdirHere's a re-roll to get this re-started.
This is exactly what happened in the meantime. file.module has update functions now, so we're running into th update not complete problem.
Comment #207
sunAt this point, I'd strongly recommend to:
update_module_enable()
for File module in this upgrade path.Create a major/critical follow-up task to resolve the essential problem of update.php, which is not able to account for module updates of modules that have been enabled during the course of a single update.php execution.
That bug is a completely independent bug on its own, and I fail to see a reason for why this badly needed conversion issue should be held off any further on fundamental flaws elsewhere in the system, which cannot reasonably be addressed nor fixed as part of this issue.
I already tried to look into that problem and relatively quickly concluded that a fix for it would require a substantial refactoring of update.php's current use of Batch API — the latter has a notion of "progressive", but the former does not leverage it; and while performing the necessary plumbing sounds relatively easy, the moment you start to realize that there is a
hook_update_dependencies()
, you'll quickly end up in some true, mind-boggling insanity.Let's face it: In the end, this patch is well done, meets all requirements, passes all gates, and is perfectly fine. It cannot do anything about a utterly broken upgrade path system. Consequently, there's absolutely no reason to hold up this innocent patch any longer on that.
Looks like we're missing phpDoc comment "*" prefixes here? I'm surprised the annotation parser didn't blow up on that (it didn't).
Comment #208
Berdir@sun We need file.module enabled for the upgrade path to work. module_enable() fails with table exists, update_module_enable() now fails with the remaining updates problem. The only way would be to leave it out and don't add upgrade tests, *knowing* that it's broken. I don't think that will be commited.
Grml, I already fixed the missing * in the previous patch, but that one was missing something else...
Comment #209
sunYes, that's essentially what I'm saying: Accept that it is currently impossible to provide a fully working upgrade path for this change.
AFAICS, the only possible and rather dirty alternative that exists is to force-enable File module in
update_fix_d8_requirements()
— that has the consequence that File module is already enabled when module updates are collected and before update dependencies are resolved, ultimately resolving the problem of remaining updates after running update.php.FWIW, I'd be fine with this either way.
Comment #210
moshe weitzman CreditAttribution: moshe weitzman commented@Berdir - perhaps you could pick one of the solutions in #20 and then reroll? I'm fine with deferring upgrade path here. This issue really shows how screwed we are with traditional upgrade path.
Comment #211
catchI would actually commit this with a critical follow-up for fixing the upgrade path for newly enabled modules, on the basis we already have a critical bug for that not working (albeit not the specific problem here) that blocks the release.
This issue isn't introducing any new bugs at all - presuming the upgrade path itself has been manually tested with more than a handful of user accounts.
Comment #212
aspilicious CreditAttribution: aspilicious commentedUpgrade path is still totally broken. I got two errors while updating.
First one:
An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: http://localhost/drupal7/core/update.php?op=selection&token=Z4-nmydRrfk95a07ghOvBnEGkHC6SfoRSDddUFnTCZI&id=3&op=do_nojs&op=do StatusText: OK ResponseText: ( ! ) Fatal error: Class name must be a valid object or a string in C:\xampp\htdocs\drupal7\core\includes\entity.inc on line 280 Call Stack #TimeMemoryFunctionLocation 10.0011222160{main}( )..\update.php:0 21.894011491768_batch_page( )..\update.php:521 31.894611531160_batch_do( )..\batch.inc:84 41.894611531176_batch_process( )..\batch.inc:108 52.868212061088call_user_func_array ( )..\batch.inc:245 62.868212061112update_do_one( )..\batch.inc:245 72.868212061336user_update_8011( )..\update.inc:601 82.869312061960file_save_data( )..\user.install:649 92.902112063792entity_create( )..\file.module:528 102.902112063816entity_get_controller( )..\entity.inc:267
Than I ran update.php again
Comment #213
webchickIs that because of this patch, or is it a general condition?
Comment #214
aspilicious CreditAttribution: aspilicious commentedFirst one sounds general, second is caused by the patch
Comment #215
BerdirTake this upgrade path!
Tested this patch with a manual 7.x -> 8.x upgrade with all modules disabled, a default image and 5000 generated users with a profile image, worked fine. That is, I noticed a number of bugs but not related to this.
Changes:
- Moved file.module enable to update_fix_d8_requirements()
- Changed file_save_data() to file_unmanaged_save_data() and doing a manual db_merge() query on file_managed. Fun. Added a number of test assertions for this.
- Fixed the wrong revision table name. Added an actual user picture including file usage to catch this in the upgrade tests.
Comment #217
Berdir#215: user-picture-field-1292470-213.patch queued for re-testing.
EDIT: Patch failed with the random breadcrumb noticed again.
Comment #219
aspilicious CreditAttribution: aspilicious commented#215: user-picture-field-1292470-213.patch queued for re-testing.
Comment #221
BerdirThis should fix those noticed for now, see comment in the interdiff for the explanation.
Comment #222
aspilicious CreditAttribution: aspilicious commentedSrry but this still doesn't work. Try the following while testing:
1) Install drupal with minimal install
2) Enable field UI and comment module
3) Create a content type with just the basic fields
4) Set a default picture
5) Create an article and write a comment in the same article (pictures appear)
6) Create a new person choose a new image
7) Copy the patched D8 code
8) Run update.php
9) Verify the changes.
10) Notice broken links, "thumbnail" urls are generated but the image styles aren't generated they don't appear in my files folder.
OS: Windows 7
Comment #223
Berdir@aspilicious: Thanks for testing, but 10) is not a problem with this patch, it's because the image style upgrade path is totally broken. If you manually re-create the thumbnail image style then the pictures work fine. There's already #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity) related to those and I think maybe even another one about the upgrade path. Not sure what you mean with 11), yes, you should be able to edit the picture of a user, worked fine for me?
Comment #224
aspilicious CreditAttribution: aspilicious commentedOk not related to this patch than. I'm testing some more:
1)
Go to the default image field. Click on edit but don't change anything, press save:
"The file used in the Default image field may not be referenced."
2)
I get an error message that I need to download more updates but when I run update.php it doesn't find any.
Probably not related at all to this patch. Maybe caused by the first error in #212 I always get when doing a majore upgrade. (I just run update.php again and than it continues)
*sigh*
The upgrade path seriously feels like a bumpy ride at the moment: http://www.youtube.com/watch?v=G2RCCDSBEGk
I'm not sure the "hack" in your latest patch is acceptable but if it is I'm willing to let this patch go for now. But I'll keep an eye on the upgrade path and this issue.
Comment #225
BerdirThanks, nice catch!
Updated the patch to add file usage for the default image and added test coverage for that.
Comment #226
BerdirTests passed, they're just not being updated above. Also, the hack in field_ui.admin.inc from #221 can be removed once #1848200: Random warnings in tests in field_ui.admin.inc is in.
I think this is close...
Comment #227
sunYeah, broken HEAD caused PIFT to not report test results for a range of patches in the queue.
Thank you for driving this home!
Comment #228
catchAlright. Committed/pushed this one to 8.x. Thanks all!
Comment #229
BerdirThe change in field UI can be reverted now that the random test failures issue has been commited. Also, I guess this needs a change notice.
Comment #230
ParisLiakos CreditAttribution: ParisLiakos commentedpatch to revert random tests workaround
Comment #231
ParisLiakos CreditAttribution: ParisLiakos commentedChange notification added http://drupal.org/node/1851200
Comment #232
ParisLiakos CreditAttribution: ParisLiakos commentedFixing phpdoc as well and demoting to normal
Comment #233
BerdirChanges look good to me, this is RTBC unless the bot disagrees, which he will tell us in ~1h.
Change notice also looks good, one additional thing that might be useful is an example on how to load/access the referenced image file.
Also, it might be worth nothing that there should be no hardcoded assumptions about the picture because it's defined by standard.profile and not user.module, as before. So modules should no longer provide special integration with user pictures, they should just integrate with image fields.
Comment #234
Dave ReidNow I wonder how a module like Gravatar is supposed to work if it can't make any kind of assumption like that. :/
Comment #235
sunCan't Gravatar just simply be image field formatter? (Or even a field type of its own?)
Re-implementing it as a field formatter likely makes most sense, since I'd guess it inherently means that you get transparent support for both remote Gravatar pictures and custom/local pictures.
Comment #236
Dave ReidThis statement in code could probably use clarification and more specifics in the change notice:
Comment #237
Dave ReidBecause for the majority use case of Gravatar people don't have uploaded images. And so you can't run a formatter for a field that has no value.
Comment #238
ParisLiakos CreditAttribution: ParisLiakos commentedChange notice updated, feel free to correct/improve it
Comment #239
webchickCommitted and pushed #232 to 8.x. Thanks!
Now to... fixed? Not sure what to do with Dave Reid's concerns here. Add them to the change notice..? Follow-up issue?
Comment #240
moshe weitzman CreditAttribution: moshe weitzman commented@Dave - Standard imagefield shows a default image when the user has not uploaded anything so clearly it is possible to query gravatar instead in this case. see image_field_prepare_view() for where thats currently done. i'm not sure if you can implement gravatar lookup asgravatar_field_prepare_view() on image fields. If not, You might create own field type and instance on the user entity. Thats what I meant by "implement own forms ..."
Comment #241
Dave Reidhook_field_prepare_view() can only be invoked on the module that owns the field type, not on formatters or random other modules like alter hooks can.
Comment #242
catchWhat about hook_entity_prepare_view()? We don't appear to have a hook_field_attach_*() equivalent but they're nearly all duplicates anyway.
Comment #243
yched CreditAttribution: yched commented@Dave_Reid : formatters also have a prepareView() method. And both prepareView() and view() are called even if the field values are empty, so a formatter can totally display stuff if the field is empty.
Comment #244
sunCreated #1851880: Ensure that formatters for fields with no values are still run
Comment #245
yched CreditAttribution: yched commentedFollowup ? #1853378: Optimize user_view('compact') in template_preprocess_node()
Comment #246
swentel CreditAttribution: swentel commentedFollow up for Field ui check in #1859352: Lock user picture field
Comment #247
yched CreditAttribution: yched commentedOther followup :-)
#1860418: Fix user_picture field creation
Comment #249
Gábor HojtsySwap to the right media tag.
Comment #250
dqdI think this issue added the compact view mode to the user_picture and author_picture theme variables and has caused a lot confusion and new issues. I really wonder why a fieldable view_mode gets rendered on such special named variables. Even other contrib modules add their fields to view modes (like Flag) by default and break the look and feel of theme settings like: "show user picture in post" or "show user picture in comments" now.
See my comments and the respective issue affected by this decision: #2247677: User picture improvements in node template
Comment #251
dqd