Some time ago I've built a patch (#594592: Human readable preset names) for ImageCache that adds the option to give presets (now "image styles") a human readable name (e.g. "Thumbnail (100x100)").
I totally forgot this patch and didn't remember to build this for D7 image.module.

The names are used in lists (e.g. field formatter info) instead of the namespace values.

Upgrade path and tests are #1464554: Upgrade path for image style configuration

Files: 
CommentFileSizeAuthor
#204 606598-core7-port-imagestyles-204.patch29.98 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,077 pass(es).
[ View ]
#203 606598-core7-port-imagestyles-203.patch4.99 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 606598-core7-port-imagestyles-203.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#203 interdiff-197-203.txt4.99 KBDavid_Rothstein
#197 606598-core7-port-imagestyles-197.patch25.94 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,347 pass(es).
[ View ]
#192 image-field-settings.png44.31 KBBarisW
#192 image-field-display-settings.png30.39 KBBarisW
#191 606598-core7-port-imagestyles-191.patch27.27 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,423 pass(es).
[ View ]
#191 interdiff-183-191.txt955 bytesDavid_Rothstein
#187 Screen Shot 2013-04-04 at 10.22.05 AM.png102.38 KBtreksler
#184 Before patch in #18359.12 KBtreksler
#184 After patch in #18387.24 KBtreksler
#183 606598-core7-port-imagestyles-182.patch26.67 KBBarisW
PASSED: [[SimpleTest]]: [MySQL] 40,295 pass(es).
[ View ]
#183 interdiff-176-182.txt1.67 KBBarisW
#176 606598-core7-port-imagestyles-176.patch26.38 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,249 pass(es).
[ View ]
#176 interdiff-175-176.txt1.3 KBDavid_Rothstein
#175 606598-core7-port-imagestyles-175.patch26.19 KBBarisW
PASSED: [[SimpleTest]]: [MySQL] 40,293 pass(es).
[ View ]
#175 interdiff-169-175.txt3.23 KBBarisW
#169 606598-core7-port-imagestyles-169.patch26.82 KBBarisW
PASSED: [[SimpleTest]]: [MySQL] 40,054 pass(es).
[ View ]
#169 interdiff-167-169.txt1.2 KBBarisW
#167 606598-core7-port-imagestyles-167.patch25.82 KBBarisW
FAILED: [[SimpleTest]]: [MySQL] 40,044 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#167 interdiff-161-167.txt3.58 KBBarisW
#161 606598-core7-port-imagestyles-161.patch23.73 KBBarisW
PASSED: [[SimpleTest]]: [MySQL] 39,685 pass(es).
[ View ]
#161 interdiff-606598-156-161.txt2.03 KBBarisW
#160 interdiff-606598-156-159.txt2.1 KBSutharsan
#159 606598-core7-port-imagestyles-159.patch23.64 KBBarisW
PASSED: [[SimpleTest]]: [MySQL] 39,776 pass(es).
[ View ]
#156 606598-core7-port-imagestyles-156.patch23.57 KBdanielbeeke2
PASSED: [[SimpleTest]]: [MySQL] 39,741 pass(es).
[ View ]
#155 manualcrop.png111.64 KBBarisW
#152 606598-core7-port-imagestyles-152.patch23.11 KBBarisW
PASSED: [[SimpleTest]]: [MySQL] 39,623 pass(es).
[ View ]
#150 606598-core7-port-imagestyles-150.patch23.08 KBBarisW
PASSED: [[SimpleTest]]: [MySQL] 39,874 pass(es).
[ View ]
#146 606598-core7-port-imagestyles-146.patch22.65 KBBarisW
PASSED: [[SimpleTest]]: [MySQL] 39,646 pass(es).
[ View ]
#130 606598-imagestyles-before-list.jpg4.5 KBandypost
#130 606598-imagestyles-before-edit.jpg10.81 KBandypost
#129 606598-imagestyles-list.jpg30.46 KBandypost
#129 606598-imagestyles-edit.jpg43.86 KBandypost
#128 606598-core8-imagestyles-128.patch22.12 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 41,338 pass(es).
[ View ]
#128 606598-interdiff-123vs128.txt2.42 KBandypost
#123 606598-core8-imagestyles-123.patch22.1 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 41,221 pass(es).
[ View ]
#123 606598-interdiff-121vs123.txt1.69 KBandypost
#121 606598-core8-imagestyles-121.patch20.87 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 41,216 pass(es), 35 fail(s), and 2 exception(s).
[ View ]
#116 drupal8.image-style-label.116.patch21.71 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.image-style-label.116.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#114 drupal8.image-style-label.114.patch21.33 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 39,797 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#110 drupal8.image-style-label.110.patch20.49 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.image-style-label.110.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#110 interdiff.txt608 bytessun
#109 drupal8.image-style-label.108.patch19.74 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,257 pass(es), 3 fail(s), and 1 exception(s).
[ View ]
#109 interdiff.txt1.84 KBsun
#107 drupal8.image-style-label.107.patch20.19 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,259 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#104 606598-imagestyles_label-104.patch17.7 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 36,670 pass(es).
[ View ]
#101 606598-imagestyles_label-101.patch17.92 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 36,671 pass(es).
[ View ]
#99 606598-imagestyles_label-99.patch17.87 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 606598-imagestyles_label-99.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#96 606598-imagestyles_label-96.patch17.88 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 36,614 pass(es).
[ View ]
#94 606598-imagestyles_label-94.patch16.42 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 36,590 pass(es), 1 fail(s), and 16 exception(s).
[ View ]
#91 606598-imagestyles_label.patch17.04 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 33,345 pass(es).
[ View ]
#90 606598-imagestyles_label.patch16.02 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 33,342 pass(es).
[ View ]
#89 606598-imagestyles_label.patch15.04 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 33,358 pass(es).
[ View ]
#87 606598-imagestyles_label.patch12.33 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 33,288 pass(es), 55 fail(s), and 9 exception(es).
[ View ]
#52 606598-imagestyle_realnames-d7.patch10.86 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,844 pass(es).
[ View ]
#37 606598-imagestyle_realnames-37.patch11.54 KBjoachim
PASSED: [[SimpleTest]]: [MySQL] 22,787 pass(es).
[ View ]
#35 606598-imagestyle_realnames-35.patch11.61 KBstBorchert
PASSED: [[SimpleTest]]: [MySQL] 18,968 pass(es).
[ View ]
#33 606598-imagestyle_realnames-33.patch11.61 KBstBorchert
PASSED: [[SimpleTest]]: [MySQL] 18,955 pass(es).
[ View ]
#28 606598-01.png3.73 KBsgabe
#28 606598-02.png21.33 KBsgabe
#24 image-style-config.png48.61 KBandypost
#21 606598-imagestyle_realnames-21.patch12.16 KBstBorchert
PASSED: [[SimpleTest]]: [MySQL] 18,737 pass(es).
[ View ]
#15 606598-imagestyle_realnames-15.patch11.77 KBstBorchert
Passed: 14717 passes, 0 fails, 0 exceptions
[ View ]
#12 606598-imagestyle_realnames-12.patch11.54 KBstBorchert
Passed: 14679 passes, 0 fails, 0 exceptions
[ View ]
#10 606598-imagestyle_realnames-10.patch8.35 KBstBorchert
Passed: 14728 passes, 0 fails, 0 exceptions
[ View ]
#4 Image styles | drupal7.localhost_1255796852878.png61.59 KBduvien
#3 Screenshot-323.jpg26.86 KBeigentor
#3 Screenshot-322.jpg52.48 KBeigentor
#2 image-style_realnames-3.patch8.16 KBstBorchert
Failed: Invalid PHP syntax in modules/image/image.field.inc.
[ View ]
image-style_realnames.patch8.13 KBstBorchert
Failed: 13958 passes, 1 fail, 155 exceptions
[ View ]
image-style_realnames-field_format.png72.99 KBstBorchert
image-style_realnames-locked_style.png103.58 KBstBorchert
image-style_realnames-list.png49.71 KBstBorchert
image-style_realnames-edit_style.png94.16 KBstBorchert
image-style_realnames-account_settings.png42.48 KBstBorchert

Comments

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.16 KB
Failed: Invalid PHP syntax in modules/image/image.field.inc.
[ View ]

Retry with some minor modifications.

StatusFileSize
new52.48 KB
new26.86 KB

UX Review of the patch:
Works as advertised, very nice. No longer guessing about image sizes when selecting.... Screenshots from admin/config/media/image-styles (holy crap, we gotta relearn paths :) ) and the display fields page:

I applied the patch on a clean install and this is what i get. Missing style names and error:

Notice: Undefined index: realname in theme_image_style_list() (line 652 of /Applications/MAMP/htdocs/drupal7/modules/image/image.admin.inc).

Running on MAMP 1.8.2

PHP 5.2.10
Apache 2
mySQL 5.1.37

@duvien:
Which one did you test? I tried with http://drupal.org/files/issues/image-style_realnames-3.patch and it works without any problems.

Here are the steps I did for testing:
* empty database
* clean checkout
* apply patch
* installed default profile
* admin/config/media/image-styles/
--> no errors or warnings

Did a clean CVS checkout added some dummy content but did not touch any imagecache settings. Tested a few patches:

Missing help button - http://drupal.org/node/607106

URL aliases: http://drupal.org/node/606888

Then decided to test this patch as i was going to start playing with image cache settings. I did add a new image field for blog content-type before applying this patch.

@duvien: please apply the patch bevor installing anything else.

This is odd, i tried it on a clean checkout of Drupal 7 then activated default profile before applying the patch image-style_realnames-3.patch (trying to follow your own setup)

But still getting same error?

This sounds like a great idea to me. Especially since we have "default" styles now that cannot be renamed (but the user should be able to change the display name).

StatusFileSize
new8.35 KB
Passed: 14728 passes, 0 fails, 0 exceptions
[ View ]

@quicksketch: good idea! Here it is.

Now you can rename the display name of a default style (after you click on "override").

Just from looking at the code, here's few suggestions:

- Instead of "realname" we should use "label", since what you consider "real" depends on your perspective (end-user or developer). It's also consistent with other places in Drupal where "name" is the machine name and "label" is what's shown to the user.
- We should use the auto-naming mechanism provided by core like when you create a new content type. As you type the Style label, it should "guess" an appropriate machine name for you. I'm not sure how this works currently, but I know it's reusable since we do this in several places in Drupal.

StatusFileSize
new11.54 KB
Passed: 14679 passes, 0 fails, 0 exceptions
[ View ]

Ok, here is a new version:
* "label" instead of "realname"
* namespace is generated automatically using the standard Drupal methods

happy testing

Haven't checked in all situations but generally looks good. Got one objection, imho namespace is the wrong term here because iiuc, machine-readable name is what is really meant. Namespace on the contrary is - as the word suggests - a space - i.e. a container - for names.... ?
Another thing indirectly related: on the style overview page, the displayed breadcrumb is fine. If you choose to edit a single preset however, breadcrumb's gooone. Surely a known issue, isn't it?

The breadcrumb issue is #483614: Better breadcrumbs for callbacks, dynamic paths, tabs (breadcrumbs just don't work on callbacks or dynamic paths, the same problem exists in D6). Switching to "machine-readable" as the terminology is probably a good call also, to keep consistent with other terminology in Drupal.

StatusFileSize
new11.77 KB
Passed: 14717 passes, 0 fails, 0 exceptions
[ View ]

Updated the patch with "machine readable name" instead of "namespace".

thx sketch good to know it's being taken care of already - OSS what a joy *lol*

Nice idea. This will be an improvement to the UI.
(I don't really care because I'm a coder and I already think in lower_case_with_underscores already, but it does make a prettier screenshot :-B )

Status:Needs review» Reviewed & tested by the community

bump it RTBC..

this would be a real nice one to have...

Status:Reviewed & tested by the community» Needs review

+++ modules/image/image.module 26 Oct 2009 21:33:31 -0000
@@ -468,6 +474,11 @@ function image_style_load($name = NULL,
+  if (isset($style)) {
+    if ($style['label'] == '') {
+      $style['label'] = $style['name'];
+    }
+  }

This is the same as empty() ?

+++ modules/image/image.module 26 Oct 2009 21:33:31 -0000
@@ -571,7 +582,9 @@ function image_style_options($include_em
+  foreach ($styles as $name => $style) {
+  $options[$name] = $style['label'];
+  }

Wrong indentation.

This review is powered by Dreditor.

StatusFileSize
new12.16 KB
PASSED: [[SimpleTest]]: [MySQL] 18,737 pass(es).
[ View ]

Ok, changed the code based on your notes and fixed a small bug.

I noticed that the creation of machine readable names didn't work in overlay (the field is not hidden). Can you confirm this in general or is it a problem with some part of my code?

Subscribe

#21: 606598-imagestyle_realnames-21.patch queued for re-testing.

Status:Needs review» Needs work
StatusFileSize
new48.61 KB

Patch applies with some offsets

Works only a defaullt Image style

Config screen brings error (screenshot attached)

Status:Needs work» Needs review

Uhm, I've tested the patch in #606598-21: Human readable image-style names and it applies with no fuzz. Additionally I couldn't produce any errors while viewing, adding or modifying any presets (default or custom).
@andypos: Which version of Drupal 7 did you use? Did you've done a clean checkout from cvs?

@stBorchert yes

@andypost: Did you apply the patch before installing?
Please describe the exact way how you got this error.

Status:Needs review» Needs work
StatusFileSize
new21.33 KB
new3.73 KB

I can confirm andypost's observation.

  1. Checked out Drupal 7 HEAD from CVS.
  2. Applied the patch in #21.
  3. Installed Drupal.
  4. Created new styles.

With default styles it works fine, but the custom styles have no dimensions in their names.

This is awesome anyway, I will try this out for 6.x for sure.

Status:Needs work» Needs review

With default styles it works fine, but the custom styles have no dimensions in their names.

Uhm, this is because you did not enter the dimensions.

The name (nor any part of it) isn't generated for you. You have to name the preset as you like. For the 3 default styles the names are pre-defined (and I decided to put the dimensions into the name).

This is *not* an error.

+++ modules/image/image.admin.inc 14 Dec 2009 17:40:26 -0000
@@ -71,11 +86,34 @@ function image_style_form($form, &$form_
       '#size' => '64',
-      '#title' => t('Image style name'),
+      '#title' => t('Image style machine readable name'),
...
+      '#size' => 64,
+      '#title' => t('Image style display name'),
@@ -228,14 +269,40 @@ function image_style_form_submit($form,
     '#size' => '64',
-    '#title' => t('Style name'),
+    '#title' => t('Image style machine readable name'),
...
+    '#size' => 64,
+    '#title' => t('Image style display name'),

Why do we specify a #size here? The value looks pretty large - can we omit it?

+++ modules/image/image.install 14 Dec 2009 17:40:28 -0000
@@ -43,11 +43,18 @@ function image_schema() {
-        'description' => 'The style name.',
+        'description' => 'The styles machine readable name.',

"The machine readable name of the style."

+++ modules/image/image.module 14 Dec 2009 17:40:31 -0000
@@ -376,7 +382,7 @@ function image_path_flush($path) {
-
+  ¶
@@ -449,7 +459,7 @@ function image_styles() {
-
+  ¶

Trailing white-space introduced here.

Powered by Dreditor.

@stBorchert: Well then, we should consider to integrate that in this feature. I'm not the last one who would expect this behavior for sure. it's OK!

Also style label should be translatable

+  foreach ($styles as $name => $style) {
+    $options[$name] = $style['label'];
+  }

StatusFileSize
new11.61 KB
PASSED: [[SimpleTest]]: [MySQL] 18,955 pass(es).
[ View ]

@sun: fixed

+++ modules/image/image.admin.inc 27 Mar 2010 22:53:51 -0000
@@ -46,6 +46,21 @@ function image_style_form($form, &$form_
+          'searchPattern' => '[^a-z0-9\\-_]+',

Is the range from \ to _ intended here? The hyphen (-) should come last in the character set.

+++ modules/image/image.module 27 Mar 2010 22:53:53 -0000
@@ -321,6 +321,8 @@ function image_image_default_styles() {
   $styles['thumbnail'] = array(
+    'name' => 'thumbnail',
@@ -331,6 +333,8 @@ function image_image_default_styles() {
   $styles['medium'] = array(
+    'name' => 'medium',
@@ -341,6 +345,8 @@ function image_image_default_styles() {
   $styles['large'] = array(
+    'name' => 'large',
@@ -392,6 +398,7 @@ function image_styles() {
         foreach ($module_styles as $style_name => $style) {
           $style['name'] = $style_name;
+          $style['label'] = ($style['label'] == '') ? $style['name'] : $style['label'];

Actually, the 'name' key is superfluous here and can be removed. The array key of the style definition is used as the internal machine name.

If this logic doesn't work everywhere yet, then we want to adjust the remaining locations, as duplicating the machine name wouldn't make sense.

137 critical left. Go review some!

StatusFileSize
new11.61 KB
PASSED: [[SimpleTest]]: [MySQL] 18,968 pass(es).
[ View ]

"Is the range from \ to _ intended here?"
Uhm, _ doesn't need to be replaced (because it would be replaced by itself).
Don't know where I get this pattern from but I guess it was intended, yes.

"The hyphen (-) should come last in the character set."
Fixed.

"Actually, the 'name' key is superfluous here and can be removed."
I couldn't get this work for real. The name is used in several locations (e.g. image_style_save() or image_style_delete()) and I don't see a way how to remove it from there ...

+++ modules/image/image.module 28 Mar 2010 13:19:49 -0000
@@ -341,6 +345,8 @@ function image_image_default_styles() {
   $styles['large'] = array(
+    'name' => 'large',

What I meant is that we can leave out the declaration in hook_image_default_styles().

http://api.drupal.org/api/function/image_styles/7 automatically assigns the array key as $style['name']

136 critical left. Go review some!

StatusFileSize
new11.54 KB
PASSED: [[SimpleTest]]: [MySQL] 22,787 pass(es).
[ View ]

Reroll of the patch in #35 with the change from #36.

Seems like all open issues have been resolved, unless questioned by sun or something else on the code. I say we should move this to RTBC

Status:Needs review» Reviewed & tested by the community

Downloaded and tested the patch, it applies and works well. Setting to RTBC.

Status:Reviewed & tested by the community» Needs work

I think this is a great idea. But, unless I'm mistaken, it's still not RTBC. Try placing a hyphen in your label (and, thus, your machine name). Images created by that image style have a mangled path and won't display.

It looks like the culprit is the preg_match() in image_field_formatter_view()

<?php
 
if (preg_match('/__([a-z0-9_]+)/', $display['type'], $matches)) {
   
$image_style = $matches[1];
  }
?>

which (if the $display['type'] has a hyphen in it) will created a truncated $image_style and (eventually) an incorrect image path.

E.g.,

A label of "example(300x300)-more to this label" has a machine name of "example_300x300_more_to_this_label".

But the initial path created is /image/generate/example_300x300/public/IMAGE_NAME.jpg which will send back a 404.

Not sure if the answer is to:

a) disallow hyphens in the label
b) modify the preg_match()

I should add: the above problem appears to exists in HEAD as well. So it should be fixed regardless of whether this patch is committed to core.

Status:Needs work» Reviewed & tested by the community

=> separate issue

+1
Go for it. I guess we do not expect this patch to break core badly.
Now would be a better time for committing than three days before beta due date.

Issue tags:-Usability, -#d7ux

#37: 606598-imagestyle_realnames-37.patch queued for re-testing.

Issue tags:+Usability, +#d7ux

#37: 606598-imagestyle_realnames-37.patch queued for re-testing.

Suppose this patch should be a bit changed after #812688: Organize image formatters around settings

Status:Reviewed & tested by the community» Needs review

#47 most definetly yes.

Status:Needs review» Reviewed & tested by the community

Folks, that patch is not even RTBC. This patch is.

Priority:Normal» Major

Suppose, we need commit this before beta

#37: 606598-imagestyle_realnames-37.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs review
Issue tags:+beta blocker
StatusFileSize
new10.86 KB
PASSED: [[SimpleTest]]: [MySQL] 22,844 pass(es).
[ View ]

Re-roll as commited #812688: Organize image formatters around settings

-Fixed image_update_7000()

Also This should be done before beta

EDIT: head2head #883152: Add: Human readable image-style names

subscribe

Status:Needs review» Reviewed & tested by the community

Still looks good to go for me.

Issue tags:-beta blocker

Also This should be done before beta

I agree, it "should" be in the beta, but it doesn't have to be. I see no reason this issue should block the beta release.

Issue tags:+beta blocker

Well, it changes the UI, so if this goes in D7, it should do so before beta. Re-tagging.

Priority:Major» Critical
Issue tags:+API change

Issue tags:-API change

Oops...could have sworn I saw an API change in this issue. I guess I miss read "UI" on my iphone.

Category:feature» task

Priority:Critical» Major

That it requres an API/interface change doesnt make it critical.

Priority:Major» Critical

If this is a beta blocker then it must be critical.

There are 6 or so beta blockers that need to get fixed in the next week or two.

Category:task» feature
Priority:Critical» Normal

Can someone explain why on earth this is a beta blocker and not a nice to have?

Its not a beta blocker, its a nice to have. And its been RTBC since April, so people are trying elevate its importance - I see no reason why it shouldn't be committed though.

April was well after feature freeze. I see no reason this shouldn't be moved to D8, unless I'm missing something obvious?

Just for the record: the first time it has been RTBC was in November 2009 :)
#606598-18: Human readable image-style names

I don't see it as a critical task either. This is (and was) a simple feature request for better user experience. Not more.

Issue tags:-beta blocker

Reverting back #55...

Version:7.x-dev» 8.x-dev

I agree with #64, this is a nice feature but it's too late for Drupal 7. For more discussion about the release cycle, please see http://drupal.org/node/927792#comment-3515044

Would this be doable as a contrib module for D7?

Image module oldskool needs this feature, so if anything does this, keep me posted :)

No, unfortunately this isn't doable with contrib.

To bad; another issue discussed to death.

@joachim: Wait. What? Is that ("Image module oldskool needs this") a qualified and verified requirement? If that is really the case and cannot be solved differently, then this issue goes back against D7. However, that's only the case if you can make a clear and strong case. (Sorry, I'm slightly out of the loop with current state of Image module affairs...)

Go @joachim! Make the case!

IMHO, and off the loop, it would fix the problem with styles changing names.

The problem:
1. Unexperienced admin uses the style name 65x40.
2. Eventually the design says you need it to be 60x45, so you change the cropping.
3. The name no longer reflects the size of it, so you change it to something more semantic "small-thumbnail"
4. You have to go around everywhere you specified this style, and reselect the new name.

I assume getting this feature in would solve this problem, as the machine name would be there unchanged, but you could change the name the user chooses. At least I hope it would.

> @joachim: Wait. What? Is that ("Image module oldskool needs this") a qualified and verified requirement?

Just to be clear we're on the same page: by 'Image module oldskool' I mean the D7 port of functionality found in the D6 contrib package 'image'.

Image D6 provides the user with a system to create named presets. The user enters a label for each image preset. This is not a machine name but a human-readable label: it preserves case and spaces. These labels are then used to create the node links on the image node which show the image at its different sizes. Furthermore, the user can change these labels at any time.

To replicate this functionality in D7, I imagined we'd use the human-readable names this patch provides. Showing the image style machine names will be ugly and also the user can't ever change these.

If this patch doesn't get in, Image oldskool would have to implement a UI to allow the user to choose labels for presets -- effectively adding this feature in a corner of contrib.

This issue was RTBCed while code-slush phase (before polish phase!!!) and ignored near year. Now we trolling about "useness"? Again just wasting a time...

Edit:
Sad to loose this great functionality in D7...

Are the image styles exportable?

Version:8.x-dev» 7.x-dev

We have no consistency in machine-readable name implementation so lets wait for #902644: Machine names are too hard to implement. Date types and menu names are not validated

Previously, there was a patch in head2head for this issue: #883152: Add: Human readable image-style names

Since beta1 is now out, I believe this patch either needs to incorporate the head2head update, or be bumped to D8.

Version:7.x-dev» 8.x-dev

I D8ed this once already. Let's do it again.

andypost: code slush phase was for specifically exempted features, and this was not one of them. It'll be a nice usability improvement in D8, though.

Version:8.x-dev» 7.x-dev

This is very usability bug. If I create and test a small patch, commited to Drupal 7? Let it be a usable Drupal 7! :)

Version:7.x-dev» 8.x-dev

There is no usability bug here. This is merely a missing feature, and adding it would invalidate documentation and screenshots.

@webchick is there any chances for that issue to be backported to 7 after D7 release?

Noticed this immediately. +1 for D7

+1 for D7, subbing...

subscribe

Status:Reviewed & tested by the community» Needs work
Issue tags:-Usability, -#d7ux+Needs usability testing

There is a schema change here but no upgrade path, so it needs re-rolling to add that. It will also be postponed on #1182290: Add boilerplate upgrade path tests in terms of actually getting committed since all upgrade path tests were removed from Drupal 8 without being replaced, so we have zero coverage for any newly added updates.

@joachim, is this still holding up an image module port to D7? Apart from that it doesn't look like a viable backport to me unless there is an explicit change to allow backports of UI enhancements like this.

> @joachim, is this still holding up an image module port to D7

Yup -- image contrib D6 has human-readable labels, so if these are not in D7 core we'll have to implement them in contrib for image contrib D7.

I also just discovered Image Styles were human readable in D6 and are no more in D7 - so get on with this. I have to rename all my image styles to lowercase and no special chars :P

Status:Needs work» Needs review
StatusFileSize
new12.33 KB
FAILED: [[SimpleTest]]: [MySQL] 33,288 pass(es), 55 fail(s), and 9 exception(es).
[ View ]

patch #52 is much outdated so here is a new one

changes
1) usage of machine_name element
2) more replacements for $style[label] in UI
3) image_update_8000() for schema change

Status:Needs review» Needs work

The last submitted patch, 606598-imagestyles_label.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new15.04 KB
PASSED: [[SimpleTest]]: [MySQL] 33,358 pass(es).
[ View ]

Tests should be fixed

StatusFileSize
new16.02 KB
PASSED: [[SimpleTest]]: [MySQL] 33,342 pass(es).
[ View ]

image_style_name_validate() is not used now

StatusFileSize
new17.04 KB
PASSED: [[SimpleTest]]: [MySQL] 33,345 pass(es).
[ View ]

+++ modules/image/image.module 14 Aug 2010 15:24:01 -0000
@@ -607,7 +617,9 @@ function image_style_options($include_em
-  $options = array_merge($options, drupal_map_assoc(array_keys($styles)));
+  foreach ($styles as $name => $style) {
+    $options[$name] = $style['label'];
+  }

Added this missed hunk for image_style_options() with small test

Am I correct in understanding that if this patch succeeds and is committed, the Core Image module will have the ability to give image styles Human-readable names? If so, that's great, I've been hoping that would happen, because that will make the UI a lot nicer.

Status:Needs review» Needs work

This patch require re-work because image-styles are converted to config storage in flawour of CMI #1479652: Convert image style config filenames/keys from plural to singular

Status:Needs work» Needs review
StatusFileSize
new16.42 KB
FAILED: [[SimpleTest]]: [MySQL] 36,590 pass(es), 1 fail(s), and 16 exception(s).
[ View ]

New patch, mostly re-roll

Status:Needs review» Needs work
Issue tags:+Usability

+++ b/core/modules/image/image.admin.inc
@@ -49,13 +48,23 @@ function image_style_form($form, &$form_state, $style) {
+      'replace_pattern' => '[^a-z0-9-_]+',
@@ -215,16 +225,25 @@ function image_style_form_submit($form, &$form_state) {
+      'replace_pattern' => '[^a-z0-9-_]+',

If the deviation from the default is intended, there should be a comment about that, i.e.: "Allow "-" in machine names."

Also the previous regular expression hat the "-" escaped ("\-"), I don't know if that is needed. Somewhere above someone said "-" should be the last character in the brackets.

+++ b/core/modules/image/image.admin.inc
@@ -215,16 +225,25 @@ function image_style_form_submit($form, &$form_state) {
+      //'replace' => '-',

??

+++ b/core/modules/image/image.module
@@ -550,6 +550,10 @@ function image_style_load($name) {
+  // Backward compatibility with styles without labels.
+  if (empty($style['label'])) {
+    $style['label'] = $style['name'];
+  }

For D8 there should simply be an upgrade path that sets the machine name as label. There is some mention of an upgrade path above, but that seems to have gotten lost.

Also image_style_options() should be converted. (That was also in earlier patches.)

Status:Needs work» Needs review
StatusFileSize
new17.88 KB
PASSED: [[SimpleTest]]: [MySQL] 36,614 pass(es).
[ View ]

Fixed broken test and image_style_options()

Changed replace_pattern to previously used and commented.

+++ b/core/modules/image/image.admin.inc
@@ -240,29 +259,16 @@ function image_style_add_form($form, &$form_state) {
-  // Check for illegal characters in image style names.
-  if (preg_match('/[^0-9a-z_\-]/', $element['#value'])) {
-    form_set_error($element['#name'], t('Please only use lowercase alphanumeric characters, underscores (_), and hyphens (-) for style names.'));

Previous implementation allows usage of hyphens so I leave it as is

Upgrade path is out of scope this patch, so I added a @todo to remove the BC in when CMI upgrade for styles lands. see #1464554: Upgrade path for image style configuration

Status:Needs review» Reviewed & tested by the community

Couldn't find anything to complain about. Was RTBC before so must be RTBC now. :)
Btw, I just saw that #91 had a small test, but I don't know if that's necessary.

Status:Reviewed & tested by the community» Needs work

YAML it is, so this needs a new patch

Status:Needs work» Needs review
StatusFileSize
new17.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 606598-imagestyles_label-99.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

.. another re-roll, now with YAML ...99 comemnts

Status:Needs review» Needs work

The last submitted patch, 606598-imagestyles_label-99.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new17.92 KB
PASSED: [[SimpleTest]]: [MySQL] 36,671 pass(es).
[ View ]

now in -p1 format

Status:Needs review» Reviewed & tested by the community

RTBC again.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/image/config/image.style.large.yml
@@ -2,6 +2,7 @@ name: large
effects:
     image_scale_480_480_1:
         name: image_scale
+        label: Large (480x480)

All of the labels in the default config files are in the wrong spot. The label is for the image style, not an effect within, so the label property has to be on the top-level.

Status:Needs work» Needs review
StatusFileSize
new17.7 KB
PASSED: [[SimpleTest]]: [MySQL] 36,670 pass(es).
[ View ]

@sun, thanx for review. BC fix hides this trouble...

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

Thanks! Looks better. Reviewed this once more in depth:

+++ b/core/modules/image/image.admin.inc
@@ -32,10 +32,9 @@ function image_style_list() {
-  $title = t('Edit %name style', array('%name' => $style['name']));
+  $title = t('Edit style %name', array('%name' => $style['label']));

This looked suspicious, but I double-checked and indeed we're using the "Edit $thing $name" page title pattern everywhere else in Drupal.

+++ b/core/modules/image/image.admin.inc
@@ -49,13 +48,24 @@ function image_style_form($form, &$form_state, $style) {
+    '#title' => t('Image style display name'),
...
+    '#description' => t('Enter the display name for this style. This name is used in listings of image styles.'),

Shouldn't we simply call the #title: "Administrative label" ?

With that #title, the entire #description looks like obsolete clutter to me.

(btw, it doesn't make sense to repeat "Image style" in #title here, since we're already in a dedicated form for an image style.)

+++ b/core/modules/image/image.admin.inc
@@ -49,13 +48,24 @@ function image_style_form($form, &$form_state, $style) {
+      // Allow hyphen ("-") in machine names.
+      'replace_pattern' => '[^a-z0-9_\-]+',

Why do we allow hyphens in image style machine names?

If it is really only because we supported hyphens before, then I'd highly prefer to adjust the upgrade path instead (in the separate issue) to convert all hyphens into underscores during the upgrade, which will have to rewrite many other things for image styles either way.

+++ b/core/modules/image/image.admin.inc
@@ -215,16 +226,25 @@ function image_style_form_submit($form, &$form_state) {
function image_style_add_form($form, &$form_state) {

A separate add form is totally superfluous. If we don't merge that form within this patch, then I'd like to see a follow-up issue for doing so.

+++ b/core/modules/image/image.module
@@ -553,6 +553,11 @@ function image_style_load($name) {
+  // @todo Remove BC when CMI upgrade path lands.
+  // Backward compatibility with styles without labels.
+  if (empty($style['label'])) {
+    $style['label'] = $style['name'];
+  }

This does not make sense to me. :)

- We do not have BC code in core.

- The only possible BC in this case is for existing D8 sites. If you want to make those sites happy, then the proper thing to do is to write a module update that loads all configured image styles, sets a label, and saves them. (which you can happily do, but [IMO, sadly] isn't strictly required)

- There is no BC for D7, because there is no upgrade path in the first place (see separate issue).

So, in any case, there are the above two options to deal with this, but permanent BC code in this location is not one of them. ;)

+++ b/core/modules/image/image.test
@@ -1141,8 +1150,6 @@ class ImageDimensionsScaleTestCase extends UnitTestBase {
   function testImageDimensionsScale() {
...
-    $test = array();

Let's revert this unrelated test change.

Status:Needs work» Needs review
StatusFileSize
new20.19 KB
FAILED: [[SimpleTest]]: [MySQL] 37,259 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Plain re-roll against HEAD, not incorporating #106 yet.

Status:Needs review» Needs work

The last submitted patch, drupal8.image-style-label.107.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.84 KB
new19.74 KB
FAILED: [[SimpleTest]]: [MySQL] 37,257 pass(es), 3 fail(s), and 1 exception(s).
[ View ]

Attached patch incorporates #106.

Merging the add form into the edit form is left for a separate follow-up issue:
#1675952: Merge image_style_add_form() into image_style_form()

StatusFileSize
new608 bytes
new20.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.image-style-label.110.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed the test exception.

Status:Needs review» Reviewed & tested by the community

Let's get this in and continue with image effects #1508872: Image effects duplicate on edit

Status:Reviewed & tested by the community» Needs work
Issue tags:+Usability, +Needs usability testing

The last submitted patch, drupal8.image-style-label.110.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new21.33 KB
FAILED: [[SimpleTest]]: [MySQL] 39,797 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

fixing conflict

Status:Needs review» Needs work

The last submitted patch, drupal8.image-style-label.114.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new21.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.image-style-label.116.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And this should fix the last exception :)

Status:Needs review» Reviewed & tested by the community

back to rtbc, once again

Status:Reviewed & tested by the community» Needs work
Issue tags:+Usability, +Needs usability testing

The last submitted patch, drupal8.image-style-label.116.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new20.87 KB
FAILED: [[SimpleTest]]: [MySQL] 41,216 pass(es), 35 fail(s), and 2 exception(s).
[ View ]

Re-roll #116

Status:Needs review» Needs work

The last submitted patch, 606598-core8-imagestyles-121.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.69 KB
new22.1 KB
PASSED: [[SimpleTest]]: [MySQL] 41,221 pass(es).
[ View ]

Fixed broken test

Status:Needs review» Reviewed & tested by the community

Thanks for re-rolling and keeping this alive!

I re-reviewed this patch once more and still think it is ready to fly.

Oh, please let it fly.

- open since 2009
- missed D7 because discussed to death
- blocking #594592: Human readable preset names and thus a stable 6.x-2.0 of ImageCache

...lets not talk this to death too till December.

PS: if this is not back-portable to D7 but #594592: Human readable preset names gets fixed, then we have a case where functionality in D6 (with contrib) was lost in D7 and re-introduced in D8 (core). So, if this is not even doable in D7 contrib (not even Backports/Fix Core/Edge), then WTF?! Right?

StatusFileSize
new2.42 KB
new22.12 KB
PASSED: [[SimpleTest]]: [MySQL] 41,338 pass(es).
[ View ]

Another re-roll after #1175764: Have theme('image_style') inject the style name as a class

Changed hunk

     // Create a style.
-    image_style_save(array('name' => 'test'));
-    $url = image_style_url('test', $original_uri);
+    image_style_save(array('name' => 'image_test'));
+    $url = image_style_url('image_test', $original_uri);
     $path = $this->randomName();
     $element = array(
       '#theme' => 'image_style',
-      '#style_name' => 'test',
+      '#style_name' => 'image_test',
       '#uri' => $original_uri,

StatusFileSize
new43.86 KB
new30.46 KB

Bojhan does review mane times, so here a current screens for D8

And shots before the patch, all other places are in issue summary

Still looks ok

We're over thresholds atm so I unfortunately can't commit any feature patches. Help with smashing critical and major bugs would be greatly appreciated.

@webchick I find that really disappointing, this request has been around since 2009 and it's been finished for a while. stborchert tried to get this in for Drupal 7 and now it's not getting in for Drupal 8 even though it's done and has been reviewed. I think not committing this only helps in stborchert not wanting to contribute much elsewhere either. Would appreciate you reconsidering your decision.

Having RTBCed this patch at least 3 or 4 times over the last couple years (!), I would agree that this patch would be one of those cases where an exception to the thresholds is viable. Of course, that is not for me to decide, and if it takes another two months for this, then so be it.

@aschiwi, thankfully commit thresholds are only temporary; once the number of critical/major bugs/tasks are under again, this will go in. And we're only over by 1 or 2 issues.

@tim.plunkett Oh thanks, that's great to hear :)

Status:Reviewed & tested by the community» Fixed

We're back under thresholds again. Woohoo!

Committed and pushed to 8.x.

Thanks everyone here for pulling this together. Something that totally should have been in the original image.module. Kudos and appreciation all around. :)

Committed and pushed to 8.x

Yeah!
Are we going to backport this to Drupal 7? The patch in #606598-52: Human readable image-style names needs some work then (and maybe we need an upgrade path).

Are we going to backport this to Drupal 7?

Can we please? ...if we're not breaking any API and all I mean.

re: backport: Nope.

A contrib module could try to inject it though. I don't really see why that shouldn't be possible.

Ok, thanx for taking the time to reply Daniel.

@evryonefollowing: if you either happen to code something or find a contrib module doing this, then please link to it here. Thanx in advance.

Status:Fixed» Closed (fixed)

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

Version:8.x-dev» 7.x-dev
Status:Closed (fixed)» Needs review
StatusFileSize
new22.65 KB
PASSED: [[SimpleTest]]: [MySQL] 39,646 pass(es).
[ View ]

Hi all,

I ported the patch to Drupal 7 and I really hope this can make it into D7.

The reason I want this is that several modules use Image style names to display them to an end-user. Take the Manual Crop module for example; in our case the end user has to choose an image style and sees image style names like "c_240_400" or "s_500". Not really helpful.

The patch does the following:

- Adds a Label column to the image_styles table
- Changes the 'name' textfield to a 'label' / 'name' combo in a 'system_name' field
- Updates all user strings to use the Label instead of the Name (without changing the strings, just the values)
- Updated the tests

Please review, and hopefully.. commit! :)

It looks possible:

  • no API changes
  • old values are valid
  • name field still there on form

Plus, I kept all strings intact. So instead of changing:

<?php
drupal_set_message
(t('Style %name was created.', array('%name' => $style['name'])));
?>

to

<?php
drupal_set_message
(t('Style %label was created.', array('%label' => $style['label'])));
?>

I kept it like this

<?php
drupal_set_message
(t('Style %name was created.', array('%name' => $style['label'])));
?>

By doing so, all existing translation strings keep working as well.

It is an API change because anyone using hook_image_default_styles() in contrib or in custom projects will need to update, that's extremely annoying, let alone everyone who's using features as well. However, it is indeed annoying those machine names, and I think with the right change sa backport should be possible.

- edit - removed my 'won't fix idea'

StatusFileSize
new23.08 KB
PASSED: [[SimpleTest]]: [MySQL] 39,874 pass(es).
[ View ]

Ah, true.

I've added a check to image_styles() to use the system name if the label is empty:

<?php
$style
['label'] = empty($style['label']) ? $style_name : $style['label'];
?>

Also, I've update the demo code in image.api.php.

+++ b/modules/image/image.admin.incundefined
@@ -61,20 +61,28 @@ function image_style_form($form, &$form_state, $style) {
+      '#title' => t('Administrative label'),

New label for translations - unless this is already used in core ?

+++ b/modules/image/image.testundefined
@@ -122,7 +122,7 @@ class ImageStylesPathAndUrlTestCase extends DrupalWebTestCase {
+    image_style_save(array('name' => $this->style_name, 'label' => $this->randomString()));

Another API change, although minor I guess, contrib or custom projects using this function will submit an empty label, although that's more or less now 'fixed' by the check.

StatusFileSize
new23.11 KB
PASSED: [[SimpleTest]]: [MySQL] 39,623 pass(es).
[ View ]

Great! Thanks for being thorough.

  • I've changed 'Administrative label' back to 'Image style name' (like it was) and removed the titles of the '#machine_name' fields.
  • I've also added a check in image_style_save() to use the system name when the label is empty.

This looks more backportable than I would have expected, but changing #type from 'textfield' to 'machine_name' is considered pretty intrusive (see http://drupal.org/node/1527558). There are also a lot of people above who have argued it shouldn't be backported...

I am sympathetic to it personally because it seems like an obvious omission and because I'm not sure it can be effectively done via a separate module (I think the main functionality definitely can, but all the drupal_set_message() calls in core would still be using the machine-readable name and those would be really difficult to override)? But it's clearly on the edge of what we can commit to Drupal 7 at this point, so it really needs more reviews and more agreement from people who have been involved in this issue.

Also, if anyone knows of any modules that heavily interact with image styles (particularly ones that might alter the image style admin forms themselves) it would be quite useful to test this patch with them and see how it behaves.

StatusFileSize
new111.64 KB

The funny thing about this issue is that the Image style Effects already use labels in core, only the Image styles themselves use the system name.

I've tested Imagecache Actions and Manual Crop, which still work fine. Even better; because all modules that use image_styles() to fetch the current list of styles now show a nicely formatted name, for instance the Manual Crop module (screenshot) instead of names like crop_200_400.

manualcrop.png

StatusFileSize
new23.57 KB
PASSED: [[SimpleTest]]: [MySQL] 39,741 pass(es).
[ View ]

I applied the patch from #152 it works fine for one small thing, I could not change a label of an existing image style. I could only change it when also changing the machine name.

I also tested the module Imagefield Focus and it works fine, nice labels instead of those machinenames.

Attached is a patch that includes the patch from #152 with a change so that it checks on the label and the name apart. Now it is possible to save an existing image style without changing the machine name.

Thanks for adding this danielbeeke. Looks good to me.

Now let's get this RTBC ;)

Status:Needs review» Needs work

Nice backport of a good Drupal 8 UX improvement, I'd love to see the coming to Drupal 7. However .... ;)
It is not possible to change the label of existing styles. This is only the case with pre-configured core styles, changing the label of a custom style is allowed. I guess this is existing behaviour to prevent breaking core's use of styles, but it should not longer apply now labels are used.

Edit: I guess this behaviour is introduced in #156. But IMO this is not acceptable, for example if I change the image size of an existing style, the label is no longer valid.

Status:Needs work» Needs review
StatusFileSize
new23.64 KB
PASSED: [[SimpleTest]]: [MySQL] 39,776 pass(es).
[ View ]

I fixed the issue in #158 by taking the $label out of if ($style['storage'] & IMAGE_STORAGE_MODULE)

This makes it possible to always change the label. For module-provided image styles, one can only change the label, for custom styles, one can change both the label and the system name.

Status:Needs review» Needs work
StatusFileSize
new2.1 KB

    // Update the image style name if it has changed.
-  if (isset($form_state['values']['name']) && $style['name'] != $form_state['values']['name']) {
+  $style = $form_state['image_style'];
+  if (isset($form_state['values']['name']) && isset($form_state['values']['label'])) {
     $style['name'] = $form_state['values']['name'];
-  }
-

What is the purpose of the if()? Both fields are required and are therefore set.
This is different than updating, when changed. The comment does not match anymore.

Note: Adding an interdiff to the issus is good practice to allow others to evaluate the changes.

Status:Needs work» Needs review
StatusFileSize
new2.03 KB
new23.73 KB
PASSED: [[SimpleTest]]: [MySQL] 39,685 pass(es).
[ View ]

Hmm, right. So I removed the if() because the fields always have a value.

Status:Needs review» Reviewed & tested by the community

I'm fine with the code, the UX and the upgrade path. As far as I can see there is minimal risk that this will break any contrib. The most popular image modules have been tested with this patch (by others: Image Field Focus, Imagecache Actions and Manual Crop) I've tested Imagestyle Flush, Insert and Image Field Crop. This is RTBC to me.

...so can we get this in please?

#161 looks good
+1 RTBC

Status:Reviewed & tested by the community» Needs work

This is a feature request and we're (way) over thresholds. I've been trying for like 3 months to figure out whether we can just commit things like this anyway, but no luck. It's frustrating :(

However... I think this might actually be a bug in disguise. More on that later...

Even so, the timing for committing it now is honestly not great. The Drupal 7.20 security release just came out and broke lots of things with image styles which are still being fixed. Although it's good that this one has been tested with several modules, right at the beginning of a release cycle would probably be a much better time to commit this.

I'm marking "needs work" for this at any rate:

-    '#description' => t('The name is used in URLs for generated images. Use only lowercase alphanumeric characters, underscores (_), and hyphens (-).'),
-    '#element_validate' => array('image_style_name_validate'),
...
+    '#machine_name' => array(
+      'exists' => 'image_style_load',
+      'source' => array('label'),
+    ),

Machine names don't allow hyphens by default, so this change will cause a problem for sites that have existing image styles with hypens in them. I think this can be solved by passing 'replace_pattern' (to use the same regular expression that was used before).

Category:feature» bug

The bug this is related to (which came up in discussion in #1923814: Existing hardcoded images can break after updating to Drupal 7.20 if image styles have been re-saved but is reproducible even before the Drupal 7.20 changes) is as follows:

  1. Insert an <img> tag with a hardcoded link to an image style inside a textarea in some content on your site (e.g. using the Insert module).
  2. Edit the image style and change its name.
  3. Your image won't be displayed anymore.

The patch here doesn't fix that directly, but makes it less likely since you can edit the human-readable name instead. Also, it provides a way to fix it for real later (disable editing an existing machine name) if we wanted to, which isn't really possible otherwise.

***

Beyond the above-mentioned issue with hyphens in the machine name, this looked pretty solid to me from looking through it and I didn't see any real showstoppers.

This seemed odd to me though:

   // Allow the name of the style to be changed, unless this style is
   // provided by a module's hook_default_image_styles().
   if ($style['storage'] & IMAGE_STORAGE_MODULE) {
     $form['name'] = array(
-      '#type' => 'item',
-      '#title' => t('Image style name'),
-      '#markup' => $style['name'],
-      '#description' => t('This image style is being provided by %module module and may not be renamed.', array('%module' => $style['module'])),
+      '#type' => 'value',
+      '#value' => $style['name'],
     );
   }

Why remove it from the user interface when it was there before?

I think the proper way to handle that is to use the same 'machine_name' type in all situations, but add a '#disabled' => $style['storage'] & IMAGE_STORAGE_MODULE property to it...

Some minor issues with documentation being out of date too (not showstoppers, but):

  1.   * @see image_style_name_validate()
      */
    function image_style_form($form, &$form_state, $style) {

    The @see is obsolete.

  2. * @return
    *   Array of image styles both key and value are set to style name.
    */
    function image_style_options($include_empty = TRUE) {

    Comment is no longer accurate?

  3. Maybe others?

Status:Needs work» Needs review
StatusFileSize
new3.58 KB
new25.82 KB
FAILED: [[SimpleTest]]: [MySQL] 40,044 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Hi David, thanks for the review. I've fixed all your points.

- I've added the replace_pattern on the #machine_name field, and also added a custom error message.
- I've applied the #disabled logic to the edit form, so that users can see the fixed machine name now.
- I've updated the docs.

Status:Needs review» Needs work

The last submitted patch, 606598-core7-port-imagestyles-167.patch, failed testing.

StatusFileSize
new1.2 KB
new26.82 KB
PASSED: [[SimpleTest]]: [MySQL] 40,054 pass(es).
[ View ]

Changing the string to a #disabled field caused the tests to fail.
Updated the tests to reflect this.

Thanks for some guidance here from chx.

Status:Needs work» Needs review

By the way; I removed the image_style_name_validate() in #167 as it isn't used anymore. Or isn't this allowed due to API changes?

"Right at the beginning of a release cycle would probably be a much better time to commit this."

I believe that's now, right?

So can we get it in? :)

Status:Needs review» Needs work

Thanks, that looks better.

I think image_style_name_validate() should be kept in Drupal 7, though (since it's a public function). How about just adding a code comment explaining why it's not used in core anymore?

Also, why does the new patch change translatable strings for #description and the #machine_name 'error'? I thought (maybe I remembered wrong) that the earlier patch didn't actually change any strings, which was a good thing and one of the reasons we could try to slip this in later than normal.

One more minor thing I noticed while looking through: Why does image_update_7005() define 'default' => '' for the new field when image_schema() doesn't?

In general, this looks very close to me. The core release last week was a minor one so there will probably be another one soon, right at the beginning of April. I think if this patch is ready to go in the next week (approximately) it could still get into Drupal 7.22, though.

Oh, also, any of the changes we're making since the Drupal 7 port started, if they're problems that need to be fixed in Drupal 8 too we should really get those committed as a Drupal 8 followup first (or at least be very sure they're ready to go around the same time). It should be pretty quick to get minor followups like that committed to Drupal 8 though.

Status:Needs work» Needs review
StatusFileSize
new3.23 KB
new26.19 KB
PASSED: [[SimpleTest]]: [MySQL] 40,293 pass(es).
[ View ]

Good catch.

I changed the description and error messages back to the ones currently being used. I removed default => '' from image_update_7005() to make them the same.

StatusFileSize
new1.3 KB
new26.38 KB
PASSED: [[SimpleTest]]: [MySQL] 40,249 pass(es).
[ View ]

Is anyone else planning to review/test this?

I looked it over and tried it out and got a PDOException running the update function. It seems the 'default' => '' was needed after all, so I added it back (but in both places). The update seems to work now, at least on MySQL.

I also fixed up the new code comment a tiny bit.

There seems to be a bug where the label of the built-in image styles is editable before the image style is overridden, but if you make changes to it and then click the "override" button your changes don't actually stick. (Only after it is overridden and you edit it later do they stick.) Not a huge bug though.

We also have to get the followup changes into Drupal 8. I rolled a patch and will put it in a new issue.

Status:Needs review» Reviewed & tested by the community

Is anyone else planning to review/test this?

yup

There seems to be a bug where the label of the built-in image styles is editable before the image style is overridden, but if you make changes to it and then click the "override" button your changes don't actually stick. (Only after it is overridden and you edit it later do they stick.) Not a huge bug though.

Looks like this possible needs follow-up?

#176 looks good for me

Status:Reviewed & tested by the community» Needs review

Sorry, but I have to ask: is the following a security risk?

@@ -768,10 +776,10 @@ function image_style_options($include_empty = TRUE) {
   if ($include_empty && !empty($styles)) {
     $options[''] = t('<none>');
   }
-  // Use the array concatenation operator '+' here instead of array_merge(),
-  // because the latter loses the datatype of the array keys, turning
-  // associative string keys into numeric ones without warning.
-  $options = $options + drupal_map_assoc(array_keys($styles));
+  foreach ($styles as $name => $style) {
+    $options[$name] = $style['label'];
+  }

$style['label'] can contain arbitrary HTML. There is no security risk if image_style_options() is used to construct a select list (which is its primary purpose) but if anyone is using it for checkboxes/radios/etc. there may be a problem.

Wondering if we should be safe and pass this through filter_xss_admin() or something like that. That's not really a standard way to do it (and for Drupal 8 we might get away with just documenting the function's purpose more clearly instead) but I think it may be reasonable in the middle of a stable release.

Other than that issue, this really looks ready to commit to me too. I guess we don't have to wait for anything currently in #1946580: Followups for human-readable image style names (machine-readable names with hyphens don't work correctly) to make it into Drupal 8 first because the D7-to-D8 update issue was already known and acknowledged when this was committed to Drupal 8 in the first place and we aren't changing anything with that here.

I think we could simply do a full check_plain() here. I think that's we usually do for select options.

I don't think so; form_select_options() runs check_plain() on it already, so this would result in a double check-plain.

Oh, you're right. I didn't realize that.

StatusFileSize
new1.67 KB
new26.67 KB
PASSED: [[SimpleTest]]: [MySQL] 40,295 pass(es).
[ View ]

Sorry for the delay, I was on a vacation :)

There seems to be a bug where the label of the built-in image styles is editable before the image style is overridden.

This is now fixed too.

About the issue with the labels not being filtered: the developer should take care of check_plain'ing radio labels, as in: http://www.dynamiteheads.com/blog/jakub-suchy/drupal-forms-api-security

Status:Needs review» Needs work
Issue tags:-Needs committer feedback+#d7ux
StatusFileSize
new87.24 KB
new59.12 KB

I applied the patch to the latest Drupal 7 and the patch kills some image image styes in the list completely. :(
See attachments.

sorry, didn't mean to alter issue tags, not sure what happened there

Hmm, can you give a bit more info? Did you clear the caches? Did you run update.php? Where do the missing image styles come from? A module? Features? It would really help if I know how to reproduce this.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new102.38 KB

Hey, they were custom image styles. The ones provided by modules worked right away. I ran update.php and cleared cache and everything works! I edited one of the image style names and it worked as expected. Hope to see this in 7.22

7.22 just shipped yesterday, unfortunately, so the earliest this would go out is 7.23 (assuming 7.23 isn't a security release). See http://drupal.org/documentation/version-info#when for timing.

Though #184 - #188 suggest that the release notes ought to warn people about a possible problem with custom image styles until the cache is cleared.

Did some more testing

scenarios:
1) vanilla Drupal 7 -> everything looks ok (except there are no custom labels)
2) apply patch, but don't clear cache or run update.php -> completely blank drop down
3) apply patch and clear cache, but no update.php -> everything but custom image styles show up (i.e. my original screenshot in #184)
4) apply patch and run update.php, but don't clear cache -> everything looks ok

so, it looks like running update.php is necessary and sufficient.
no warning about clearing cache separately seems to be needed.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new955 bytes
new27.27 KB
PASSED: [[SimpleTest]]: [MySQL] 40,423 pass(es).
[ View ]

About the issue with the labels not being filtered: the developer should take care of check_plain'ing radio labels, as in: http://www.dynamiteheads.com/blog/jakub-suchy/drupal-forms-api-security

Normally, yes, but there's no reason they would have had to do it in this case since the labels were previously guaranteed to be safe. I don't think we can assume people were doing it here if there was no security reason to do it before.

The attached patch and interdiff shows what I mean... does this make sense? (Again, this is for Drupal 7 only - for Drupal 8 we can just tidy up the documentation and/or add a change notification.)

StatusFileSize
new30.39 KB
new44.31 KB

Not sure if we want this, Drupal core double-escapes the values now.

image-field-settings.pngimage-field-display-settings.png

So either we have to change these dropdowns as well, or keep using #183

Hm, good point.

Well, another alternative (not a great one, but it would work) would be to leave image_style_options() alone and introduce a new (almost-duplicate) function which uses the labels instead. Code which wants to display the labels rather than the machine names in Drupal 7 would need to explicitly switch to using the new function.

That wouldn't work too, because if we introduce a new API function that outputs the labels, all contrib modules that are now available (like Manual Crop) all needed to be updated too.

With the patch in #183:
- Core works fine
- All contrib modules that were tested still work fine

Mind you, #183 was RTBC. I'd be very happy to not start discussing this solution again, as we missed a few D7 releases already.
Can we just get this in?

This issue has been running for three and a half years now.
Do I get it right that it was committed to D8 already and what is being discussed is a D7 backport?

So - if what Baris sais is right and everything works, could this be "good enough" for D7? I understand David is the D7 maintainer and a solid level of scrutiny should be applied to every- and anything that gets into core.

But seeing this from the outside, how can this be such big a deal. Sure, it must not break existing implementations, and what is more, no contrib that is relying on Image styles. But if that is taken care of...could we not just get it in.

A new function would just mean some inconsistency in the UI until contrib modules switched to using the new one... not ideal but not that big of a deal.

I would like to see more opinions on the security aspects here. I'll see if I can solicit some.

To recap the issue, image_style_options() currently produces output that's safe to insert directly in HTML, but with the patch in #183 it wouldn't anymore. So:

  • The reasons not to make that change to an API function in a stable release are hopefully obvious.
  • The reasons to consider it are that (a) this function is usually used for select lists where it's not an issue (I'm not sure we actually know any examples of it being used for radios/checkboxes), and (b) in practice, most sites trust people who have been given permission to administer image styles on their site (even though it's not on http://drupal.org/security-advisory-policy). Thus, it is quite likely that no actual security issue would result in real life. Combined with the fact that we need a change notification for this issue either way, we could just call it out loudly in the change notification and let it pass.

StatusFileSize
new25.94 KB
PASSED: [[SimpleTest]]: [MySQL] 40,347 pass(es).
[ View ]

Meanwhile, the patch in #183 no longer applied (lots of conflicts due to #1797328: Remove t() from assertion messages in tests for the image module).

Here is a reroll.

@David_Rothstein instead of always returning the unsafe string, how about a flag like we added to drupal_set_title in D7? The default return could be safe text still, but cases where it's used as select options, core at least could supply the added argument?

So the issue with that is that it's not just core but also contrib modules that are using this in select lists, and they would need to change to pass that flag too.

So in that sense it's similar to adding a new API function... the difference being (in the case where a module has not yet updated their code for the new API):

  1. New API function: Image style machine names would be shown exactly like they are now (rather than the new image style labels introduced in this issue).
  2. Flag on existing function: Image style labels would be shown, but they'd be double-escaped.

So which is better? I guess there's a good argument that the second is better; it's probably not too common that people will have a "&" in their labels anyway.

It sounds like a reasonable idea to me... thanks!

2. Would mean that bug reports show up in people's queues like '& does not work as image style label in yourmodule'. But given that the resolution to that bug is

-image_style_options()
+image_style_options(FALSE)

(or whatever) I personally think that would be acceptable, especially if we announce that as part of the release announcement.
I must say, however, that I do not personally maintain any image-related modules.

David, now we've passed the 200 comments (FWIW), can we go for #2: Flag on existing function?

It sounds like everyone who has commented agrees. So we just need an updated patch?

StatusFileSize
new4.99 KB
new4.99 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 606598-core7-port-imagestyles-203.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's an updated patch to move this along.

StatusFileSize
new29.98 KB
PASSED: [[SimpleTest]]: [MySQL] 40,077 pass(es).
[ View ]

Oops, I apparently uploaded the interdiff as the patch. Let's try again.

Status:Needs review» Reviewed & tested by the community

Thanks David,

great work. The patch applies nicely. I've tried creating several image styling with the weirdest names and everywhere in the interface they are being displayed as expected. After all the testing that's been done in the above 200 issues, I feel comfortable enough to change this to RTBC myself :)

Great work!

Yes! Lets get this more-than-3-years-standing issue fixed ;)

Title:Human readable image-style namesChange notice: Human readable image-style names
Category:bug» task
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record, +7.23 release notes, +7.23 release blocker

OK, looked it over one more time and I think it is ready. Great work everyone - thanks!! Committed to 7.x. http://drupalcode.org/project/drupal.git/commit/3201287

I believe this needs two change notices? One for Drupal 7 (the overall API change, plus the specific change to image_style_options() that added the PASS_THROUGH/CHECK_PLAIN option for the second parameter). And one for Drupal 8 (basically saying that image_style_options() no longer has that parameter and now always returns options that aren't appropriate for direct insertion into HTML).

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

Bump, since this is a release blocker for the next bugfix release of Drupal core - we need the change notification. Anyone game to write a first draft of it?

I'll also bump it to Drupal 8 to get more attention (since as mentioned above we need a Drupal 8 change notification too).

Issue tags:-needs backport to D7

Added an initial Drupal 7 change notification at https://drupal.org/node/2058503 to move this along and get a release out. It needs review.

I did not write one for Drupal 8.

Issue tags:+needs backport to D7

Didn't actually mean to remove that tag.

Change notice looks good to me.

Yeah for D7 this looks great. Nice one!

Thanks Cottser for pointing me here on IRC.

Drupal 8 image module needs to take into account these changes in the upgrade path. The current ImageUpgradePathTest passes only because it uses an drupal7.bare.standard_all.

I filed this as a critical follow-up bug report for Drupal 8: #2062341: Fix image upgrade path to work with image-style name backport.

Title:Change notice: Human readable image-style namesHuman readable image-style names
Priority:Critical» Normal
Status:Active» Fixed

We need update upgrade path tests and shipped database files in #2049465: Upgrade of image styles and effects broken
Files to update: drupal-7.filled.standard_all.database.php.gz and drupal-7.bare.standard_all.database.php.gz

Title:Human readable image-style namesChange notice: Human readable image-style names
Priority:Normal» Critical
Status:Fixed» Active

I don't see a Drupal 8 change notice yet.

Hm. Can you explain what we'd put in a D8 change notice? We'll probably be on Drupal 7.30+ by the time Drupal 8 ships, and only support upgrades from the most recent version of the previous release. This issue will have been old news by then, as it was introduced in 7.23.

Status:Active» Needs review

Just this, I think (see #208): https://drupal.org/node/2073933

In addition, the Drupal 8 patch removed the image_style_name_validate() function, but I don't suppose every single removed function needs to be documented in a change notice.

Title:Change notice: Human readable image-style namesHuman readable image-style names
Priority:Critical» Normal
Status:Needs review» Fixed

CN looks great!

Issue tags:-Needs change record

Now only upgrade path needs more eyes #2049465: Upgrade of image styles and effects broken

Status:Fixed» Closed (fixed)
Issue tags:-Usability, -#d7ux, -needs backport to D7, -7.23 release notes, -7.23 release blocker

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

Issue summary:View changes

Updated issue summary.