Customized Title

Thanks for this great patch! Here is my review:

* Is the patch in unified diff format? Yep
* Does the patch meet Drupal's coding standards? The only problem I see is that the CSS isn't alphabetized, which I've correct in this patch.
* Is the code secure? Don't know.
* If the patch makes major changes to a function or adds a new one, does it have appropriate code comments? No major changes.
* Does the patch degrade performance and does it need benchmarking? No
* Does the patch add theme functions or .tpl.php files where appropriate? NA
* If new pages are added, are they are in a logical place? If forms or other UI elements are added, are they intuitive and logical to use? Does it come with appropriate contextual help? Is the interface consistent? NA

Let me know if this patch works!

+++ service_links.module	13 Jun 2010 21:04:53 -0000
@@ -493,7 +494,8 @@ function theme_service_links_build_link(
+  $title = variable_get('service_links_links_title', t('Bookmark/Search this post with'));
+  return '<div class="service-links"><div class="service-label">'. t($title) .' </div>'. theme('links', $links) .'</div>';

If the user doesn't enter anything in the variable the default text is ran through t() twice. Recommend replacing with the following where default isn't run through t():

  $title = variable_get('service_links_links_title', 'Bookmark/Search this post with');
  return '<div class="service-links"><div class="service-label">'. t($title) .' </div>'. theme('links', $links) .'</div>';

The fieldset and CSS stuff is another feature and typically it's best to separate features in patches. One to make the title customizable and another for fieldset + CSS, but I'll let the module maintainer make the call. I haven't applied the patch, just looked at it.

Actually the $title placed directly in t() would open a cross-site scripting attack via the links title. Text pass directly to t() is not sanitized. You should instead do something like following example, or review http://drupal.org/writing-secure-code for other approaches.

$title = variable_get('service_links_links_title', 'Bookmark/Search this post with');
return '<div class="service-links"><div class="service-label">'. t('@title', array('@title' => $title)) .' </div>'. theme('links', $links) .'</div>';

Thanks Coltrane. I've added your suggestions to the patch. I also found some missing spaces on lines 12-15 that I corrected. So- so far this patch adds in customized title, created CSS files for the admin and node displays for vertical tabs, and corrects some code styling.

Good to see this coming up, and with i18n support. Thanks!

#5: What do you mean when you say "node displays for vertical tabs?" I have the vertical tab module installed but I'm not seeing any integration with vertical tabs. Sorry if I misunderstood what that actually does.

Added it, Thank you!