Core Features

[live627]

Does not process the image entry in the defining array like the comment says. The code is simply... missing.

[Arantor]

Never has, it always uses a filename based on the array key.

[emanuele]

What is the intended behaviour?
According to the comment it should use 'image' if available but it's not. (trunk is the same)

[live627]

If the image isn't in the usual place and a mod specifies 'image' it should use the custom path.

[emanuele]

And why the image shouldn't be in the usual place? (Just asking to understand if there is a strong reason to foreseen this possibility )

[live627]

Say a mutant mod came along  that decided not to dump its images in the themes dir... rare, yes, possible, yes, makes sense especially for hooking (not hacking), yes, most likely seen in portals and super-mods.

[Joshua Dickerson]

I think it makes sense to track/fix this. Any objections?

[emanuele]

No objections for the tracking.

Regarding the point: I'm for complete flexibility in user interface and experience, but (quite strict) standards for developers.

If a "mutant" mod comes, you said it all: it's mutant, it's not following the "guidelines" we are providing to support mods.

I would remove the "image" entry from the documentation and that's all...no, there is still something missing: the current admin.template.php code is:

Code Select Expand

<img class="features_image" src="', $settings['images_url'], '/admin/feature_', $id, '.png" alt="', $feature['title'], '" />
that means a theme should provide all the images for all the possible mods...that doesn't make much sense to me.
I would change it to:

Code Select Expand

<img class="features_image" src="', $settings['default_images_url'], '/admin/feature_', $id, '.png" alt="', $feature['title'], '" />
it oesn't make sense in this case (since it's something extensible) to implement it on a theme by theme basis.
If you would be completely flexible a file_exists could be used to check if the image is available on the current theme and if not load the default one, but it would be almost useless...IMHO.

[Joshua Dickerson]

I would prefer to load it from the current theme and then use the default. Hmm... surprising there isn't a function that will check that for us.

[emanuele]

Not particularly in favour not so against. Classic case "it's easier to implement than discuss"

Done in the branch I'm using to work on the core features page, that now should be ajax and without save button!

[live627]

No objections for the tracking.

Regarding the point: I'm for complete flexibility in user interface and experience, but (quite strict) standards for developers.

If a "mutant" mod comes, you said it all: it's mutant, it's not following the "guidelines" we are providing to support mods.

I would remove the "image" entry from the documentation and that's all...no, there is still something missing: the current admin.template.php code is:

Code Select Expand

<img class="features_image" src="', $settings['images_url'], '/admin/feature_', $id, '.png" alt="', $feature['title'], '" />
that means a theme should provide all the images for all the possible mods...that doesn't make much sense to me.
I would change it to:

Code Select Expand

<img class="features_image" src="', $settings['default_images_url'], '/admin/feature_', $id, '.png" alt="', $feature['title'], '" />
it oesn't make sense in this case (since it's something extensible) to implement it on a theme by theme basis.
If you would be completely flexible a file_exists could be used to check if the image is available on the current theme and if not load the default one, but it would be almost useless...IMHO.

how about checking if an image is in the current theme and load it if not, try the default theme.

How's this? It goes in ManageSetttings.php, within the array that defines $context['features'][$id]:

Code Select Expand

'image' => isset($feature['image']) ? $feature['image'] : (file_exists($settings['images_url'], '/admin/feature_', $id, '.png') ? $settings['default_images_url'], '/admin/feature_', $id, '.png' : $settings['default_images_url'], '/admin/feature_', $id, '.png'),

[emanuele]

That's basically what I did (except I used theme_dir instead of images_url.

[live627]

What? The image output is still a valid URI and it's been tested to ensure it works?

[emanuele]

Sorry, the font is small and I didn't see exactly the code...

Code Select Expand

'image' => (file_exists($settings['theme_dir'] . '/images/admin/feature_' . $id . '.png') ? $settings['images_url'] : $settings['default_images_url']) . '/admin/feature_' . $id . '.png',

[live627]

Now, all that needs done if that code is final is to remove the comment about the image parameter to avoid future confusion with people who yell "BUG!!"