User picture dimensions not validated: Split user picture dimensions into two fields

user_picture_dimensions.patch queued for re-testing.

reroll of first patch, hopefully without errors

Looks good for me now.
Can someone else review this.

Still applies, rtbc

Agreed this is better

No idea

Does this patch introduce a dependency on image module (for user module)? _image_field_resolution_validate() is only available when image module is enabled AFAIK.

#14: user_picture_dimensions-14.patch queued for re-testing.

I'm away from my test copy of D7, but I just want to say that this should look identical to the FieldAPI image uploader.

If we can't do it in Drupal 7, there should at least be a @todo that this should be turned into a proper Image field in Drupal 8.

#21: 686528-user_picture_dimensions-21.patch queued for re-testing.

The patch seems to be working well on my setup and the code looks good (apply with offset).
Little reroll with a minor change. I reverted the default values back to 85x85 instead of 0x0.

+++ modules/user/user.admin.inc
@@ -397,14 +397,28 @@ function user_admin_settings() {
+    '#element_validate' => array('_user_picture_field_resolution_validate'),

+++ modules/user/user.module
@@ -3514,6 +3514,20 @@ function user_image_style_save($style) {
+ * Element validate function for user picture resolution field.
...
+function _user_picture_field_resolution_validate($element, &$form_state) {

To be consistent I think this function should be called _user_picture_field_max_resolution_validate (with MAX in the middle).
Also, is there really a need for the leading underscore here?

More importantly, if this is still on the table for D7, then we need an upgrade path, for people who already have the existing variable set to something other than the default. Should be nothing more than a variable_get() and two variable_set()'s.

Powered by Dreditor.

Needs work.

Here is a patch with the suggestions from #25. I'm not the sure about the explode and check in the update hook, maybe there is a better way to do this.

+++ modules/user/user.admin.inc
@@ -397,14 +397,28 @@ function user_admin_settings() {
-    '#description' => t('Pictures larger than this will be scaled down to this size.'),
+    '#description' => t('The maximum allowed image size (e.g. 85x85). Set to 0x0 for no restriction. If an <a href="!image-toolkit-link">image toolkit</a> is installed, files exceeding this value will be scaled down to fit.', array('!image-toolkit-link' => url('admin/config/media/image-toolkit'))),

While I personally like the text change, I don't think we can get this in D7, if we make this string change.

+++ modules/user/user.install
@@ -855,6 +855,23 @@ function user_update_7015() {
+
+  // There was no validation on this field. Assign default values if dimensions are invalid
+  if (count($dimensions) != 2) {

If there was no validation, I guess we should also be checking
is_numeric($dimensions[0]) && is_numeric($dimension[1])

+++ modules/user/user.install
@@ -855,6 +855,23 @@ function user_update_7015() {
+  variable_set('user_picture_max_resolution_x', $dimensions[0]);
+  variable_set('user_picture_max_resolution_y', $dimensions[1]);

Since the defaults haven't changed we should wrap this in a
if ($dimensions[0] != 85 && $dimensions[1] != 85) { }
so that people who haven't ever submitted the settings form don't get an unneeded variable in their DB.

+++ modules/user/user.install
@@ -855,6 +855,23 @@ function user_update_7015() {
+  variable_del('user_picture_dimensions');

Since this is still being used to generate the user pictures, this would be a regression for upgraders, as they would have to resubmit the settings page.

+++ modules/user/user.module
@@ -3514,6 +3514,20 @@ function user_image_style_save($style) {
+  form_set_value($element, intval($element['user_picture_max_resolution_x']['#value']) . 'x' . intval($element['user_picture_max_resolution_y']['#value']), $form_state);

Instead of checking is_numeric above and then doing intval() here, can we not simply do is_int() in the first place?

I'll see if I can reroll later.

Powered by Dreditor.

Something like this.

I moved the validation handler into user.admin.inc

Also, this still violates string freeze because of the error message. That's basically a bug fix, though. At least it seems unnecessarily fragile to not validate this.

An old CVS patch, how cute :)

This might be obsolete after #1292470: Convert user pictures to Image Field. We could move it back to D7 then. Not sure if we can change the UI there, but at least there should be validation to not allow any input.

Making a bug report and adding Needs tests.

User picture is a field, don't think we'll get this into D7