It is not currently possible for theme and theme engines to define custom validation or submit functions. I suggest passing the original $form to those forms callback, and backporting that to D6.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Status: Active » Needs review
Issue tags: +Quick fix
FileSize
1.76 KB
Stefan Nagtegaal’s picture

Status: Needs review » Reviewed & tested by the community

Nice! Simple and straight forward, and this opens up a can of possibilities for live validating and error reporting of form entries..
Imo RTBC... :-)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Damien Tournoud’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Reviewed & tested by the community

What about backporting this? This is perfectly backward compatible. Gabor, it's your call.

Damien Tournoud’s picture

Title: Allow theme engine and theme settings form » Allow theme engine and theme define submit handlers for their setting forms
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

New features are not added to Drupal 6 even if otherwise backwards compatible.

sun’s picture

Status: Closed (won't fix) » Reviewed & tested by the community

This is not a feature, but a bugfix that can be safely backported as is.

Gábor Hojtsy’s picture

How is it a bug?

sun’s picture

Sometimes, original issue descriptions contain valuable information:

It is not currently possible for theme and theme engines to define custom validation or submit functions.

You need a separate module to change the theme settings form for one theme, while this very form is supposed to be handled by the theme.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

Contrary to what you seem to assume, I've read both the issue description and reviewed the patch. Now I've also looked into the code of this function in Drupal 6 and 5:

http://api.drupal.org/api/function/system_theme_settings/5
http://api.drupal.org/api/function/system_theme_settings/6

Look into Drupal 5, there the function is called without even $settings passed into it. This was extended in Drupal 6 by passing on $settings. Then, with this patch, Drupal 7 is extended by passing on $form, so there is then no actual form altering required in the theme, but you can do it with this special hook.

Looking back at the CVS logs, I committed this change 1.5 years ago to pass on $settings and allow for theme-settings.php at all. Patch was from http://drupal.org/node/57676 The reason I've looked it up, is that it is not looking attractive at all, and even less so with allowing for more power to the theme. I don't really consider putting logic such as form submission handlers or validation functions in the theme layer a good practice so I'd not even consider this a good way to solve the problem for a development branch. Plus I've seen other places where similar reverse-form-alter type of invocations (such as nodeapi op form: http://drupal.org/node/39778) were suggested for removal and then subsequently replaced with actual form_alter use. Special casing these things and then still further extending the special casing is not the way I would suggest this to go.

Anyway, just like passing $settings was not backported to Drupal 5 (because it was a feature change), passing $form is not to be committed to Drupal 6. It is an API change and I believe not even in a good direction.

Damien Tournoud’s picture

Category: bug » feature

@Gabor, I agree on your "not a bug" qualification, but I really cannot agree with this following analysis:

The reason I've looked it up, is that it is not looking attractive at all, and even less so with allowing for more power to the theme. I don't really consider putting logic such as form submission handlers or validation functions in the theme layer a good practice so I'd not even consider this a good way to solve the problem for a development branch.

We are not putting logic inside the *theme layer*, we are putting logic inside a *theme*. I don't consider this a bad practice at all.

Gábor Hojtsy’s picture

Well, putting aside religious issues, like whether the theme should do form API and submission/validation handlers, I believe my point on replacing special cased functions with generic API use is a good idea. Here we have a theme settings form, where it assumes someone wants to alter it and calls out specifically to that someone(s). From my example, the same was happening up to 2005 in nodeapi but then was eliminated, since that someone could just implement a form_alter() listener hook and alter when suitable.

That obviously gets complicated when that someone is a theme, since the normal form_alter() type of hooks are called with module_invoke_all() and friends, which only consider modules. I think from a clear API perspective, since you are doing form altering and submit/validation functions anyway, it would be cleaner to just allow the form_alter() hook to operate here with themes. We have for example a "fake" form_alter() in the install process for the install forms, so that we keep to use the same API instead of making up our own special casing for adding and modifying stuff on the form. I believe going into the form_alter() direction in some way would be much better for Drupal 7.

Damien Tournoud’s picture

@Gabor, I completely agree. I believe that making themes behaving a little bit more like modules (and, for example, allowing them to depend on other modules, altering forms, etc.) would make a lot of sense.