Logic problem with per-type settings and isset()
| Project: | Theme Settings |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | mikey_p |
| Status: | needs review |
This pattern is used for both readmore and comment link settings in hook_nodeapi and hook_link_alter:
<?php
$readmore_node_type = isset($settings["ts_{$node->type}_readmore"]) ? $node->type : 'default';
?>This is problematic because as soon as the theme settings page is submitted, this returns true (it's set to en empty string) regardless of whether their is text or not. This also, fails to check whether the "Enable separate settings for each content type" is set.
What I would propose, is a separate setting such as ts_{$node->type}_enable_custom_settings for each content type, and eliminate the global "Enable separate settings for each content type" setting. An added benefit of this, is that having a checkbox with this option for each content type could be used to dependently show or hide the form options for that content type (instead of attempting to figure it out in the form alter, for the fieldsets).
In order to make this reasonably upgrade compatible, a hook_update_N function could loop over the theme_settings and if ts_{$type}_readmore is set to some value (and it's different than the default) then we could set the custom setting for that type to true.
Thoughts?
#1
Yes I know, no patch should be this big, but trust me, it's worth it.
This also covers: #297720: Port to Drupal 6.x? Remove per-theme settings? Assigned to: JohnAlbin and remove the per theme settings and simplifies the default loading.
This also introduces a JS file that allows dependent items in forms, and this allows simplifying the logic associated with the settings form, to allow a override checkbox for each content type, which address the problems addressed above. This also makes it easier for the end user as they only have to specify their settings on the types that they want to differ from the default, otherwise the default values are used. See the attached screenshot for more details. This also always expands the fieldset and the default items, but keeps each content type fieldset completely hidden until the user chooses to override the settings for that content type.\
This patch is a followup to: #538362: Initial port to Drupal 6 and requires that it be applied first.
#2
Setting to needs review. I haven't tested the comment bits of this, only the readmore links.