Problem/Motivation

When creating a date format, when typing the PHP format the focus is lost. Moving the cursor also makes that the focus is lost. So you end writing your format in a text editor and pasting it in Drupal.

Proposed resolution

The focus should not be lost. Additionally, it is not actually necessary to send an Ajax request. The proposed solution instead provides sample formatting values to the form element so that the sample output can be generated entirely on the client side, without the overhead of extra requests.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because this is definitely not the intended behavior.
Issue priority Major because the form is virtually unusable, but not critical because there is a workaround (typing the value first and pasting it in the field) and only this one form is impacted.
Prioritized changes The main goal of this issue is a usability bugfix, so it is a prioritized change during the beta.
Disruption Minor changes to the date format form which would impact modules altering this form. The system JS files are also moved to a subdirectory (as of #62 - #65).

Remaining tasks

The patch has been manually tested and was reviewed by a JavaScript maintainer in an earlier version. Automated testing for the form element is not possible because the patch now uses a purely JS approach. Patch needs final review (and possibly a unit test for the new getSampleDateFormats() method).

User interface changes

Date format preview is shown in real time, but without losing focus.

API changes

None.

Files: 
CommentFileSizeAuthor
#140 Screen Shot 2015-05-16 at 11.55.15.png181.27 KBvijaycs85
#140 Screen Shot 2015-05-16 at 11.55.04.png211.22 KBvijaycs85
#140 2429443-diff-136-140.txt4.35 KBvijaycs85
#140 2429443-140.patch20.12 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,291 pass(es). View
#136 2429443-diff-134-136.txt3.13 KBvijaycs85
#136 2429443-136.patch23.52 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,367 pass(es). View

Comments

geertvd’s picture

Component: datetime.module » system.module

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

geertvd’s picture

Status: Active » Needs review
FileSize
946 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,257 pass(es). View

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

penyaskito’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks @geertvd! Patch works as expected. However we should have test coverage exposing the bug and ensuring the fix is right.

geertvd’s picture

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.

penyaskito’s picture

Shouldn't we test that the InvokeCommand is added at least?

xjm’s picture

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.)

geertvd’s picture

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.

xjm’s picture

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.

geertvd’s picture

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.

vijaycs85’s picture

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.

Wim Leers’s picture

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.

Wim Leers’s picture

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

geertvd’s picture

$.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.

vijaycs85’s picture

$.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/

geertvd’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
5.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,685 pass(es). View

#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.

geertvd’s picture

Status: Needs review » Needs 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.

Wim Leers’s picture

Assigned: Unassigned » nod_
Issue tags: +JavaScript

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.

Wim Leers’s picture

Status: Needs work » Needs review

Since this still needs more reviews, back to NR.

Fabianx’s picture

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

nod_’s picture

Assigned: nod_ » Unassigned

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.

Fabianx’s picture

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.

geertvd’s picture

+1 on keeping that gimmick

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

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

geertvd’s picture

Status: Reviewed & tested by the community » Needs review

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

geertvd’s picture

Status: Needs review » Needs work
Wim Leers’s picture

#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.

nod_’s picture

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.

vijaycs85’s picture

Assigned: Unassigned » vijaycs85

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

vijaycs85’s picture

Assigned: vijaycs85 » Unassigned
Status: Needs work » Needs review
FileSize
8.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,872 pass(es), 0 fail(s), and 19 exception(s). View
9.68 KB

#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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
7.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,993 pass(es). View
3.62 KB

Fixed test fails...

vijaycs85’s picture

Issue tags: +D8MI, +sprint
FileSize
13.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,125 pass(es). View
6.88 KB

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.

vijaycs85’s picture

Issue summary: View changes
FileSize
199.94 KB
Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +language-config
  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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
14.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,796 pass(es). View

Updated as per #35

Gábor Hojtsy’s picture

Can you please post an interdiff?

vijaycs85’s picture

FileSize
7.17 KB

sorry, forgot to upload.

Wim Leers’s picture

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

This wasn't the case yet?

vijaycs85’s picture

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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
14.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
1.81 KB

#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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ 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)"

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
14.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,079 pass(es). View
1.07 KB

Updated as per #43.

Gábor Hojtsy’s picture

Issue summary: View changes

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

nod_’s picture

FileSize
15.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,176 pass(es). View
6.82 KB

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?

vijaycs85’s picture

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.

Fabianx’s picture

+++ 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.

nod_’s picture

FileSize
781 bytes
15.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,197 pass(es). View

Reused the thing from DateFormat.

Gábor Hojtsy’s picture

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

vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

xjm’s picture

Issue summary: View changes
FileSize
48.75 KB

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.

xjm’s picture

Issue summary: View changes
FileSize
52.54 KB

Sorry wrong screenshot.

xjm’s picture

Status: Needs review » Needs work

Per #54 / #55.

xjm’s picture

Issue summary: View changes

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

nod_’s picture

You mean delete?

Fabianx’s picture

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?

nod_’s picture

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.

nod_’s picture

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
15.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,726 pass(es). View
577 bytes

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

xjm’s picture

Issue summary: View changes
FileSize
15.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,750 pass(es). View
2.54 KB

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.)

xjm’s picture

FileSize
15.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,774 pass(es). View
782 bytes

Oops.

xjm’s picture

Issue summary: View changes

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?

rteijeiro’s picture

FileSize
15.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,113 pass(es). View
964 bytes

Just fixed a couple of nitpicks.

vijaycs85’s picture

tim.plunkett’s picture

  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 []

rteijeiro’s picture

FileSize
15.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
3.4 KB

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

xjm’s picture

+++ 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?

rteijeiro’s picture

FileSize
746 bytes
15.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,117 pass(es). View

You are right! Just skipped that, sorry.

tim.plunkett’s picture

+++ 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.

vijaycs85’s picture

FileSize
19.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,117 pass(es). View
5 KB

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...

tim.plunkett’s picture

+++ 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!

vijaycs85’s picture

FileSize
19.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,134 pass(es). View
972 bytes

good catch @tim.plunkett. Removing it.

vijaycs85’s picture

Assigned: vijaycs85 » Unassigned

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

rteijeiro’s picture

FileSize
19.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,124 pass(es). View
4.15 KB

A few nitpicks more :)

nod_’s picture

Status: Needs review » Needs work

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

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
19.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,159 pass(es). View
1.33 KB

Sorry for the mess :(

vijaycs85’s picture

FileSize
18.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,168 pass(es). View
andypost’s picture

  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?

vijaycs85’s picture

FileSize
18.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
1.82 KB

fixed items in #83. thanks @andypost

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
737 bytes
18.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,094 pass(es). View
rteijeiro’s picture

FileSize
18.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,704 pass(es). View
1.5 KB

Sorry for the nitpicks again :(

The rest of the code looks good! :)

vijaycs85’s picture

FileSize
769 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,484 pass(es), 2 fail(s), and 0 exception(s). View
3.12 KB
19.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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...

Status: Needs work » Needs review

vijaycs85 queued 88: 2429443-88.patch for re-testing.

vijaycs85 queued 87: 2429443-87.patch for re-testing.

xjm’s picture

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.

vijaycs85’s picture

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...

penyaskito’s picture

Status: Needs work » Needs review
FileSize
2.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
19.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Rerolled, automatic 3-way merge.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
875 bytes
2.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
19.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,776 pass(es). View

Fixed the exception with core/tests/Drupal/Tests/Core/Datetime/DateTest.

pjonckiere’s picture

Status: Needs review » Needs work

Generally looks good. Some feedback on the php code:

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

    You shouldn't use the REQUEST_TIME constant, but the one on the $_SERVER object. See #2459155: Remove REQUEST_TIME from bootstrap.php.

  2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
    @@ -226,4 +254,17 @@ protected function country() {
    +   * @return $this
    
    +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
    @@ -176,6 +176,10 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
    +        // Merge, if there any form attributes from element.
    

    "if there are any attributes on the form element"? Or just drop the whole comment, since the code is readable enough.

  3. +++ 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' => $date_formatter->format(REQUEST_TIME, 'custom', $translation_config)));
    

    REQUEST_TIME should not be used.

  4. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
    @@ -446,6 +446,9 @@ public function testDateFormatTranslation() {
    +      // Make sure date library added from the element form.
    

    'Make sure that the date library is added.'

  5. +++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
    @@ -128,4 +135,37 @@ public function testFormatIntervalZeroSecond() {
    +    $ts = strtotime('2015-03-22 14:23:00');
    

    $timestamp is not that long of a variable name ;)

Fabianx’s picture

#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!

penyaskito’s picture

Status: Needs work » Needs review
FileSize
19.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,801 pass(es). View
4.56 KB

Attended #101, #102. Thanks for the reviews!

pjonckiere’s picture

Status: Needs review » Needs work

I manually tested this again to verify this still works as expected, and it does.

I looked over the new patch again. This looks almost ready, but I fear I have found some more nitpicks (see below). After those are fixed, we can RTBC this.

  1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
    @@ -193,6 +193,33 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
    +   *   (optional) The Unix timestamp to format. REQUEST_TIME is used by default.
    

    This comment should be adjusted as well: "The request time"

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

    This should be @covers instead of @see?

gloob’s picture

Status: Needs work » Needs review
FileSize
1.78 KB

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.

gloob’s picture

FileSize
19.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,777 pass(es), 18 fail(s), and 0 exception(s). View
penyaskito’s picture

+++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
@@ -199,7 +199,7 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
+   *   (optional) The Unix timestamp to format. The request time is used by default.

Comments should be max 80 chars length.

Suggestion: "(optional) The Unix timestamp to format, defaults to the request time."

gloob’s picture

FileSize
19.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,785 pass(es), 18 fail(s), and 0 exception(s). View
783 bytes

Shortened the comments to less than 80 chars length.

pjonckiere’s picture

Status: Needs review » Needs work

Comment looks good now.

+++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
@@ -57,7 +64,7 @@ protected function setUp() {
+   * @covers \Drupal\Core\Datetime\DateFormatter::formatInterval()

Remove the parentheses at the end to fix the test failures.

geertvd’s picture

+++ 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.

gloob’s picture

FileSize
19.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,802 pass(es). View
1.02 KB

Addressed #110 and #112.

gloob’s picture

Status: Needs work » Needs review
pjonckiere’s picture

Status: Needs review » Needs work

This looks almost ready. The patch already looks great, IS is up to date and we have a beta evaluation. And manually testing reveals that this works very smooth!

I just have one question left:

+++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
@@ -226,4 +254,17 @@ protected function country() {
+  /**
+   * Setter for country code.
+   *
+   * @param string $country_code
+   *   String country code.
+   *
+   * @return $this
+   */
+  public function setCountryCode($country_code) {
+    $this->country = $country_code;
+    return $this;
+  }
+

It seems like this is only added to be able to set the country code in a test case. Can we alter 'country.default' from the 'system.date' in that test case instead?

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
19.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,823 pass(es). View
1.92 KB

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

pjonckiere’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready now.

  • xjm committed 37e278b on 8.0.x
    Issue #2429443 by vijaycs85, rteijeiro, penyaskito, gloob, xjm, nod_,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

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. :)

alexpott’s picture

Status: Fixed » Needs work

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.

  • alexpott committed ea0ec70 on 8.0.x
    Revert "Issue #2429443 by vijaycs85, rteijeiro, penyaskito, gloob, xjm,...
alexpott’s picture

+++ 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.

penyaskito’s picture

Looks like it was done because we need _format_date_callback. Let's move that one to \Drupal\Core\Datetime\DrupalDateTime, which is the only usage.

alexpott’s picture

+++ 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.

alexpott’s picture

+++ 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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
147.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,821 pass(es). View
4.7 KB

Updated as per above comments.

alexpott’s picture

Status: Needs review » Needs work
+++ 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.

pjonckiere’s picture

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

Apart from the above, I have another concern:

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
@@ -98,15 +98,52 @@ public function format($format, $settings = array()) {
+   * Callback for preg_replace_callback() within format_date().
+   */
+  public static function formatCallback(array $matches = NULL, $new_langcode = NULL) {

This is called "callback", but that is not what is happening here I think. I verified that this is the only place where this is used. Now, if we move it, I think we should rename it properly to what it does (what does it do?). And then clean up the unneeded code (e.g. parameter $matches is always NULL).

vijaycs85’s picture

FileSize
22.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,909 pass(es). View

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.

vijaycs85’s picture

Status: Needs work » Needs review
geertvd’s picture

Status: Needs review » Needs work
FileSize
2.71 KB
  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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
23.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,832 pass(es), 2 fail(s), and 6 exception(s). View
3.25 KB

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

Status: Needs review » Needs work

The last submitted patch, 132: 2429443.132.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
23.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,922 pass(es). View
1.36 KB

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.

alexpott’s picture

Status: Needs review » Needs work
  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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
23.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,367 pass(es). View
3.13 KB

#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

gloob’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

alexpott’s picture

+++ 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)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
20.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,291 pass(es). View
4.35 KB
211.22 KB
181.27 KB

#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.

vijaycs85’s picture

Issue tags: +LONDON_2015_MAY
Saphyel’s picture

Status: Needs review » Reviewed & tested by the community

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!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 140: 2429443-140.patch, failed testing.

pjonckiere’s picture

The error of the failure doesn't look patch related, so let's just retest.

pjonckiere queued 140: 2429443-140.patch for re-testing.

pjonckiere’s picture

Status: Needs work » Reviewed & tested by the community

The previous testbot failure (not able to checkout from repo) doesn't occur anymore, resulting in green. So back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 4e3e39c on 8.0.x
    Issue #2429443 by vijaycs85, rteijeiro, penyaskito, gloob, xjm, nod_,...
vijaycs85’s picture

Issue tags: -sprint

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

Fabianx’s picture

Congratulations, vijaycs85 & @all!

Gábor Hojtsy’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.