Extend default max meta description length to 350 characters

Nodewords 6.x-1.x, and the official releases generated by that development snapshot (6.x-1.1, 6.x-1.2, 6.x-1.3-alpha1) use a database field that can contain text of any length; the only limit is the one set in the settings, which allows to set any number with 6 digits.

By the Google change it would be a good idea to change the modules default value of Maximum meta tags length to 350 and upgrade all sites having 255 set to 350.

Patch attached

It's not clear to me the reason of the patch, when the users can change the setting, and set the max length to the value they want.

Yes I know it's changeable, but if the most important search engine supports 350 letters we should use this as the default. Most people are not aware about the optimal defaults and this is why the current default of 255 was used in past - but this is no longer the optimal value. Default values should always have optimal settings. Not everyone is a search engine specialist :-)

If admin users don't want to allow more than 255 characters for the meta tags in their web site, then the module should respect their decision.

The least I can do is to add a note about the maximum length accepted by the search engines in the description for the form field shown in the screenshot.

Are you only arguing about my upgrade hook or the new default value in general?

Why should an admin only allow the old default of 255? If he implemented previous defaults he must also be fine with newer defaults. I'm with you with adding another sentence to the setting that Google's optimal setting is now 350, but the module should use the new 350 as default. I'm often deploying sites and I do not like to configure nodewords every time by hand to the optimal settings if the module can do this out of the box. 255 was a good setting for all sites in past - but this is now obsolete. By changing this setting to 350 all sites using nodewords can only benefit from this change by a more beautiful SERP.

PS: Nodewords are only used for SERP... there is no other reason to use nodewords module...!

After more thinking I do not like to extend the form description with the Google comment as it would invalidate the translatable string. I would only add another drupal_set_message() to the new upgrade hook and explain there that this value has been extended for better SERP. Maybe link to the blog post linked above...

Why should an admin only allow the old default of 255?

It's just a matter of preference.
The fact the search engines uses the first 350 characters from the meta tags content doesn't mean all the meta tags content must be 350 characters. Using 255 characters is still allowed.

To show a message, and reporting the above link is perfectly fine with me.

Default values are not a matter of preference. They are a matter of what's BEST for SERPs. This is why 255 was used as default in past. If someone decide to reduce it to 255 nevertheless the optimal value is 350 around the world - this is a matter of preference.

New patch attached.

The code has been changed, and committed in CVS.

Do not use t() in hook_update. It cannot shown translated as the translation files are not imported.

The upgrade for existing ~35.000 sites having the previous default setting seems missing in the commits.

I found just an Drupal update function that uses t(). I guess that translation files are not used because a recent change in Drupal core code.

function forum_update_6000() {
  $ret = array();

  $vid = variable_get('forum_nav_vocabulary', 0);
  $vocabularies = taxonomy_get_vocabularies();
  if (!isset($vocabularies[$vid])) {
    $vocabulary = array(
      'name' => t('Forums'),
      'multiple' => 0,
      'required' => 0,
      'hierarchy' => 1,
      'relations' => 0,
      'module' => 'forum',
      'weight' => -10,
      'nodes' => array('forum' => 1),
    );
    taxonomy_save_vocabulary($vocabulary);

    variable_set('forum_nav_vocabulary', $vocabulary['vid']);
  }

  return $ret;
}

The code has been changed.

You have found a minor bug in core.

Cross post, but update path is still missing

/**
 * Implementation of hook_update_N().
 */
function nodewords_update_6142() {
  $ret = array();

  // Only update if the setting have the default value.
  if (variable_get('nodewords_max_size', 255) == 255) {
    $ret[] = update_sql("UPDATE {variable} SET value = 350 WHERE name = 'nodewords_max_size'");
    drupal_set_message('The default maximum meta tags length has been extended to 350 characters to improve Google results pages. See <a href="http://googleblog.blogspot.com/2009/03/two-new-improvements-to-google-results.html">Two new improvements to Google results pages</a> in the official Google blog for more information.');
  }

  return $ret;
}

This is an incorrect fix, please use my one. You have missed that the entry may not exists in the database - and in this case the UPDATE will fail. This is why you should only use variable_set() and rely in core API functions and do not use your own buggy way. The blank "" in variable_get() is intentionally there to make sure it's only "changed" if the variable exist in variable table and has a value of 255.

+  if (variable_get('nodewords_max_size', '') == 255) {
+    variable_set('nodewords_max_size', 350);
+    $ret[] = array('success' => TRUE, 'query' => 'The maximum meta tags length has been extended to 350 letters.');
+  }

The code has been corrected.

The variable_get() is still wrong.

Why should it be wrong?

if (variable_get('nodewords_max_size', '') == 255) { is correct.

if (variable_get('nodewords_max_size', 255) == 255) { is wrong.

The second parameter is a *fallback* value and therefore if the variable is not set in variable table it becomes now 255 - nevertheless it is not saved with a value of 255 in the database. And therefore the IF evals to TRUE, but it should be FALSE.

The second parameter is a *fallback* value and therefore if the variable is not set in variable table it becomes now 255

With variable_get('nodewords_max_size', '') == 255) you cannot be sure the variable has not been set before, because in the textfield the user can insert an empty string (to notice that the settings form has never checked if the value entered for that Drupal variable was the correct one, nor the form field marked the value as required).

If you want to be sure the Drupal variable is not previously set, there are just two options:

1. Verify if the database table contains a value for that Drupal variable.
2. Use the following code:

     $value = variable_get('nodewords_max_size', NULL);
     if (isset($value) && $value == 255) {
       // Change the variable value.
     }
   

If the code as it is now is wrong, then also the proposed change is wrong.

The code has been changed as per my previous comment.

The change looks ok, but you have not understood the trick. You can use any value as fallback if it's not 255. The code should only update a value of 255. Other values are expected to be custom values and shouln't be changed or the admin loose his custom setting. So you made it more complex than it need to be. I don't care.

From your comment it also sounds like there is a validation missing. Empty strings or non-numbers are invalid and negative values also. This need to fire a form error when the form is submitted.

So you made it more complex than it need to be. I don't care.

I made it correct.
You first said the code was not correct because it set the Drupal variable nodewords_max_size even when the variable has not been set before; the only way to check if a Drupal variable (which is set through a setting page) is not set is to use NULL because is not possible to set a Drupal variable to NULL through a setting page (especially if the form field used is a textfield).

From your comment it also sounds like there is a validation missing.

The validation function was missing; I added it in one of the last commits I made before I wrote my comment #27.