System settings forms are not validated

Heh, well found.

Tested with and without the patch, handles all the cases well, error text is fine, code style looks clean to me.

Well, well, all core or contrib settings forms are wrapped in system_settings_form(), so centralizing their validation here is not exactly a good idea. Actually, each form should have its own validation function, not one central handler.

And here it is applied to each of the two forms.

Thats just two of the three from the patch.

Code looks good. Feel free to RTBC if it works as well :)

Some more changes:

• On "site information":
  • Set "anonymous" field required.
  • Set "site_frontpage" field required.
• On "image-toolkit":
  • Consolidated error messages. Now there's one string less to translate.

Tested, works.

It works.
edit: cross posted. #6 and #9 both work :P

should note I agree with making them required per #9 - so should probably go with that one.

Looks good, thanks, committed #9.

While testing this, I came up with #202393. Though it's featureish, I think it would further improve validation.

Unfortunately there's a new bug introduced by this patch.

The patch adds validation for the JPEG quality option on the Image toolkit page. However, that option is not defined in system.admin.inc, but in image.gd.inc, which means it will only be present in the form when the GD toolkit is used. If another toolkit is chosen, the form will falsely complain about invalid values for the JPEG quality and no further changes to the settings are possible after that point, leaving the user with a broken image toolkit.

Attached is a patch against current DRUPAL-6 which moves the validation to image.gd.inc.

To test this, use image.imagemagick.inc from the Image module (you will however need a patch for that file which I am about to post to #181809).

Please consider committing this before the next RC – people will probably start trying to use ImageMagick soon. Thanks!

Hm, a bit more testing would be great here.

For what it's worth, I did test it myself. GD and ImageMagick both seem to work. But yes, more testers would be great.

For convenience, I'm attaching my patched copy of image.imagemagick.inc. Testers should install it in their includes directory, and make sure to rename it to "image.imagemagick.inc" in case it's been renamed somewhere along the way. Without the patch above you should see an error about invalid JPEG quality when you try to switch to IM, and those should go away once you apply said patch. And, obviously, image operations (such as downscaling of profile pictures) should still be working – both with GD and IM!

There is another patch in the queue that removes validation of 403/404 handler paths: http://drupal.org/node/209584. It might be unrelated, but I figured I'd point this out. :)

On second look, #15 looks quite right, so committed, thanks.

Apparently, the issue at #138433: Anonymous and default front page fields should be required was a duplicate of this, so I thought I'd raise me concern from there, here:

Also, I don't think we can stop anyone from specifying something like:

$conf = array(
  'anonymous' => '',
  'site_frontpage' => '',
); 

in settings.php, so perhaps it would be a good idea to have some default options to fall back to in case of $conf[...] = ''?