Coming from #1292470: Convert user pictures to Image Field. We need to lock the user picture field so it doesn't get deleted.

There's another interesting question though: the user picture field is not created in user.install but in standard.install, this means we need to inform people who create custom install profiles they need to call _user_install_picture_field(). Wondering whether we shouldn't move this into user.install anyway, but that's maybe for a follow up.

CommentFileSizeAuthor
#1 1859352-1.patch496 bytesswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Active » Needs review
FileSize
1859352-1.patch496 bytes

And the patch.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Makes sense.

In another patch, I'd be fine with moving field creation to user.install as proposed. The swappability of user picture is going to come from changing formatter, and not installing an alternate Field.

swentel’s picture

swentel CreditAttribution: swentel commented

Interestingly enough, calling _user_install_picture_field() in user_install() doesn't work as field_config table doesn't exist yet, so let's wait after we moved field api to CMI.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Reviewed & tested by the community » Needs review

We need to lock the user picture field so it doesn't get deleted.

Why must it not get deleted?

The fact it is just added in standard.profile means that we can't rely on it's existence anyway. We would have to do it in user.install if that's the case. Making locked but not moving it to user.install alone wouldn't really help. Tests for example could not rely on it.

I see that it's problematic to not be able rely on it, but if we have to, then this might require more discussion and adding the lock alone doesn't really change much. It doesn't guarantee that it exists, it just guarantees that once it does, you can't remove it again ;)

swentel’s picture

swentel CreditAttribution: swentel commented

All true, the lock is only a first step I guess.

There are hardcoded properties in the theme settings like node_user_picture and comment_user_picture so we kind of need be sure it's always there, or add checks to make sure those properties don't popup on the theme settings. However, right now, the only way to make sure it really exists is adding it after the field_config tables exist, so at this point hook_field_install() is the only reliable place to install it.

It also feels like we need some kind of lock on 'display settings' too, because it's easy now to put something completely different in the compact mode instead of only the user picture.

   // From template_preprocess_node().
   // To change user picture settings (e.g. image style), edit the 'compact'
   // view mode on User entity.
   $variables['user_picture'] = user_view($node->account, 'compact');

We maybe need a hook for default fields and default display settings/view modes :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Hmm $variables['user_picture'] feels wrong in general.

Really it should just be $variables['account'] no? And then we could stick username in there too for now. And people can rely on there being $node->account or $comment->account because there's always a uid field property on those entities.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

We can't Fatal if this field does not exist but I think we have to rely on it to provide rich experience like pictures on content and so on. I think marking the Field as locked is a good next step. We can strengthen the meaning of Locked if we want in a separate issue.

It also feels like we need some kind of lock on 'display settings' too, because it's easy now to put something completely different in the compact mode instead of only the user picture.

Thats by design. Folks are welcome to customize this View Mode for their needs and show username, bio, etc. along with a picture. Thats why the View Mode is called Compact and not Picture.

#6 - there is a patch for that at #1857324: Use default theme name and entity key in $build for user entity.

yched’s picture

yched CreditAttribution: yched commented

I'd tend to agree with @Berdir #4. Preventing the deletion of something provided by standard.profile feels weird. Standard profile is about providing base defaults, not locking you with them. (although, well, 'locked' only prevent deletion through the UI, deletion is still possible through CRUD API)

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

I think there is consensus to moving the picture Field creation to user.install. Swentel reports in #3 that we are blocked on that until Field API moves to CMI. So, I propose we add the Locked in this issue and do the move to user.install in a follow-up. OK?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work

I don't think there's consensus on that at all. user pictures could be disabled since at least 6.x if not much earlier, enforcing the user field on user accounts seems completely wrong - what if you want it on a profile entity instead?

No-one responded to #6 yet either, if account is always available, but not necessarily picture, then is there a problem? The variable is already misnamed anyway.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs work » Postponed

If #1853378: Optimize user_view('compact') in template_preprocess_node() goes in as proposed, we won't need a lock at all. It will be totally valid to move/delete the picture Field.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.0.x-dev » 8.1.x-dev
Issue summary: View changes
Related issues: +#2274433: Do not allow to alter Locked field via UI

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi as a volunteer commented
Status: Postponed » Closed (won't fix)
Issue tags: +user picture

There is no longer a reason to lock/prevent deletion of user picture field now that the field is optional.

The field is not present on minimal profile installs.