Date format form is unusable

I can confirm this one, looks like a system.module issue though.

This at least makes it a bit more usable. I think there is still room for improvement though.

As far as I know it's not possible to test (using simpletests) if an element lost focus or not. In any case someone else should probably have a look at this since the fix is not really ideal.

This bug is maddening for sure. Things like the Add field form write to the page output without the field losing focus -- maybe we could look at how those work? (I thought the Place Block form used to as well but it's not behaving as I expect.)

WRT test coverage, there's some POST testing of the form in \Drupal\system\Tests\Common\FormatDateTest; maybe we could add web test coverage for the Ajax there? (The unit test coverage for the commands themselves is in \Drupal\Tests\Core\Ajax\AjaxCommandsTest, for reference, and also a whole ajax_forms_test test module it uses.)

Things like the Add field form write to the page output without the field losing focus

I don't think the add form field does an ajax call on textinput, any idea where something like that is happening besides the date format form?

WRT test coverage, there's some POST testing of the form in \Drupal\system\Tests\Common\FormatDateTest; maybe we could add web test coverage for the Ajax there?

I'll try that.

I don't think the add form field does an ajax call on textinput, any idea where something like that is happening besides the date format form?

Yeah true, that may be just Javascript. (Though actually, is there any reason the date format form couldn't also be purely client-side? All it's doing is calculating the rendered output based on the date formula. I guess It'd require something like http://phpjs.org/functions/date/ for that since it's about the PHP date format.)

Anyway, autocomplete the other example I can think of that does an Ajax call on input keystrokes, but that probably doesn't have the same problem since the rewriting is done in the field that already has focus. And actually, turns out that autocompletion doesn't use our Ajax API at all; FAPI just includes an #autocomplete_route_name property which attaches its own separate JS library and returns its own JSON response as far as I can see.

I grepped for:
grep -rl "use Drupal\\\\Core\\\\Ajax" *
Modals, editor, the upload widgets, Views UI... nothing that seems exactly analogous.

TBH I'm fine with the fix, though it does seem unintuitive that this would require two separate commands. "Do a thing and retain current focus" seems like a pretty basic expectation for Ajax. I'll ping a couple people who might know more about the API.

I guess It'd require something like http://phpjs.org/functions/date/ for that since it's about the PHP date format.

That was my guess also.

While I was investigating this a bit further I stumbled on #1904912: Display updating Javascript triggered in Format string of date formats interfers with keyboard navigation, not clear to me which one should stay open.
Definitely worth looking into.

is there any reason the date format form couldn't also be purely client-side? All it's doing is calculating the rendered output based on the date formula. I guess It'd require something like http://phpjs.org/functions/date/ for that since it's about the PHP date format.

+1
http://jsfiddle.net/vijaycs85/3834m8fz/1/ just does the jquery format very well.

Patch looks legit, but this points to one of the major flaws in our AJAX API. For as long as the client is talking to the server (i.e. Drupal), focus is going to be lost. This should not be implemented using purely the AJAX API, but should have some custom JS to avoid this.

Give this a try while adding a sleep(4) in the AJAX callback. I'm sure you'll find it incredibly frustrating still.

Changing it to the blur event instead of keyup would be better already.

Oh, #8 & #10 +++++, yes, this could and should be purely client-side, for the reasons outlined in #11.

$.datepicker.formatDate() and PHP's dateformatter format characters don't match though.

D7 didn't use the ajax api for this, I think we should do the same thing and do some custom ajax implementation here.

$.datepicker.formatDate() and PHP's dateformatter format characters don't match though.

Looks like all we need is to just generate this array(in demo link) and set it in Drupal.settings.
http://jsfiddle.net/vijaycs85/3834m8fz/2/

#14 seems like a very fragile approach wouldn't go down that lane.

#15: Do you have any scenario in mind that would fail with this approach? It looks fine to me?

Here is a patch. Can be improved a lot. This is MVP (minimal viable product) to prove that this approach can work.

Tried this, it works smoothly. Still some formatting issues and we should probably implement this for the translate form also.

Still not a big fan of sending the $date_chars array via js settings like this, somehow this seems like a dirty approach.
Can't find anyway this might break though (besides the fact that it's not showing the real time date but that can be ignored IMO).

Still I'd like to hear someone else's opinion on this.

Can individual fields associate a specific timezone? If so, the data sent via JS settings won't be universally applicable.

This removes another occurrence of "talking to the server needlessly", so I'm happy with that, but also have my doubts about the approach — it feels icky, but can't immediately think of something better :)

Assigning to our JS maintainer, nod_, to get his feedback.

Since this still needs more reviews, back to NR.

I like that approach, kinda slick to run the date() function in JS, when all it does is string replacements anyway :).

I'd be cool with the JS date function. Not too keen on adding new JS for this, see below.

In the end it's somewhat of a gimick, we don't do that for image format for example. Since we say 'refer to the PHP doc' I don't see why we absolutely have to make a dynamic preview. Once you save the format you get a preview anyway. We could just get rid of all the JS/PHP involved.

I really like that gimmick though and it has helped me in the past to find the right date format.

I think it would be a UX regression if we removed it and that JS is only added conditionally on this admin page, so should be fine.

+1 on keeping that gimmick

As it is JS it can't have tests right now. It was manually tested by several people though.

As that approach has gotten a +1 from xjm (originally proposed client side) and Wim and a 'meh' (at least that is my interpretation) from nod_ and several have said it would be nice to keep this functionality, and the code looks good, I think this is ready to go.

Hence: RTBC

This is still not working for the same form in config_translation though.

#24: I hadn't actually reviewed the code yet, only the concept. Here's a review.

1. +++ b/core/misc/dateformat.js
   @@ -0,0 +1,43 @@
   +      var $target = $context.find('#edit-date-format-suffix');
   +        // All elements have to exist.
   +        if (!$source.length || !$target.length) {
   +          return;
   +        }
   +        // Skip processing upon a form validation error on the machine name.
   +        if ($source.hasClass('error')) {
   +          return;
   +        }
   

   The indentation of the first quoted line is fine.

   All other quoted lines are intended too much though.

2. +++ b/core/modules/system/src/Form/DateFormatFormBase.php
   @@ -192,4 +165,51 @@ public function save(array $form, FormStateInterface $form_state) {
   +  protected function getSampleDateFormats() {
   

   Needs docs.

3. +++ b/core/modules/system/src/Form/DateFormatFormBase.php
   @@ -192,4 +165,51 @@ public function save(array $form, FormStateInterface $form_state) {
   +     $date_chars = array(
   +       'd',
   +       'D',
   +       'j',
   +       'l',
   +       'N',
   +       'S',
   +       'w',
   +       'z',
   +       'W',
   +       'F',
   +       'm',
   +       'M',
   +       'n',
   +       't',
   +       'L',
   +       'o',
   +       'Y',
   +       'y',
   +       'a',
   +       'A',
   +       'B',
   +       'g',
   +       'G',
   +       'h',
   +       'H',
   +       'i',
   +       's',
   +       'u',
   +       'e',
   +       'I',
   +       'O',
   +       'P',
   +       'T',
   +       'Z',
   +       'c',
   +       'r',
   +       'U',
   +       );
   

   Let's put these on a single line, or even better, do something like $date_chars = explode('', 'dDjlNS…') — that will be easier for future git blameing, to see why the list of available "date chars" has changed.

JS file should be in the system module, not misc.
Eslint errors should be fixed.

1. +++ b/core/misc/dateformat.js
   @@ -0,0 +1,43 @@
   +      var $source = $context.find('.date-formatter');
   +      var $target = $context.find('#edit-date-format-suffix');
   

   Please add some data- attribute that we can use to select relevant elements, we don't want to rely on classes or ids.

   Also it's probably a good idea to use .once() on them.

2. +++ b/core/misc/dateformat.js
   @@ -0,0 +1,43 @@
   +      var $formats = settings.dateFormats;
   

   $ is only for jQuery-related variables, this is just an array, no need for it here.

3. +++ b/core/misc/dateformat.js
   @@ -0,0 +1,43 @@
   +      var replaceCallback = function (key, value) {
   +          return $formats[key] ? $formats[key] : value;
   +        };
   ...
   +      function dateFormatHandler(e) {
   +        var data = e.data;
   +        var baseValue = $(e.target).val();
   +        var dateString = baseValue.replace(/\\?(.?)/gi, replaceCallback)
   +        $(data.$target).html(dateString);
   +      }
   

   Pick one way of declaring functions, I suggest you pick the one used for dateFormatHandler

   Also functions should be at the top, or at the very least, before they're used in the code.

Cool, thanks for summarising @Fabianx and all for valuable input. I'll work on both #27 and #28 and may be some tests.

#27:
1. Fixed.
2. Fixed.
3. Fixed.

#28:
1. Not Fixed - Tried like some other core example(e.g. data-table), but no luck. Will try again later. any help/tip/ most welcome.
2. Not Fixed - Same as above^
3. Fixed - Moved.

On top of those reviews, this patch touches few more items:
1. Moved all system module js to system/js/
2. Removed the form() override of DateFormatEditForm
3. Unified preview text prefix - Looks like we used to have 'Displayed as' for edit and nothing for add.

Yet to do:

1. Fix #28.1
2. Add test.
3. Fix other instances like config_translation.
4. Eslint errors should be fixed.

Fixed test fails...

Here is the patch to fix this element in config_translation module. Added D8MI tags to get the review from them.

Side-effect is, now we get the format displayed in the translated language (attached screenshot).

Fixes:
1. Fix other instances like config_translation.

Yet to list:
1. Fix #28.1
2. Add test - It doesn't look like we can test this in UI.
3. Eslint errors should be fixed.

1. +++ b/core/modules/config_translation/src/FormElement/DateFormat.php
   @@ -29,44 +29,47 @@ public function getTranslationElement(LanguageInterface $translation_language, $
   +   * Provides all date format characters for date format preview on client side.
   +   *
   +   * @param int|null $timestamp
   +   * @return array
   +   *   An array of date character values keyed by characters.
   +   */
   +  protected function getSampleDateFormats($langcode, $timestamp = NULL, $timezone = NULL) {
   
   +++ b/core/modules/system/src/Form/DateFormatFormBase.php
   @@ -192,4 +165,23 @@ public function save(array $form, FormStateInterface $form_state) {
   +  /**
   +   * Provides all date format characters for date format preview on client side.
   +   *
   +   * @param int|null $timestamp
   +   * @return array
   +   *   An array of date character values keyed by characters.
   +   */
   +  protected function getSampleDateFormats($timestamp = NULL) {
   

   That's only one of the arguments, can you please explain the others (and document this one too?)

   Also why are you duplicating this code in two places? Can we reuse the same code in config translation?

2. +++ b/core/modules/config_translation/src/FormElement/ElementInterface.php
   @@ -70,4 +70,12 @@ public function getTranslationBuild(LanguageInterface $source_language, Language
   +  /**
   +   * Builds a form attributes for the configuration form.
   +   *
   +   * @return mixed
   +   */
   +  public function getFormAttributes();
   

   Please explain what form attributes may be and what kind of return values are possible.

3. +++ b/core/modules/config_translation/src/FormElement/FormElementBase.php
   @@ -74,6 +74,10 @@ public function getTranslationBuild(LanguageInterface $source_language, Language
    
   +  public function getFormAttributes() {
   +    return array();
   +  }
   

   @inheritdoc missing

Updated as per #35

Can you please post an interdiff?

sorry, forgot to upload.

Side-effect is, now we get the format displayed in the translated language (attached screenshot).

This wasn't the case yet?

This wasn't the case yet?

We had all options, but wasn't implemented

+++ b/core/modules/config_translation/src/FormElement/DateFormat.php
@@ -22,51 +22,34 @@ class DateFormat extends FormElementBase {
-      $format = t('Displayed as %date_format', array('%date_format' => \Drupal::service('date.formatter')->format(REQUEST_TIME, 'custom', $format_value)));

never handled language code part.

All right, I tried this manually too, it works like a charm, real great work. Its way better than before and we don't need the HTTP roundtrip indeed when we can do it all on the client :) I was thinking of test coverage but not sure how can that be done at this point (no frontend tests possible). So just some more nitpicks left AFAIS:

1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +193,33 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   * @param string|null $langcode
   +   *   Language code of date format.
   

   What if not provided? Let's document that it falls back on the current language of the request.

2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +193,33 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   * @param int|null $timestamp
   +   *   Unix timestamp.
   +   * @param string|null $timezone
   +   *   String timezone.
   

   These are never provided in the calling code now. Why do we need them to be possible to provide? If we need them to be optional, let's document what it means when they are not provided.

3. +++ b/core/modules/config_translation/src/FormElement/ListElement.php
   @@ -97,6 +97,23 @@ public function setConfig(Config $base_config, LanguageConfigOverride $config_tr
   +        $attributes +=  $form_attributes;
   

   One too many spaces.

4. +++ b/core/modules/system/js/system.date.js
   @@ -0,0 +1,67 @@
   +      };
   +
   +
   +      /**
   +       * Callback to check if the given character has a date value.
   

   One too many blank lines.

#41.1 - Fixed.
#41.2 - Documented. We are sending all the param that DateFormatter->format() accept to make this function flexible to use other places (e.g. config_translation need to pass langcode, but not the origin date format form.)
#41.3 - Fixed.
#41.4 - Fixed.

Yep, there isn't anyway to test this UI. May be we can add some unit test for the getSampleDateFormat() method.

Yet to fix:

1. Eslint errors should be fixed.

2.

+++ b/core/misc/dateformat.js
@@ -0,0 +1,43 @@
+      var $source = $context.find('.date-formatter');
+      var $target = $context.find('#edit-date-format-suffix');

Please add some data- attribute that we can use to select relevant elements, we don't want to rely on classes or ids.

Also it's probably a good idea to use .once() on them.

+++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
@@ -193,6 +193,33 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
+   * @param string|null $langcode
+   *   Language code of date format. Use site default language, if not provided.
+   * @param int|null $timestamp
+   *   Unix timestamp to format. Use REQUEST_TIME if not provided.
+   * @param string|null $timezone
+   *   String timezone. Use site default timezone, if not provided.

As per https://www.drupal.org/coding-standards/docs#param optional params should get a doc line prefix "(optional)"

Updated as per #43.

Great, looks good for me, works well. No concerns left on my end. Copied remainng tasks from #42 to issue summary.

Simplified the code and fixed the issues left.

I'm using the formUpdated event, while making the patch I noticed it wasn't complete so I'll open a followup to fix it (try hitting backspace, it doesn't update the form). But it's not a concern for this issue.

@Gabor: what about :

+      '#field_suffix' => ' <small class="js-hide" data-drupal-date-formatter="preview">Displayed as <em></em></small>',

Isn't it a translation issue?

great cleanup @nod_. It looks much better now.

I'm using the formUpdated event, while making the patch I noticed it wasn't complete so I'll open a followup to fix it (try hitting backspace, it doesn't update the form). But it's not a concern for this issue.

IMHO, this issue is a no-go, without the back space fix.

+++ b/core/modules/system/src/Form/DateFormatFormBase.php
@@ -126,20 +102,16 @@ public function form(array $form, FormStateInterface $form_state) {
+      ),
+      '#field_suffix' => ' <small class="js-hide" data-drupal-date-formatter="preview">Displayed as <em></em></small>',
     );

Indeed, good catch, this needs to be:

' . t('Displayed as ') . 
<em></em>';

The context is a little tricky, but not sure its better to put em into the text.

Reused the thing from DateFormat.

I think the remaining tasks in the summary are not up to date. What is left to be done here?

Tested manually; this is so much better. The patch is so so much more usable than what is in HEAD.

The fact that backspace doesn't work is indeed a bug, but I think that's just a normal bug. It only matters if you only press backspace and do not enter anything new in the field; a workaround is to simply re-enter a different character in the string.

That said, if we do want to fix the thing with the backspace before this goes in, then this is NW.

Sorry wrong screenshot.

Per #54 / #55.

Oh, I just noticed forward-delete also has the same bug. No idea what character that is...

You mean delete?

Should we just use .change() event for now with a @todo to change it back to .formUpdated() even once the bug fix for that lands?

It's the same thing, formUpdated uses change event as well as keypress. The issue here is that keypress is not triggered for keys that don't add a character of some sort. It doesn't register copy/paste either. That's why we need a more comprehensive formUpdated event and that's outside of this scope.

Let's go with the normal event on this issue. We can replace them with formUpdated as part of #2456225: Improve formUpdated event.

I manually tested again and confirmed the delete bug is gone. Yay!

I made some docs improvements (attached). A few other questions:

1. +++ b/core/modules/config_translation/src/FormElement/DateFormat.php
   @@ -22,51 +22,34 @@ class DateFormat extends FormElementBase {
   +    /** @var \Drupal\Core\Datetime\DateFormatter $date_formatter */
   +    $date_formatter = \Drupal::service('date.formatter');
        $description = $this->t('A user-defined date format. See the <a href="@url">PHP manual</a> for available options.', array('@url' => 'http://php.net/manual/function.date.php'));
   -    $format = $this->t('Displayed as %date_format', array('%date_format' => \Drupal::service('date.formatter')->format(REQUEST_TIME, 'custom', $translation_config)));
   +    $format = $this->t('Displayed as %date_format', array('%date_format' => $date_formatter->format(REQUEST_TIME, 'custom', $translation_config)));
   ...
   +        'drupalSettings' => array('dateFormats' => $date_formatter->getSampleDateFormats($translation_language->getId())),
   

   I didn't touch this, but is there a reason we're not injecting a dependency on the date formatter? Edit: I guess my comment is out of scope since it was already using \Drupal::service() in HEAD, two lines down, and we're just assigning the service to a variable to use it again later.

2. similarity index 100%
   rename from core/modules/system/system.js
   rename to core/modules/system/js/system.js
   
   similarity index 100%
   rename from core/modules/system/system.modules.js
   rename to core/modules/system/js/system.modules.js
   
   +++ b/core/modules/system/system.libraries.yml
   @@ -29,7 +29,7 @@ maintenance:
   -    system.js: {}
   +    js/system.js: {}
   
   @@ -39,7 +39,7 @@ drupal.system:
   -    system.modules.js: {}
   +    js/system.modules.js: {}
   

   Is moving these files really in scope? (Maybe it is, just seemed a bit like an unrelated cleanup.)

Oops.

Adding a beta evaluation to the summary.

+++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
@@ -193,6 +193,36 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
+  public function getSampleDateFormats($langcode = NULL, $timestamp = NULL, $timezone = NULL) {

Should we add a unit test for this method?

Just fixed a couple of nitpicks.

Thanks @rteijeiro. I'm working on the unit-testing. Currently introduced two follow ups - #2457345: Remove unnecessary formatDate call for 'custom' format & code cleanup. and #2457251: Remove unnecessary call to drupal_get_user_timezone() in Drupal/Core/Datetime/DrupalDateTime::prepareTimezone() method.

1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +193,36 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +  public function getSampleDateFormats($langcode = NULL, $timestamp = NULL, $timezone = NULL) {
   ...
   +    $tz = $timezone ?: NULL;
   

   Why not just use $timezone? This assignment to $tz looks like a no-op

2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +193,36 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +    $ts = $timestamp ?: REQUEST_TIME;
   

   I think this should overwrite $timestamp instead of abbreviating to $ts, which we usually don't do.

3. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
   @@ -176,6 +176,9 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
   +        if ($attributes = $form_element->getFormAttributes()) {
   +          $form = array_merge_recursive($form, $attributes);
   +        }
   

   Is this related? Can we get a code comment and some test coverage?

4. +++ b/core/modules/config_translation/src/FormElement/DateFormat.php
   @@ -22,51 +22,34 @@ class DateFormat extends FormElementBase {
   +      '#attributes' => array(
   +        'data-drupal-date-formatter' => 'source',
          ),
   ...
   +        'drupalSettings' => array('dateFormats' => $date_formatter->getSampleDateFormats($translation_language->getId())),
   ...
   +    return array(
   +      '#attached' => array(
   +        'library' => array('system/drupal.system.date'),
   +      ),
   +    );
   
   +++ b/core/modules/config_translation/src/FormElement/FormElementBase.php
   @@ -75,6 +75,13 @@ public function getTranslationBuild(LanguageInterface $source_language, Language
   +    return array();
   
   +++ b/core/modules/config_translation/src/FormElement/ListElement.php
   @@ -97,6 +97,23 @@ public function setConfig(Config $base_config, LanguageConfigOverride $config_tr
   +    $attributes = array();
   

   Should use new []

Addressed @tim.plunkett points 1, 2 and 4. Not sure about point 3. Also fixed wrong type in js comment.

+++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
@@ -211,14 +211,14 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
+    $timezone = $timezone ?: NULL;

Hm, as per @tim.plunkett's comment, do we need this at all? The default is NULL. If someone passes in FALSE, zero, or emptystring, do we need to recast that?

You are right! Just skipped that, sorry.

+++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
@@ -193,6 +193,35 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
+    $language_code = $langcode ?: NULL;

Same goes for this. No need to rename the variable. Sorry for missing it earlier.

Adding test... added changes from #2457345: Remove unnecessary formatDate call for 'custom' format & code cleanup. and #2457251: Remove unnecessary call to drupal_get_user_timezone() in Drupal/Core/Datetime/DrupalDateTime::prepareTimezone() method. to make the test work. Since they are not *JUST* to fix this test, added as separate issues(easy to get them in as well).

Reg #69.3 - Yes, they are related to this issue on config_translation module part. Will add some test coverage...

+++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
@@ -128,4 +130,42 @@ public function testFormatIntervalZeroSecond() {
+    $container = new ContainerBuilder();
+    $container->set('string_translation', $this->getStringTranslationStub());
+    \Drupal::setContainer($container);
...
+  if (!function_exists('t')) {
+    function t($string, array $args = []) {
+      return String::format($string, $args);
+    }
+  }

You shouldn't have to do either of these, but definitely not both!

good catch @tim.plunkett. Removing it.

AFAIK, #69.3 - "Add test coverage for getFormAttributes()" is the only pending item.

A few nitpicks more :)

The current code to declare variables in the JS is according to standards. We want several var, not just one.

Sorry for the mess :(

Reroll after #2457251: Remove unnecessary call to drupal_get_user_timezone() in Drupal/Core/Datetime/DrupalDateTime::prepareTimezone() method. is in.

1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +193,34 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +    $date_characters = 'dDjlNSwzWFmMntLoYyaABgGhHisueIOPTZcrU';
   +    $date_chars = str_split($date_characters);
   

   $date_characters = str_split('dDjlNSwzWFmMntLoYyaABgGhHisueIOPTZcrU');

   more readable

2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -204,6 +232,9 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +    if ($format == 'custom') {
   +      return NULL;
   

   @return should be updated, also confusing to returning NULL when string expected

3. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -226,4 +257,13 @@ protected function country() {
   +   * @param string $country_code
   +   */
   +  public function setCountry($country_code) {
   

   Needs description for param, and setters mostly returns themselves.
   Maybe better setCountryCode() better?

fixed items in #83. thanks @andypost

Sorry for the nitpicks again :(

The rest of the code looks good! :)

In this patch:
1. removed changes related to #2457345: Remove unnecessary formatDate call for 'custom' format & code cleanup.
2. Addressed #69.3

I think, all done here and ready for reviews...

When a patch fails the bot multiple times like that, we should investigate why rather than continuing to requeue it... Actually we should do that also when it fails only once, but especially when it fails multiple times. I'd like to see the exact failure documented on this issue with an explanation.

When a patch fails the bot multiple times like that

Sorry, I should have done some documentation before sending the patch in #88 to test second time. However the reason behind testing twice is, the only change that I added in #88 compare to #87 is one line addition in test file. However just that change works fine(no fatal) in test-only patch. Finally sent #87 to make sure, still it's fine.

Anyway, sorry for the noise...

#101: Thanks for the review, I want that this patch can go in some time, so gonna add a focus based reply - even though its not my own patch.

1. / 3. : Yes, REQUEST_TIME is deprecated, but it is pre-existing in this class and the code only moves it and hence this should be done in a follow-up to keep this patch focused. Unless the author really wants to do so ...

2./4. Agree

5. Yep, abbreviations are a coding style violation. Good catch!

Nitpickers free ;-)

I added:

- * @see \Drupal\Core\Datetime\DateFormatter::formatInterval()
+ * @covers \Drupal\Core\Datetime\DateFormatter::formatInterval()

because I think is also covering that method instead that using a reference to check that method. Maybe I'm wrong.

Shortened the comments to less than 80 chars length.

+++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
@@ -128,4 +135,37 @@ public function testFormatIntervalZeroSecond() {
+   * @cover \Drupal\Core\Datetime\DateFormatter::getSampleDateFormats()

Should be @covers and without parentheses.

Addressed #110 and #112.

Thanks for the review @pjonckiere. Here is the patch...

I now get to commit this patch -- yay! (My work on it was minor and awhile ago; there have been many thorough reviews since.)

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x with profligate credit for reviewers. :)

This broke running unit tests from the command line. cd core; phpunit --testsuite unit;.

There was 1 error:

1) Drupal\Tests\Core\Datetime\DateTest::testGetSampleDateFormats
Constant SAVED_NEW already defined

/Volumes/devdisk/dev/drupal/core/includes/common.inc:72
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Datetime/DateTest.php:149
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:104

The error indicates that something is bleeding across unit test runs.

+++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
@@ -128,4 +140,36 @@ public function testFormatIntervalZeroSecond() {
+    include_once $this->root . '/core/includes/common.inc';

This is the culprit. We shouldn't be including procedural code in a unit test.

+++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
@@ -193,6 +193,33 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
+   * @param int|null $timestamp
+   *   (optional) The Unix timestamp to format, defaults to the request time.
...
+    $timestamp = $timestamp ?: (int) $_SERVER['REQUEST_TIME'];

This is introducing a dependency on the request. I think it would be okay to default to something else here. For example Dries' birthday.

+++ b/core/modules/config_translation/src/FormElement/DateFormat.php
@@ -22,51 +22,29 @@ class DateFormat extends FormElementBase {
-    $format = $this->t('Displayed as %date_format', array('%date_format' => \Drupal::service('date.formatter')->format(REQUEST_TIME, 'custom', $translation_config)));
+    $format = $this->t('Displayed as %date_format', array('%date_format' => $date_formatter->format((int) $_SERVER['REQUEST_TIME'], 'custom', $translation_config)));

I think the change here is out-of-scope. Unless using REQUEST_TIME is impossible due to additional unit testing being added.

Updated as per above comments.

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
@@ -98,15 +98,52 @@ public function format($format, $settings = array()) {
-      // Pass the langcode to _format_date_callback().
-      _format_date_callback(NULL, $settings['langcode']);
+      // Pass the langcode to formatCallback().
+      self::formatCallback(NULL, $settings['langcode']);
 
       // Translate the marked sequences.
-      $value = preg_replace_callback('/\xEF([AaeDlMTF]?)(.*?)\xFF/', '_format_date_callback', $format);
+      $value = preg_replace_callback('/\xEF([AaeDlMTF]?)(.*?)\xFF/', array($this, 'formatCallback'), $format);
...
+  public static function formatCallback(array $matches = NULL, $new_langcode = NULL) {
+    // We cache translations to avoid redundant and rather costly calls to t().
+    static $cache, $langcode;
+
+    if (!isset($matches)) {
+      $langcode = $new_langcode;
+      return;
+    }

This can be rewritten to not use the passing of the langcode to set the static. There is no need for the method to be static. And the cache can be stored as a property on the class.

This can be rewritten to not use the passing of the langcode to set the static.

Changed the function a bit. Please let me know, if it's not OK.

There is no need for the method to be static. And the cache can be stored as a property on the class.

Updated.

The patch in #126 is not right, it's way too big. The interdiff seems to display the proper attempt though.

Yeah, some update on 8.0.x caused some issue with my patch script. I have issued new patch now.

This is called "callback", but that is not what is happening here I think.

yeah, updated.

1. +++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
   @@ -98,15 +104,43 @@ public function format($format, $settings = array()) {
   +        return self::getFormatTranslation($settings['langcode'], $matches[1], $matches[2]);
   

   Why not $this->getFormatTranslation()?

2. +++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
   @@ -98,15 +104,43 @@ public function format($format, $settings = array()) {
   +        $this->formatTranslationCache[$langcode][$code][$string] = t($string, array(), $options);
   

   Shouldn't we use $this->t() here?

Adding an interdiff for easier reviews.

Just spoke with @pjonckiere and @alexpott online. Here is an update.

2 Fails:
1. Fatal error

The test did not complete due to a fatal error.	Completion check	AggregatorAdminTest.php	20	Drupal\aggregator\Tests\AggregatorAdminTest->testSettingsPage()	
copy(/var/lib/drupaltestbot/sites/default/files/checkout/sites/simpletest/937535/settings.php): failed to open stream: No such file or directorycopy('/var/lib/drupaltestbot/sites/default/files/checkout/sites/default/default.settings.php', '/var/lib/drupaltestbot/sites/default/files/checkout/sites/simpletest/937535/settings.php') Drupal\simpletest\WebTestBase->setUp() Drupal\aggregator\Tests\AggregatorTestBase->setUp() Drupal\simpletest\TestBase->run(Array) simpletest_script_run_one_test('7', 'Drupal\aggregator\Tests\AggregatorAdminTest')

2. DateTest.php fails

Drupal\Tests\Core\Datetime\DateTest::testGetSampleDateFormats Undefined variable: value /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Datetime/DrupalDateTime.php:136 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Datetime/DateFormatter.php:160 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Datetime/DateFormatter.php:218 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Datetime/DateFormatter.php:219 /var/lib/drupaltestbot/sites/default/files/checkout/core/tests/Drupal/Tests/Core/Datetime/DateTest.php:150	Other	DateTest.php	148	Drupal\Tests\Core\Datetime\DateTest->testGetSampleDateFormats()	

#1 seems random testbot error. Updated fix for #2.

1. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
   index 729afee..45aab74 100644
   --- a/core/modules/config_translation/src/FormElement/DateFormat.php
   
   --- a/core/modules/config_translation/src/FormElement/DateFormat.php
   +++ b/core/modules/config_translation/src/FormElement/DateFormat.php
   

   use Drupal\Component\Utility\NestedArray;
   use Drupal\Core\Ajax\AjaxResponse;
   use Drupal\Core\Ajax\ReplaceCommand;
   use Drupal\Core\Form\FormStateInterface;
   

   These can all be removed now.

2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +193,33 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +    $timestamp = $timestamp ?: (int) $_SERVER['REQUEST_TIME'];
   

   We don't have to use $_SERVER here - let's just use a hard coded timestamp. For example, 280299600 - Dries's birthday. Or we do something random here.

3. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -200,8 +227,9 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
       *   The langcode of the language to use.
   
   +++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
   @@ -128,4 +141,35 @@ public function testFormatIntervalZeroSecond() {
   +namespace {
   ...
   +
   +  if (!function_exists('t')) {
   +    function t($string, array $args = []) {
   +      return String::format($string, $args);
   +    }
   +  }
   

   This should not be needed - why is it needed? If it is using Drupal\Component\Utility\String; is wrong.

4. +++ b/core/modules/system/src/Form/DateFormatFormBase.php
   @@ -80,30 +80,6 @@ public function exists($entity_id, array $element) {
   -    $response = new AjaxResponse();
   -    $response->addCommand(new ReplaceCommand('#edit-date-format-suffix', '<small id="edit-date-format-suffix">' . $format . '</small>'));
   

   Can remove AjaxResponse and ReplaceCommand from the use statements.

#135.1 - Fixed
#135.2 - As discussed with @alexpott on IRC, replacing it with time().
#135.3 - Fixed. Yes added to cover the t() inside which has been replaced by $this->t()
#135.4 Fixed

Last patch addressed all the last suggested changes in #135 and it works. Seems RTBC to me.

1. I've tested admin/config/regional/date-time/formats/add and it works great - nice!

2. +++ b/core/modules/config_translation/src/FormElement/DateFormat.php
   @@ -22,51 +18,29 @@ class DateFormat extends FormElementBase {
   +  public function getFormAttributes() {
   +    return ['#attached' => ['library' => ['system/drupal.system.date']]];
      }
   

   Why do we need getFormAttributes()? Why can't it go in the #attached in the getTranslationElement()?

3. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
   @@ -176,6 +176,9 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
   +        if ($attributes = $form_element->getFormAttributes()) {
   +          $form = array_merge_recursive($form, $attributes);
   +        }
   
   +++ b/core/modules/config_translation/src/FormElement/ElementInterface.php
   @@ -70,4 +70,12 @@ public function getTranslationBuild(LanguageInterface $source_language, Language
   +  /**
   +   * Allows to provide form attributes for the configuration form.
   +   *
   +   * @return array
   +   *   An array of form attributes.
   +   */
   +  public function getFormAttributes();
   

   If we do need this I think this needs better documentation. Also the need for these changes seems to be completely undocumented in the issue summary.

+++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
@@ -176,6 +176,9 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
+        if ($attributes = $form_element->getFormAttributes()) {
+          $form = array_merge_recursive($form, $attributes);
+        }

+++ b/core/modules/config_translation/src/FormElement/DateFormat.php
@@ -22,51 +18,29 @@ class DateFormat extends FormElementBase {
+  public function getFormAttributes() {
+    return ['#attached' => ['library' => ['system/drupal.system.date']]];
   }

+++ b/core/modules/config_translation/src/FormElement/ElementInterface.php
@@ -70,4 +70,12 @@ public function getTranslationBuild(LanguageInterface $source_language, Language
+  /**
+   * Allows to provide form attributes for the configuration form.
+   *
+   * @return array
+   *   An array of form attributes.
+   */
+  public function getFormAttributes();

+++ b/core/modules/config_translation/src/FormElement/FormElementBase.php
@@ -75,6 +75,13 @@ public function getTranslationBuild(LanguageInterface $source_language, Language
   /**
+   * {@inheritdoc}
+   */
+  public function getFormAttributes() {
+    return [];
+  }

+++ b/core/modules/config_translation/src/FormElement/ListElement.php
@@ -97,6 +97,23 @@ public function setConfig(Config $base_config, LanguageConfigOverride $config_tr
   /**
+   * {@inheritdoc}
+   */
+  public function getFormAttributes() {
+    $attributes = [];
+    foreach ($this->element as $key => $element) {
+      if ($form_element = ConfigTranslationFormBase::createFormElement($element)) {
+        $form_attributes = $form_element->getFormAttributes();
+        if (empty($form_attributes)) {
+          continue;
+        }
+        $attributes += $form_attributes;
+      }
+    }
+    return $attributes;
+  }

As per #69.3 I'm not sure we have appropriate coverage of this complexity (if it is required see #139)

#139: yeah, the initially added this, so that we will have a way to add additional elements/attributes to form level. I agree that this is unnecessary for this issue scope. Removed them all and still functionality is working as before.

P.S: The translation page looks bit mis-aligned when you look at desktop, but fine in lower screen size. Not related to this issue, will create an issue.

I tried everything and now works fine. don't lose the focus and refresh the date while you write.

You need use cache-rebuild after apply the patch!

I reviewed this many times and manually tested it after the last patch - everything is working nicely and we have good test coverage. Thanks everyone. I've added myself to the commit credit since my comments on this issue provided feedback that was incorporated into the final patch.

Committed 4e3e39c and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Thanks everyone. special thanks to @alexpott for valuable feedback, reviews and commit.

Congratulations, vijaycs85 & @all!

This is amazing! Thanks @vijaycs85 for sticking to this! Yay!