Fix user_picture field creation [#1860418]

Followup to #1292470: Convert user pictures to Image Field.

The creation of the user_picture field is made, in both
- fresh D8 install (standard.profile)
- and in hook_update_N() for upgraded D7 sites,
through a single _user_install_picture_field() helper function.

Both code paths should actually be using different APIs:
- field_create_field() / field_create_instance() within fresh installs
- _update_7000_field_create_field() / _update_7000_field_create_instance() within the upgrade path.

I hit this in #1852966: Rework entity display settings around EntityDisplay config entity, which widens the gap by moving field display settings to separate EntityDisplay config entities, that also need to be manipulated with different functions depending on the context (regular entity_load() / save() during install flow, direct CMI file manipulation during updates)

Comment	File	Size	Author
#11	user_picture_upgrade_fix-1860418-11.patch	10.59 KB	yched
#11
#6	user_picture_upgrade_fix-1860418-6.patch	10.85 KB	yched
#6
#6	interdiff.txt	635 bytes	yched
#1	user_picture_upgrade_fix-1860418-1.patch	10.83 KB	yched
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

yched CreditAttribution: yched commented 6 December 2012 at 19:18

Status:

Active

» Needs review

File	Size
user_picture_upgrade_fix-1860418-1.patch	10.83 KB

Patch attached.

- Inlines the code in user_update_8011()
- For standard_install(), still uses a separate _standard_create_user_picture_field() helper function within standard.install, that allows UserPictureTest to keep using the minimal profile, since it seems that was the original intention.

I'd personally tend to bite the bullet and inline the whole thing within standard_install() and have UserPictureTest run the standard profile (after all this is what we're testing, right ?), but well - opinions welcome :-)

Comment #2

yched CreditAttribution: yched commented 6 December 2012 at 19:21

Note : #1852966: Rework entity display settings around EntityDisplay config entity currently includes that fix (plus the code changes its own scope implies in this code area), but it seems it should be fixed in its own issue.
And the patch over there is big enough as is :-)

Comment #3

swentel CreditAttribution: swentel commented 6 December 2012 at 19:33

This is a tricky one, it has hit me hard in the Field API cmi conversion too (see #1735118: Convert Field API to CMI) - where there's an extreme racing condition between upgrading the user picture field, the conversion of the id column in the file usage and the user->data upgrade. We haven't seen the end of this one .. :)

Comment #4

yched CreditAttribution: yched commented 8 December 2012 at 14:07

#3 : yes, order of upgrades is going to need some special care between "Field API -> CMI", "$instance['display'] -> EntityDisplay", and updates that create some fields.

This is not strictly what this issue is about though. It's just about using the right helper function for the task depending on the context in which the code runs (install profile or upgrade function).

Should be an easy RTBC, btw ;-)

Comment #5

yched CreditAttribution: yched commented 8 December 2012 at 14:08

Title:

Fix user_picture initialisation

» Fix user_picture field creation

Better title

Comment #6

yched CreditAttribution: yched commented 10 December 2012 at 15:18

File	Size
interdiff.txt	635 bytes
user_picture_upgrade_fix-1860418-6.patch	10.85 KB

Spotted by @swentel in #1852966: Rework entity display settings around EntityDisplay config entity : Fixed 80 chars wrapping in a comment.

Comment #7

moshe weitzman CreditAttribution: moshe weitzman commented 10 December 2012 at 15:30

Status:

Needs review

» Reviewed & tested by the community

Wait for green before commit.

Comment #8

10 December 2012 at 16:02

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, user_picture_upgrade_fix-1860418-6.patch, failed testing.

Comment #9

yched CreditAttribution: yched commented 10 December 2012 at 16:25

Status:

Needs work

» Needs review

Er - really ?

#6: user_picture_upgrade_fix-1860418-6.patch queued for re-testing.

[edit: double checked - can't reproduce the fail locally]

Comment #10

sun

German

Karlsruhe

CreditAttribution: sun commented 10 December 2012 at 16:33

Hm. Can we move the code from standard.install into user.install/user.module?

That would allow other installation profiles to re-use the same code (as well as the test), without a dependency on the Standard profile.

Essentially, I'd expect user_add_picture_field() + user_add_picture_field_instance() functions, similar to node_add_body_field(), or more specifically, like the helper functions in #1751210: Convert URL alias form element into a field and field widget.

Comment #11

yched CreditAttribution: yched commented 10 December 2012 at 17:06

File	Size
user_picture_upgrade_fix-1860418-11.patch	10.59 KB

OK, moved to user.install.
If this is to allow other install profiles to use it, though, I don't see the point in separating helper functions for creation of the field and of the instance.