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.

CommentFileSizeAuthor
#232 core.user-picture-field-1292470-232.patch1.49 KBParisLiakos
#230 core.user-picture-field-1292470-230.patch962 bytesParisLiakos
#225 user-picture-field-1292470-225.patch85.13 KBBerdir
#225 user-picture-field-1292470-225-interdiff.txt2.2 KBBerdir
#221 user-picture-field-1292470-221.patch84.57 KBBerdir
#221 user-picture-field-1292470-221-interdiff.txt962 bytesBerdir
#215 user-picture-field-1292470-213.patch83.63 KBBerdir
#215 user-picture-field-1292470-213-interdiff.txt8.9 KBBerdir
#205 user-picture-field-1292470-205.patch82.05 KBBerdir
#205 user-picture-field-1292470-205-interdiff.txt2.63 KBBerdir
#191 user-picture-field-1292470-191.patch77.52 KBBerdir
#189 user-picture-field-1292470-189.patch84.47 KBBerdir
#189 user-picture-field-1292470-189-interdiff.txt4.61 KBBerdir
#186 user-picture-field-1292470-186.patch81.58 KBBerdir
#186 user-picture-field-1292470-186-interdiff.txt7.09 KBBerdir
#181 current-8.x.png42.78 KBwebchick
#181 patched-8.x.png95.56 KBwebchick
#171 user-picture-field-1292470-171.patch80.47 KBBerdir
#159 user-picture-field-1292470-159.patch80.52 KBBerdir
#158 user-picture-field-1292470-158.patch92.57 KBBerdir
#156 user-picture-field-1292470-156.patch75.2 KBBerdir
#156 user-picture-field-1292470-156-interdiff.txt679 bytesBerdir
#154 user-picture-field-1292470-154.patch74.54 KBBerdir
#154 user-picture-field-1292470-154-interdiff.txt1.94 KBBerdir
#146 user-image-1292470-146.patch73.53 KBdagmar
#146 interdiff.txt466 bytesdagmar
#144 user-image-1292470-144.patch74.4 KBpcambra
#141 user-image-1292470-141.patch74.21 KBpcambra
#138 user-image-1292470-138.patch73.9 KBpcambra
#134 user-image-1292470-134.patch71.73 KBpcambra
#133 user-image-1292470-133.patch67.55 KBpcambra
#128 user-image-1292470-128.patch71.73 KBtim.plunkett
#128 interdiff.txt4.35 KBtim.plunkett
#127 user-image-1292470-127.patch71.62 KBtim.plunkett
#122 platform.user-picture.122.patch71.04 KBsun
#122 interdiff.txt1.71 KBsun
#119 platform.user-picture.119.patch71.08 KBsun
#113 interdiff.txt1.07 KBtim.plunkett
#113 drupal-1292470-113.patch71.3 KBtim.plunkett
#111 drupal-1292470-111.patch71.09 KBtim.plunkett
#108 interdiff.txt2.26 KBtim.plunkett
#108 drupal-1292470-108.patch71.07 KBtim.plunkett
#107 drupal-1292470-106.patch71.92 KBtim.plunkett
#102 drupal-1292470-102-fail.patch65.87 KBtim.plunkett
#102 drupal-1292470-102-combined.patch69.73 KBtim.plunkett
#101 interdiff.txt4.17 KBtim.plunkett
#101 drupal-1292470-101-test_with_1619898.patch109.72 KBtim.plunkett
#101 drupal-1292470-101-actual_patch.patch69.73 KBtim.plunkett
#101 drupal-1292470-101-combined_with_1619898.patch115.88 KBtim.plunkett
#98 drupal-1292470-98.patch65.59 KBManuel Garcia
#97 drupal-1292470-97.patch65.59 KBtim.plunkett
#95 drupal-1292470-95.patch65.63 KBtim.plunkett
#95 interdiff.txt4.9 KBtim.plunkett
#83 drupal-1292470-83.patch65.24 KBtim.plunkett
#83 interdiff.txt940 bytestim.plunkett
#78 drupal-1292470-78.patch64.89 KBtim.plunkett
#72 drupal8.user-picture-field.69.patch64.9 KBAugust1914
#68 drupal8.user-picture-field.68.patch64.95 KBsun
#68 interdiff.txt1.3 KBsun
#66 drupal8.user-picture-field.66.patch64.56 KBsun
#64 1292470-user_picture-64.patch61.55 KBandypost
#64 57vs64.patch2.9 KBandypost
#63 1292470-1upload.png20.04 KBkbasarab
#63 1292470-2uploadsuccess.png70.2 KBkbasarab
#63 1292470-3photoshow.png66.08 KBkbasarab
#63 1292470-4.png49.89 KBkbasarab
#63 1292470-5.png58.11 KBkbasarab
#57 drupal8.user-picture-field.57.patch60.28 KBsun
#55 drupal8.user-picture-field.55.patch59.47 KBsun
#54 drupal8.user_.picture-field.53.patch59.54 KBsun
#52 drupal8.user-picture-field.52a.patch59.44 KBsun
#52 interdiff.txt2.43 KBsun
#51 drupal8.user-picture-field.52.patch58.62 KBsun
#51 interdiff.txt3.5 KBsun
#50 drupal8.user-picture-field.50.patch57.28 KBsun
#50 interdiff.txt6.43 KBsun
#48 drupal8.user-picture-field.48.patch57.17 KBsun
#48 interdiff.txt20.26 KBsun
#46 1292470-user-picture-field-46.patch43.8 KBNiklas Fiekas
#46 1292470-user-picture-field-46-interdiff.txt14.82 KBNiklas Fiekas
#44 1292470-user-picture-field-44.patch50.06 KBNiklas Fiekas
#38 drupal8.user-picture-field.38.patch45.83 KBsun
#36 drupal8.user-picture-field.36.patch46.25 KBsun
#34 drupal8.user-picture-field.34.patch46.23 KBsun
#32 drupal8.user-picture-field.32.patch43.73 KBsun
#28 0001-Move-user-picture-to-Field-API.patch54.58 KBmoshe weitzman
#24 pic.diff52.21 KBmoshe weitzman
#22 pic.diff52.19 KBmoshe weitzman
#21 pic.diff51.34 KBmoshe weitzman
#18 pic.diff48.83 KBmoshe weitzman
#16 pic.diff48.83 KBmoshe weitzman
#15 pic.diff48.83 KBmoshe weitzman
#14 av1.svg477 byteschx
#11 pic.diff50.4 KBmoshe weitzman
#10 pic.diff50.47 KBmoshe weitzman
#4 pic.diff48.91 KBmoshe weitzman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aaron’s picture

subscribe. this allows lolcat videos for user pics w00t

moshe weitzman’s picture

Great 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?

catch’s picture

There'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.

moshe weitzman’s picture

Assigned: Unassigned » moshe weitzman
Status: Active » Needs review
Issue tags: -Media Sprint 2011 +API change, +API clean-up
FileSize
48.91 KB

26 files changed, 153 insertions(+), 586 deletions(-)

Mission accomplished (infamous last words).

  1. Used catch's suggestion to enrich $node and $comment with the whole $account object of the author.
  2. Like D7, we build the user picture during template_preprocess_[node|comment]. We use field_view_field() which defaults to the 'thumbnail' image style but that is overridable during later preprocess or process hooks.
  3. Surely someone is going to propose that we add a profile entity and put this and other stuff on it. This is worth discussing, but lets do that elsewhere. Right now, we are cleaning up a lot of legacy code. Moving this to a different entity is an easy change.
  4. We have temporarily lost the somewhat obscure feature of showing a default image when user has not uploaded a picture. Ideas on how that should be implemented are welcome. IMO, we can proceed without this feature.
  5. There is no update path from user picture to Field API. For cases like this, I propose we add an item section to the new Change Record content type that tracks Done/Not_Done. When we get near alpha release, I will lead the upgrade coding as per #1052692: New import API for major version upgrades (was migrate module). We can start this earlier than alpha release if needed.
Dave Reid’s picture

Some 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?

Status: Needs review » Needs work

The last submitted patch, pic.diff, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review

@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.

Lars Toomre’s picture

Status: Needs review » Needs work

cross-posting

sun’s picture

First 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.

+++ b/modules/comment/comment.module
@@ -2269,7 +2280,8 @@ function template_preprocess_comment(&$variables) {
-  $variables['picture']   = theme_get_setting('toggle_comment_user_picture') ? theme('user_picture', array('account' => $comment)) : '';
+  $picture = field_view_field('user', $comment->account, 'field_user_picture', array('label' => 'hidden', 'settings' => array('image_link' => 'content', 'image_style' => 'thumbnail')));
+  $variables['picture']   = theme_get_setting('toggle_comment_user_picture') ? $picture : '';

+++ b/modules/node/node.module
@@ -1468,7 +1479,9 @@ function template_preprocess_node(&$variables) {
+    // To change user picture settings (e.g. image style), implement hook_preprocess_node().
+    $picture = field_view_field('user', $node->account, 'field_user_picture', array('label' => 'hidden', 'settings' => array('image_link' => 'content', 'image_style' => 'thumbnail')));
+    $variables['user_picture'] = theme_get_setting('toggle_node_user_picture') ? $picture : '';

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.

+++ b/modules/system/system.admin.inc
@@ -416,7 +416,7 @@ function system_theme_settings($form, &$form_state, $key = '') {
+  if (!user_pictures_enabled()) {

+++ b/modules/user/user.module
@@ -191,6 +187,37 @@ function user_uri($user) {
+function user_picture_enabled() {

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.

+++ b/modules/user/user.admin.inc
@@ -347,79 +347,6 @@ function user_admin_settings() {
-    $picture_path =  file_default_scheme() . '://' . variable_get('user_picture_path', 'pictures');
...
-    '#default_value' => variable_get('user_picture_path', 'pictures'),
...
-    '#default_value' => variable_get('user_picture_default', ''),
...
-      '#options' => image_style_options(TRUE),
-      '#default_value' => variable_get('user_picture_style', ''),
...
-    '#default_value' => variable_get('user_picture_dimensions', '85x85'),
...
-    '#default_value' => variable_get('user_picture_file_size', '30'),

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.

+++ b/modules/user/user.install
@@ -209,12 +209,6 @@ function user_schema() {
-      'picture' => array(
-        'type' => 'int',
-        'not null' => TRUE,
-        'default' => 0,
-        'description' => "Foreign key: {file_managed}.fid of user's picture.",
-      ),

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.

+++ b/modules/user/user.module
@@ -191,6 +187,37 @@ function user_uri($user) {
+/*
+ * Populate $entity->account for each prepared entity. Called by
+ * hook_entity_prepare_view() implementations.
...
+ *
+ */

Minor:

1) Should be /**.

2) Second sentence should be split into the description.

3) Superfluous blank phpDoc line at the end.

+++ b/modules/user/user.module
@@ -191,6 +187,37 @@ function user_uri($user) {
+    if (in_array($entity->uid, array_keys($accounts))) {

isset() ?

+++ b/modules/user/user.module
@@ -191,6 +187,37 @@ function user_uri($user) {
+/*

/**

+++ b/modules/user/user.module
@@ -191,6 +187,37 @@ function user_uri($user) {
+function user_picture_enabled() {
+  $instances = field_info_instances('user', 'user');
+  return array_key_exists('field_user_picture', $instances);

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)

+++ b/modules/user/user.module
@@ -1381,55 +1282,6 @@ function user_block_view($delta = '') {
-      $alt = t("@user's picture", array('@user' => format_username($account)));

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.

+++ b/modules/user/user.module
@@ -3515,6 +3365,9 @@ function user_form_process_password_confirm($element) {
 /**
  * Implements hook_node_load().
+ *
+ * @todo: Deprecated by node_entity_prepare_view(). Update code that depends on
+ * these properties.
  */
 function user_node_load($nodes, $types) {

Not sure what this refers to.

+++ b/modules/user/user.test
@@ -849,248 +848,6 @@ class UserCancelTestCase extends DrupalWebTestCase {
-class UserPictureTestCase extends DrupalWebTestCase {

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.

moshe weitzman’s picture

FileSize
50.47 KB

This 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.

moshe weitzman’s picture

Title: Make user pictures fields » Convert user pictures to Image Field
FileSize
50.4 KB

chx 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.

moshe weitzman’s picture

Status: Needs work » Needs review

Lets get more feedback from humans and test bot ... As stated, upgrade path is still outstanding.

Status: Needs review » Needs work

The last submitted patch, pic.diff, failed testing.

chx’s picture

FileSize
477 bytes

This 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.

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
48.83 KB

try to satisfy bot patch apply. no changes.

moshe weitzman’s picture

FileSize
48.83 KB

Re-upload per bizarre PIFT msg.

Status: Needs review » Needs work

The last submitted patch, pic.diff, failed testing.

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
48.83 KB

Fix test fail, I hope.

Status: Needs review » Needs work

The last submitted patch, pic.diff, failed testing.

moshe weitzman’s picture

The 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} ...

moshe weitzman’s picture

FileSize
51.34 KB

Added 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.

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
52.19 KB

Adds update function to migrate to Field API. Now ready for review.

Status: Needs review » Needs work

The last submitted patch, pic.diff, failed testing.

moshe weitzman’s picture

FileSize
52.21 KB

Fixed a unit test fail.

moshe weitzman’s picture

Status: Needs work » Needs review

Test bot is ignoring this latest patch? Any ideas?

Status: Needs review » Needs work

The last submitted patch, pic.diff, failed testing.

andypost’s picture

+++ b/modules/user/user.installundefined
@@ -335,3 +329,35 @@ function user_install() {
+/**
+ * Migrate user pictures to Field API.
+ */
+function user_update_8000() {
+  $results = db_select('users', 'u')
+                ->fields('u', array('uid', 'picture'))
+                ->condition('picture', 0, '>')
+                ->execute()
+                ->fetchAllKeyed();
+  if ($uids = array_keys($results)) {

This 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

+++ b/profiles/standard/standard.installundefined
@@ -258,12 +258,6 @@ function standard_install() {
-  // Enable user picture support and set the default to a square thumbnail option.
-  variable_set('user_pictures', '1');
-  variable_set('user_picture_dimensions', '1024x1024');
-  variable_set('user_picture_file_size', '800');
-  variable_set('user_picture_style', 'thumbnail');

This settings should be migrated to image field settings

+++ b/profiles/standard/standard.installundefined
@@ -387,6 +381,82 @@ function standard_install() {
+  $field = array(
+    'field_name' => 'field_user_picture',

Suppose this name should be user_picture so lives in user's module namespace

+++ b/profiles/standard/standard.installundefined
@@ -387,6 +381,82 @@ function standard_install() {
+      'max_filesize' => '',
+      'max_resolution' => '',
+      'min_resolution' => '',

Defaults should be set from old variables

26 days to next Drupal core point release.

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
54.58 KB

Attached 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.

Status: Needs review » Needs work

The last submitted patch, 0001-Move-user-picture-to-Field-API.patch, failed testing.

moshe weitzman’s picture

Assigned: moshe weitzman » Unassigned

perhaps someone else can finish this

sun’s picture

Assigned: Unassigned » sun

I'll try to have a stab at this in the next days, unless someone beats me to it.

sun’s picture

Status: Needs work » Needs review
FileSize
43.73 KB

First of all, a non-trivial re-roll against latest HEAD.

Status: Needs review » Needs work

The last submitted patch, drupal8.user-picture-field.32.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
46.23 KB

Some more clean-ups and fixes. But no effin' idea what's going on in that SimpleTest test case.

Status: Needs review » Needs work

The last submitted patch, drupal8.user-picture-field.34.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
46.25 KB

Resolves 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.

Status: Needs review » Needs work

The last submitted patch, drupal8.user-picture-field.36.patch, failed testing.

sun’s picture

Re-rolled against latest HEAD.

Deciphered’s picture

Status: Needs work » Needs review

Marking as 'Need review' to get patch tested.

Status: Needs review » Needs work

The last submitted patch, drupal8.user-picture-field.38.patch, failed testing.

Dave Reid’s picture

Issue tags: +Media Initiative
Dries’s picture

Issue tags: +Favorite-of-Dries

Tagging. I like.

catch’s picture

Category: feature » task

I'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.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
50.06 KB

Rerolled. Uploading to see how it performs.

Edit: Careful when basing work on this: I accidantely reverted some of the user entity conversion.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/user.cssundefined
@@ -78,12 +78,11 @@ div.password-suggestions ul {
-/* Generated by user.module but used by profile.module: */
 .profile {
   clear: both;
   margin: 1em 0;
 }
-.profile .user-picture {
+.profile .field-name-field-user-picture {
   float: right; /* LTR */
   margin: 0 1em 1em 0; /* LTR */

If it's only used by profile.module, doesn't that mean that it's not used anymore at all? :)

+++ b/core/modules/user/user.installundefined
@@ -243,7 +237,6 @@ function user_schema() {
       'mail' => array('mail'),
-      'picture' => array('picture'),

Hah, that just got added less than 24h hours ago ;)

+++ b/core/modules/user/user.installundefined
@@ -399,5 +392,96 @@ function user_update_8001() {
+    $accounts = user_load_multiple($uids);
+    foreach ($results as $uid => $fid) {
+      if ($file = file_load($fid)) {
+        $edit['field_user_picture'][LANGUAGE_NONE][0] = (array) $file;
+        user_save($accounts[$uid], $edit);
+      }

$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.

+++ b/core/modules/user/user.moduleundefined
@@ -355,6 +394,191 @@ function user_load_by_name($name) {
+ *   An array of fields and values to save. For example array('name'
+ *   => 'My name'). Key / value pairs added to the $edit['data'] will be
+ *   serialized and saved in the {users.data} column.
+ *
+ * @return
+ *   A fully-loaded $user object upon successful save or FALSE if the save failed.
+ *
+ * @todo D8: Drop $edit and fix user_save() to be consistent with others.
+ */
+function user_save($account, $edit = array()) {
+  $transaction = db_transaction();
+  try {
+    if (!empty($edit['pass'])) {
+      // Allow alternate password hashing schemes.
+      require_once DRUPAL_ROOT . '/' . variable_get('password_inc', 'core/includes/password.inc');
+      $edit['pass'] = user_hash_password(trim($edit['pass']));
+      // Abort if the hashing failed and returned FALSE.
+      if (!$edit['pass']) {
+        return FALSE;

Oh, you re-added user_save() as well ;)

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
14.82 KB
43.8 KB
  1. Didn't find the .profile rules used anywhere, so dumped them.
  2. Removed a few hacks.
  3. Removed more obsolete functions.
  4. - 'picture' => array('picture'),
    Yeah. Now it's gone ;)
  5. Leaving the upgrade broken, but marked with a todo.
  6. Re-removed user_save().
  7. Fix comment preview test.
  8. Fix the test this patch adds.

Status: Needs review » Needs work

The last submitted patch, 1292470-user-picture-field-46.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
20.26 KB
57.17 KB
+++ b/core/modules/user/user.module
@@ -391,25 +430,24 @@ function user_validate_name($name) {
-function user_validate_picture(&$form, &$form_state) {
...
+function user_validate_mail($mail) {

Wrong rebase/merge? Doesn't seem to be related to this patch, unless I'm missing something.

Resolved some test failures.

Status: Needs review » Needs work

The last submitted patch, drupal8.user-picture-field.48.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
6.43 KB
57.28 KB

Updated UserPictureTestCase accordingly. Still contains one commented out test, which verified that previous user pictures supported external URLs as default images. (?)

sun’s picture

Final.

sun’s picture

+++ b/core/modules/user/user.install
@@ -399,5 +392,95 @@ function user_update_8001() {
+function user_update_8003() {
+  // @todo Chunk into 20 per batch pass.

Fixed in attached patch.

+++ b/core/modules/user/user.install
@@ -399,5 +392,95 @@ function user_update_8001() {
+    // @todo Must not use API functions invoking hooks.

To be resolved in a separate issue.

+++ b/core/modules/user/user.test
@@ -919,227 +918,51 @@ class UserCancelTestCase extends DrupalWebTestCase {
+ * @todo Test public and private methods?

That's a test for image fields, which may or may not exist already. Irrelevant for this issue.

+++ b/core/modules/user/user.test
@@ -919,227 +918,51 @@ class UserCancelTestCase extends DrupalWebTestCase {
   /**
    * Test HTTP schema working with user pictures.
...
-  function testExternalPicture() {
...
     // Set the default picture to an URI with a HTTP schema.

Removed this test.

Dave Reid’s picture

+++ b/core/modules/user/user.installundefined
@@ -399,5 +392,109 @@ function user_update_8001() {
+  $results = db_query('SELECT uid, picture FROM {users} WHERE picture > 0 LIMIT 0, 20')->fetchAllKeyed();

Should use db_query_range?

+++ b/core/modules/user/user.moduleundefined
@@ -199,6 +199,45 @@ function user_label($entity_type, $entity) {
+function user_picture_enabled() {
+  $instances = field_info_instances('user', 'user');
+  return isset($instances['field_user_picture']) && !$instances['field_user_picture']['deleted'];

Could be simplified using field_info_instance()?

-25 days to next Drupal core point release.

sun’s picture

Prevent a division by zero in the update function.

sun’s picture

Incorporated @Dave Reid's suggestions. Thanks!

Status: Needs review » Needs work

The last submitted patch, drupal8.user-picture-field.55.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
60.28 KB

whoopsie - endless loop in that update function ;)

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I looked at this one more time and I'm quite pleased with where it is now. imo this is rtbc.

Dave Reid’s picture

Am 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?

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

Agreed; we'll make this a priority for office hours in the upcoming week if others don't manually test it first.

sun’s picture

I 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.

Berdir’s picture

Hm, 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 :)

kbasarab’s picture

Testing 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.

andypost’s picture

Added migration of variables, not sure about using defualt 'medium' imagestyle

Also we need migrate tests

Status: Needs review » Needs work

The last submitted patch, 57vs64.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
64.56 KB

Re-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.

Status: Needs review » Needs work

The last submitted patch, drupal8.user-picture-field.66.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
64.95 KB

Fixed those remaining exceptions.

kbasarab’s picture

Synced 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.

sun’s picture

Status: Needs review » Needs work
Issue tags: +Favorite-of-Dries, +Needs manual testing, +API change, +API clean-up, +Media Initiative

The last submitted patch, drupal8.user-picture-field.68.patch, failed testing.

August1914’s picture

Re-rolled, no functional test, just to get it to apply.

sun’s picture

Status: Needs work » Needs review

#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.

tim.plunkett’s picture

Lets see what the bot thinks!

kendramyers’s picture

Assigned: sun » kendramyers
Issue tags: +tcdrupal2012

assigned to myself to test manually

sun’s picture

Status: Needs review » Needs work
Issue tags: +Favorite-of-Dries, +Needs manual testing, +API change, +API clean-up, +Media Initiative, +tcdrupal2012

The last submitted patch, drupal8.user-picture-field.69.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
64.89 KB

Rerolled, and setting back to RTBC as sun asked.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-1292470-78.patch, failed testing.

tim.plunkett’s picture

I guess #1401558: Remove the usage handling logic from file_delete() broke this somehow. Didn't dig in.

Berdir’s picture

Yes, 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.

webchick’s picture

Issue tags: +theme system cleanup

Since this removes user-picture.tpl.php and associated preprocess functions in favour of general field API stuff, tagging as theme system cleanup.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
940 bytes
65.24 KB

The last submitted patch, drupal-1292470-83.patch, failed testing.

marcingy’s picture

#83: drupal-1292470-83.patch queued for re-testing.

marcingy’s picture

Locally no fails

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks Tim. Back to RTBC.

Eric_A’s picture

@@ -399,5 +392,175 @@ function user_update_8001() {
 }
 
 /**
- * @} End of "addtogroup updates-7.x-to-8.x".
+ * @} End of "addtogroup updates-7.x-to-8.x"

There was a code style fix in a commit from a week ago: http://drupalcode.org/project/drupal.git/commitdiff/467a825 (#1358944: Misused @ingroup commands)

Dave Reid’s picture

So, 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?

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs review
Dave Reid’s picture

Also 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?

yched’s picture

'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 ! :-)

catch’s picture

Status: Needs review » Needs work
+  if (!module_exists('image')) {

Apart 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.

+      file_usage_delete($file, 'user', 'user', $uid);
+    }

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.

Dave Reid’s picture

Note 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.

tim.plunkett’s picture

Assigned: kendramyers » tim.plunkett
Status: Needs work » Needs review
FileSize
4.9 KB
65.63 KB

I 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.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Okay, rerolled after #1361226: Make the file entity a classed object.

Still to be done:

  • Upgrade needs to work even if image module is disabled.
  • Ensure that the upgrade database dump actually has user pictures in it.

Since I have no idea how to do the first, I'm unassigning from myself.

tim.plunkett’s picture

FileSize
65.59 KB

Whoops.

Manuel Garcia’s picture

FileSize
65.59 KB

patch applies fine, but going to update.php gives you:

Fatal error: Cannot redeclare user_update_8002() (previously declared in /home/manuel/htdocs/dhead/core/modules/user/user.install:382) in /home/manuel/htdocs/dhead/core/modules/user/user.install on line 473

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:

  1. I did not test migrating existing pictures.
  2. I had to enable the display of the picture so it would show in the user's page (people may want that as default perhaps).
  3. Also, I did not have user pictures enabled before updating, after I had the field there. Not sure it should be enabled by default.
cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

Looks pretty solid to me. RTBC

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

Still CNW for the two points in #96

tim.plunkett’s picture

This 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.

tim.plunkett’s picture

Okay, that went in. Uploading both with and with upgrade functions again.

tim.plunkett’s picture

Oh, 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.

Berdir’s picture

What 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?

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Moshe suggested update_check_requirements, I'm going to add something along those lines now.

Dave Reid’s picture

How 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?

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
FileSize
71.92 KB

Okay, 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.

tim.plunkett’s picture

FileSize
71.07 KB
2.26 KB

catch 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.

Status: Needs review » Needs work

The last submitted patch, drupal-1292470-108.patch, failed testing.

aspilicious’s picture

Argument 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 ;)

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
71.09 KB

Duh. Thanks aspilicious.

Status: Needs review » Needs work

The last submitted patch, drupal-1292470-111.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
71.3 KB
1.07 KB

So the awesome new update_module_enable doesn't account for dependencies. So added checks for file as well.

tim.plunkett’s picture

Yay!
+215 lines of upgrades and upgrade tests.
+299, -692 lines of actual changes.

And now it just needs some real manual testing.

aspilicious’s picture

Status: Needs review » Needs work
An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: http://localhost/upgrade/test2/core/update.php?op=selection&token=83Trn3Fgohuj9jWmi7o7WxrJpJAkpmZD9om6Pxyf-MM&id=4&op=do StatusText: OK ResponseText: Recoverable fatal error: Argument 1 passed to file_usage_delete() must be an instance of Drupal\Core\File\File, instance of stdClass given

So 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*

aspilicious’s picture

aspilicious: 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 ;)

Berdir’s picture

Note that the names in #116 are reversed.

xjm’s picture

Oho, timely. We're working on the following documentation patch in office hours: #1134088: Properly document update-api functions

sun’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Platform Initiative
FileSize
71.08 KB

#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)

aspilicious’s picture

So for this to go in, we should rip out all the api functions in the update and switch them to custom update functions.

sun’s picture

I 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.

sun’s picture

Assigned: Unassigned » sun
FileSize
1.71 KB
71.04 KB

Since an "interdiff impossible; taking evasive action", I manually compared the current patch against the original #72, and reviewed it once more in-depth.

+++ b/core/modules/user/user.install
@@ -442,5 +435,187 @@ function user_update_8002() {
+function user_update_8004(&$sandbox) {
...
+      // Remove file usage.
+      // file_usage_delete() does not invoke File API functions or hooks and can
+      // be safely called here.
+      $file = (object) array('fid' => $fid);
+      file_usage_delete($file, 'user', 'user', $uid);

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.

sun’s picture

Good. 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.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

This is really nice work. Let's commit this please. Resist the nitpicking!

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work

I 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.

tim.plunkett’s picture

Assigned: sun » tim.plunkett

I want to get this done.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
71.62 KB

First, 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

tim.plunkett’s picture

Okay, 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.

Status: Needs review » Needs work

The last submitted patch, user-image-1292470-128.patch, failed testing.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Issue tags: -Configuration system, -tcdrupal2012

I really wanted to see this one through, but I can't deal with upgrade path issues this month.

moshe weitzman’s picture

Anyone available to finish up the upgrade path? Please please.

larowlan’s picture

pcambra’s picture

Status: Needs work » Needs review
FileSize
67.55 KB

Just rerolling to see how it looks to the bot now

pcambra’s picture

FileSize
71.73 KB

Missed new files.

Dave Reid’s picture

larowlan’s picture

groan, sorry about the tags

Status: Needs review » Needs work

The last submitted patch, user-image-1292470-134.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
73.9 KB

Here's a new reroll:

  • Core tpls have been moved to a templates subfolder.
  • User has now a compact view mode so that makes one of the field ui tests fail, transformed that to taxonomy (we'll probably want to open a followup issue to include drupalCreateVocabulary in WebTestBase.php)
  • Moved update_N to the current numbers.
  • Fixed += $settings which is probably something I might have introduced

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.

Status: Needs review » Needs work

The last submitted patch, user-image-1292470-138.patch, failed testing.

pcambra’s picture

Just opened #1806970: Add schema version to image module for minimal databases for fixing the extra upgrade tests errors on minimal databases

pcambra’s picture

Status: Needs work » Needs review
FileSize
74.21 KB

Another reroll and a first try of using a local image

Status: Needs review » Needs work

The last submitted patch, user-image-1292470-141.patch, failed testing.

Eric_A’s picture

Using file_save_data() within a hook_update_N() implementation must be a no go, no...?

pcambra’s picture

Status: Needs work » Needs review
FileSize
74.4 KB

Using file_save_data() within a hook_update_N() implementation must be a no go, no...?

Not sure, normally you already have the folder locally so maybe the creation won't be necessary?

Anyways, here's another try.

Status: Needs review » Needs work

The last submitted patch, user-image-1292470-144.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
466 bytes
73.53 KB

Re-rolled. Also, probaly the tests were failing because the progress was never incremented.

Status: Needs review » Needs work

The last submitted patch, user-image-1292470-146.patch, failed testing.

moshe weitzman’s picture

Only 2 fails. Go pcambra and dagmar!

Berdir’s picture

Ok, 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.

chx’s picture

Schema 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).

Berdir’s picture

Ok, 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 ;)

catch’s picture

Ok, 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.

It 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).

catch’s picture

double post.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.94 KB
74.54 KB

Ok, 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.

Status: Needs review » Needs work

The last submitted patch, user-picture-field-1292470-154.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
679 bytes
75.2 KB

Ok, 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.

Status: Needs review » Needs work

The last submitted patch, user-picture-field-1292470-156.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
92.57 KB

Interesting 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 :)

Berdir’s picture

Hm, didn't reroll the branch, let's try that again.

swentel’s picture

Berdir 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).

yched’s picture

That _update_7000_field_create_instance() hunk is fine by me as well.

Lars Toomre’s picture

There 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?

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

ParisLiakos’s picture

awesomeness! good job Berdir:D

Lars Toomre’s picture

Thanks @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.

Berdir’s picture

@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.

Lars Toomre’s picture

As 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.

Berdir’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Favorite-of-Dries, +Needs manual testing, +API change, +API clean-up, +Platform Initiative, +Media Initiative, +theme system cleanup, +#pnx-sprint

The last submitted patch, user-picture-field-1292470-159.patch, failed testing.

ParisLiakos’s picture

Berdir’s picture

Status: Needs work » Needs review
FileSize
80.47 KB

Re-rolled. Let's see if that was all or some other code needs to be updated.

aspilicious’s picture

Status: Needs review » Needs work

Once 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

xjm’s picture

Status: Needs work » Reviewed & tested by the community

@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.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Opps, xpost.

Berdir’s picture

Status: Needs work » Needs review

I actually think that was a nested crosspost and it should be needs review to be manually tested.

aspilicious’s picture

I 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...

Berdir’s picture

Can 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.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Agree with fixing generic upgrade issues in separate issue as per #177.

catch’s picture

Is there an issue for the hosed upgrade path? Shame upgrade path tests aren't catching the failures.

BTMash’s picture

I'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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
95.56 KB
42.78 KB

This 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:

In current 8.x, picture is small sized and shows up next to both node and comment.

In patched 8.x, both picture and 'member for' show up, distorting the view.

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?

sun’s picture

Status: Needs work » Reviewed & tested by the community

Can 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.

sun’s picture

xjm’s picture

Why does a normal task get to spawn a critical bug? -1 on not fixing it before commit.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Yeah, 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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.09 KB
81.58 KB

Turns 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.

Berdir’s picture

The last submitted patch, user-picture-field-1292470-186.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.61 KB
84.47 KB

Re-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.

Status: Needs review » Needs work

The last submitted patch, user-picture-field-1292470-189.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
77.52 KB

Fixed the annotion, no other changes.

aspilicious’s picture

Assigned: Unassigned » aspilicious

Assigning to me so I can test the upgrade path again :)

Status: Needs review » Needs work

The last submitted patch, user-picture-field-1292470-191.patch, failed testing.

aspilicious’s picture

Assigned: aspilicious » Unassigned

Still 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

Warning: array_flip() expects parameter 1 to be array, null given in search_get_info() (line 281 of core\modules\search\search.module).
Warning: array_intersect_key(): Argument #2 is not an array in search_get_info() (line 281 of core\modules\search\search.module).
Warning: reset() expects parameter 1 to be array, null given in search_get_default_module_info() (line 298 of core\modules\search\search.module).

<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

Berdir’s picture

The last submitted patch, user-picture-field-1292470-191.patch, failed testing.

Berdir’s picture

These test failures don't really look related?

Drupal\file\Tests\FileFieldWidgetTest
Drupal\system\Tests\Menu\BreadcrumbTest

Undefined index: field_types	Notice	field_ui.admin.inc	458	field_ui_widget_type_options()	
Invalid argument supplied for foreach()	Warning	field_ui.admin.inc	458	field_ui_widget_type_options()	

Triggering a re-test.

Berdir’s picture

The last submitted patch, user-picture-field-1292470-191.patch, failed testing.

fago’s picture

With 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.

moshe weitzman’s picture

I don't really follow the last comment by @fago. What exactly needs to be done here (versus what is noce to have)?

ParisLiakos’s picture

+++ b/core/modules/user/user.installundefined
@@ -614,5 +619,212 @@ function user_update_8009(&$sandbox) {
+      $table_name = "field_data_user_picture";
+      $revision_name = "field_revision_picture";

But 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!

Berdir’s picture

Thanks, 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.

dawehner’s picture

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
82.05 KB

Here's a re-roll to get this re-started.

- 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.

[...]

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.

This is exactly what happened in the meantime. file.module has update functions now, so we're running into th update not complete problem.

Status: Needs review » Needs work

The last submitted patch, user-picture-field-1292470-205.patch, failed testing.

sun’s picture

Priority: Normal » Major
Issue tags: -#pnx-sprint

At this point, I'd strongly recommend to:

  1. Remove the update_module_enable() for File module in this upgrade path.
  2. 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.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.php
@@ -47,7 +47,11 @@
+ *     },
+ *     "compact" = {
+         "label" = "Compact",
+         "custom_settings" = TRUE
+       }
  *   }

Looks like we're missing phpDoc comment "*" prefixes here? I'm surprised the annotation parser didn't blow up on that (it didn't).

Berdir’s picture

Issue tags: +#pnx-sprint

@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...

sun’s picture

Yes, 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.

moshe weitzman’s picture

@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.

catch’s picture

I 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.

aspilicious’s picture

Upgrade 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

user module

Update #8011
Failed: Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "entity.query" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of C:\xampp\htdocs\drupal7\core\vendor\symfony\dependency-injection\Symfony\Component\DependencyInjection\ContainerBuilder.php).
webchick’s picture

Is that because of this patch, or is it a general condition?

aspilicious’s picture

First one sounds general, second is caused by the patch

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.9 KB
83.63 KB

Take 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.

The last submitted patch, user-picture-field-1292470-213.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#215: user-picture-field-1292470-213.patch queued for re-testing.

EDIT: Patch failed with the random breadcrumb noticed again.

Status: Needs review » Needs work

The last submitted patch, user-picture-field-1292470-213.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

The last submitted patch, user-picture-field-1292470-213.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
962 bytes
84.57 KB

This should fix those noticed for now, see comment in the interdiff for the explanation.

aspilicious’s picture

Srry 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

Berdir’s picture

@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?

aspilicious’s picture

Ok 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.

Berdir’s picture

Thanks, nice catch!

Updated the patch to add file usage for the default image and added test coverage for that.

Berdir’s picture

Tests 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...

sun’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, broken HEAD caused PIFT to not report test results for a range of patches in the queue.

Thank you for driving this home!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Alright. Committed/pushed this one to 8.x. Thanks all!

Berdir’s picture

Title: Convert user pictures to Image Field » Change notice: Convert user pictures to Image Field (and field UI check removal follow-up)
Priority: Major » Critical
Status: Fixed » Active

The 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.

ParisLiakos’s picture

Status: Active » Needs review
FileSize
962 bytes

patch to revert random tests workaround

ParisLiakos’s picture

Title: Change notice: Convert user pictures to Image Field (and field UI check removal follow-up) » Convert user pictures to Image Field (and field UI check removal follow-up)
Priority: Critical » Major

Change notification added http://drupal.org/node/1851200

ParisLiakos’s picture

Priority: Major » Normal
FileSize
1.49 KB

Fixing phpdoc as well and demoting to normal

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Changes 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.

Dave Reid’s picture

Now I wonder how a module like Gravatar is supposed to work if it can't make any kind of assumption like that. :/

sun’s picture

Can'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.

Dave Reid’s picture

This statement in code could probably use clarification and more specifics in the change notice:

Alternate user picture implementations (e.g., Gravatar) should provide their own add/edit/delete forms and populate the 'picture' variable during the preprocess stage.

Dave Reid’s picture

Because 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.

ParisLiakos’s picture

Change notice updated, feel free to correct/improve it

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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?

moshe weitzman’s picture

@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 ..."

Dave Reid’s picture

hook_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.

catch’s picture

What about hook_entity_prepare_view()? We don't appear to have a hook_field_attach_*() equivalent but they're nearly all duplicates anyway.

yched’s picture

@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.

sun’s picture

yched’s picture

swentel’s picture

Title: Convert user pictures to Image Field (and field UI check removal follow-up) » Convert user pictures to Image Field

Follow up for Field ui check in #1859352: Lock user picture field

yched’s picture

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

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -Media Initiative +D8Media

Swap to the right media tag.

dqd’s picture

I 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

dqd’s picture