Provide default styles in image.module (add hook_image_default_styles)

No tests?

Tested with a fresh co from head. Patch applied fine, but I received the following notices:

Some weirdness-- i originally only saw a 'thumbnail' style and received the above notices. Once I manually created a style the notices went away and medium and large styles styles appeared in addition to thumbnail and the one I created.

Image style table after adding my style:

Gonna finish testing, but thought you might want to see this right away.

seem we crossposted

Ok, tried again with the current patch and another fresh head:

One offset:
Hunk #1 succeeded at 173 (offset 20 lines)

Notices and weirdness are gone.

Not sure what else to test so going with the items in #7:

- Check that default styles are not editable
they are-- there's an edit link and they can be overridden and reverted.

- Check that default styles generate images properly
check

- Override a default style
check

- Check that overridden styles may have effects added
check

- Check that overridden styles may not have their names changed
couldn't figure out how to check this

- Check that overridden styles generate images properly
check

- Revert an overridden style
check

- Check that the default effects are restored
check

@13 -- it was a fresh checkout, but installed before I patched. In any case, weirdness is gone.

BTW image/imagefield/imagecache in core is f'in AMAZING! Nice nice nice job!

- Check that overridden styles may not have their names changed
couldn't figure out how to check this

I'm an idiot-- they're not so 'check' here also.

One wrinkle-- seems to be a problem with added styles not rendering at all. Playing around with it now.

Unfortunately I confirmed it-- no manually added styles render at all. I created a dupe of the 'medium' style and it doesn't render at all. I added a bunch of other manually styles and they won't render either.

Per our discussion in irc, this problem exists without the patch as well-- i just confirmed it. So the patch itself seems fine.

There are no breadcrumbs on /admin/config/media/image-styles/edit/thumbnail.

"Storage" (Overridden / Default / Normal) is confusing – I thought this was describing the storage backend for the image files themselves. (ooooh! configurable storage! Does this mean I can put my images on S3?)

Confirmed WorldFallz report in #19 - user-defined styles don't display at all.

Coder reports quite a few style issues, mostly tabs it seems (Edit: nevermind, these seem to be false positives, coder doesn't like indented phpdocs).

meh-- i can't get coder to work at all lately, lol. And I see what you mean about 'storage" -- maybe status or type?

talk about a drupal wtf, i didn't change the status, lol.

ok, retested patch from 10 with #604650: New image styles are not usable as formatters as well -- applies with offsets (obviously), and seems to work fine!

The labels in #28 sound good, thanks!

Sounds good to me-- did anything else in the patch change?

/me trys to kick the bot...

This looks solid to me. I also like that this (indirectly) makes image styles "exportable". Tagging so we don't forget to add the API docs for hook_image_default_styles.

Patch applied:

Hunk #1 succeeded at 173 (offset 20 lines).

EEEEEK.

so, i am WSOD'ing over here. i recreated it twice (totally wiped the install and set up same settings and got it again), and it doesn't happen before adding the patch and adding the config for image field. *but* i am not 100% if it is from the patch, as tha_sun was saying that the same thing came up with radio button field.

anyway, so if anyone wants to try and duplicate this, here are screenshots of all the settings:

- initial field config page: http://skitch.com/arianek/ndg6j/article-localhost.drupal7

- second longer config page (sorry it's in parts!): 1) http://skitch.com/arianek/ndg6q/imageswow--localhost.drupal7 2) http://skitch.com/arianek/ndg6x/imageswow--localhost.drupal7 3) http://skitch.com/arianek/ndg6t/imageswow--localhost.drupal7

i'm going to poke at this some more and see if i can narrow down what's causing it, but not feeling terribly hopeful! and i am not gonna be able to do much for testing it if i can't get the WSOD narrowed down! thought i should post this right away tho... hope it's not really caused by the patch!

re-adding the needs doc tag (i overwrote that)

ps. just removed the image field that i had added to article and the WSOD went away on the node/add page - it's gotta be one of the config settings for the field...

pps. just added an image field without changing any of the default settings and DO get the WSOD. then reverted the patch and added an image field without changing defaults and still get the WSOD. deleted the image field altogether, and no WSOD on node add. i'm stumped.

Sorry, been a little out of pocket since last night (code freeze of all days, lol)-- but this looks good. Docs made sense to me-- so that's an accomplishment. ;-)

+++ modules/image/image.module	15 Oct 2009 08:15:32 -0000
@@ -6,6 +6,31 @@
+/**
+ * Image style constant for user presets in the database.
+ */
+define('IMAGE_STORAGE_NORMAL', 1);
+
+/**
+ * Image style constant for user presets that override module-defined presets.
+ */
+define('IMAGE_STORAGE_OVERRIDE', 2);
+
+/**
+ * Image style constant for module-defined presets in code.
+ */
+define('IMAGE_STORAGE_DEFAULT', 4);
+

The one thing I don't like about this is it's *so* close to being a fully-fledged CTools-esque "make defaulty" feature that it seems a shame to tie it to images specifically.

But, there are a couple things:

1. A full blown Export API would be a new feature, and we're well past feature freeze, and it also would be too much to try and cram into this last few days.

2. Nate explicitly does NOT want to provide a "disable" facility for this, so that modules can count on there being things like "thumbnail" available. Without that assurance, there's not much point in this patch since we'll continue to get "style bloat".

+++ modules/image/image.module	15 Oct 2009 08:15:32 -0000
@@ -87,15 +112,27 @@
+    'load arguments' => array(5, (string) IMAGE_STORAGE_EDITABLE),
@@ -105,6 +142,7 @@
+    'load arguments' => array(5, (string) IMAGE_STORAGE_EDITABLE),

I wanted to know why these were cast to string, and Nate pointed out that it's because otherwise the menu system will interpret them as URL arguments.

+++ modules/image/image.module	15 Oct 2009 08:15:32 -0000
@@ -310,15 +388,44 @@
+      drupal_alter('image_styles', $styles);

I can haz docs or hook_image_styles_alter()?

While Nate works on docs, I'm going to give this a run-through from a UI perspective.

This review is powered by Dreditor.

Screenshots of the UI:

Here's the main styles listing. The first three are module-defined, and either are shown as "Default" (meaning they're just as the module designed), "Overridden" (meaning I changed something), or "Custom" (meaning I created it). Overridden styles can be reverted back to the original, custom ones can be deleted. All styles may be edited:

If I edit a default style, I'm taken to a screen with a notice and an "Override defaults" button:

If I click the button, I get the normal image style interface with a notice at the top that I'm overriding:

Revert will give me a confirm form:

This all looks good to me, so no further complaints!

Do it! We discussed how heavily we should promote this override stuff in the UI and agreed to not hold this one up on it.

nooo, sorry!

In #drupal, yoroy and Bojhan pointed out that this Default / Overridden / Custom business probably falls under "too much information" for the vast majority of site builders, who are not deploying code through something like SVN, though it is very handy for those who are. I'm on the fence about it, personally. I can see both sides.

As a site builder who *does* follow best practices, this information is really useful so I know which database-related thingies I haven't moved to code yet. Yet, core itself doesn't do anything with this information (there's no image style export functionality) and we also do not do show this type of information in other places in core. For example, with blocks, the way I know whether something is module-provided or custom is whether it has a "remove" link next to it. If so, I added it myself, and I need to move it to code. If not, it's already provided by a module. However, blocks don't have this interim "Overriden" step: they're either module-provided, or custom, and you're stuck with whatever the module gives you.

I think it's worth discussing this more as part of the UI clean-up phase, so I've created #606016: Figure out how best to present overridden/default/custom data as a follow-up.

The UX team confirmed they do not want the patch held up on this issue, so I've committed it to HEAD!

This needs documenting in the module upgrade guide.

for the record, i'm just poking at this some more, and totally not getting the same WSOD problem. :-)

changing tagging scheme for update guide

In #47, webchick said this needs documentation in the 6/7 module update guide.

However, I think we are only documenting API changes there, and this seems like an API addition (new hooks to deal with the totally new image module and capability of having image styles, which didn't exist at all in d6 outside of contrib). It may be changing previous functionality from 7.0-alpha-something, but it doesn't change functionality from Drupal 6. I think anyway?

So am I missing something -- what needs to be added to the 6/7 module update guide for this patch/issue?

And by the way, there is no other information in the 6/7 module update guide about the new Image module at all, as far as I can tell...

The ImageCache contrib module feature was introduced after Image module went into core. Way too late for me, and isn't really a core-core upgrade thing, but anyway, I'd simply suggest to go with a streamlined version:

D6:

hook_imagecache_default_presets()

D7:

hook_image_default_styles()

See hook_image_default_styles() for more information.

We don't document contrib -> core upgrades on the core 6/7 page. Or core API additions, for the most part. So if the ImageCache contrib module people want to write a "how to migrate to core" doc page, we can link from http://drupal.org/update/modules/6/7 (at the top, in the section about contrib to core migrations).

But until that happens, I think this core issue is fixed.