Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.(Noticed this while working on #1332580: Clean up API docs for color module.)
In color.module, we find this form_alter implementation:
/**
* Implements hook_form_FORM_ID_alter().
*/
function color_form_system_themes_alter(&$form, &$form_state) {
_color_theme_select_form_alter($form, $form_state);
}
This is the only place where _color_theme_select_form_alter() is called, so we probably just should move its function body here in any case.
More importantly, there is no form with the ID "system_themes" in D8 or D7---and even though the themes page used to be form in D6, there wasn't a form with the ID "system_themes" in D6 either (though there was one with the ID "system_themes_form").
Which means that, at least since D6, this implementation has altered a form that does not exists---viz. has not done anything. Given that no-one has missed what it is trying to do, it looks like this function (an _color_theme_select_form_alter()) can just be ripped out.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | 1337198.patch | 1.15 KB | bleen |
Comments
Comment #1
bleen CreditAttribution: bleen commentedSo the intent of this code was to change the screenshot that appears on the Theme selection form to reflect the color settings for the theme in question:
However, in D6 this code relied on the fact that the color module created a screenshot that could be used for the preview. In D7 there is no longer an image created so I think it is safe to remove this code.
Comment #2
amateescu CreditAttribution: amateescu commentedResearch checks out and the patch looks good to me. Clean-up++ :)
Comment #3
sven.lauer CreditAttribution: sven.lauer commentedTagging also for backport to D6, since, as the OP indicates, it seems that even in D6, this does not do what it is supposed to. So once this gets in for (D8 and possibly) D7, we should figure out what to do in D6.
Comment #4
Dries CreditAttribution: Dries commentedCommitted to 8.x. Moving to 7.x so we can consider backporting this. Technically, it would be an API change, although not a big one.
Comment #5
sunApparently, this is still supposed to work:
Comment #6
sven.lauer CreditAttribution: sven.lauer commentedAlas, but it doesn't work, even in D7. Since no-one has cried foul over this (at least I can find nothing in the issue queue), the question is whether this is actually "supposed to work".
Basically we have to decide:
(a) That this functionality has been implicitly removed, so we simply should also remove the code that generates the colored screenshots.
or
(b) Bring back the form alter function and make it work.
Comment #7
sven.lauer CreditAttribution: sven.lauer commentedP. S. Some cursory testing seems to confirm that this also does not work in current D6 (so, did it ever?), which is not surprising, given the wrong FORM_ID mentioned in the OP.
Comment #8
sunCan you try to make it work? If it's only the $form_id and/or form structure...?
That said, you might need to test in D7/D6 with Garland, as it's possible (and reasonable) that newer themes don't support... err... don't even know of this capability.
I'm undecided myself, for
1) this is quite a super-advanced feature of Color module, and I'm surprised myself that Color module actually contains code for it, which is supposed work (and most likely did at some point). As an end-user (and Color module is designed for the end-user, right?), I guess I'd appreciate it if the screenshot would show the actual colors of the theme.
but OTOH, there's also
2) based on a super-quick glance on the code after grepping, there seems to be quite some (also: untested) code involved for this dirty little fancy feature. Perhaps, but not sure, that's too much.
Now, I won't state that I'd be in favor of 1) on the grounds of exposing what is actually possible in terms of theme customization overrides, but I didn't state that, did I?
Comment #9
sven.lauer CreditAttribution: sven.lauer commentedI did test with Garland, as I suspect(ed) that, at some point in time _color_theme_select_form_alter() was intended to be called by the theme in its own form_alter implementation (on the model of things like _color_html_alter()), until at some point someone realized that you can do this in a single form_alter in color module. Hence I thought that maybe Garland still does this "manually", but it does not seem to.
It should not be too hard to make this work, but I kind of agree with your (2)---color_scheme_form_submit() could be simplified quite a bit if we decide to get rid of this nice-to-have feature that no-one seems to have missed in the longish time since its defunct.
(I just poked around to see how long this has been defunct: In D6, the form 'system_themes' was renamed to 'system_themes_form' in http://drupalcode.org/project/drupal.git/commit/5bbbf10ba84042b8576d6757... in April 2007. So this has been broken four almost five years.)
Comment #10
sven.lauer CreditAttribution: sven.lauer commentedDo note that this also means that the screenshot-generating code has not had a visible effect in almost five years. So just fixing the form alter code without thoroughly testing the screenshot-generating code would be a bad idea. For all we know, the screenshot-generation make be breaking in all kinds of cases.
Comment #11
markhalliwellThe following is still in D8:
We should just remove this, really.
Comment #27
quietone CreditAttribution: quietone at PreviousNext commentedThis issue was committed to Drupal 7 and Drupal 8. It was then reopened when it was noticed that functionality was lost, #6. Instead of this old issue lingering let's close it and have a new issue to decide what to do next. The new issue is #3270443: Screenshot no longer shows the acutal color.
Marking fixed in Drupal 8.0