Set realistic image upload limits for user picture field

All of this sounds reasonable.

1) I don't see why we need to change the size of the image on upload. What is the purpose in this? Even if we set it to 100 x 100 it may not match the image style if a site builder decides to override it. My vote would be for no upload resizing. Maybe a very large maximum so someone malicious doesn't try to upload some kind of crazy large image that will crash things.

2) 30 Kb is definitely too small, maybe 1 MB is better?

3) Assuming #1 is acceptable, I think we can stick to the thumbnail image style as the default.

I'm down to patch this once we have agreement.

These settings likely date back multiple core version, certainly way before there were image styles in core.

Back then, these limitations made a lot of sense. Today, they no longer do and we can probably simply remove them. The image size is still limited by whatever the maximum is your server allows.

And if you're building sites with user profiles there is a high chance that you want your profile images to be in a decent quality.

To remove them, we should probably make sure that we always use an image style for the default places where it's displayed.

Both the comment and node modules actually provide the user pictures by rendering the 'compact' (used to be called 'teaser') display of the user entity before passing to the template, so I think we're safely displaying the picture using an image style everywhere it gets displayed.

Attached patch removes the restrictions and corrects a little typo a few lines down. Tested it, works fine.

I think I'll open up a separate issue about how 'user pictures' can actually contain any number of different fields, since checking the 'show user pictures with posts' box in your theme settings actually displays a user entity's compact view mode. Seems like we need a name change.

Checked the user display fields and it's also using the 100 x 100 px thumbnail there. The only weird thing there is that it links to content, which in case of a user profile/page is the same page. Not sure if we want to clean that up here as well or not.

Also not sure about the upgrade path but I assume we keep the existing settings there and don't try to be too intelligent.

I think it could be argued that 'link to content' should be the standard case for the user entity, so perhaps we should leave that out for now.

This patch replaces the upgrade path defaults with the new default settings as well.

Discussed this with berdir in IRC and decided that the default link to 'content' is not right, so this patch removes that as well.

#6: drupal_user_picture_1930934_6.patch queued for re-testing.

This makes sense to me.

But I think we want to keep a maximum allowed filesize; this can and will be abused by users otherwise — something along the lines of the suggested 1 MB sounds sensible, although that's pretty much already... We should orient ourselves at typical image file sizes of today's cameras.

We definitely want to do this, but it's definitely also not "major". :) (As a general rule of thumb, anything that is tagged as Novice cannot really be major ;))

Sounds reasonable...a full res iphone or 8 megapixel android phone picture can sometimes be greater than 1 MB. I'd suggest 2MB, which I think is the standard upload limit for stock php anyway. A high res photo from a DSLR might exceed this, but for a profile field that defaults to 100 x 100, I think its acceptable to force that user to re-size the file.

This can of course be overridden by the site builder to whatever they desire.

Anyone else have opinions?

Looks good to me.

Ok, I think those are sane defaults, it takes care of both new installations and upgrade (uses the new default value if nothing has been explicitly configured and the configured values otherwise).

Looks good to go to me.

#10: drupal_user_picture_1930934_10.patch queued for re-testing.

This looks good overall, but one quick thing. We should take the lesser of 2MB vs. php.ini's "upload_max_filesize" variable. It's possible for this to be set to 1MB on a server and then Drupal barfs in interesting ways. (It's unlikely it would be set to < 30K, so this wasn't really required before.)

Good point.

Let's do that literally though; i.e.:

$limit = min(2048000, file_upload_max_size());
$limit = ($limit > 1024 ? format_size($limit) : 0);
// or perhaps rather this?
$limit = $limit / DRUPAL_KILOBYTE . ' KB';

// ...
  'max_filesize' => $limit,

Hows this

+++ b/core/modules/user/user.installundefined
@@ -332,10 +332,10 @@ function user_install_picture_field() {
-      'max_filesize' => '30 KB',
+      'max_filesize' => '2 MB',

Shouldn't we be taking the system configuration into account here as well?

+++ b/core/modules/user/user.installundefined
@@ -718,6 +718,10 @@ function user_update_8011() {
+  // Use 2 MB as a default user picture file size, without exceeding the configured PHP
+  // limit.

Minor: Looks like this comment is wrapping a bit too late. It should wrap as close to 80 chars total (including the spaces at the beginning) without going over.

Duplicate the same chunk of code?

wtf, netbeans decided to put the little 80 character line where it felt like it

bullets

Novice issue cleanup.

I'm at the sprint in Austin taking a look at it...

The patch in #19 did not apply cleanly.

+1 for this, the 30kb limit is really odd in todays web, in fact its just weird for most users to hit this very, very low limit.

I think this is a major WTF to be frank, absolutely no other web service out there expects average users to be restricted to 30kb.

Now that this configuration is a simple yaml file in the installation profile the patch becomes quite trivial.

Note that we are losing the arbitrary limitation of uploaded images to 2mb, although that is a common default php configuration so in most case there is no change for the final limitation.

Patch from #24 no longer applies, needs a simple re-roll.

Did any body tried just configure user picture field in acount settings as i did? No issue appeared after configuration. Yes i've got it at first before configuring the field settings but after that everything working fine. so why we need that patch ? Am i missing somesting?

Sure, you can change it. This is just about changing the default so you don't have to do that on every site you use this feature on.

Just re-uploading the patch, still applies but wasn't tested before.

This is consistent with the image field on articles, so if we can get away without a max filesize there, I think we are good here as well.

Yes the limit isn't relevant any more with image derivatives, so agreed on just removing it.