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
Comment | File | Size | Author |
---|---|---|---|
#204 | 606598-core7-port-imagestyles-204.patch | 29.98 KB | David_Rothstein |
#203 | 606598-core7-port-imagestyles-203.patch | 4.99 KB | David_Rothstein |
#203 | interdiff-197-203.txt | 4.99 KB | David_Rothstein |
#197 | 606598-core7-port-imagestyles-197.patch | 25.94 KB | David_Rothstein |
#192 | image-field-settings.png | 44.31 KB | BarisW |
Comments
Comment #2
stBorchertRetry with some minor modifications.
Comment #3
eigentor CreditAttribution: eigentor commentedUX 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:
Comment #4
onejam CreditAttribution: onejam commentedI 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
Comment #5
stBorchert@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
Comment #6
onejam CreditAttribution: onejam commentedDid 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.
Comment #7
stBorchert@duvien: please apply the patch bevor installing anything else.
Comment #8
onejam CreditAttribution: onejam commentedThis 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?
Comment #9
quicksketchThis 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).
Comment #10
stBorchert@quicksketch: good idea! Here it is.
Now you can rename the display name of a default style (after you click on "override").
Comment #11
quicksketchJust 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.
Comment #12
stBorchertOk, here is a new version:
* "label" instead of "realname"
* namespace is generated automatically using the standard Drupal methods
happy testing
Comment #13
eMPee584 CreditAttribution: eMPee584 commentedHaven'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?
Comment #14
quicksketchThe 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.
Comment #15
stBorchertUpdated the patch with "machine readable name" instead of "namespace".
Comment #16
eMPee584 CreditAttribution: eMPee584 commentedthx sketch good to know it's being taken care of already - OSS what a joy *lol*
Comment #17
dman CreditAttribution: dman commentedNice 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 )
Comment #18
eMPee584 CreditAttribution: eMPee584 commentedbump it RTBC..
Comment #19
eigentor CreditAttribution: eigentor commentedthis would be a real nice one to have...
Comment #20
sunThis is the same as empty() ?
Wrong indentation.
This review is powered by Dreditor.
Comment #21
stBorchertOk, 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?
Comment #22
andypostSubscribe
Comment #23
cosmicdreams CreditAttribution: cosmicdreams commented#21: 606598-imagestyle_realnames-21.patch queued for re-testing.
Comment #24
andypostPatch applies with some offsets
Works only a defaullt Image style
Config screen brings error (screenshot attached)
Comment #25
stBorchertUhm, 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?
Comment #26
andypost@stBorchert yes
Comment #27
stBorchert@andypost: Did you apply the patch before installing?
Please describe the exact way how you got this error.
Comment #28
sgabe CreditAttribution: sgabe commentedI can confirm andypost's observation.
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.
Comment #29
stBorchertUhm, 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.
Comment #30
sunWhy do we specify a #size here? The value looks pretty large - can we omit it?
"The machine readable name of the style."
Trailing white-space introduced here.
Powered by Dreditor.
Comment #31
sgabe CreditAttribution: sgabe commented@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!Comment #32
andypostAlso style label should be translatable
Comment #33
stBorchert@sun: fixed
Comment #34
sunIs the range from \ to _ intended here? The hyphen (-) should come last in the character set.
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!
Comment #35
stBorchert"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()
orimage_style_delete()
) and I don't see a way how to remove it from there ...Comment #36
sunWhat 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!
Comment #37
joachim CreditAttribution: joachim commentedReroll of the patch in #35 with the change from #36.
Comment #38
Bojhan CreditAttribution: Bojhan commentedSeems 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
Comment #39
drifter CreditAttribution: drifter commentedDownloaded and tested the patch, it applies and works well. Setting to RTBC.
Comment #40
CrookedNumber CreditAttribution: CrookedNumber commentedI 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()
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()
Comment #41
CrookedNumber CreditAttribution: CrookedNumber commentedI 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.
Comment #42
sun=> separate issue
Comment #43
CrookedNumber CreditAttribution: CrookedNumber commented@sun: This is true. Created a separate issue at #818686: Hyphens in style names cause paths that point to non-existent derivative images
Comment #44
eigentor CreditAttribution: eigentor commented+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.
Comment #45
andypost#37: 606598-imagestyle_realnames-37.patch queued for re-testing.
Comment #46
andypost#37: 606598-imagestyle_realnames-37.patch queued for re-testing.
Comment #47
andypostSuppose this patch should be a bit changed after #812688: Organize image formatters around settings
Comment #48
Manuel Garcia CreditAttribution: Manuel Garcia commented#47 most definetly yes.
Comment #49
sunFolks, that patch is not even RTBC. This patch is.
Comment #50
andypostSuppose, we need commit this before beta
Comment #51
sun#37: 606598-imagestyle_realnames-37.patch queued for re-testing.
Comment #52
andypostRe-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
Comment #53
rfaysubscribe
Comment #54
sunStill looks good to go for me.
Comment #55
philbar CreditAttribution: philbar commentedI 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.
Comment #56
tstoecklerWell, it changes the UI, so if this goes in D7, it should do so before beta. Re-tagging.
Comment #57
philbar CreditAttribution: philbar commentedComment #58
philbar CreditAttribution: philbar commentedOops...could have sworn I saw an API change in this issue. I guess I miss read "UI" on my iphone.
Comment #59
philbar CreditAttribution: philbar commentedComment #60
Bojhan CreditAttribution: Bojhan commentedThat it requres an API/interface change doesnt make it critical.
Comment #61
philbar CreditAttribution: philbar commentedIf 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.
Comment #62
webchickCan someone explain why on earth this is a beta blocker and not a nice to have?
Comment #63
Bojhan CreditAttribution: Bojhan commentedIts 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.
Comment #64
webchickApril was well after feature freeze. I see no reason this shouldn't be moved to D8, unless I'm missing something obvious?
Comment #65
stBorchertJust 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.
Comment #66
philbar CreditAttribution: philbar commentedReverting back #55...
Comment #67
grendzy CreditAttribution: grendzy commentedI 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
Comment #68
joachim CreditAttribution: joachim commentedWould this be doable as a contrib module for D7?
Image module oldskool needs this feature, so if anything does this, keep me posted :)
Comment #69
stBorchertNo, unfortunately this isn't doable with contrib.
To bad; another issue discussed to death.
Comment #70
sun@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...)
Comment #71
rfayGo @joachim! Make the case!
Comment #72
Manuel Garcia CreditAttribution: Manuel Garcia commentedIMHO, 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.
Comment #73
joachim CreditAttribution: joachim commented> @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.
Comment #74
andypostThis 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?
Comment #75
andypostWe 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
Comment #76
grendzy CreditAttribution: grendzy commentedPreviously, 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.
Comment #77
webchickI 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.
Comment #78
nevergone CreditAttribution: nevergone commentedThis is very usability bug. If I create and test a small patch, commited to Drupal 7? Let it be a usable Drupal 7! :)
Comment #79
webchickThere is no usability bug here. This is merely a missing feature, and adding it would invalidate documentation and screenshots.
Comment #80
andypost@webchick is there any chances for that issue to be backported to 7 after D7 release?
Comment #81
mansspams CreditAttribution: mansspams commentedNoticed this immediately. +1 for D7
Comment #82
wjaspers CreditAttribution: wjaspers commented+1 for D7, subbing...
Comment #83
bryancasler CreditAttribution: bryancasler commentedsubscribe
Comment #84
catchThere 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.
Comment #85
joachim CreditAttribution: joachim commented> @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.
Comment #86
eigentor CreditAttribution: eigentor commentedI 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
Comment #87
andypostpatch #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
Comment #89
andypostTests should be fixed
Comment #90
andypostimage_style_name_validate() is not used now
Comment #91
andypostAdded this missed hunk for image_style_options() with small test
Comment #92
Ankabout CreditAttribution: Ankabout commentedAm 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.
Comment #93
andypostThis 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
Comment #94
andypostNew patch, mostly re-roll
Comment #95
tstoecklerIf 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.
??
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.)
Comment #96
andypostFixed broken test and image_style_options()
Changed
replace_pattern
to previously used and commented.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
Comment #97
tstoecklerCouldn'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.
Comment #98
aspilicious CreditAttribution: aspilicious commentedYAML it is, so this needs a new patch
Comment #99
andypost.. another re-roll, now with YAML ...99 comemnts
Comment #101
andypostnow in -p1 format
Comment #102
tstoecklerRTBC again.
Comment #103
sunAll 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.
Comment #104
andypost@sun, thanx for review. BC fix hides this trouble...
Comment #105
tstoecklerComment #106
sunThanks! Looks better. Reviewed this once more in depth:
This looked suspicious, but I double-checked and indeed we're using the "Edit $thing $name" page title pattern everywhere else in Drupal.
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.)
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.
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.
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. ;)
Let's revert this unrelated test change.
Comment #107
sunPlain re-roll against HEAD, not incorporating #106 yet.
Comment #109
sunAttached 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()
Comment #110
sunFixed the test exception.
Comment #111
andypostLet's get this in and continue with image effects #1508872: Image effects duplicate on edit
Comment #112
sun#110: drupal8.image-style-label.110.patch queued for re-testing.
Comment #114
aspilicious CreditAttribution: aspilicious commentedfixing conflict
Comment #116
aspilicious CreditAttribution: aspilicious commentedAnd this should fix the last exception :)
Comment #117
andypostback to rtbc, once again
Comment #118
sun#116: drupal8.image-style-label.116.patch queued for re-testing.
Comment #120
nevergone CreditAttribution: nevergone commentedRelated issue: #1751634: Image styles description
Comment #121
andypostRe-roll #116
Comment #123
andypostFixed broken test
Comment #124
sunThanks for re-rolling and keeping this alive!
I re-reviewed this patch once more and still think it is ready to fly.
Comment #125
klonosOh, 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.
Comment #126
klonosPS: 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?
Comment #127
andypost#123: 606598-core8-imagestyles-123.patch queued for re-testing.
PS related #1768484: Indentation in YAML files violates Drupal coding standards
Comment #128
andypostAnother re-roll after #1175764: Have theme('image_style') inject the style name as a class
Changed hunk
Comment #129
andypostBojhan does review mane times, so here a current screens for D8
Comment #130
andypostAnd shots before the patch, all other places are in issue summary
Comment #131
Bojhan CreditAttribution: Bojhan commentedStill looks ok
Comment #132
andypostCan we get this commited and continue with #1788542: Use EntityFormController and EntityListController for image styles
Comment #133
webchickWe're over thresholds atm so I unfortunately can't commit any feature patches. Help with smashing critical and major bugs would be greatly appreciated.
Comment #134
aschiwi CreditAttribution: aschiwi commented@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.
Comment #135
tstoecklerHaving 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.
Comment #136
tim.plunkett@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.
Comment #137
aschiwi CreditAttribution: aschiwi commented@tim.plunkett Oh thanks, that's great to hear :)
Comment #138
webchickWe're back under thresholds again. Woohoo!
Committed and pushed to 8.x.
Comment #139
quicksketchThanks everyone here for pulling this together. Something that totally should have been in the original image.module. Kudos and appreciation all around. :)
Comment #140
stBorchertYeah!
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).
Comment #141
nevergone CreditAttribution: nevergone commentedperhaps description? #1751634: Image styles description
Comment #142
klonosCan we please? ...if we're not breaking any API and all I mean.
Comment #143
sunre: backport: Nope.
A contrib module could try to inject it though. I don't really see why that shouldn't be possible.
Comment #144
klonosOk, 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.
Comment #146
BarisW CreditAttribution: BarisW commentedHi 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! :)
Comment #147
andypostIt looks possible:
Comment #148
BarisW CreditAttribution: BarisW commentedPlus, I kept all strings intact. So instead of changing:
to
I kept it like this
By doing so, all existing translation strings keep working as well.
Comment #149
swentel CreditAttribution: swentel commentedIt 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'
Comment #150
BarisW CreditAttribution: BarisW commentedAh, true.
I've added a check to
image_styles()
to use the system name if the label is empty:Also, I've update the demo code in image.api.php.
Comment #151
swentel CreditAttribution: swentel commentedNew label for translations - unless this is already used in core ?
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.
Comment #152
BarisW CreditAttribution: BarisW commentedGreat! Thanks for being thorough.
Comment #153
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.
Comment #154
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, 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.
Comment #155
BarisW CreditAttribution: BarisW commentedThe 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.Comment #156
danielbeeke2 CreditAttribution: danielbeeke2 commentedI 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.
Comment #157
BarisW CreditAttribution: BarisW commentedThanks for adding this danielbeeke. Looks good to me.
Now let's get this RTBC ;)
Comment #158
Sutharsan CreditAttribution: Sutharsan commentedNice 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.
Comment #159
BarisW CreditAttribution: BarisW commentedI 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.
Comment #160
Sutharsan CreditAttribution: Sutharsan commentedWhat 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.
Comment #161
BarisW CreditAttribution: BarisW commentedHmm, right. So I removed the if() because the fields always have a value.
Comment #162
Sutharsan CreditAttribution: Sutharsan commentedI'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.
Comment #163
klonos...so can we get this in please?
Comment #164
podarok#161 looks good
+1 RTBC
Comment #165
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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:
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).
Comment #166
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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:
<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).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:
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):
The @see is obsolete.
Comment is no longer accurate?
Comment #167
BarisW CreditAttribution: BarisW commentedHi 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.
Comment #169
BarisW CreditAttribution: BarisW commentedChanging the string to a #disabled field caused the tests to fail.
Updated the tests to reflect this.
Thanks for some guidance here from chx.
Comment #170
BarisW CreditAttribution: BarisW commentedComment #171
BarisW CreditAttribution: BarisW commentedBy 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?
Comment #172
BarisW CreditAttribution: BarisW commented"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? :)
Comment #173
David_Rothstein CreditAttribution: David_Rothstein commentedThanks, 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.
Comment #174
David_Rothstein CreditAttribution: David_Rothstein commentedOh, 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.
Comment #175
BarisW CreditAttribution: BarisW commentedGood 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.Comment #176
David_Rothstein CreditAttribution: David_Rothstein commentedIs 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.
Comment #177
David_Rothstein CreditAttribution: David_Rothstein commented#1946580: Followups for human-readable image style names (machine-readable names with hyphens don't work correctly)
Comment #178
podarokyup
Looks like this possible needs follow-up?
#176 looks good for me
Comment #179
David_Rothstein CreditAttribution: David_Rothstein commentedSorry, but I have to ask: is the following a security risk?
$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.
Comment #180
tstoecklerI think we could simply do a full check_plain() here. I think that's we usually do for select options.
Comment #181
David_Rothstein CreditAttribution: David_Rothstein commentedI don't think so; form_select_options() runs check_plain() on it already, so this would result in a double check-plain.
Comment #182
tstoecklerOh, you're right. I didn't realize that.
Comment #183
BarisW CreditAttribution: BarisW commentedSorry for the delay, I was on a vacation :)
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
Comment #184
treksler CreditAttribution: treksler commentedI applied the patch to the latest Drupal 7 and the patch kills some image image styes in the list completely. :(
See attachments.
Comment #185
treksler CreditAttribution: treksler commentedsorry, didn't mean to alter issue tags, not sure what happened there
Comment #186
BarisW CreditAttribution: BarisW commentedHmm, 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.
Comment #187
treksler CreditAttribution: treksler commentedHey, 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
Comment #188
webchick7.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.
Comment #189
webchickThough #184 - #188 suggest that the release notes ought to warn people about a possible problem with custom image styles until the cache is cleared.
Comment #190
treksler CreditAttribution: treksler commentedDid 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.
Comment #191
David_Rothstein CreditAttribution: David_Rothstein commentedNormally, 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.)
Comment #192
BarisW CreditAttribution: BarisW commentedNot sure if we want this, Drupal core double-escapes the values now.
So either we have to change these dropdowns as well, or keep using #183
Comment #193
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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.
Comment #194
BarisW CreditAttribution: BarisW commentedThat 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?
Comment #195
eigentor CreditAttribution: eigentor commentedThis 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.
Comment #196
David_Rothstein CreditAttribution: David_Rothstein commentedA 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:
Comment #197
David_Rothstein CreditAttribution: David_Rothstein commentedMeanwhile, 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.
Comment #198
pwolanin CreditAttribution: pwolanin commented@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?
Comment #199
David_Rothstein CreditAttribution: David_Rothstein commentedSo 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):
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!
Comment #200
tstoeckler2. 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
(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.
Comment #201
BarisW CreditAttribution: BarisW commentedDavid, now we've passed the 200 comments (FWIW), can we go for #2: Flag on existing function?
Comment #202
David_Rothstein CreditAttribution: David_Rothstein commentedIt sounds like everyone who has commented agrees. So we just need an updated patch?
Comment #203
David_Rothstein CreditAttribution: David_Rothstein commentedHere's an updated patch to move this along.
Comment #204
David_Rothstein CreditAttribution: David_Rothstein commentedOops, I apparently uploaded the interdiff as the patch. Let's try again.
Comment #205
BarisW CreditAttribution: BarisW commentedThanks 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 :)
Comment #206
rootworkGreat work!
Comment #207
klonosYes! Lets get this more-than-3-years-standing issue fixed ;)
Comment #208
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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).
Comment #209
andypostFor D8 there's follow-up #1946580: Followups for human-readable image style names (machine-readable names with hyphens don't work correctly)
Comment #210
David_Rothstein CreditAttribution: David_Rothstein commentedBump, 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).
Comment #211
David_Rothstein CreditAttribution: David_Rothstein commentedAdded 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.
Comment #212
David_Rothstein CreditAttribution: David_Rothstein commentedDidn't actually mean to remove that tag.
Comment #213
BarisW CreditAttribution: BarisW commentedChange notice looks good to me.
Comment #214
tstoecklerYeah for D7 this looks great. Nice one!
Comment #215
mradcliffeThanks 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.
Comment #216
andypostWe 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
anddrupal-7.bare.standard_all.database.php.gz
Comment #217
David_Rothstein CreditAttribution: David_Rothstein commentedI don't see a Drupal 8 change notice yet.
Comment #218
webchickHm. 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.
Comment #219
David_Rothstein CreditAttribution: David_Rothstein commentedJust 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.
Comment #220
andypostCN looks great!
Comment #221
andypostNow only upgrade path needs more eyes #2049465: Upgrade of image styles and effects broken
Comment #222.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #224
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.