Allow for defining the plugin settings.

I don't mind the premise of this patch (allowing people to configure the strings from within the module's settings page) but it can't break backwards compatibility. That means existing translation files should continue working after upgrading. Additionally, input in the settings fields must be translatable so that it works for multiple languages. And variables need to be deleted individually since deleting anything that starts with "timeago_" could unintentionally remove variables not set by this module.

I've tweaked this so if there is a translation file then the module makes no attempt to use the strings definable via the UI. I've also added an error message to the UI form stating this.

Ideally we'd also hide the UI that allows defining custom strings in that case.

Via the new method this will be translatable via locale etc in a standard way as the strings are passed through t(); prior to being passed to the JavaScript settings.

t() does not support reading variables. This requires i18n support.

(we're missing "wordSeparator" here by the way)

String translation in Drupal 7 has a one-to-one mapping, so translating a single space to a different character or set of characters could have unexpected repercussions. Under the current system, the wordSeparator defaults to a space (which makes sense in almost all cases) and can be overridden by translation files if necessary, so since using Drupal.t() didn't make sense for it there was no reason to include it. A better way to translate the wordSeparator might be a hack like t('@wordseparator', ' '). I'm not sure whether your approach of just not translating it works for all languages.

On the variables, whilst true it's poor practice to not namespace your variables with "modulename_" and if you have a lot of variables I don't see anything too wrong with this, and plenty of other contrib projects do this. That said I can adjust the patch if you feel strongly about this.

The problem is not that this project could use variables that are improperly prefixed; the problem is that other modules could have "timeago_" at the start of their names. For example, someone could write a module like "timeago_node" or "timeago_js" or something and uninstalling this module shouldn't affect those variables.

Patch review:

1. +++ b/timeago.module
   @@ -108,6 +111,159 @@ function timeago_admin($form, $form_state) {
   +    '#title' => t('Plugin Settings'),
   

   Better: "Override Timeago script settings"

2. +++ b/timeago.module
   @@ -108,6 +111,159 @@ function timeago_admin($form, $form_state) {
   +  $form['settings']['timeago_js_refresh_millis'] = array(
   +    '#type' => 'textfield',
   +    '#title' => t('Milliseconds to refresh after'),
   +    '#required' => TRUE,
   +    '#default_value' => variable_get('timeago_js_refresh_millis', 60000),
   +  );
   

   '#title' => t('Refresh Timeago dates after'),
   '#suffix' => ' '. t('milliseconds'),

3. +++ b/timeago.module
   @@ -108,6 +111,159 @@ function timeago_admin($form, $form_state) {
   +    '#title' => t('Locale title'),
   

   '#title' => t(''Set the "title" attribute of Timeago dates to a locale-sensitive date'),
   '#description' => t('If this is disabled (the default) then the "title" attribute defaults to the original date that the Timeago script is replacing.'),

4. +++ b/timeago.module
   @@ -108,6 +111,159 @@ function timeago_admin($form, $form_state) {
   +    '#title' => t('Cutoff for using timeago'),
   +    '#description' => t('This should be in milliseconds.'),
   

   '#title' => t('Do not use Timeago dates after'),
   '#suffix' => ' '. t('milliseconds'),
   '#description' => t('Set to zero or leave blank to always use Timeago dates.'),

5. +++ b/timeago.module
   @@ -108,6 +111,159 @@ function timeago_admin($form, $form_state) {
   +  $form['settings']['strings'] = array(
   +    '#type' => 'fieldset',
   +    '#title' => t('Strings'),
   +    '#collapsible' => TRUE,
   +    '#collapsed' => TRUE,
   +  );
   

   The default value passed to each variable_get() here should be passed through t().

6. +++ b/timeago.module
   @@ -108,6 +111,159 @@ function timeago_admin($form, $form_state) {
   +  $form['settings']['strings']['warning'] = array(
   +    '#markup' => '<div class="messages warning">' . t('If you are using the JavaScript translation files the following settings will not apply, you must first remove them then make use of the string settings.') . '</div>',
   +  );
   

   This message should (A) be an error if we can detect that translation files will in fact be used, (B) use a semicolon instead of a comma, and (C) ideally use theme_status_messages() instead of assuming the default markup.

7. +++ b/timeago.module
   @@ -108,6 +111,159 @@ function timeago_admin($form, $form_state) {
   +    '#description' => t('By default this is set to " ".'),
   

   " " (a space)

8. +++ b/timeago.module
   @@ -108,6 +111,159 @@ function timeago_admin($form, $form_state) {
      return system_settings_form($form);
   

   Numeric fields you added should be validated

9. +++ b/timeago.module
   @@ -352,6 +508,15 @@ function timeago_add_js() {
   +    'refreshMillis' => intval(variable_get('timeago_js_refresh_millis', 60000)),
   +    'allowFuture' => variable_get('timeago_js_allow_future', 1) ? TRUE : FALSE,
   +    'localeTitle' => variable_get('timeago_js_locale_title', 0) ? TRUE : FALSE,
   +    'cutoff' => intval(variable_get('timeago_js_cutoff', 0)),
   

   I prefer explicit casts: (int) and (bool)

10. +++ b/timeago.module
    @@ -361,4 +526,34 @@ function timeago_add_js() {
    +    // If the JavaScript translation files are not in use, we can pass the
    +    // string settings in.
    

    Why is this done before translation? Also the default values passed to variable_get here should be passed through t() first

11. +++ b/timeago.module
    @@ -361,4 +526,34 @@ function timeago_add_js() {
    +    // Translate the strings.
    +    $settings['strings'] = array_map('t', $settings['strings']);
    

    Only fields with values should be translated; t('') makes no sense

If we want to provide the opportunity to translate these strings then we should implement hook_variable_info so it can be translatable via i18n + variable modules (see link). This would mean that we shouldn't t the default values in the form, and in timeago_add_js(); we would switch between using variable_get(); or variable_get_value(); if it's present.

This is basically what should happen.

Changed with slight tweak, we don't want to allow for anything other than an int.

Actually I was suggesting custom validation to allow leaving the field blank, because I dislike the pattern of using zero as a default value for non-coders.

theme_status_messages(). behaves a bit different, you can't pass it the messages to output as it grabs anything set via drupal_set_messages();. Open to suggestion on this.

There is a workaround but I forget what it is. Oh well.

I'm pretty happy with this, just a few remaining (mostly semantic) fixes. Thanks for working on this.

1. +++ b/timeago.js
   @@ -2,6 +2,7 @@
   +    jQuery.extend(jQuery.timeago.settings, Drupal.settings.timeago);
   

   Use $, not jQuery. $ gets captured in scope with the version of jQuery that exists when the file is loaded, whereas if you use jQuery it will use whatever has since taken over that variable name, which may not have the relevant timeago methods and properties attached.

2. +++ b/timeago.js
   @@ -12,7 +13,7 @@ Drupal.behaviors.timeago = {
   -$.timeago.settings.strings = {
   +/*$.timeago.settings.strings = {
   

   Instead of commenting out code, I'd prefer to just remove it

3. +++ b/timeago.module
   @@ -108,6 +111,76 @@ function timeago_admin($form, $form_state) {
   +    '#description' => t('Set to zero to always use Timeago dates.'),
   +    '#required' => TRUE,
   +    '#default_value' => variable_get('timeago_js_cutoff', 0),
   +    '#element_validate' => array('element_validate_integer'),
   

   element_validate_integer is incorrect, because it will pass for negative integers. The description should be "Leave blank to always use Timeago dates," the default value should be the empty string, and the validation condition should be empty($val) || (is_numeric($val) && intval($val) == $val && $val > 0). No need to change the code that populates the JavaScript settings because it already casts to an integer.

4. +++ b/timeago.module
   @@ -108,6 +111,76 @@ function timeago_admin($form, $form_state) {
   +    '#markup' => '<div class="messages warning">' . t('If you are using the JavaScript translation files the following settings will not apply; you must first remove them hen make use of the string settings.') . '</div>',
   

   "remove them to make use of"... also, we should be able to detect whether a JS file will be used instead and change the error to say that the settings definitely won't apply.

5. +++ b/timeago.module
   @@ -352,6 +443,15 @@ function timeago_add_js() {
   +    'refreshMillis' => (int) intval(variable_get('timeago_js_refresh_millis', 60000)),
   +    'allowFuture' => (bool) variable_get('timeago_js_allow_future', 1),
   +    'localeTitle' => (bool) variable_get('timeago_js_locale_title', 0),
   +    'cutoff' => (int) intval(variable_get('timeago_js_cutoff', 0)),
   

   (int) intval(...) is redundant

6. +++ b/timeago.module
   @@ -361,4 +461,136 @@ function timeago_add_js() {
   + * Helper function to grab an array of settings used by the timeago plugin.
   

   Function descriptions should be in active tense, e.g. "Grab an array of settings used by the Timeago plugin."

7. +++ b/timeago.module
   @@ -361,4 +461,136 @@ function timeago_add_js() {
   + * This is used to build the admin form, integration with the variable module
   + * and at the point of output when populating the JavaScript settings array.
   

   "This is used to build the admin form, integrate with the variable module, and populate the JavaScript settings array."

Committed to dev, thanks. I made a few tweaks and attached the interdiff.

Going to let this sit in -dev for a bit as I'd like some additional testing before making a stable release.

Thanks for working on this!

:C No commit credit!

Your name is in the commit message? That's what the docs say to do. I use a graphical git client but I ran the equivalent command to what Dreditor suggests.

How long do you expect you'll have this in dev?

I would like more people to test it. Historically, the majority of bugs in this module have been found after a change has been made to how the JavaScript for the plugin is added to the page. In practice, no one is using the dev branch right now according to the usage stats, so I might just wait a few weeks and then tag a new release anyway.

There are a few things I'd like to check on related to my needs elsewhere in the module but it's been over a year since this project has had a stable release. What would you need to feel comfortable tagging another release?

I'm happy to add you as a maintainer if you'd like, as long as you give me a reasonable amount of time to review changes you make before committing, at least for awhile.