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

CommentFileSizeAuthor
#204 606598-core7-port-imagestyles-204.patch29.98 KBDavid_Rothstein
#203 606598-core7-port-imagestyles-203.patch4.99 KBDavid_Rothstein
#203 interdiff-197-203.txt4.99 KBDavid_Rothstein
#197 606598-core7-port-imagestyles-197.patch25.94 KBDavid_Rothstein
#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
#191 interdiff-183-191.txt955 bytesDavid_Rothstein
#187 Screen Shot 2013-04-04 at 10.22.05 AM.png102.38 KBtreksler
#184 Screen Shot 2013-04-03 at 2.28.27 PM.png59.12 KBtreksler
#184 Screen Shot 2013-04-03 at 2.29.19 PM.png87.24 KBtreksler
#183 606598-core7-port-imagestyles-182.patch26.67 KBBarisW
#183 interdiff-176-182.txt1.67 KBBarisW
#176 606598-core7-port-imagestyles-176.patch26.38 KBDavid_Rothstein
#176 interdiff-175-176.txt1.3 KBDavid_Rothstein
#175 606598-core7-port-imagestyles-175.patch26.19 KBBarisW
#175 interdiff-169-175.txt3.23 KBBarisW
#169 606598-core7-port-imagestyles-169.patch26.82 KBBarisW
#169 interdiff-167-169.txt1.2 KBBarisW
#167 606598-core7-port-imagestyles-167.patch25.82 KBBarisW
#167 interdiff-161-167.txt3.58 KBBarisW
#161 606598-core7-port-imagestyles-161.patch23.73 KBBarisW
#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
#156 606598-core7-port-imagestyles-156.patch23.57 KBdanielbeeke2
#155 manualcrop.png111.64 KBBarisW
#152 606598-core7-port-imagestyles-152.patch23.11 KBBarisW
#150 606598-core7-port-imagestyles-150.patch23.08 KBBarisW
#146 606598-core7-port-imagestyles-146.patch22.65 KBBarisW
#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
#128 606598-interdiff-123vs128.txt2.42 KBandypost
#123 606598-core8-imagestyles-123.patch22.1 KBandypost
#123 606598-interdiff-121vs123.txt1.69 KBandypost
#121 606598-core8-imagestyles-121.patch20.87 KBandypost
#116 drupal8.image-style-label.116.patch21.71 KBaspilicious
#114 drupal8.image-style-label.114.patch21.33 KBaspilicious
#110 drupal8.image-style-label.110.patch20.49 KBsun
#110 interdiff.txt608 bytessun
#109 drupal8.image-style-label.108.patch19.74 KBsun
#109 interdiff.txt1.84 KBsun
#107 drupal8.image-style-label.107.patch20.19 KBsun
#104 606598-imagestyles_label-104.patch17.7 KBandypost
#101 606598-imagestyles_label-101.patch17.92 KBandypost
#99 606598-imagestyles_label-99.patch17.87 KBandypost
#96 606598-imagestyles_label-96.patch17.88 KBandypost
#94 606598-imagestyles_label-94.patch16.42 KBandypost
#91 606598-imagestyles_label.patch17.04 KBandypost
#90 606598-imagestyles_label.patch16.02 KBandypost
#89 606598-imagestyles_label.patch15.04 KBandypost
#87 606598-imagestyles_label.patch12.33 KBandypost
#52 606598-imagestyle_realnames-d7.patch10.86 KBandypost
#37 606598-imagestyle_realnames-37.patch11.54 KBjoachim
#35 606598-imagestyle_realnames-35.patch11.61 KBstBorchert
#33 606598-imagestyle_realnames-33.patch11.61 KBstBorchert
#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
#15 606598-imagestyle_realnames-15.patch11.77 KBstBorchert
#12 606598-imagestyle_realnames-12.patch11.54 KBstBorchert
#10 606598-imagestyle_realnames-10.patch8.35 KBstBorchert
#4 Image styles | drupal7.localhost_1255796852878.png61.59 KBonejam
#3 Screenshot-323.jpg26.86 KBeigentor
#3 Screenshot-322.jpg52.48 KBeigentor
#2 image-style_realnames-3.patch8.16 KBstBorchert
image-style_realnames.patch8.13 KBstBorchert
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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

stBorchert’s picture

Status: Needs work » Needs review
FileSize
8.16 KB

Retry with some minor modifications.

eigentor’s picture

FileSize
52.48 KB
26.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:

onejam’s picture

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

stBorchert’s picture

@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

onejam’s picture

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.

stBorchert’s picture

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

onejam’s picture

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?

quicksketch’s picture

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).

stBorchert’s picture

@quicksketch: good idea! Here it is.

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

quicksketch’s picture

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.

stBorchert’s picture

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

happy testing

eMPee584’s picture

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?

quicksketch’s picture

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.

stBorchert’s picture

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

eMPee584’s picture

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

dman’s picture

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 )

eMPee584’s picture

Status: Needs review » Reviewed & tested by the community

bump it RTBC..

eigentor’s picture

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

sun’s picture

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.

stBorchert’s picture

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?

andypost’s picture

Subscribe

cosmicdreams’s picture

andypost’s picture

Status: Needs review » Needs work
FileSize
48.61 KB

Patch applies with some offsets

Works only a defaullt Image style

Config screen brings error (screenshot attached)

stBorchert’s picture

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?

andypost’s picture

@stBorchert yes

stBorchert’s picture

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

sgabe’s picture

Status: Needs review » Needs work
FileSize
21.33 KB
3.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.

stBorchert’s picture

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.

sun’s picture

+++ 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.

sgabe’s picture

@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!

andypost’s picture

Also style label should be translatable

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

@sun: fixed

sun’s picture

+++ 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!

stBorchert’s picture

"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 ...

sun’s picture

+++ 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!

joachim’s picture

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

Bojhan’s picture

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

drifter’s picture

Status: Needs review » Reviewed & tested by the community

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

CrookedNumber’s picture

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()

  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()

CrookedNumber’s picture

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.

sun’s picture

Status: Needs work » Reviewed & tested by the community

=> separate issue

CrookedNumber’s picture

eigentor’s picture

+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.

andypost’s picture

Issue tags: -Usability, -#d7ux
andypost’s picture

Issue tags: +Usability, +#d7ux
andypost’s picture

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

Manuel Garcia’s picture

Status: Reviewed & tested by the community » Needs review

#47 most definetly yes.

sun’s picture

Status: Needs review » Reviewed & tested by the community

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

andypost’s picture

Priority: Normal » Major

Suppose, we need commit this before beta

sun’s picture

andypost’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +beta blocker
FileSize
10.86 KB

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

rfay’s picture

subscribe

sun’s picture

Status: Needs review » Reviewed & tested by the community

Still looks good to go for me.

philbar’s picture

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.

tstoeckler’s picture

Issue tags: +beta blocker

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

philbar’s picture

Priority: Major » Critical
Issue tags: +API change
philbar’s picture

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.

philbar’s picture

Category: feature » task
Bojhan’s picture

Priority: Critical » Major

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

philbar’s picture

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.

webchick’s picture

Category: task » feature
Priority: Critical » Normal

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

Bojhan’s picture

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.

webchick’s picture

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

stBorchert’s picture

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.

philbar’s picture

Issue tags: -beta blocker

Reverting back #55...

grendzy’s picture

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

joachim’s picture

Would this be doable as a contrib module for D7?

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

stBorchert’s picture

No, unfortunately this isn't doable with contrib.

To bad; another issue discussed to death.

sun’s picture

@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...)

rfay’s picture

Go @joachim! Make the case!

Manuel Garcia’s picture

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’s picture

> @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.

andypost’s picture

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?

andypost’s picture

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

grendzy’s picture

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.

webchick’s picture

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.

nevergone’s picture

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! :)

webchick’s picture

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.

andypost’s picture

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

mansspams’s picture

Noticed this immediately. +1 for D7

wjaspers’s picture

+1 for D7, subbing...

bryancasler’s picture

subscribe

catch’s picture

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’s picture

> @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.

eigentor’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
12.33 KB

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
15.04 KB

Tests should be fixed

andypost’s picture

image_style_name_validate() is not used now

andypost’s picture

+++ 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

Ankabout’s picture

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.

andypost’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
16.42 KB

New patch, mostly re-roll

tstoeckler’s picture

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.)

andypost’s picture

Status: Needs work » Needs review
FileSize
17.88 KB

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

tstoeckler’s picture

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.

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work

YAML it is, so this needs a new patch

andypost’s picture

Status: Needs work » Needs review
FileSize
17.87 KB

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

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review
FileSize
17.92 KB

now in -p1 format

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

RTBC again.

sun’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
17.7 KB

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

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
sun’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
20.19 KB

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.

sun’s picture

Status: Needs work » Needs review
FileSize
1.84 KB
19.74 KB

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()

sun’s picture

Fixed the test exception.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

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

sun’s picture

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.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
21.33 KB

fixing conflict

Status: Needs review » Needs work

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

aspilicious’s picture

Status: Needs work » Needs review
FileSize
21.71 KB

And this should fix the last exception :)

andypost’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc, once again

sun’s picture

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.

nevergone’s picture

andypost’s picture

Status: Needs work » Needs review
FileSize
20.87 KB

Re-roll #116

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
22.1 KB

Fixed broken test

sun’s picture

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.

klonos’s picture

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.

klonos’s picture

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?

andypost’s picture

andypost’s picture

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,
andypost’s picture

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

andypost’s picture

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

Bojhan’s picture

Still looks ok

andypost’s picture

Issue tags: +Needs committer feedback
webchick’s picture

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.

aschiwi’s picture

@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.

tstoeckler’s picture

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.

tim.plunkett’s picture

@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.

aschiwi’s picture

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

We're back under thresholds again. Woohoo!

Committed and pushed to 8.x.

quicksketch’s picture

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

stBorchert’s picture

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).

nevergone’s picture

klonos’s picture

Are we going to backport this to Drupal 7?

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

sun’s picture

re: backport: Nope.

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

klonos’s picture

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.

BarisW’s picture

Version: 8.x-dev » 7.x-dev
Status: Closed (fixed) » Needs review
FileSize
22.65 KB

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! :)

andypost’s picture

It looks possible:

  • no API changes
  • old values are valid
  • name field still there on form
BarisW’s picture

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.

swentel’s picture

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'

BarisW’s picture

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.

swentel’s picture

+++ 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.

BarisW’s picture

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.
David_Rothstein’s picture

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.

David_Rothstein’s picture

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.

BarisW’s picture

FileSize
111.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

danielbeeke2’s picture

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.

BarisW’s picture

Thanks for adding this danielbeeke. Looks good to me.

Now let's get this RTBC ;)

Sutharsan’s picture

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.

BarisW’s picture

Status: Needs work » Needs review
FileSize
23.64 KB

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.

Sutharsan’s picture

Status: Needs review » Needs work
FileSize
2.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.

BarisW’s picture

Status: Needs work » Needs review
FileSize
2.03 KB
23.73 KB

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

Sutharsan’s picture

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.

klonos’s picture

...so can we get this in please?

podarok’s picture

#161 looks good
+1 RTBC

David_Rothstein’s picture

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).

David_Rothstein’s picture

Category: feature » bug

The bug this is related to (which came up in discussion in #1923814: Existing hardcoded images can break after updating from Drupal 7 earlier than 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?
BarisW’s picture

Status: Needs work » Needs review
FileSize
3.58 KB
25.82 KB

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.

BarisW’s picture

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.

BarisW’s picture

Status: Needs work » Needs review
BarisW’s picture

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?

BarisW’s picture

"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? :)

David_Rothstein’s picture

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.

David_Rothstein’s picture

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.

BarisW’s picture

Status: Needs work » Needs review
FileSize
3.23 KB
26.19 KB

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.

David_Rothstein’s picture

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.

podarok’s picture

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

David_Rothstein’s picture

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.

tstoeckler’s picture

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

David_Rothstein’s picture

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

tstoeckler’s picture

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

BarisW’s picture

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

treksler’s picture

Status: Needs review » Needs work
Issue tags: -Needs committer feedback +#d7ux
FileSize
87.24 KB
59.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.

treksler’s picture

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

BarisW’s picture

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.

treksler’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
102.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

webchick’s picture

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.

webchick’s picture

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.

treksler’s picture

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.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
955 bytes
27.27 KB

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.)

BarisW’s picture

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

image-field-settings.png

image-field-display-settings.png

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

David_Rothstein’s picture

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.

BarisW’s picture

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?

eigentor’s picture

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.

David_Rothstein’s picture

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.
David_Rothstein’s picture

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.

pwolanin’s picture

@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?

David_Rothstein’s picture

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!

tstoeckler’s picture

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.

BarisW’s picture

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

David_Rothstein’s picture

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

David_Rothstein’s picture

Here's an updated patch to move this along.

David_Rothstein’s picture

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

BarisW’s picture

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 :)

rootwork’s picture

Great work!

klonos’s picture

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

David_Rothstein’s picture

Title: Human readable image-style names » Change 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).

andypost’s picture

David_Rothstein’s picture

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).

David_Rothstein’s picture

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.

David_Rothstein’s picture

Issue tags: +Needs backport to D7

Didn't actually mean to remove that tag.

BarisW’s picture

Change notice looks good to me.

tstoeckler’s picture

Yeah for D7 this looks great. Nice one!

mradcliffe’s picture

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.

andypost’s picture

Title: Change notice: Human readable image-style names » Human 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

David_Rothstein’s picture

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

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

webchick’s picture

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.

David_Rothstein’s picture

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.

andypost’s picture

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

CN looks great!

andypost’s picture

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.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

David_Rothstein’s picture

Issue summary: View changes

$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.

I was actually mistaken here. Drupal 7 does sanitize checkbox and radio labels already, via a roundabout way in theme_form_element_label(); see also https://www.drupal.org/node/28984. (Drupal 6 did not, which is where the mistake came from.)

So from a pure security standpoint some of the code changes we made here in response to the above weren't necessary. However they were still a good idea, since the form API sanitizes those labels using filter_xss_admin() but image style labels are entered as plain text (not HTML) and therefore still need to be run through check_plain() to display properly.

I did go ahead and update the two change records here to resolve this confusion:
https://www.drupal.org/node/2058503/revisions/view/2794191/9001247
https://www.drupal.org/node/2073933/revisions/view/8839851/9001337

For the exact same reason as above, Drupal 8 should run image style labels through Html::escape() before using them as radio or checkbox labels, so I changed the Drupal 8 example to mention that again (a recent update had removed it saying it's no longer needed). The issue that would make it unnecessary again is #2568647: Make #title in form elements autoescape instead of auto XSS admin filter.