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.

Files: 
CommentFileSizeAuthor
#232 core.user-picture-field-1292470-232.patch1.49 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 48,814 pass(es).
[ View ]
#230 core.user-picture-field-1292470-230.patch962 bytesParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 48,763 pass(es).
[ View ]
#225 user-picture-field-1292470-225.patch85.13 KBBerdir
Test request sent.
[ View ]
#225 user-picture-field-1292470-225-interdiff.txt2.2 KBBerdir
#221 user-picture-field-1292470-221.patch84.57 KBBerdir
Test request sent.
[ View ]
#221 user-picture-field-1292470-221-interdiff.txt962 bytesBerdir
#215 user-picture-field-1292470-213.patch83.63 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 48,539 pass(es), 0 fail(s), and 12 exception(s).
[ View ]
#215 user-picture-field-1292470-213-interdiff.txt8.9 KBBerdir
#205 user-picture-field-1292470-205.patch82.05 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 48,166 pass(es), 4 fail(s), and 16 exception(s).
[ View ]
#205 user-picture-field-1292470-205-interdiff.txt2.63 KBBerdir
#191 user-picture-field-1292470-191.patch77.52 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 46,343 pass(es), 0 fail(s), and 12 exception(s).
[ View ]
#189 user-picture-field-1292470-189.patch84.47 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#189 user-picture-field-1292470-189-interdiff.txt4.61 KBBerdir
#186 user-picture-field-1292470-186.patch81.58 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-picture-field-1292470-186.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 46,175 pass(es).
[ View ]
#159 user-picture-field-1292470-159.patch80.52 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-picture-field-1292470-159.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#158 user-picture-field-1292470-158.patch92.57 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 45,991 pass(es).
[ View ]
#156 user-picture-field-1292470-156.patch75.2 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 45,970 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#156 user-picture-field-1292470-156-interdiff.txt679 bytesBerdir
#154 user-picture-field-1292470-154.patch74.54 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 42,746 pass(es), 0 fail(s), and 307 exception(s).
[ View ]
#154 user-picture-field-1292470-154-interdiff.txt1.94 KBBerdir
#146 user-image-1292470-146.patch73.53 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 42,408 pass(es), 2 fail(s), and 574 exception(s).
[ View ]
#146 interdiff.txt466 bytesdagmar
#144 user-image-1292470-144.patch74.4 KBpcambra
FAILED: [[SimpleTest]]: [MySQL] 42,052 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#141 user-image-1292470-141.patch74.21 KBpcambra
FAILED: [[SimpleTest]]: [MySQL] 41,950 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
#138 user-image-1292470-138.patch73.9 KBpcambra
FAILED: [[SimpleTest]]: [MySQL] 42,165 pass(es), 3 fail(s), and 2 exception(s).
[ View ]
#134 user-image-1292470-134.patch71.73 KBpcambra
FAILED: [[SimpleTest]]: [MySQL] 41,928 pass(es), 11 fail(s), and 0 exception(s).
[ View ]
#133 user-image-1292470-133.patch67.55 KBpcambra
FAILED: [[SimpleTest]]: [MySQL] 41,914 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#128 user-image-1292470-128.patch71.73 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 40,645 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#128 interdiff.txt4.35 KBtim.plunkett
#127 user-image-1292470-127.patch71.62 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 40,633 pass(es).
[ View ]
#122 platform.user-picture.122.patch71.04 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,635 pass(es).
[ View ]
#122 interdiff.txt1.71 KBsun
#119 platform.user-picture.119.patch71.08 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,633 pass(es).
[ View ]
#113 interdiff.txt1.07 KBtim.plunkett
#113 drupal-1292470-113.patch71.3 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 36,774 pass(es).
[ View ]
#111 drupal-1292470-111.patch71.09 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 36,753 pass(es), 35 fail(s), and 8 exception(s).
[ View ]
#108 interdiff.txt2.26 KBtim.plunkett
#108 drupal-1292470-108.patch71.07 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 36,795 pass(es), 0 fail(s), and 8 exception(s).
[ View ]
#107 drupal-1292470-106.patch71.92 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 36,782 pass(es).
[ View ]
#102 drupal-1292470-102-fail.patch65.87 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 36,776 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#102 drupal-1292470-102-combined.patch69.73 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 36,793 pass(es).
[ View ]
#101 interdiff.txt4.17 KBtim.plunkett
#101 drupal-1292470-101-test_with_1619898.patch109.72 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#101 drupal-1292470-101-actual_patch.patch69.73 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#101 drupal-1292470-101-combined_with_1619898.patch115.88 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 36,771 pass(es).
[ View ]
#98 drupal-1292470-98.patch65.59 KBManuel Garcia
PASSED: [[SimpleTest]]: [MySQL] 36,779 pass(es).
[ View ]
#97 drupal-1292470-97.patch65.59 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 36,617 pass(es).
[ View ]
#95 drupal-1292470-95.patch65.63 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 36,627 pass(es).
[ View ]
#95 interdiff.txt4.9 KBtim.plunkett
#83 drupal-1292470-83.patch65.24 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 36,579 pass(es).
[ View ]
#83 interdiff.txt940 bytestim.plunkett
#78 drupal-1292470-78.patch64.89 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 36,580 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#72 drupal8.user-picture-field.69.patch64.9 KBAugust1914
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.user-picture-field.69.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#68 drupal8.user-picture-field.68.patch64.95 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.user-picture-field.68.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#68 interdiff.txt1.3 KBsun
#66 drupal8.user-picture-field.66.patch64.56 KBsun
FAILED: [[SimpleTest]]: [MySQL] 36,417 pass(es), 0 fail(s), and 11 exception(s).
[ View ]
#64 1292470-user_picture-64.patch61.55 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 34,943 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#64 57vs64.patch2.9 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 57vs64.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 35,286 pass(es).
[ View ]
#55 drupal8.user-picture-field.55.patch59.47 KBsun
FAILED: [[SimpleTest]]: [MySQL] 44,103 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#54 drupal8.user_.picture-field.53.patch59.54 KBsun
FAILED: [[SimpleTest]]: [MySQL] 44,076 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#52 drupal8.user-picture-field.52a.patch59.44 KBsun
FAILED: [[SimpleTest]]: [MySQL] 35,290 pass(es), 0 fail(s), and 5 exception(s).
[ View ]
#52 interdiff.txt2.43 KBsun
#51 drupal8.user-picture-field.52.patch58.62 KBsun
PASSED: [[SimpleTest]]: [MySQL] 35,299 pass(es).
[ View ]
#51 interdiff.txt3.5 KBsun
#50 drupal8.user-picture-field.50.patch57.28 KBsun
FAILED: [[SimpleTest]]: [MySQL] 35,186 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#50 interdiff.txt6.43 KBsun
#48 drupal8.user-picture-field.48.patch57.17 KBsun
FAILED: [[SimpleTest]]: [MySQL] 35,169 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#48 interdiff.txt20.26 KBsun
#46 1292470-user-picture-field-46.patch43.8 KBNiklas Fiekas
FAILED: [[SimpleTest]]: [MySQL] 35,237 pass(es), 37 fail(s), and 3 exception(s).
[ View ]
#46 1292470-user-picture-field-46-interdiff.txt14.82 KBNiklas Fiekas
#44 1292470-user-picture-field-44.patch50.06 KBNiklas Fiekas
FAILED: [[SimpleTest]]: [MySQL] 35,250 pass(es), 53 fail(s), and 5 exception(s).
[ View ]
#38 drupal8.user-picture-field.38.patch45.83 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,490 pass(es), 45 fail(s), and 1 exception(es).
[ View ]
#36 drupal8.user-picture-field.36.patch46.25 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.user-picture-field.36.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#34 drupal8.user-picture-field.34.patch46.23 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,463 pass(es), 45 fail(s), and 5 exception(es).
[ View ]
#32 drupal8.user-picture-field.32.patch43.73 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,498 pass(es), 48 fail(s), and 43 exception(es).
[ View ]
#28 0001-Move-user-picture-to-Field-API.patch54.58 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] 33,743 pass(es), 27 fail(s), and 42 exception(es).
[ View ]
#24 pic.diff52.21 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] 33,212 pass(es), 23 fail(s), and 42 exception(es).
[ View ]
#22 pic.diff52.19 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] 33,184 pass(es), 5 fail(s), and 0 exception(es).
[ View ]
#21 pic.diff51.34 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] 33,218 pass(es), 23 fail(s), and 42 exception(es).
[ View ]
#18 pic.diff48.83 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] 33,181 pass(es), 23 fail(s), and 42 exception(es).
[ View ]
#16 pic.diff48.83 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] 33,148 pass(es), 23 fail(s), and 80 exception(es).
[ View ]
#15 pic.diff48.83 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] 33,172 pass(es), 23 fail(s), and 80 exception(es).
[ View ]
#14 av1.svg477 byteschx
#11 pic.diff50.4 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pic_1.diff. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#10 pic.diff50.47 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pic_0.diff. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#4 pic.diff48.91 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] 32,965 pass(es), 58 fail(s), and 114 exception(es).
[ View ]

Comments

subscribe. this allows lolcat videos for user pics w00t

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?

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.

Assigned:Unassigned» moshe weitzman
Status:Active» Needs review
Issue tags:-Media Sprint 2011+API change, +API clean-up
StatusFileSize
new48.91 KB
FAILED: [[SimpleTest]]: [MySQL] 32,965 pass(es), 58 fail(s), and 114 exception(es).
[ View ]

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.

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.

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.

Status:Needs review» Needs work

cross-posting

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.

StatusFileSize
new50.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pic_0.diff. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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.

Title:Make user pictures fieldsConvert user pictures to Image Field
StatusFileSize
new50.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pic_1.diff. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new48.83 KB
FAILED: [[SimpleTest]]: [MySQL] 33,172 pass(es), 23 fail(s), and 80 exception(es).
[ View ]

try to satisfy bot patch apply. no changes.

StatusFileSize
new48.83 KB
FAILED: [[SimpleTest]]: [MySQL] 33,148 pass(es), 23 fail(s), and 80 exception(es).
[ View ]

Re-upload per bizarre PIFT msg.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new48.83 KB
FAILED: [[SimpleTest]]: [MySQL] 33,181 pass(es), 23 fail(s), and 42 exception(es).
[ View ]

Fix test fail, I hope.

Status:Needs review» Needs work

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

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

StatusFileSize
new51.34 KB
FAILED: [[SimpleTest]]: [MySQL] 33,218 pass(es), 23 fail(s), and 42 exception(es).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new52.19 KB
FAILED: [[SimpleTest]]: [MySQL] 33,184 pass(es), 5 fail(s), and 0 exception(es).
[ View ]

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.

StatusFileSize
new52.21 KB
FAILED: [[SimpleTest]]: [MySQL] 33,212 pass(es), 23 fail(s), and 42 exception(es).
[ View ]

Fixed a unit test fail.

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.

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

Status:Needs work» Needs review
StatusFileSize
new54.58 KB
FAILED: [[SimpleTest]]: [MySQL] 33,743 pass(es), 27 fail(s), and 42 exception(es).
[ View ]

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.

Assigned:moshe weitzman» Unassigned

perhaps someone else can finish this

Assigned:Unassigned» sun

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

Status:Needs work» Needs review
StatusFileSize
new43.73 KB
FAILED: [[SimpleTest]]: [MySQL] 34,498 pass(es), 48 fail(s), and 43 exception(es).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new46.23 KB
FAILED: [[SimpleTest]]: [MySQL] 34,463 pass(es), 45 fail(s), and 5 exception(es).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new46.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.user-picture-field.36.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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.

StatusFileSize
new45.83 KB
FAILED: [[SimpleTest]]: [MySQL] 34,490 pass(es), 45 fail(s), and 1 exception(es).
[ View ]

Re-rolled against latest HEAD.

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.

Issue tags:+Media Initiative

Issue tags:+Favorite-of-Dries

Tagging. I like.

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.

Status:Needs work» Needs review
StatusFileSize
new50.06 KB
FAILED: [[SimpleTest]]: [MySQL] 35,250 pass(es), 53 fail(s), and 5 exception(s).
[ View ]

Rerolled. Uploading to see how it performs.

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

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

Status:Needs work» Needs review
StatusFileSize
new14.82 KB
new43.8 KB
FAILED: [[SimpleTest]]: [MySQL] 35,237 pass(es), 37 fail(s), and 3 exception(s).
[ View ]
  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.

Status:Needs work» Needs review
StatusFileSize
new20.26 KB
new57.17 KB
FAILED: [[SimpleTest]]: [MySQL] 35,169 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new6.43 KB
new57.28 KB
FAILED: [[SimpleTest]]: [MySQL] 35,186 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

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

StatusFileSize
new3.5 KB
new58.62 KB
PASSED: [[SimpleTest]]: [MySQL] 35,299 pass(es).
[ View ]

Final.

StatusFileSize
new2.43 KB
new59.44 KB
FAILED: [[SimpleTest]]: [MySQL] 35,290 pass(es), 0 fail(s), and 5 exception(s).
[ View ]

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

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

StatusFileSize
new59.54 KB
FAILED: [[SimpleTest]]: [MySQL] 44,076 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Prevent a division by zero in the update function.

StatusFileSize
new59.47 KB
FAILED: [[SimpleTest]]: [MySQL] 44,103 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Incorporated @Dave Reid's suggestions. Thanks!

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new60.28 KB
PASSED: [[SimpleTest]]: [MySQL] 35,286 pass(es).
[ View ]

whoopsie - endless loop in that update function ;)

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.

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?

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.

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.

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

StatusFileSize
new58.11 KB
new49.89 KB
new66.08 KB
new70.2 KB
new20.04 KB

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.

StatusFileSize
new2.9 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 57vs64.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new61.55 KB
FAILED: [[SimpleTest]]: [MySQL] 34,943 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new64.56 KB
FAILED: [[SimpleTest]]: [MySQL] 36,417 pass(es), 0 fail(s), and 11 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.3 KB
new64.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.user-picture-field.68.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed those remaining exceptions.

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.

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.

StatusFileSize
new64.9 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.user-picture-field.69.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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.

Lets see what the bot thinks!

Assigned:sun» kendramyers
Issue tags:+tcdrupal2012

assigned to myself to test manually

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new64.89 KB
FAILED: [[SimpleTest]]: [MySQL] 36,580 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new940 bytes
new65.24 KB
PASSED: [[SimpleTest]]: [MySQL] 36,579 pass(es).
[ View ]

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

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

Locally no fails

Status:Needs review» Reviewed & tested by the community

Thanks Tim. Back to RTBC.

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

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?

Status:Reviewed & tested by the community» Needs review

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?

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

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.

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.

Assigned:kendramyers» tim.plunkett
Status:Needs work» Needs review
StatusFileSize
new4.9 KB
new65.63 KB
PASSED: [[SimpleTest]]: [MySQL] 36,627 pass(es).
[ View ]

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.

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.

StatusFileSize
new65.59 KB
PASSED: [[SimpleTest]]: [MySQL] 36,617 pass(es).
[ View ]

Whoops.

StatusFileSize
new65.59 KB
PASSED: [[SimpleTest]]: [MySQL] 36,779 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

Looks pretty solid to me. RTBC

Status:Reviewed & tested by the community» Needs work

Still CNW for the two points in #96

Status:Needs work» Needs review
StatusFileSize
new115.88 KB
PASSED: [[SimpleTest]]: [MySQL] 36,771 pass(es).
[ View ]
new69.73 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
new109.72 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new4.17 KB

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.

StatusFileSize
new69.73 KB
PASSED: [[SimpleTest]]: [MySQL] 36,793 pass(es).
[ View ]
new65.87 KB
FAILED: [[SimpleTest]]: [MySQL] 36,776 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

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

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.

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?

Assigned:Unassigned» tim.plunkett

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

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?

Assigned:tim.plunkett» Unassigned
StatusFileSize
new71.92 KB
PASSED: [[SimpleTest]]: [MySQL] 36,782 pass(es).
[ View ]

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.

StatusFileSize
new71.07 KB
FAILED: [[SimpleTest]]: [MySQL] 36,795 pass(es), 0 fail(s), and 8 exception(s).
[ View ]
new2.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.

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

Status:Needs work» Needs review
StatusFileSize
new71.09 KB
FAILED: [[SimpleTest]]: [MySQL] 36,753 pass(es), 35 fail(s), and 8 exception(s).
[ View ]

Duh. Thanks aspilicious.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new71.3 KB
PASSED: [[SimpleTest]]: [MySQL] 36,774 pass(es).
[ View ]
new1.07 KB

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

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

And now it just needs some real manual testing.

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

Note that the names in #116 are reversed.

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

Status:Needs work» Needs review
Issue tags:+Configuration system, +Platform Initiative
StatusFileSize
new71.08 KB
PASSED: [[SimpleTest]]: [MySQL] 39,633 pass(es).
[ View ]

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

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

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.

Assigned:Unassigned» sun
StatusFileSize
new1.71 KB
new71.04 KB
PASSED: [[SimpleTest]]: [MySQL] 39,635 pass(es).
[ View ]

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.

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.

Status:Needs review» Reviewed & tested by the community

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

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.

Assigned:sun» tim.plunkett

I want to get this done.

Status:Needs work» Needs review
StatusFileSize
new71.62 KB
PASSED: [[SimpleTest]]: [MySQL] 40,633 pass(es).
[ View ]

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

StatusFileSize
new4.35 KB
new71.73 KB
FAILED: [[SimpleTest]]: [MySQL] 40,645 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new67.55 KB
FAILED: [[SimpleTest]]: [MySQL] 41,914 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Just rerolling to see how it looks to the bot now

StatusFileSize
new71.73 KB
FAILED: [[SimpleTest]]: [MySQL] 41,928 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

Missed new files.

groan, sorry about the tags

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new73.9 KB
FAILED: [[SimpleTest]]: [MySQL] 42,165 pass(es), 3 fail(s), and 2 exception(s).
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new74.21 KB
FAILED: [[SimpleTest]]: [MySQL] 41,950 pass(es), 3 fail(s), and 3 exception(s).
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new74.4 KB
FAILED: [[SimpleTest]]: [MySQL] 42,052 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new466 bytes
new73.53 KB
FAILED: [[SimpleTest]]: [MySQL] 42,408 pass(es), 2 fail(s), and 574 exception(s).
[ View ]

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.

Only 2 fails. Go pcambra and dagmar!

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.

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

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: Rewrite batch api to be more....sane would allow it?

Oh so many problems ;)

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

double post.

Status:Needs work» Needs review
StatusFileSize
new1.94 KB
new74.54 KB
FAILED: [[SimpleTest]]: [MySQL] 42,746 pass(es), 0 fail(s), and 307 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new679 bytes
new75.2 KB
FAILED: [[SimpleTest]]: [MySQL] 45,970 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new92.57 KB
PASSED: [[SimpleTest]]: [MySQL] 45,991 pass(es).
[ View ]

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

StatusFileSize
new80.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-picture-field-1292470-159.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

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

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?

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.

awesomeness! good job Berdir:D

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.

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new80.47 KB
PASSED: [[SimpleTest]]: [MySQL] 46,175 pass(es).
[ View ]

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

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

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.

Status:Reviewed & tested by the community» Needs work

Opps, xpost.

Status:Needs work» Needs review

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

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

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.

Status:Needs review» Reviewed & tested by the community

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

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

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.

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new95.56 KB
new42.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?

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new7.09 KB
new81.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-picture-field-1292470-186.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new4.61 KB
new84.47 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new77.52 KB
FAILED: [[SimpleTest]]: [MySQL] 46,343 pass(es), 0 fail(s), and 12 exception(s).
[ View ]

Fixed the annotion, no other changes.

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.

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

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

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.

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

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.

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

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

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.

Status:Needs work» Needs review
StatusFileSize
new2.63 KB
new82.05 KB
FAILED: [[SimpleTest]]: [MySQL] 48,166 pass(es), 4 fail(s), and 16 exception(s).
[ View ]

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.

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

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

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.

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

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.

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

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

First one sounds general, second is caused by the patch

Status:Needs work» Needs review
StatusFileSize
new8.9 KB
new83.63 KB
FAILED: [[SimpleTest]]: [MySQL] 48,539 pass(es), 0 fail(s), and 12 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review

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

Status:Needs work» Needs review
StatusFileSize
new962 bytes
new84.57 KB
Test request sent.
[ View ]

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

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

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

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.

StatusFileSize
new2.2 KB
new85.13 KB
Test request sent.
[ View ]

Thanks, nice catch!

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

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

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!

Status:Reviewed & tested by the community» Fixed

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

Title:Convert user pictures to Image FieldChange 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.

Status:Active» Needs review
StatusFileSize
new962 bytes
PASSED: [[SimpleTest]]: [MySQL] 48,763 pass(es).
[ View ]

patch to revert random tests workaround

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

Priority:Major» Normal
StatusFileSize
new1.49 KB
PASSED: [[SimpleTest]]: [MySQL] 48,814 pass(es).
[ View ]

Fixing phpdoc as well and demoting to normal

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.

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

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.

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.

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.

Change notice updated, feel free to correct/improve it

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?

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

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.

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

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

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

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