Problem/Motivation

Originally started as a request to add localisation to the jQuery UI datepicker, the issue is now targeted at fixing an error that remained in that addition:
- Incorrect indexing of the $libraries array in hook_library_info_alter.

Remaining tasks

- Get it RTBC. done on #130 and #132
- Backport to D7 (easy, once it is in D8). done on #131
- Document the original request.

Original report by [Rob Loach]

Once #315100: Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries) is in, we can provide internationalisation support for the jQuery UI date picker.

This patch implements locale_library_alter() and adds in the locale language translation if it exists. The language files were found from the jQuery UI dev bundle 1.7.2 i18n minified folder.

Files: 
CommentFileSizeAuthor
#129 507502-129-D7.patch7.02 KBfietserwin
PASSED: [[SimpleTest]]: [MySQL] 39,165 pass(es).
[ View ]
#129 507502-129-with-calendar-popup-at-authoring-date-D7.patch8.08 KBfietserwin
PASSED: [[SimpleTest]]: [MySQL] 39,160 pass(es).
[ View ]
#125 507502-125-D7.patch7.03 KBfietserwin
PASSED: [[SimpleTest]]: [MySQL] 39,157 pass(es).
[ View ]
#125 507502-125-with-node-test-D7.patch8.08 KBfietserwin
PASSED: [[SimpleTest]]: [MySQL] 39,169 pass(es).
[ View ]
#122 core-js-ui-datepicker-locale-507502-122.patch6.91 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 36,851 pass(es).
[ View ]
#117 507502_117_locale_jquery.patch8 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 36,825 pass(es).
[ View ]
#114 507502_locale_jquery.patch8.01 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 36,811 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#113 507502_locale_jquery.patch0 bytescosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 36,814 pass(es).
[ View ]
#111 507502_108_local_jquery.patch8.04 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 36,800 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#109 507502_106_local_jquery.patch8.04 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#94 drupal-507502-94-D8.patch7.82 KBfietserwin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-507502-94-D8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#86 drupal-507502-86-D8.patch7.86 KBfietserwin
PASSED: [[SimpleTest]]: [MySQL] 35,358 pass(es).
[ View ]
#86 507502-86-D7.patch.txt8.13 KBfietserwin
#83 drupal-507502-70.patch7.86 KBfietserwin
PASSED: [[SimpleTest]]: [MySQL] 35,361 pass(es).
[ View ]
#82 507502-82.patch8.16 KBfietserwin
PASSED: [[SimpleTest]]: [MySQL] 38,717 pass(es).
[ View ]
#78 507502-78.patch7.7 KBfietserwin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 507502-78.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#70 drupal-507502-70.patch7.86 KBfietserwin
PASSED: [[SimpleTest]]: [MySQL] 35,792 pass(es).
[ View ]
#69 drupal-507502-69.patch5.3 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 35,724 pass(es).
[ View ]
#67 drupal-507502-67.patch5.21 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 35,609 pass(es), 0 fail(s), and 1,379 exception(s).
[ View ]
#63 drupal8.locale-datepicker.63.patch5.19 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,553 pass(es).
[ View ]
#61 drupal8.locale-datepicker.61.patch2.66 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,549 pass(es).
[ View ]
#60 507502-60.patch2.34 KBfietserwin
PASSED: [[SimpleTest]]: [MySQL] 34,410 pass(es).
[ View ]
#57 507502-57.patch2.25 KBfietserwin
PASSED: [[SimpleTest]]: [MySQL] 34,393 pass(es).
[ View ]
#50 507502-50.patch2.1 KBfietserwin
PASSED: [[SimpleTest]]: [MySQL] 34,397 pass(es).
[ View ]
#49 507502-49.patch1.04 KBfietserwin
FAILED: [[SimpleTest]]: [MySQL] 34,277 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#45 507502-43.patch1.06 KBfietserwin
PASSED: [[SimpleTest]]: [MySQL] 34,286 pass(es).
[ View ]
#44 507502-43.patch0 bytesfietserwin
PASSED: [[SimpleTest]]: [MySQL] 34,281 pass(es).
[ View ]
#33 507502-33.patch874 bytesfietserwin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 507502-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#31 507502-31.patch858 bytesfietserwin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 507502-31.patch.
[ View ]
#21 507502.patch1.94 KBfietserwin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 507502_0.patch.
[ View ]
#18 drupal-locale-507502-17.patch2.47 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 31,566 pass(es).
[ View ]
#13 jqueryuii18n.patch72.76 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 19,921 pass(es).
[ View ]
#11 locale.datepicker.patch3.25 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 19,598 pass(es).
[ View ]
#10 0001-Provide-Locale-support-for-jQuery-UI.patch1.38 KBmarvil07
PASSED: [[SimpleTest]]: [MySQL] 19,593 pass(es).
[ View ]
#2 507502.patch3.02 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 18,726 pass(es).
[ View ]
datepickeri18n.patch42.69 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch datepickeri18n.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Comments

Damien Tournoud’s picture

Why not using our Drupal.t() function for this?

It might be as simple as creating a ui.datepicker.drupal.js and putting this inside:

<?php
$.datepicker.regional['drupal-locale'] = {
 
closeText: Drupal.t('Done'),
 
prevText: Drupal.t('Prev'),
 
nextText: Drupal.t('Next'),
 
currentText: Drupal.t('Today'),
 
monthNames: [
   
Drupal.t('January'),
   
Drupal.t('February'),
   
Drupal.t('March'),
   
Drupal.t('April'),
   
Drupal.t('May'),
   
Drupal.t('June'),
   
Drupal.t('July'),
   
Drupal.t('August'),
   
Drupal.t('September'),
   
Drupal.t('October'),
   
Drupal.t('November'),
   
Drupal.t('December')
  ],
 
monthNamesShort: [
   
Drupal.t('Jan'),
   
Drupal.t('Feb'),
   
Drupal.t('Mar'),
   
Drupal.t('Apr'),
   
Drupal.t('May'),
   
Drupal.t('Jun'),
   
Drupal.t('Jul'),
   
Drupal.t('Aug'),
   
Drupal.t('Sep'),
   
Drupal.t('Oct'),
   
Drupal.t('Nov'),
   
Drupal.t('Dec')
  ],
 
dayNames: [
   
Drupal.t('Sunday'),
   
Drupal.t('Monday')
   
Drupal.t('Tuesday')
   
Drupal.t('Wednesday')
   
Drupal.t('Thursday')
   
Drupal.t('Friday')
   
Drupal.t('Saturday')
  ],
 
dayNamesShort: [
   
Drupal.t('Sun')
   
Drupal.t('Mon')
   
Drupal.t('Tue')
   
Drupal.t('Wed')
   
Drupal.t('Thu')
   
Drupal.t('Fri')
   
Drupal.t('Sat')
  ],
 
dayNamesMin: [
   
Drupal.t('Su')
   
Drupal.t('Mo')
   
Drupal.t('Tu')
   
Drupal.t('We')
   
Drupal.t('Th')
   
Drupal.t('Fr')
   
Drupal.t('Sa')
  ],
 
dateFormat: 'mm/dd/yy',
 
firstDay: 0,
 
isRTL: false
};

$.
datepicker.setDefaults($.datepicker.regional[Drupal.t('drupal-locale')]);
?>

The only real thing to figure out is how to deal with the last three parameters, which are more localization then translation (dateFormat, firstDay, isRTL). We do have this information now in Drupal, we only need to properly pass it to the javascript layer.

RobLoach’s picture

Priority:Normal» Critical
Issue tags:+Libraries
StatusFileSize
new3.02 KB
PASSED: [[SimpleTest]]: [MySQL] 18,726 pass(es).
[ View ]

Deal! Where would we get the dateFormat attribute?

#510334: Cache the sets of JavaScript/CSS in drupal_get_library() would be nice because then it wouldn't run the alter every page load.

Pushing to critical as we need some localization for the date picker.

RobLoach’s picture

Status:Postponed» Needs review

Still applies cleanly to HEAD.

drifter requested that failed test be re-tested.

RobLoach’s picture

Status:Needs review» Needs work
sun.core’s picture

This solution is too simple to not count as not critical. But we need to demote in case we don't get this in by March 1st. So let's get this in!

cosmicdreams’s picture

Status:Needs work» Needs review

#2: 507502.patch queued for re-testing.

tstoeckler’s picture

Just a pointer because it seems all the right people are in this issue and it seems very related: #488496: Implement missing msgctx context in JS translation for feature parity with t()

RobLoach’s picture

Status:Needs review» Needs work

It's hook_library_alter(&$libraries, $module), not hook_library_alter(&$libraries).

marvil07’s picture

Status:Needs work» Needs review
StatusFileSize
new1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 19,593 pass(es).
[ View ]
RobLoach’s picture

StatusFileSize
new3.25 KB
PASSED: [[SimpleTest]]: [MySQL] 19,598 pass(es).
[ View ]

Thanks for updating. Probably need locale.datepicker.js in there too.

mfer’s picture

Just to clarify, we are not going to use the jQuery UI translation files but instead use our own? I do understand the lack of overlap between the translation files offered for jQuery UI and for Drupal.

RobLoach’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new72.76 KB
PASSED: [[SimpleTest]]: [MySQL] 19,921 pass(es).
[ View ]

What if we did both solutions?............. DON'T COMMIT THIS! Look at #11.

Damien Tournoud’s picture

Status:Needs review» Reviewed & tested by the community

There is absolutely no reason to use the translation files provided by jQuery, because we already have our own Javascript translation system. Using those files would make those translation not use the proper workflow (they will not be exportable as .po files, would not be imported when loading new languages, will not be modifiable by the translation UI).

RTBC-ing #11 (careful, not #13!).

webchick’s picture

Priority:Critical» Normal
Status:Needs review» Needs work
Issue tags:+Needs Documentation

Oh, good. ;) #13 is scary. ;)

This looks good to me from the Drupal side, and I guess since both Rob and sun think it's good to go, it must be.

Committed #11 to HEAD.

However, I think we need to document this ... somewhere. People who are used to jQuery UI will assume it works a certain way and we are effectively "forking" it to use our own localization system (which is fine with me; consistency++).

Is there a handbook page on localizing JS where we can add a note about jQuery UI libraries? This would be useful information for those coming from "stock" jQuery UI, as well as anyone trying to write a replacement for Locale, or for whoever writes the 7.x version of jQuery Update module.

marvil07’s picture

Not sure, bu tit seem like Translating strings in JavaScript is the relevant documentation page.

The problem is that page is for 6.x, and I'm really not sure if we have to mention it there or create another page, any suggestion?

fietserwin’s picture

Component:javascript» locale.module
Category:task» bug
Priority:Normal» Critical

I don't know what happened, but there are still errors in this part of code as if this has never been run/tested.

file: locale.module: The $libraries that get passed in is not indexed by the module name 'system', correct as follows:

function locale_library_alter(&$libraries, $module) {
  global $language;
//  if ($module == 'system' && isset($libraries['system']['ui.datepicker'])) {
  if ($module == 'system' && isset($libraries['ui.datepicker'])) {
    $datepicker = drupal_get_path('module', 'locale') . '/locale.datepicker.js';
//    $libraries['system']['ui.datepicker']['js'][$datepicker] = array('group' => JS_THEME);
//    $libraries['system']['ui.datepicker']['js'][] = array(
    $libraries['ui.datepicker']['js'][$datepicker] = array('group' => JS_THEME);
    $libraries['ui.datepicker']['js'][] = array(
      'data' => array(
        'jqueryuidatepicker' => array(
          'rtl' => $language->direction == LANGUAGE_RTL,
          'firstDay' => variable_get('date_first_day', 0),
        ),
      ),
      'type' => 'setting',
    );
  }
}

file: locale.datepicker.js: from line 39 onwards, comma's are missing at the end of the lines, correct as follows:

    ...
    Drupal.t('Monday'),
    Drupal.t('Tuesday'),
    Drupal.t('Wednesday'),
    Drupal.t('Thursday'),
    Drupal.t('Friday'),
    Drupal.t('Saturday')
  ],
  dayNamesShort: [
    Drupal.t('Sun'),
    Drupal.t('Mon'),
    Drupal.t('Tue'),
    Drupal.t('Wed'),
    Drupal.t('Thu'),
    Drupal.t('Fri'),
    Drupal.t('Sat')
  ],
  dayNamesMin: [
    Drupal.t('Su'),
    Drupal.t('Mo'),
    Drupal.t('Tu'),
    Drupal.t('We'),
    Drupal.t('Th'),
    Drupal.t('Fr'),
    Drupal.t('Sa')
  ],
  dateFormat: Drupal.t('mm/dd/yy'),
  firstDay: Drupal.settings.jqueryuidatepicker.firstDay,
  isRTL: Drupal.settings.jqueryuidatepicker.rtl
};
$.datepicker.setDefaults($.datepicker.regional['drupal-locale']);

})(jQuery);

We're finally there. The js now gets included, does not contain syntax errors anymore, but now fails on retrieving the valuesDrupal.settings.jqueryuidatepicker.firstDay and Drupal.settings.jqueryuidatepicker.rtl as these are not yet defined when this script is loaded.

Rendered HTML:

...
<script type="text/javascript" src="http://localhost/drupal7-test/modules/locale/locale.datepicker.js?v=1.8.7"></script>
<script type="text/javascript">
<!--//--><![CDATA[//><!--
jQuery.extend(Drupal.settings, {...,"jqueryuidatepicker":{"rtl":false,"firstDay":"1"}});
//--><!]]>
</script>

So the load is done before defining the settings. I have no clue how to solve this order problem, so I did not attach a patch for the obvious errors, I just outlined them in the text.

tim.plunkett’s picture

Priority:Critical» Major
StatusFileSize
new2.47 KB
PASSED: [[SimpleTest]]: [MySQL] 31,566 pass(es).
[ View ]

Here's a patch encompassing the changes from the last comment. Not sure if this should be its own issue or not, so leaving here.

fietserwin’s picture

Thanks, it's a step but this doesn't (completely) solve the issue, that's why I didn't make a patch out of it.

Any ideas on how to solve the ordering problem?

Can we wrap the code of local.datepicker.js in a function and call that at the end of the Drupal.settings (e.g. by adding a fake property that gets "initialized" with the result of the wrapping function call. something like:

file: locale.datepicker.js:

function locale_datepicker_delay_until_after_drupal_settings_is_initialized() {
  ...
  all current code
  ...
  return true;
}

and in the locale.module library_alter hook:

    $libraries['ui.datepicker']['js'][] = array(
      'data' => array(
        'jqueryuidatepicker' => array(
          'rtl' => $language->direction == LANGUAGE_RTL,
          'firstDay' => variable_get('date_first_day', 0),
        ),
        'jqueryuidatepicker_localize_now' => 'locale_datepicker_delay_until_after_drupal_settings_is_initialized()',
      ),

Or can this be done using some features already available in Drupal?
Anybody any other ideas or suggestions?

tstoeckler’s picture

JavaScript is added to the page in groups and inside of those groups by a weighting system.
jQuery UI has a weight of -11, so if you want the settings to go before that, you need to choose a weight of e.g. -12, and you also need to choose the same group. For example:
drupal_add_js($settings, array('type' => 'setting', 'group' => JS_LIBRARY, 'weight' => -12));
That should work.

fietserwin’s picture

StatusFileSize
new1.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 507502_0.patch.
[ View ]

Settings are always output at the end (see implementation of drupal_get_js), so this didn't do the trick. What does work though is using one of these:

    $libraries['ui.datepicker']['js'][$datepicker] = array('scope' => 'footer', 'group' => JS_THEME);
    $libraries['ui.datepicker']['js'][$datepicker] = array('defer' => 'defer', 'group' => JS_THEME);

I'm not sure about which one to use. Using scope moves the line to the end of the rendered html output. Using defer, defers execution and is used by e.g. administration menu. But I'm not sure if all (modern) browsers act the same on this attribute. So I'm inclined to use the scope. So, the attached patch uses the scope key.

tstoeckler’s picture

Settings are always output at the end (see implementation of drupal_get_js), so this didn't do the trick.

If that's true, that's a bug.
If for some reason now a bug with drupal_add_js(), then at least with the documentation.

fietserwin’s picture

Status:Needs work» Needs review
rfay’s picture

Status:Needs review» Needs work

Setting to CNW just because the testbot is testing over and over and we need to figure out why. OK to retest later.

xandeadx’s picture

please read http://drupal.org/node/507502#comment-4129142 . $libraries['system']['ui.datepicker'] always return FALSE

fietserwin’s picture

Status:Needs work» Needs review
tim.plunkett’s picture

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7
RobLoach’s picture

Status:Needs review» Needs work
Issue tags:+Needs Documentation, +localization, +jQuery UI, +drupal_add_library, +Libraries, +needs backport to D7

The last submitted patch, 507502.patch, failed testing.

fietserwin’s picture

Status:Needs work» Needs review

This patch is against 7.2. The js part has now been patched, probably as the result of another issue. What remains is the wrong indexing of the $libraries array and the order problem.

If it patches against 8.x-dev it can be rolled against both versions, if not some work has to be done to get a D8 patch.

fietserwin’s picture

StatusFileSize
new858 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 507502-31.patch.
[ View ]

oops, forgot the patch.

Status:Needs review» Needs work

The last submitted patch, 507502-31.patch, failed testing.

fietserwin’s picture

Status:Needs work» Needs review
StatusFileSize
new874 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 507502-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And now with the correct directory offset...

sun’s picture

Priority:Major» Normal
Status:Needs review» Needs work
Issue tags:+Needs tests

The changes of this patch look like we clearly forgot to add tests for the functionality. Of course, we can't test JS, but we can test expected loading of JS files.

RobLoach’s picture

Status:Needs work» Needs review

There are tests to check whether Libraries are alterable via common_test_library_info_alter() and JavaScriptTestCase::testLibraryAlter(). We should probably have tests in locale.test to make sure the JavaScript library gets locale.datepicker.js and the Data array accordingly though.

Throwing back to review to see how badly the test bot needs a re-roll of this.

RobLoach’s picture

Status:Needs review» Needs work

The last submitted patch, 507502-33.patch, failed testing.

osysltd’s picture

datepickeri18n.patch queued for re-testing.

osysltd’s picture

Any working patch?

fietserwin’s picture

#33 should work when rerolled for the move to /core but that has not been done yet.

Pasqualle’s picture

the datepicker date format should not depend on locale module..
US dates are mm/dd/yyyy whereas UK dates are dd/mm/yyyy
So without enabling the locale module I should be able to change the date format used in detepicker..

KarenS’s picture

Priority:Normal» Major

I was told to remove date translations from Date because core is handling this and am getting lots of bug reports because it isn't working. Webform dates are also being bitten by this #1326230: In webform with popup datepicker not appears the right translation. If we can't get this into core soon D7 contrib modules are going to have to go back to providing something themselves because it is totally non-functional the way it is on multilingual sites.

This is not a high priority for core, per se, but it is creating big problems down the line.

fietserwin’s picture

OK, first a new patch. Since #33, the hook name has been changed from hook_library_alter to hook_library_info_alter and we have had the move to /core.

#34, #35: I am willing to add a test, but I don't understand what the mentioned common_test_library_info_alter() is supposed to do or how it is used in a test, nor how I can test for expected loading of JS files.

fietserwin’s picture

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,281 pass(es).
[ View ]

and now the patch :(

fietserwin’s picture

StatusFileSize
new1.06 KB
PASSED: [[SimpleTest]]: [MySQL] 34,286 pass(es).
[ View ]

And now a non-empty patch ...

KarenS’s picture

I tried the patch in #33 on D7 and it seems to work fine. The patch at #45 won't work in D7 because of structural changes. But I think they are essentially the same.

jannise’s picture

#45: 507502-43.patch queued for re-testing.

xjm’s picture

  • The next thing we need here is an automated test to confirm the file is loaded properly. The test should fail without the patch, and pass with it.
  • If someone could post a list of steps to manually test the issue--which demonstrate the bug before the patch--that would also be helpful.
  • Finally, since this issue has gone through at least three different phases I can see, an issue summary would probably help as well.

Tagging as novice for any of the above.

fietserwin’s picture

StatusFileSize
new1.04 KB
FAILED: [[SimpleTest]]: [MySQL] 34,277 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Look s a bit like overkill to me. Bottom line, we are talking 2 minimal changes here:

1) using $module to access the array passed by drupal_alter('library_info', ...)

From the calling code (common.inc, function drupal_get_library) it is very clear that the $libraries parameter is NOT indexed by the module name. The only other use of this alter hook can be found in common_test.module (in core/modules/simpletest/tests) which does not use the module name to index the array

2) Moving the library to the footer.

See #17, #19, #21 about locale.datepicker.js accessing Drupal.settings on load and why that is too early if it is placed in the header. AFAICC, It won't be possible to test this.

However, a test is always a good thing. So here is a test only patch, supposed to fail.

fietserwin’s picture

StatusFileSize
new2.1 KB
PASSED: [[SimpleTest]]: [MySQL] 34,397 pass(es).
[ View ]

And a patch with the solution as posted in #45 and its test from #49. Thus this is the patch as should get committed.

xjm’s picture

Issue tags:-Needs tests

Thanks @fietserwin. That test looks fine.

fietserwin’s picture

fietserwin’s picture

@KarenS (or others): if you want progress, have it reviewed by 1 more person (e.g. a colleague), and set it to RTBC if no further problems/questions are found.

xjm’s picture

@fietserwin -- That's where my comment in #48 comes in, to crowd source the review. A clear summary/steps to test can bring in more eyes and get a patch in faster.

Also, an aside, I think KarenS probably has a good handle on Drupal issue queues. ;)
http://drupal.org/user/45874
http://certifiedtorock.com/u/45874

RobLoach’s picture

#50: 507502-50.patch queued for re-testing.

RobLoach’s picture

Status:Needs review» Needs work
+++ b/core/modules/locale/locale.testundefined
@@ -2823,3 +2823,28 @@ class LocaleLanguageNegotiationInfoFunctionalTest extends DrupalWebTestCase {
   }
}
+
+class LocaleLibraryInfoAlterTest extends DrupalWebTestCase {
+  public static function getInfo() {

Looks great! Minor: We might want to stick a documentation code block in here to keep our PHP docs clean ;-) . Other than that, looks pretty good to me.

-4 days to next Drupal core point release.

fietserwin’s picture

Status:Needs work» Needs review
StatusFileSize
new2.25 KB
PASSED: [[SimpleTest]]: [MySQL] 34,393 pass(es).
[ View ]

Modified as per #56.

fietserwin’s picture

Issue summary:View changes

issue summary

RobLoach’s picture

Status:Needs review» Reviewed & tested by the community
sun’s picture

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/locale/locale.module
@@ -935,11 +935,11 @@ function locale_css_alter(&$css) {
+    $libraries['ui.datepicker']['js'][$datepicker] = array('scope' => 'footer', 'group' => JS_THEME);

Both the scope and group look bogus/questionable to me.

At least the group is wrong - this file is added by a module, not by a theme.

On the scope, I'm not sure whether this was done on purpose (and not documented), or whether it's unnecessary.

fietserwin’s picture

Status:Needs work» Needs review
StatusFileSize
new2.34 KB
PASSED: [[SimpleTest]]: [MySQL] 34,410 pass(es).
[ View ]

- scope is per #17, #19 and #21. In short, if placed in head it will be placed before Drupal.settings which it accesses. I will add a comment.
- group has not been changed, that is already in the current version. I'm fine if we want to "solve" that in this issue as well. Note that not specifying it, will result in JS_LIBRARY.

sun’s picture

StatusFileSize
new2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 34,549 pass(es).
[ View ]

1) The comment states that it accesses Drupal.settings on load (and the JS code seems to confirm this). Drupal.settings is already defined on load, as it is declared in the global scope when the file core/misc/drupal.js is loaded (i.e., it exists before load).

2) Good point about the default group of JS_LIBRARY for items loaded via drupal_add_library(). Actually, in light of 1), and given that locale.datepicker.js merely defines an object and calls into datepicker afterwards, the only dependency seems to be on the loaded translations. As of now, the JS translations are loaded at the end of locale_js_alter using the default weight of JS_DEFAULT. This means we should explicitly use JS_DEFAULT, too, and additionally assign a >0 weight.

See attached patch.

That said, how do you test this? ui.datepicker doesn't seem to be used anywhere throughout core. ;)

KarenS’s picture

If we can keep both a D7 and a D8 patch going, we can test the D7 patch using the Date Popup module in Date API. I tried doing that back in #46, but the patch has changed and won't work in D7.

This needs to be backported to D7 anyway, so it's not really extra work to do both.

sun’s picture

StatusFileSize
new5.19 KB
PASSED: [[SimpleTest]]: [MySQL] 34,553 pass(es).
[ View ]

So this patch changes a bit more, contains more comments and explanations, and additionally - temporarily - changes the node form to use a datepicker for the node's authored date (not functional though, only triggers the datepicker, mind you).

fietserwin’s picture

#61 1) On load of the file, not the html document. This may indeed be confusing.

#61 2) Drupal.settings may be declared in core/misc/drupal.js, but is it is only filled (extend'ed) when the settings are rendered, which is indeed in JS_SETTING. So #63 will work, while #61 won't.

To be sure that the code in this file is executed after Drupal.settings has been extend'ed we could:

  • Use the code as in #60 :).
  • Use the code as in #63.
  • Change #63 to something like group => JS_SETTING + 1 and leave out the weight, so we are absolutely sure it will be after the settings. IMHO this even better expresses that we want to be rendered after the settings.
  • Wrap the javascript code in locale.drupal.js in a Drupal behavior, but I am not sure about the consequences (too late instead of too early?).

Pick your preference.

naoliva’s picture

#33: 507502-33.patch queued for re-testing.

giga-b’s picture

Hi,
Sorry, my english is too bad...
Is there any way to translate directly to other language? My web is only on spanish and the popup(dropdown) of the calendar is in english.
Thanks!

tim.plunkett’s picture

StatusFileSize
new5.21 KB
FAILED: [[SimpleTest]]: [MySQL] 35,609 pass(es), 0 fail(s), and 1,379 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal-507502-67.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new5.3 KB
PASSED: [[SimpleTest]]: [MySQL] 35,724 pass(es).
[ View ]

I went to all that trouble to find out why the patch wouldn't apply cleanly, and I still didn't fix it right.

fietserwin’s picture

StatusFileSize
new7.86 KB
PASSED: [[SimpleTest]]: [MySQL] 35,792 pass(es).
[ View ]

Some changes to the previous patch:
- locale.datepicker.js: placed the js code in in an attach behavior thereby preventing all the difficulties we had in assuring correct order.
- locale.module: as a consequence the changes to the code in locale_library_info_alter() could be simplified.
- locale.test: a test comment still referred to adding it to the footer.
- nodes.pages.inc: the addition to the node edit form (for test purposes only) no longer has to include group and weight.

Notes:
- Placing the js code in an attach behavior seems the right thing to do. It executes not too early but also not too late, i.e. before a user can popup a date picker. Even if another attach behavior would initialize a date picker right away, this behavior will execute earlier as it is registered earlier because it is in the JS_LIBRARY group.

- This issue is tagged as needs manual testing. Even in a clean D8 environment this is not too difficult because of sun's nice addition to the node edit form in this patch. enable language and locale, add a language, define a translation for the current month, add a basic page, and go to the authoring tab.

- The first day of the week setting is a setting that is not defined by the locale module. Yet, with the locale module disabled, this setting will not be passed to the date picker, but this should be filed as a separate error.

cosmicdreams’s picture

If I have time I will manually test Friday night.

ace11’s picture

Tryed to translate calendar with patch for D7, but no luck. Has anyone succeeded?

fietserwin’s picture

It works for my sites. You probably need to clear the caches though, to get new translate javascript files.

ace11’s picture

Which patch did you use?

Actually I got it work, kind of, BUT I have customized calendar so it shows calendar straight at page (no popup). So problem is that translation works after user clicks/changes month and ofcourse it is not good.

chrispane8’s picture

#33: 507502-33.patch queued for re-testing.

fietserwin’s picture

Version:8.x-dev» 7.x-dev

Switch to 7.x to test patch #33. I will be resetting to 8.x afterwards.

#74: Apparently you are showing the calendar too early. At what moment are you executing the js to display the calendar? I guess it should be in a Drupal attach behavior. And even that may be too early as patch #33 places the code in the footer of the page. Patch from #70 should solve that as well, but is for D8. I will see if I can post a D7 patch based on #70 later this evening.

fietserwin’s picture

#33: 507502-33.patch queued for re-testing.

fietserwin’s picture

StatusFileSize
new7.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 507502-78.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rework of patch #70 for D7.

Status:Needs review» Needs work
Issue tags:-Needs Documentation, -Novice, -Needs manual testing, -needs backport to D7

The last submitted patch, 507502-78.patch, failed testing.

fietserwin’s picture

Status:Needs work» Needs review

#78: 507502-78.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs Documentation, +Novice, +Needs manual testing, +needs backport to D7

The last submitted patch, 507502-78.patch, failed testing.

fietserwin’s picture

Status:Needs work» Needs review
StatusFileSize
new8.16 KB
PASSED: [[SimpleTest]]: [MySQL] 38,717 pass(es).
[ View ]

No idea what is wrong with my patch. It applies locally. It looks OK, but it fails....

Next try.

fietserwin’s picture

Version:7.x-dev» 8.x-dev
StatusFileSize
new7.86 KB
PASSED: [[SimpleTest]]: [MySQL] 35,361 pass(es).
[ View ]

Thus we have a working D7 patch in #82 and can back to D8. I'm reposting the patch from #70. Please review, test manually, and set to RTBC. Manual testing may be easier in D7. Patches are conceptually equal.

fietserwin’s picture

Aside: I think my my git diff does something wrong when the changes are at the end of a file. Does this ring a bell to anyone? I use git on windows and am using the git-gui and git bash that come with standard git.

ace11’s picture

Actually I'm not sure when js is executed to show calendar. I used this patch: http://drupal.org/node/775526

Just tryed your patch #83 but it still loads translation after user chances month not when page is loaded. Any other help/solution for this?

fietserwin’s picture

StatusFileSize
new8.13 KB
new7.86 KB
PASSED: [[SimpleTest]]: [MySQL] 35,358 pass(es).
[ View ]

#85: The D7 patch contains an error (a regression of an old patch: it still added the js file to the JS_THEME group instead of to the default JS_LIBRARY).

Comparing the 2 patches, I fond and removed a superfluous space in a comment in the D8 patch.

@ace11: can you confirm that the D7 patch works with inline datepickers, it did for me. That would be a great step forward to RTBC'ing this issue.

ace11’s picture

I installed new drupal to my server and I'll test it there. Where and how I should add translation file to calendar? Just to get this done right.
I already activated locale-module. Should translation be added from Translate interface -> Import and importing date-7.x.po file?

fietserwin’s picture

#87: For D7 use Localization update. For D8 just enter translation of 1 or 2 months/days via thelocal translation admin interface and/or change first day of the week.

ace11’s picture

Just did test to fresh D7 installation. Installed date, webform and Localization update -modules. Then added new language and set it to default. After that I created new webform with popup-calendar and yes. Translation works in date dropdown fields and also in popup-calendar!
And it also works if webform is patched with this patch to show calendar straight on the page (no popup).

Nice work!

Module versions I used: Date 7.x-2.5, Webform 7.x-3.17 and Localization update 7.x-1.0-beta3

I used this patch: 507502-86-D7.patch.txt
When pathcing I got this message:
patching file locale.datepicker.js
patching file locale.module
Hunk #1 succeeded at 927 (offset 7 lines).
patching file locale.test
patching file node.pages.inc
patch unexpectedly ends in middle of line
Hunk #1 succeeded at 229 with fuzz 1.

rreimche’s picture

Hmm... am I doing something wrong?

I tried to apply the patch as it is described at http://drupal.org/node/60818 and got this:

MyServer:drupalroot me$ patch -p0 < drupal-507502-86-D8.patch
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/locale/locale.datepicker.js b/core/modules/locale/locale.datepicker.js
|index 81f1e17..f819282 100644
|--- a/core/modules/locale/locale.datepicker.js
|+++ b/core/modules/locale/locale.datepicker.js
--------------------------
File to patch: q
q: No such file or directory
Skip this patch? [y] y
Skipping patch.
1 out of 1 hunk ignored
can't find file to patch at input line 153
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/locale/locale.module b/core/modules/locale/locale.module
|index 6d3ded0..928c98a 100644
|--- a/core/modules/locale/locale.module
|+++ b/core/modules/locale/locale.module
--------------------------
File to patch: ^C

Then I thought I should patch exactly the locale module and got this:

MyServer:drupalroot me$ cd modules/locale/
MyServer:locale me$ patch < drupal-507502-86-D8.patch
patching file locale.datepicker.js
patching file locale.module
Hunk #1 FAILED at 873.
1 out of 1 hunk FAILED -- saving rejects to file locale.module.rej
patching file locale.test
Hunk #1 succeeded at 2772 (offset -348 lines).
can't find file to patch at input line 232
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/node/node.pages.inc b/core/modules/node/node.pages.inc
|index 4e94b26..897dd3a 100644
|--- a/core/modules/node/node.pages.inc
|+++ b/core/modules/node/node.pages.inc
--------------------------
File to patch:

What am I doing wrong? D
Do I have to roll back to last backup before trying to patch again?

My Drupal version is 7.12

tstoeckler’s picture

You need to use the -p1 option and patch from the Drupal root directory. For more info see http://drupal.org/patch/apply

rreimche’s picture

Yep, the patch and the scheme ace11 described worked for me too. Now Datepicker is in Russian.

xjm’s picture

Thanks for all the manual testing everyone!

I reviewed the code in the patch and it looks great. I've just a few stylistic comments.

+++ b/core/modules/locale/locale.testundefined
@@ -3120,3 +3120,33 @@ class LocaleLanguageNegotiationInfoFunctionalTest extends DrupalWebTestCase {
+ * Functional test for localization of the javascript libraries.

This should start with a third-person verb. ("JavaScript" should also be capitalized.) I'd say:
"Tests localization of the JavaScript libraries."

+++ b/core/modules/locale/locale.testundefined
@@ -3120,3 +3120,33 @@ class LocaleLanguageNegotiationInfoFunctionalTest extends DrupalWebTestCase {
+ * Currently only the jQuery datepicker is localized, using Drupal translations.

I'd put "using Drupal translations" in parentheses instead of after a comma.

+++ b/core/modules/locale/locale.testundefined
@@ -3120,3 +3120,33 @@ class LocaleLanguageNegotiationInfoFunctionalTest extends DrupalWebTestCase {
+   * Verify that locale_library_info_alter adds localisation to the datepicker.

Minor issues here (wrong verb tense and localization is not using the American spelling.) I'd say simply:
"Verifies that the datepicker can be localized."

If someone could reroll the patch cleaning these things up, that would be great. Please include an interdiff with your changes. Thanks!

Also, can someone confirm that there are no regressions for the datepicker in English?

fietserwin’s picture

StatusFileSize
new7.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-507502-94-D8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Changed texts as indicated, except this one:
Currently only the jQuery datepicker is localized, using Drupal translations.
to
Currently, only the jQuery datepicker is localized using Drupal translations.

There are no regressions in English, except for first day of the week which is now taken from the Drupal setting instead of the (jquery ui datepicker) default of Sunday.

nod_’s picture

Status:Needs review» Needs work

Please change Drupal.t() to

var t = Drupal.t;

t('')

There is no need to repeat Drupal.t all over the place.

fietserwin’s picture

Status:Needs work» Needs review

If I'm correct, this is needed for the parser that creates the js files with the actual translations. Like you "may not" use $var = 'text'; $output = t($var); in your PHP.

mesimes’s picture

#33: 507502-33.patch queued for re-testing.

Pls’s picture

Applied D7 patch from #86, works great. Thanks!

GaëlG’s picture

#86 D7 worked for me too.

fietserwin’s picture

So, how do we get this in, if people only test in D7? Anyone who dares to set this to RTBC?

nod_’s picture

Duplicating Drupal.t all over the place is not good. Can someone look into how the parser is supposed to pick up translatable strings from JS files and see if renaming this would break it?

fietserwin’s picture

See file locale.module, function _locale_parse_js_file(). The regexp is explicitly looking for Drupal.t and Drupal.formatPlural.

What are your objections against the repeated use of Drupal.t? If it is file size, or actually response size, I would say that a minifier should do that replacement, even though there's not a (real) minifier in place right now.

Another option might be to allow Drupal.t to accept (multi-level) arrays/objects of strings to translate at once, returning the same structure, property names, etc.

Anyway, IMO that would be a separate issue and should not block this issue. After all we are talking a major bug here that is also present in the current D7 release.

GeduR’s picture

#86 works for me on D7 instalation, Thanks!

estepix’s picture

Same here, patch on #86 works great on a D7 installation
Will this be added to D7 Core?

nod_’s picture

Status:Needs review» Reviewed & tested by the community

meh let's just go with this and backport, i'll address my concerns in an other more global issue.

Can anyone apply the patch in #94 on D8 and rtbc if working please? D7 has been pretty much tested to death already :p that'll be an easy backport.

nod_’s picture

Status:Reviewed & tested by the community» Needs review

sorry, someone need to test for D8.

nod_’s picture

Status:Needs review» Needs work
Issue tags:+Needs Documentation, +Novice, +Needs manual testing, +needs backport to D7

The last submitted patch, drupal-507502-94-D8.patch, failed testing.

cosmicdreams’s picture

Status:Needs work» Needs review
StatusFileSize
new8.04 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

For D8: Patch needed to be rerolled due to the local.test being converted to PSR-0. I think I've made the conversion properly.

Status:Needs review» Needs work

The last submitted patch, 507502_106_local_jquery.patch, failed testing.

cosmicdreams’s picture

Status:Needs work» Needs review
StatusFileSize
new8.04 KB
FAILED: [[SimpleTest]]: [MySQL] 36,800 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Ah WebTestBase not WebTestCase (anymore). Fixed

Status:Needs review» Needs work

The last submitted patch, 507502_108_local_jquery.patch, failed testing.

cosmicdreams’s picture

Status:Needs work» Needs review
StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,814 pass(es).
[ View ]

didn't update the setUp() to new syntax. Here's another try

cosmicdreams’s picture

StatusFileSize
new8.01 KB
FAILED: [[SimpleTest]]: [MySQL] 36,811 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

screwed up the upload

Status:Needs review» Needs work

The last submitted patch, 507502_locale_jquery.patch, failed testing.

alexpott’s picture

Just change

  parent::setUp(array('locale', 'locale_test'));

to

  parent::setUp('locale');

There is no locale_test module and setUp seems to take any number of module names as strings to enable.

cosmicdreams’s picture

Status:Needs work» Needs review
StatusFileSize
new8 KB
PASSED: [[SimpleTest]]: [MySQL] 36,825 pass(es).
[ View ]

Thanks alexpott: here's the updated patch.

alexpott’s picture

Status:Needs review» Reviewed & tested by the community

Looks good for D8

fietserwin’s picture

Yessss!. Thanks for rerolling. But a note to the committer: I guess we don't want to commit the change to core/modules/node/node.pages.inc. Though it may be a nice addition on its own, It does not serve any other purpose for this issue other then to be able to manually test it.

tstoeckler’s picture

Re #119 being able to test this manually (which is required for JavaScript) is enough justification to include this with the patch.
It is also a big usability improvement on its own, but that is not the point.

nod_’s picture

Status:Reviewed & tested by the community» Needs work

Oh right i missed this one. I agree with #119, it should be removed from the patch, the HTML datetime input element will land at some point in core and should be used for this field instead of datepicker.

Also, scope creep. It's a nice UI bonus but it's not major or in scope for D8. Might be something we can backport in D7 though, that'd be cool.

minus the change in pages.inc rtbc for me too.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new6.91 KB
PASSED: [[SimpleTest]]: [MySQL] 36,851 pass(es).
[ View ]

(edit) The date format of the datepicker is totally wrong for the input, that code in node.pages.inc was for testing purpose.

nod_’s picture

Status:Needs review» Reviewed & tested by the community
catch’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Patch looks good and I agree about not adding the datepicker here at all.

Committed/pushed to 8.x, moving to 7.x for backport.

fietserwin’s picture

Issue tags:-needs backport to D7
StatusFileSize
new8.08 KB
PASSED: [[SimpleTest]]: [MySQL] 39,169 pass(es).
[ View ]
new7.03 KB
PASSED: [[SimpleTest]]: [MySQL] 39,157 pass(es).
[ View ]

Attached is the patch from #86 plus the remarks from #93 (/#94) in 2 versions: 1 with the change to the node form to manually test this patch and 1 without. both should pass, so both have .patch as extension.

fietserwin’s picture

Status:Patch (to be ported)» Needs review
fietserwin’s picture

BTW: there already is an issue about the authoring date field: #1562798: Its hard for users to provide valid input in the authoring date field .

nod_’s picture

There is one extra space on the last } in the JS file but beside that it's good to go.

fietserwin’s picture

StatusFileSize
new8.08 KB
PASSED: [[SimpleTest]]: [MySQL] 39,160 pass(es).
[ View ]
new7.02 KB
PASSED: [[SimpleTest]]: [MySQL] 39,165 pass(es).
[ View ]

Space removed. Thanks.

nod_’s picture

Status:Needs review» Reviewed & tested by the community

Works for me :) 507502-129-D7.patch is the one to commit.

David_Rothstein’s picture

Title:Provide Locale support for jQuery UI» (needs documentation) Provide Locale support for jQuery UI
Version:7.x-dev» 8.x-dev
Category:bug» task
Priority:Major» Normal
Status:Reviewed & tested by the community» Active
Issue tags:+7.15 release notes

Thanks for all the work (and testing) on this patch!

Committed to 7.x (with a small change to fix the capitalization of "JavaScript" in the test description - the D8 patch already had it): http://drupalcode.org/project/drupal.git/commit/09b754a

This still has the "Needs Documentation" tag (from way back in #15) so I think it's still supposed to be open for that.

fedExpress’s picture

Thanks, that worked for me.

fedExpress’s picture

Issue summary:View changes

Updated issue summary.

tengoku’s picture

Issue summary:View changes

updated issue summary

Patrick Storey’s picture

I am removing the Novice tag from this issue because this issue has gone back and forth between versions and has over 100 comments. That makes the issue pretty long for new contributors.

I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid

Patrick Storey’s picture

Issue tags:-Novice