Problem/Motivation

The date field handler in views has a lot of options (see Drupal\views\Plugin\views\field\Date).
This includes:

  • selecting a date format
  • specify a custom date format
  • select the timezone which should be used to render the field (useful for some rss feeds for example)
  • Use both a date as well as a time ago rendered format.

Sadly the timestamp field handler doesn't provide those at all, nor do the Datetime module's field formatters.

Proposed resolution

  • Port over the existing features into the views field handlers that are currently being used for base timestamp entity fields
  • Both the Datetime field formatters and the base Timestamp formatters need these features.
  • Discuss which of those could be dropped

Remaining tasks

Features to port

  • selecting a date format - Core feature
  • specify a custom date format
  • select the timezone which should be used to render the field (useful for some rss feeds for example)
  • Use both a date as well as a time ago rendered format.
  • Drop timezone support in timeago
  • Simplify timeago to use a format string instead of umpteen format choices

Still to do: get these into the Timestamp formatters, which are currently missing from the patch (it just covers Datetime formatters so far).

User interface changes

New settings form options for DateTime fields and Timestamp fields.

API changes

Display setting keys have changed for places manually rendering entity fields.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it was outlined as such by the parent META, #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality
Issue priority Major because while the parent is now down to a major; this isn't critical as it won't lead to a SA down the road, it does restore/add back desired functionality for 8.0.0
Prioritized changes The main goal of this issue is improved user and developer experience; views output will be identical to entity output. See also the rationale outlined in the parent meta.
Disruption Minimal.
CommentFileSizeAuthor
#201 interdiff-194-201.txt5.32 KBmpdonadio
#201 support_all_options-2399211-201.patch65.44 KBmpdonadio
#194 interdiff-186-194.txt18.14 KBmpdonadio
#194 support_all_options-2399211-194.patch65.31 KBmpdonadio
#186 interdiff-185-186.txt7.94 KBmpdonadio
#186 support_all_options-2399211-186.patch61.15 KBmpdonadio
#185 interdiff-179-185.txt8.05 KBmpdonadio
#185 support_all_options-2399211-185.patch60.21 KBmpdonadio
#179 interdiff-174-179.txt1.65 KBmpdonadio
#179 support_all_options-2399211-179.patch60.27 KBmpdonadio
#174 interdiff-168-174.txt2.12 KBmpdonadio
#174 support_all_options-2399211-174.patch60.29 KBmpdonadio
#168 interdiff-164-168.txt3.37 KBmpdonadio
#168 support_all_options-2399211-168.patch60.48 KBmpdonadio
#164 interdiff-163-164.txt20.03 KBmpdonadio
#164 support_all_options-2399211-164.patch60.62 KBmpdonadio
#163 interdiff-159-163.txt2.6 KBmpdonadio
#163 support_all_options-2399211-163.patch40.59 KBmpdonadio
#159 interdiff-150-159.txt1.56 KBmpdonadio
#159 support_all_options-2399211-159.patch40.61 KBmpdonadio
#150 interdiff-149-150.txt1020 bytesmpdonadio
#150 support_all_options-2399211-150.patch40.66 KBmpdonadio
#149 support_all_options-2399211-149.patch40.79 KBmpdonadio
#146 interdiff-144-146.txt872 bytesmpdonadio
#146 support_all_options-2399211-146.patch40.78 KBmpdonadio
#144 interdiff-131-144.txt7.31 KBmpdonadio
#144 support_all_options-2399211-144.patch40.82 KBmpdonadio
#131 interdiff-127-131.txt5.69 KBmpdonadio
#131 support_all_options-2399211-131.patch39.86 KBmpdonadio
#127 interdiff-126-127.txt7.7 KBmpdonadio
#127 support_all_options-2399211-127.patch36.27 KBmpdonadio
#126 interdiff-120-126.txt4.44 KBmpdonadio
#126 support_all_options-2399211-126.patch37.98 KBmpdonadio
#120 interdiff-117-120.txt7 KBmpdonadio
#120 support_all_options-2399211-120.patch36.88 KBmpdonadio
#117 interdiff-112-117.txt3.2 KBmpdonadio
#117 support_all_options-2399211-117.patch36.88 KBmpdonadio
#112 interdiff-107-112.txt5.58 KBmpdonadio
#112 support_all_options-2399211-112.patch35.77 KBmpdonadio
#107 2399211-diff-104-107.txt1.92 KBvijaycs85
#107 2399211-date-formatters-107.patch37.01 KBvijaycs85
#104 support_all_options-2399211-104.patch34.14 KBAnonymous (not verified)
#104 interdiff-103-104.txt3.33 KBAnonymous (not verified)
#98 interdiff-93-98.txt1.86 KBmpdonadio
#98 support_all_options-2399211-98.patch36.95 KBmpdonadio
#93 interdiff-90-93.txt8.19 KBmpdonadio
#93 interdiff-85-93.txt12.06 KBmpdonadio
#93 support_all_options-2399211-93.patch35.09 KBmpdonadio
#90 interdiff-85-90.txt3.87 KBmpdonadio
#90 support_all_options-2399211-90.patch43.02 KBmpdonadio
#85 interdiff-81-85.txt3.2 KBmpdonadio
#85 support_all_options-2399211-85.patch40.53 KBmpdonadio
#81 interdiff-77-81.txt5.28 KBmpdonadio
#81 support_all_options-2399211-81.patch40.39 KBmpdonadio
#77 interdiff-76-77.txt10.51 KBmpdonadio
#77 support_all_options-2399211-77.patch40.37 KBmpdonadio
#76 interdiff-70-76.txt2.62 KBmpdonadio
#76 support_all_options-2399211-76.patch38.8 KBmpdonadio
#70 interdiff.txt3.08 KBrteijeiro
#70 support_all_options-2399211-70.patch38.58 KBrteijeiro
#69 interdiff-67-69.txt7.26 KBmpdonadio
#69 support_all_options-2399211-69.patch38.59 KBmpdonadio
#67 interdiff-61-67.txt850 bytesmpdonadio
#67 support_all_options-2399211-67.patch40.27 KBmpdonadio
#61 interdiff-52-61.txt8.13 KBmpdonadio
#61 support_all_options-2399211-61.patch40.16 KBmpdonadio
#52 interdiff-50-52.txt12.9 KBmpdonadio
#52 support_all_options-2399211-52.patch36.56 KBmpdonadio
#50 interdiff-43-50.txt3.07 KBmpdonadio
#50 support_all_options-2399211-50.patch30.39 KBmpdonadio
#43 interdiff-41-43.txt4.05 KBmpdonadio
#43 support_all_options-2399211-43.patch29.98 KBmpdonadio
#41 interdiff-39-41.txt6.7 KBmpdonadio
#41 support_all_options-2399211-41.patch25.93 KBmpdonadio
#39 support_all_options-2399211-39.patch0 bytesmpdonadio
#34 2399211-date-formatters-34.patch27.14 KBvijaycs85
#25 2399211-date-formatters-25.patch27.65 KBvijaycs85
#24 support_all_options-2399211-24.patch24.33 KBlegolasbo
#24 interdiff-18-24.txt4.98 KBlegolasbo
#18 support_all_options-2399211-18.patch19.35 KBlegolasbo
#18 interdiff-15-18.txt7.81 KBlegolasbo
#15 support_all_options-2399211-15.patch16.25 KBlegolasbo
#12 support_all_options-2399211-12.patch16.21 KBlegolasbo
#12 interdiff-8-12.txt12.11 KBlegolasbo
#8 interdiff-5-8.txt6.38 KBlegolasbo
#8 support_all_options-2399211-8.patch10.52 KBlegolasbo
#5 support_all_options-2399211-5.patch6.88 KBlegolasbo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Time ago sounds like a different formatter to me, not a setting.

Not sure about supporting custom date formats, supporting proper localization for them won't be easy.

dawehner’s picture

@Berdir
Agreed about the time ago.

Regarding the custom one: Isn't it enough to simply specify those to be translatable and be done?

Gábor Hojtsy’s picture

If those date format settings are stored in configuration, they can be marked translatable. For shipped config, their default configs can be translated and even get a proper string context to aid translators :) From core.data_types.schema.yml:

# PHP Date format string that is translatable.
date_format:
  type: string
  label: 'Date format'
  translatable: true
  translation context: 'PHP date format'
legolasbo’s picture

Issue tags: +SprintWeekend2015

I am working on this

legolasbo’s picture

Status: Active » Needs review
FileSize
6.88 KB

I've created a new field formatter which is a straight port of views' time ago formatter.

legolasbo’s picture

Issue summary: View changes
yched’s picture

Thanks @legolasbo !
Just a review of what the code does, not a review of whether it adresses the issue :-)

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,182 @@
    +  public function viewElements(FieldItemListInterface $items) {
    +
    +    $elements = array();
    +
    +    foreach ($items as $delta => $item) {
    +
    +      $output = '';
    

    We can remove the empty lines after function() and foreach()

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,182 @@
    +        // The date was created and verified during field_load(), so it is safe
    

    Did field_load() ever existed ? At any rate, that comment is a bit stale now.

    Also, that code block could simply be moved to a ternary ?
    $elements[$delta] = array('#markup' => ($item->date ? $this->dateFormat($item->date) : '');

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,182 @@
    +    $elements['granularity'] = array(
    +      '#type' => 'number',
    +      '#title' => 'Granularity',
    +      '#default_value' => $this->getSetting('granularity'),
    +    );
    

    As a user, I don't think I would know what to input here. Can we add some #description ?

  4. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,182 @@
    +   * @param object $date
    ...
    +  function dateFormat($date) {
    

    Method lacks visibility - protected, I guess ?
    Also, the param and its phpdoc should typehint to the right interface

  5. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,182 @@
    \ No newline at end of file
    

    Not good :-)

legolasbo’s picture

Issue summary: View changes
FileSize
10.52 KB
6.38 KB

Thanks for your feedback @yched,

Attached patch fixes the issues you mentioned and adds another formatter "Custom" which enables the user to display a date using a custom format string.

yched’s picture

Did not review the new Custom formatter from #8 yet

+++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
@@ -0,0 +1,180 @@
+  function dateFormat(DrupalDateTime $date) {

Still lacks "protected" visibility :-)

Also, method name should be formatDate() - verb first.

yched’s picture

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,80 @@
    +      if (!empty($item->date)) {
    +        $date = $item->date;
    +        $date->setTimeZone(timezone_open(drupal_get_user_timezone()));
    +
    +        $output = $date->format($this->getSetting('date_format'));
    +      }
    

    Nitpick - Lose the empty line ?

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,80 @@
    +  }
    +}
    

    There should be an empty line between the closing } of the last method and the closing } of the class.

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,180 @@
    +  public static function defaultSettings() {
    +    return array(
    +      'format_type' => 'time span',
    +      'granularity' => 2,
    +    ) + parent::defaultSettings();
    +  }
    +
    +  /**
    +   * The date formatter service.
    +   *
    +   * @var \Drupal\Core\Datetime\DateFormatter
    +   */
    +  protected $dateFormatter;
    +
    

    Methods should be defined after properties
    (and typically after the __construct() too - even though I guess that could be debated for static methods)

  4. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,180 @@
    +  }
    +}
     
    

    Same as above

Status: Needs review » Needs work

The last submitted patch, 8: support_all_options-2399211-8.patch, failed testing.

legolasbo’s picture

Issue summary: View changes
FileSize
12.11 KB
16.21 KB

@yched some of your comments also applied to the original formatter. I've also corrected them there. Besides that I've also added a timezone override setting to force the rendering in a certain timezone. This works for all formatters except the time ago formatter because a timespan doesn't change if you change the timezone and it seems illogical to me to compare two different timezones.

I think the current patch at least adds all the features mentioned in the issue summary, but still needs some work regarding UX when timezones are forced. I think there should be an option to append the rendered date with the timezone used, in order to alert the user to the fact that the rendered datetime is in a different timezone than expected.. Something like this:

17 jan 2015 - 21:41 (Europe/Amsterdam)

legolasbo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: support_all_options-2399211-12.patch, failed testing.

legolasbo’s picture

Oops, missed a use statement

legolasbo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 15: support_all_options-2399211-15.patch, failed testing.

legolasbo’s picture

Status: Needs work » Needs review
FileSize
7.81 KB
19.35 KB

Fixed (at least part of) the failing tests by appending the configuration schema and added the option to append the timezone used.

Status: Needs review » Needs work

The last submitted patch, 18: support_all_options-2399211-18.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: support_all_options-2399211-18.patch, failed testing.

legolasbo’s picture

Issue tags: +Needs tests

I could not get the failing tests to pass/fail consistently while trying to find out why the tests fail. When i step trough them using the debugger every assertEqual returns true. The test does fail most of the time, but sometimes it passes miraculously.

I'm at a loss here and welcome anyone to take a stab at this.

The new functionality also needs to have some tests written. Tagged the issue accordingly

dawehner’s picture

.

legolasbo’s picture

Status: Needs work » Needs review
FileSize
4.98 KB
24.33 KB

I found out that a difference in the order of the array keys caused the tests to fail. Since the order doesn't influence any functionality I resorted to sorting the arrays before assertEqual() which makes the tests pass again.

vijaycs85’s picture

FileSize
27.65 KB

Reroll of #24. Still needs test for new formatters.

jhodgdon’s picture

There's a related issue, just want to point out. Its issue summary is bare. Will update it.

jhodgdon’s picture

So... Shouldn't this patch also make the base EntityViewsData class use the 'field' plugin for the timestamp fields, here:

 protected function mapSingleFieldViewsData ...
      case 'timestamp':
      case 'created':
      case 'changed':
        $views_field['field']['id'] = 'date';

That last should then be:

        $views_field['field']['id'] = 'field';

which would make the entity views use the Field formatter for their date fields. Either that or we need to make that change on the parent issue. Or it needs to be a new issue? I'm not sure which, but right now it's not done on either issue.

dawehner’s picture

@jhodgdon
Please don't, this issue should be in scope ... converting all the places, fixing potential issues, is IMHO part of a new issue.

jhodgdon’s picture

jhodgdon’s picture

And by the way I'm now keeping track of what still needs to be done in the "convert the Entity base fields to use Field rendering" effort on #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality. so it won't fail to happen. ;)

dawehner’s picture

In general, I'm glad we made progress here!

  1. index 0718e04..72f8067 100644
    --- a/core/modules/datetime/config/schema/datetime.schema.yml
    
    --- a/core/modules/datetime/config/schema/datetime.schema.yml
    +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    
    +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -30,10 +30,56 @@ field.formatter.settings.datetime_default:
    

    First question: The timestamp handler in views is part of views module itself, so it could be applied to for example the node.created field. At the same time, the TimestampItem is in core, so I kinda think this should belong into core .

  2. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -30,10 +30,56 @@ field.formatter.settings.datetime_default:
    +field.formatter.settings.datetime_custom:
    ...
    +field.formatter.settings.datetime_time_ago:
    

    I like the separation of datetime_custom and datetime_time_ago.

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,82 @@
    +  public function settingsForm(array $form, FormStateInterface $form_state) {
    +    $elements = parent::settingsForm($form, $form_state);
    +
    +    $elements['date_format'] = array(
    +      '#type' => 'textfield',
    +      '#title' => $this->t('Format string'),
    +      '#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')),
    +      '#default_value' => $this->getSetting('date_format'),
    +    );
    +
    +    return $elements;
    +  }
    

    I'm curious, can we apply any kind of validation here?

  4. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,102 @@
    +
    +
    

    Nitpick: Two empty lines

Gábor Hojtsy’s picture

Priority: Normal » Major

@vijaycs85: still working on this one?

Elevating to major being a subissue of #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality.

vijaycs85’s picture

Assigned: Unassigned » vijaycs85

I will try to address #31 and some test.

vijaycs85’s picture

Assigned: vijaycs85 » Unassigned
FileSize
27.14 KB

here is a reroll.

#31.1 - the field is from datetime module, so the schema.
#31.3 - Doesn't look like we do any validation - core/modules/system/src/Form/DateFormatFormBase::dateTimeLookup
#31.4 - Fixed.

P.S: didn't manage to update the tests.

jhodgdon’s picture

Status: Needs review » Needs work

Nitpicks:

a)

++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
@@ -0,0 +1,82 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\datetime\Plugin\Field\FieldFormatter\DateTimePlainFormatter.

Should be contains ... DateTimeCustomFormatter

b) I do not think @ingroup field_formatter should be on the base formatter class. If you look at https://api.drupal.org/api/drupal/core!modules!field!field.api.php/group... the only things listed there are the base classes/interfaces for the entire formatter system. DateTimeBase doesn't belong there.

c) In the base formatter class:

+   * @param $rendered_date
+   *   A formatted date string.
+   * @return string
+   *   A formatted date string with the timezone appended.
+   */
+  protected function appendTimezone($rendered_date){

Needs blank line between param and return. Also the param needs a type (string) in the @param line.

d)

+      return "$rendered_date <em>($timezone)</em>";

Really, we're doing this??!? How about using formatString()? Putting EM tags in directly and substituting variables like that seems wrong.

e) In the Ago formatter:

+    $elements['granularity'] = array(
+      '#type' => 'number',
+      '#title' => 'Granularity',
+      '#default_value' => $this->getSetting('granularity'),
+      '#description' => $this->t('The number of different time units to display.'),
+    );

I had to go look up DateFormatter::formatInterval() and read the code to figure out what "Granularity" and "The number of different time units to display" meant. I think this needs better/more documentation. What is happening in the formatInterval function is:
- It goes down a list of units: year, month, week, day, hour, minute, second -- starting at year.
- It checks to see if the interval is bigger than that number of seconds. If it is, that counts as one "unit to display". The output is formatted and the interval is subtracted off.
- It continues until the required granularity is reached (a certain different number of time units have been added to the display).
- This whole way of doing things in the formatInterval() function is also wrong -- I filed #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known. Just wrong. Can't believe this is in Core.

f) I guess this all needs tests.

xjm’s picture

Issue tags: +VDC
mpdonadio’s picture

Assigned: Unassigned » mpdonadio

I'll bring this home. Is the feedback in #35, and some unit tests, the only outstanding work?

mpdonadio’s picture

Issue tags: +Needs reroll

#34 doesn't apply anymore. I'll post a reroll tonight before I start work on this.

mpdonadio’s picture

Issue tags: -Needs reroll
FileSize
0 bytes

Rebase w/ choosing the patch over HEAD. All of the conflicts were in the Migrate tests.

mpdonadio’s picture

Status: Needs work » Needs review

Now onto the feedback.

mpdonadio’s picture

I read through the issue a few times, and I think all feedback has been addressed, other than test coverage for the new formatters. If I missed anything, let me know.

I also did a visual pass through the three new files (DateTimeCustomFormatter.php, DateTimeFormatterBase.php, and DateTimeTimeAgoFormatter.php) and fixed some coding standard things as well as a few things PhpStorm was complaining about.

OK, let's see if I broke anything with the existing tests before moving onto new tests.

mpdonadio’s picture

Why does DateTimeFieldTest rely on the system timezone (not the test system it is running in) being UTC?

mpdonadio’s picture

First pass at test coverage for the new formatters.

jhodgdon’s picture

Thanks! Changes in #41 addressed most of my comments from #35. I still think that the settings form for Granularity may need more documentation for the user though:

+    $elements['granularity'] = array(
+      '#type' => 'number',
+      '#title' => 'Granularity',
+      '#default_value' => $this->getSetting('granularity'),
+      '#description' => $this->t('The number of different time units to display.'),
+    );

Do you think they'll actually understand what this means?

mpdonadio’s picture

#44 May bad; I forgot about that one. That description pretty much matches the docblock DateFormatter::formatInterval(), which is also obtuse.

  '#description' => $this->t('The number of time units to display, from years (1) down to seconds (7).'),
jhodgdon’s picture

Well that is even worse, because really what happens if you say granularity is 2, and say your time interval is 3 hrs, 20 mins, and 22 seconds, you'll get "3 hours 20 minutes" as output, whereas that doc in #44 would imply that you'd get years and months.

dawehner’s picture

+++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
@@ -178,12 +133,26 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
 
+    $elements['granularity'] = array(
+      '#type' => 'number',
+      '#title' => 'Granularity',
+      '#default_value' => $this->getSetting('granularity'),
+      '#description' => $this->t('The number of different time units to display.'),

What about not adding the granularity for now? Isn't that a feature which did not existed in views at all?

+++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
@@ -0,0 +1,82 @@
+/**
+ * Plugin implementation of the 'datetime_custom' formatter.
+ *

That line of documentation isn't helpful

mpdonadio’s picture

#47-1: It is called Interval in the Date module for Drupal 7, and is exposed to Views 3 when you add a Date field to the row output (I double checked on a D7 site). The description there is "How many time units should be shown in the 'time ago' string.". See date_interval_formatter_settings_form()

#47-2: Unhelpful, but consistent all of the formatters in \Drupal\Core\Field\Plugin\Field\FieldFormatter and the core modules that define field formatters in HEAD.

dawehner’s picture

#47-1: It is called Interval in the Date module for Drupal 7, and is exposed to Views 3 when you add a Date field to the row output (I double checked on a D7 site). The description there is "How many time units should be shown in the 'time ago' string.". See date_interval_formatter_settings_form()

Right, but in order to resolve the critical, we just have to get the options which are currently supported by views, just saying.

#47-2: Unhelpful, but consistent all of the formatters in \Drupal\Core\Field\Plugin\Field\FieldFormatter and the core modules that define field formatters in HEAD.

Well, I'd prefer sanity over consistency.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
FileSize
30.39 KB
3.07 KB

OK, @dawehner and I had a quick IRC conversation this morning. Since we can't think of better language, we are going to go with the Drupal 7 label / description for the granularity (see my link in #48). We can create a followup to address this, though, as it is confusing but not critical for the patch.

We also decided that the short description on the class was more better in this patch, even though it isn't consistent with the other field formatters.

I think this is good to go.

dawehner’s picture

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeDefaultFormatter.php
    @@ -160,15 +155,19 @@ public function viewElements(FieldItemListInterface $items) {
    -  function dateFormat($date) {
    +  function formatDate($date) {
    

    I guess we can make it protected on the longrun?

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -191,10 +160,41 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +    $interval = REQUEST_TIME - $date->getTimestamp();
    

    Let's replace the usage of REQUEST_TIME, see #2459155: Remove REQUEST_TIME from bootstrap.php

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -191,10 +160,41 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
    +      default: // Fallback to raw time ago.
    

    Note: We usually don't use those kind of style of comments.

  4. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
    @@ -155,6 +155,33 @@ function testDateField() {
    +    $this->assertText($expected, format_string('Formatted date field using datetime_custom format displayed as %expected.', array('%expected' => $expected)));
    ...
    +    $this->assertText($expected, format_string('Formatted date field using datetime_time_ago format displayed as %expected.', array('%expected' => $expected)));
    
    @@ -221,6 +248,33 @@ function testDatetimeField() {
    +    $this->assertText($expected, format_string('Formatted date field using datetime_custom format displayed as %expected.', array('%expected' => $expected)));
    ...
    +    $this->assertText($expected, format_string('Formatted date field using datetime_time_ago format displayed as %expected.', array('%expected' => $expected)));
    

    Let's not use format_string anymore, but SafeMarkup::format()

  5. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateFieldFormatterSettingsTest.php
    @@ -157,79 +157,79 @@ public function testEntityDisplaySettings() {
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    

    +1 for not using assertIdentical as more strict checking is not necessarily a better idea, due to its costs during changes of code.

mpdonadio’s picture

OK, #51 should be addressed. Per @alexpott, REQUEST_TIME is still in simpletest, so I didn't change that.

I have no idea if this will pass. There are a few things really wrong. Unfortunately, I think HEAD and the patch have problems, and both the formatters and the tests have problems.

One, a Date-Only field renders out with the time. HEAD does this, too. This is a regression from Drupal 7. Haven't dug into this.

Two, with HEAD, the rendered time for a Date-Only field is always 12:00. With the patch (tested back to #39, it is 12:00 adjusted for the user's system timezone (but not adjusted for DST). I'm UTC-5 w/o DST, so it prints out as 07:00 for me.

Oddly enough, the Date+Time seems to work as expected, but I haven't actually looked at storage to see the values there.

I think the timeago formatters are OK, but I didn't adjust the test expectation when I started chasing my tail with timezone problems. I'm not 100% sure they are immune to the TZ problem mentioned above.

I wanted to get others thoughts on this before just make the patch pass in case there is actually a problem with the code, especially since I jumped in here halfway.

dawehner’s picture

Thank you for addressing my points of the review! I'll read your comment later today. Are you interested in working on #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter once this issue is in?

mpdonadio’s picture

Priority: Major » Critical

Also, elevating this to Critical as the parent is a critical.

Status: Needs review » Needs work

The last submitted patch, 52: support_all_options-2399211-52.patch, failed testing.

effulgentsia’s picture

Priority: Critical » Major

Thanks for the great work on this. I don't think it's a release blocker though, so back to Major. The proposed resolution in #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality distinguishes between the children that are required for respecting access control (critical) vs. those that add/restore all the desired options (important, but not critical), and I think this child is the latter.

effulgentsia’s picture

Priority: Major » Critical
Issue tags: +blocker

Apologies. I see now that #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter, which is critical due to access control, is postponed on this, which does make this critical. Sorry for the noise.

mpdonadio’s picture

I'll try to catch @dawehner on IRC tomorrow and chat about this. If we think we can proceed on #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter w/o this patch in final form, I will unpostpone and demote back to Major.

effulgentsia’s picture

Issue tags: +Critical Office Hours

Per #2393339-57: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality, tagging for critical office hours, but if there's a reason to not have this particular child issue in that list (e.g., if it's already almost done and the people already working on this don't want new help), please untag it.

dawehner’s picture

The date field in D7 is quite elaborated, as it takes into account both the user and the server timezone.

@mpdonadio
I'm curious whether we could defer that kind of work into a follow up and enable you to work on the views issue at the same time?

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
40.16 KB
8.13 KB

Throwing this at TestBot. DateTimeFieldTest and all PhpUnit work locally for me.

I think I may have made an out of scope change, but DateTimeFieldTest wouldn't pass w/o it. I will explain all when I have time, but reading the interdiff will show the general problems.

Nobody is listed in MAINTAINERS for the DateTime module. Who unofficially owns it?

Oh, I hate timezones.

YesCT’s picture

Issue summary: View changes

.

mpdonadio’s picture

OK, explanation.

WebTestBase doesn't set a timezone, so tests will run in your default timezone per PHP. This patch explicitly sets one for predictable results, and picks one near the extreme end.

Storage uses UTC.

As far as I can tell, Date+Time gets handled properly through UI -> DB -> UI roundtrip. Objects get created w/ the UTC timezone, and then you be can set whatever timezone you need. Yay.

Storage for DateOnly just stores the date. This is where things get weird. When you read from storage, just the Y/m/d get read. The time portion is set to the local time. There is a helper function, datetime_date_default_time(), to set this back to noon. The problem is if you set the timezone before doing this, the day may leap forward or backward. Blech. So, this version of the patch sets the default time before adjusting the timezone in all of the formatters. As I write this out, I think we can move this logic into DateTimeFormatterBase::setTimeZone(), but it still needs to be done for timeago, even though a timezone isn't needed for that (I need to play with this a little).

The above also happened in the field widget. I fixed it there, too.

mpdonadio’s picture

OK, I am really confused now that I have read some related issues. The IS mentions the "timestamp" handler, but the first patch (and what I continued to work on) updated the "datetime" handlers. #27 mentions this, but #28 vetos it.

On the remaining tasks, "select the timezone ..." was struck as a remaining task, but wasn't in the patch that was posted when it was struck out.

So, at the least this needs an IS update and a Beta Eval. Should the IS just be updated to reflect what the current patch does and we will handle the timezone selection for "datetime" and "timestamp" fields as a followup?

mpdonadio’s picture

Priority: Critical » Major
Issue summary: View changes
Issue tags: -blocker, -Critical Office Hours

This doesn't actually block #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter. They are related, but this does not need to be in for work to begin on that. I don't think this needs to be critical, either, since it is only adding formatters and not directly making the change to use field formatters instead of views plugins.

dawehner’s picture

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,91 @@
    +        if ($this->getFieldSetting('datetime_type') == 'date') {
    +          // A date without time will pick up the current time, use the default.
    +          datetime_date_default_time($date);
    +          $this->setTimeZone($date);
    +        }
    

    Don't we want to pick up the timezone from the current user?

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,91 @@
    +      }
    +      $elements[$delta] = array('#markup' => $this->appendTimezone($output));
    

    I guess we need the timezone cache contexts added here.

mpdonadio’s picture

#66-1, this is already done. `$this->setTimeZone($date)` calls `DateTimeFormatterBase::setTimeZone()`, which takes into account the timezone override setting and the system/user timezone logic.

#66-2, added the cache context to the render array.

dawehner’s picture

It looks really great so far! We also have additional test coverage.

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,98 @@
    +
    +        $output = $date->format($this->getSetting('date_format'));
    +      }
    

    I'm curious whether we have to pass along the langcode somehow, similar to the discussions, which happen around #2449597: Number formatters: Make it possible to configure format_plural on the formatter level (comment 30 for example)

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,105 @@
    +    $elements['timezone_override'] = array(
    

    Nitpick: Using $form feels just more natural here as variable name.

mpdonadio’s picture

#68-1: the constructor for DrupalDateTime() already sets up the language for the $date object:

public function __construct($time = 'now', $timezone = NULL, $settings = array()) {
    if (!isset($settings['langcode'])) {
      $settings['langcode'] = \Drupal::languageManager()->getCurrentLanguage()->getId();
    }

I guess the option we would have is to replace all of the formatter calls with

$langcode = $this->view->storage->get('langcode') ?: \Drupal::languageManager()->getCurrentLanguage()->getId();
$output = $date->format($format, ['langcode' => $langcode]);

or maybe

$output = $date->format($format, $item->getLangcode ?: \Drupal::languageManager()->getCurrentLanguage()->getId());

and then add the proper cache context tag to the render array.

Is the current language ever different than the language of the view? As a stupid American :) I am not terribly familiar with this aspect of Drupal.

#68-2: Changed to $form. Poked through core, and it isn't terribly consistent with $element vs $form for the settings forms in the plugins, but I think you are right here.

Also cleaned up some PhpStorm warnings, and add calls to parent methods where necessary.

rteijeiro’s picture

Code looks good. Just fixed a few nitpicks.

jhodgdon’s picture

So... I've also been following/reviewing #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known, already marked related by the way, and I thought I had commented here before about this, but I guess I hadn't.

The question I have is whether all of these options on the Ago formatter really make sense. I realize they've been adopted from the existing Views formatter, but... we have the opportunity here to make things better since this Time Ago formatter didn't exist before in Core.

So my concerns are about the options:

  1. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -30,10 +30,56 @@ field.formatter.settings.datetime_default:
    +field.formatter.settings.datetime_time_ago:
    +  type: mapping
    +  label: 'Datetime time ago display format settings'
    +  mapping:
    +    timezone_override:
    +      type: string
    +      label: 'Timezone override'
    +    append_timezone:
    +      type: boolean
    +      label: 'Append timezone'
    +    format_type:
    

    Time zones for "ago" formats don't actually make sense.

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,212 @@
    +  public static function defaultSettings() {
    ...
    +    ) + parent::defaultSettings();
    

    So here I think we should get rid of the time zone pieces...

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,212 @@
    +  public function settingsForm(array $form, FormStateInterface $form_state) {
    +    $form = parent::settingsForm($form, $form_state);
    

    And here also I think we should remove the time zone settings.

  4. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,212 @@
    +  public function settingsSummary() {
    ...
    +    $future_date = new DrupalDateTime('1 year 1 month 1 week 1 day 1 hour 1 minute');
    +    $past_date = new DrupalDateTime('-1 year');
    +    $summary[] = t('Future date: !display', array('!display' => $this->formatDate($future_date)));
    +    $summary[] = t('Past date: !display', array('!display' => $this->formatDate($past_date)));
    +
    

    Shouldn't the past date be just as interesting as the future date for showing the settings? Past dates are *way* more common (e.g., the date a node was created/updated).

  5. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,212 @@
    +    switch ($format) {
    +      case 'time ago':
    +        return $this->t('%time ago', array('%time' => $this->dateFormatter->formatInterval($interval, $granularity)));
    +      case 'raw time hence':
    +        return $this->dateFormatter->formatInterval(-$interval, $granularity);
    +      case 'time hence':
    +        return $this->t('%time hence', array('%time' => $this->dateFormatter->formatInterval(-$interval, $granularity)));
    +      case 'raw time span':
    +        return ($interval < 0 ? '-' : '') . $this->dateFormatter->formatInterval(abs($interval), $granularity);
    +      case 'inverse time span':
    +        return ($interval > 0 ? '-' : '') . $this->dateFormatter->formatInterval(abs($interval), $granularity);
    +      case 'time span':
    +        return $this->t(($interval < 0 ? '%time hence' : '%time ago'), array('%time' => $this->dateFormatter->formatInterval(abs($interval), $granularity)));
    +      case 'raw time ago':
    +      default:
    +        return $this->dateFormatter->formatInterval($interval, $granularity);
    +    }
    

    I don't get why we really need all these options, or how a user is really going to understand the differences? I mean, they're called things like "Raw this" and "span" and "hence", are they really going to understand which option does what? I think this would be a *lot* better if we just let them put in a formatting string for past and a formatting string for future. So the settings form would say:
    Format for past: [text box, defaulting to "@time ago", with description saying to use @time to indicate the formatted interval]
    Format for future: [same, defaulting to "@time hence"]

    And then the code would just say
    if( it's in the future) {
    return format_string($future_setting, array(@time => formatInterval(...)));
    }
    etc.

    These past/future strings could be translated with Config Translate and are general enough to cover all the possibilities.

Thoughts?

jhodgdon’s picture

The comment I thought I had left here is actually on #2455125-42: Update EntityViewsData use of generic timestamp to use Field API formatter, but then I realized on #2456521-62: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known that prefix/suffix wasn't easily translatable. So I'm advocating making a @time ago type of string for all the cases.

Anonymous’s picture

Status: Needs review » Needs work

See #71.

As for #71.5. I've given this some thought, and while you are absolutely right, this issue is about porting the existing features. IMO, changing the formatters (even if it's simplifying and good for UX) is out of scope here.

I looked at the patch, and I was wondering if we have sufficient test coverage? It seems like we are not testing all formatters properly. Do we have that already?

mpdonadio’s picture

OK, had brief IRC conversation with @dawehner and he is for simplifying the timeago stuff. We should move forward with the idea from #44 to use the prefix/suffix fields when a user want timeago. Since https://www.drupal.org/node/2456521 is frozen, we aren't going to use the new method that it introduces; that can be done as a followup.

If anyone wants to work on this feel free; I'll assign myself if I can actually block out some time for it.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio
Status: Needs work » Needs review
FileSize
38.8 KB
2.62 KB

OK, just a baby step here to address #71:1-4. Didn't touch any of the tests.

I did notice that the settings summary for this and #70 both output 0 sec for timeago dates in the future. Not sure if this is the problem, if if the problem is in the dateformatter itself.

Next patch will have the reworked prefix/suffix options based on [#9831765]

mpdonadio’s picture

OK, more better test coverage. DateTimeFieldTest passes locally. Ready for review.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Issue summary: View changes
Issue tags: -Needs issue summary update, -Needs beta evaluation

I think IS accurately reflects the patch now. The siblings don't seem to have beta evals, so I am not adding one.

Anonymous’s picture

Status: Needs review » Needs work

This looks great already!

Looking at the tests, I think we don't have test coverage for DateTimeDefaultFormatter?

Some more feedback:

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,100 @@
    +        if ($this->getFieldSetting('datetime_type') == 'date') {
    +          // A date without time will pick up the current time, use the default.
    +          datetime_date_default_time($date);
    +          $this->setTimeZone($date);
    +        }
    +        else {
    +          $this->setTimeZone($date);
    +        }
    

    You could just use $this->setTimeZone($date) once and drop the else clause.

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeDefaultFormatter.php
    @@ -116,13 +113,16 @@ public function viewElements(FieldItemListInterface $items) {
    +          $this->setTimeZone($date);
             }
    -        $formatted_date = $this->dateFormat($date);
    +        else {
    +          $this->setTimeZone($date);
    +        }
    

    Here that can be changed as well.

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,109 @@
    +      '#description' => $this->t('Always use the selected timezone'),
    

    This description confuses me. The timezone_override is of type select, but this description make me think I want to have an answer with yes/no.

    If I understand this correctly, it could say: "The timezone selected here, will always be used."

  4. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,109 @@
    +   *   A date object.
    

    It also holds time, so "A date and time object." might be more suitable?

  5. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,109 @@
    +  protected function setTimeZone(DrupalDateTime $date) {
    

    This naming confuses me. Even though the visibility is not public, I think it looks quite a bit like a setter for a timezone on the object you call it with. Instead this is a setter for the object you pass along. I'm not sure how this is handled in core elsewhere, I just wanted to note it.

  6. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,109 @@
    +   * Appends the timezone used to render the date.
    

    "Appends the timezone to a rendered date string."?

    I also think we should note here (possibly in a new paragraph) that it is possible that a timezone is not appended.

  7. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,109 @@
    +   *   A formatted date string with the timezone appended.
    

    This comment also is not accurate, since the timezone is not always appended.

  8. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
    @@ -21,29 +20,30 @@
    +          $this->setTimeZone($date);
    ...
    +        else {
    +          $this->setTimeZone($date);
    

    Again the double setTimeZone thing.

  9. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,214 @@
    +  /**
    +   * The current Request object.
    +   *
    +   * @var \Symfony\Component\HttpFoundation\Request
    +   */
    +  protected $request;
    

    Could you explain why we need the request object here?

    Found it's usage in formatDate(), but I think it could be removed, and by extension this variable as well.

  10. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,214 @@
    +      $container->get('request_stack')->getCurrentRequest()
    

    Coding standards want a trailing comma. See https://www.drupal.org/coding-standards#array

  11. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,214 @@
    +    $interval = $this->request->server->get('REQUEST_TIME') - $date->getTimestamp();
    

    You could use "(int) $_SERVER['REQUEST_TIME'])" here.

  12. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
    @@ -103,9 +113,17 @@ function testDateField() {
    +    // Built up a UTC date.
    

    Suggestion: Build up a date in the UTC timezone.

  13. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
    @@ -103,9 +113,17 @@ function testDateField() {
    +    // Update the timezone to the system default.
    +    $date->setTimezone(timezone_open(drupal_get_user_timezone()));
    

    Some lines earlier it was set to "UTC", so what was the use of that?

  14. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
    @@ -154,7 +170,55 @@ function testDateField() {
    +    $this->assertText($expected, SafeMarkup::format('Formatted date field using datetime_custom format displayed as %expected.', array('%expected' => $expected)));
    

    Shouldn't this (and several other occurrences) use t() instead of SafeMarkup::format()?

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Thanks. I fix this up today or tomorrow. However, re #79-11, using the request object to get the request time is the better thing to do, as the superglobal should be avoided. Globals are being phased out so we can move away from SimpleTest in the future.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
40.39 KB
5.28 KB

I added debug and the default formatter is indeed being tested.

#79 should be addressed, except

9,11: Using the $_SERVER superglobal should be avoided. It is bad practice, and hampers testability; DI should be used instead. The proper thing is to use the request stack from the container, and then use the current request object to get the REQUEST_TIME.

10: This is a constructor call using late static binding, not an array, Adding a trailing comma to the last parameter is a syntax error.

13: The date-only objects use a time of noon (that is what datetime_date_default_time does), which we want on the UTC date before we adjust for the system timezone. This just does everything with the API instead of magic values.

14. Assertion messages in SimpleTest should not be translated per the documentation (look at the docblock on everything in AssertContentTrait).

Anonymous’s picture

Thanks mpdonadio!

Re #81:
9,11: I stumbled upon this several times now, and we always ended up using the superglobal (or the constant if it was a refactor of existing code). Thanks for explaining the proper way to do this!

10. Oh dear. You are totally right.

13. Aha, I see. That makes sense.

14: Ok, I did not know that.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

It looks great for me now! Thank you @mpdonadio for your long term effort here!!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 81: support_all_options-2399211-81.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
40.53 KB
3.2 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

That is pretty sensible!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update
  1. Do we not get to remove any views code with this patch?
  2. This issue is a task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
  3. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -30,10 +30,53 @@ field.formatter.settings.datetime_default:
    +    timezone_override:
    +      type: string
    +      label: 'Timezone override'
    +    append_timezone:
    +      type: boolean
    +      label: 'Append timezone'
     
     field.formatter.settings.datetime_plain:
       type: mapping
       label: 'Datetime plain display format settings'
    +  mapping:
    +    timezone_override:
    +      type: string
    +      label: 'Timezone override'
    +    append_timezone:
    +      type: boolean
    +      label: 'Append timezone'
    +
    +field.formatter.settings.datetime_custom:
    +  type: mapping
    +  label: 'Datetime plain display format settings'
    +  mapping:
    +    timezone_override:
    +      type: string
    +      label: 'Timezone override'
    +    append_timezone:
    +      type: boolean
    +      label: 'Append timezone'
    +    date_format:
    +      type: string
    +      label: 'Format string'
    +      translatable: true
    +      translation context: 'PHP date format'
    +
    

    Given that timezone_override and append_timezone come from an abstract base class it is really nice when the schema inheritance follows the same structure. That way if the base class changes you only have one place to change things.

  4. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -30,10 +30,53 @@ field.formatter.settings.datetime_default:
    +field.formatter.settings.datetime_time_ago:
    +  type: mapping
    +  label: 'Datetime time ago display format settings'
    +  mapping:
    +    future_format:
    +      type: string
    +      label: 'Future format string'
    +    past_format:
    +      type: string
    +      label: 'Past format string'
    +    granularity:
    +      type: integer
    +      label: 'Granularity'
    

    This is missing the timezone_override and append_timezone schema - would be solved it is used the new type proposed above.

  5. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,214 @@
    +   * @todo Refactor when https://www.drupal.org/node/2456521 is comitted.
    

    committed is misspelt.

  6. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
    @@ -391,7 +503,6 @@ function testDefaultValue() {
    -
    
    @@ -417,7 +528,7 @@ function testInvalidField() {
    -    $this->assertText('date is invalid', format_string('Invalid year value %date has been caught.', array('%date' => $date_value)));
    +    $this->assertText('date is invalid', SafeMarkup::format('Invalid year value %date has been caught.', array('%date' => $date_value)));
    
    @@ -425,7 +536,7 @@ function testInvalidField() {
    -    $this->assertText('date is invalid', format_string('Invalid month value %date has been caught.', array('%date' => $date_value)));
    +    $this->assertText('date is invalid', SafeMarkup::format('Invalid month value %date has been caught.', array('%date' => $date_value)));
    
    @@ -433,7 +544,7 @@ function testInvalidField() {
    -    $this->assertText('date is invalid', format_string('Invalid day value %date has been caught.', array('%date' => $date_value)));
    +    $this->assertText('date is invalid', SafeMarkup::format('Invalid day value %date has been caught.', array('%date' => $date_value)));
    
    @@ -451,7 +562,7 @@ function testInvalidField() {
    -    $this->assertText('date is invalid', format_string('Invalid hour value %time has been caught.', array('%time' => $time_value)));
    +    $this->assertText('date is invalid', SafeMarkup::format('Invalid hour value %time has been caught.', array('%time' => $time_value)));
    
    @@ -460,7 +571,7 @@ function testInvalidField() {
    -    $this->assertText('date is invalid', format_string('Invalid minute value %time has been caught.', array('%time' => $time_value)));
    +    $this->assertText('date is invalid', SafeMarkup::format('Invalid minute value %time has been caught.', array('%time' => $time_value)));
    
    @@ -469,7 +580,7 @@ function testInvalidField() {
    -    $this->assertText('date is invalid', format_string('Invalid second value %time has been caught.', array('%time' => $time_value)));
    +    $this->assertText('date is invalid', SafeMarkup::format('Invalid second value %time has been caught.', array('%time' => $time_value)));
    

    This all looks out-of-scope.

  7. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateFieldFormatterSettingsTest.php
    @@ -157,79 +157,79 @@ public function testEntityDisplaySettings() {
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    ...
    -    $this->assertIdentical($expected, $component);
    +    $this->assertEqual(ksort($component), ksort($expected));
    

    Why are these changes necessary?

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

I'll try to address everything and get a new patch posted soon.

1: I will defer to @dawehner on this, but I am pretty sure we are keeping all of the Views code for sitebuilders who need the plugins for hook_views_data()

2: I'm happy to add a beta eval, but I thought this and all of the siblings were already triaged by the core maintainers as part of #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality? (None of the siblings that have been committed seem to have beta evals, either).

3+4: In #76, we removed the timezones from the timeago formatters, so I am not sure how to handle this with the schemas?

6: In #51, @dawehner requested Safemarkup::format(). Should I just use it in the assertions that were added, and leave the rest as format_string()?

7. This was explained in #24, but I will double check this (it was before I got involved in the patch).

alexpott’s picture

So I missed...

+++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
@@ -0,0 +1,214 @@
+class DateTimeTimeAgoFormatter extends DateTimeFormatterBase implements ContainerFactoryPluginInterface {
...
+    // Timezones don't make sense for "ago" formats, so they should be removed.
+    unset($settings['timezone_override'], $settings['append_timezone']);
...
+    // Timezones don't make sense for "ago" formats, so they should be removed.
+    unset($form['timezone_override'], $form['append_timezone']);

What is the point of extending the class if we are unsetting this?

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
43.02 KB
3.87 KB

OK, this just addresses 3, 4, and 5. DateTimeTimeAgoFormatter does not need to extend DateTimeFormatterBase. I think I did the schema inheritance properly. I'll look at the rest later.

dawehner’s picture

1: I will defer to @dawehner on this, but I am pretty sure we are keeping all of the Views code for sitebuilders who need the plugins for hook_views_data()

Wel yeah for tables, like the tracker ones, we can't use those entity field/formatters.

mpdonadio’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

OK, took a stab at a beta eval. I didn't find a sibling with one after more searching, but feel free to tweak it.

I have everything else addressed, but want to do another once over of the patch to double check it before posting it.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
FileSize
35.09 KB
12.06 KB
8.19 KB

OK, I think this is good.

#87 should all be addressed. In particular,

1. No views code is being removed; there is still use for those plugins.

6. I reverted those, as those tests do not cover portions of the code that changed.

7. I reverted this to HEAD, and it passes locally for me. I suspect something else was wrong that was causing this to fail. However, I do agree with the spirt of that hunk, which could be a worthwhile followup.

I uploaded two interdiffs to make this easier to review, since there was an intermediate patch to double check the change to the parent class of the timeago formatter.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I agree we should not convert existing format_string() calls but not introduce new ones.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 93: support_all_options-2399211-93.patch, failed testing.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

I swear I ran that test locally before posting this, and it passed. I fails locally for me after I did a full site rebuild.

The patch from #24 makes this pass, but I think it is hiding problems. HEAD uses assertIdentical(); that patch uses assertEquals()

If I expand out the error messages:

Value array ( 'label' => 'above', 'weight' => 10, 'type' => 'datetime_default', 'settings' => array ( 'format_type' => 'fallback', ), 'third_party_settings' => array ( ), ) is identical to 
value array ( 'label' => 'above', 'weight' => 10, 'type' => 'datetime_default', 'settings' => array ( 'format_type' => 'fallback', 'timezone_override' => '', 'append_timezone' => false, ), 'third_party_settings' => array ( ), ).

you will see that the differences are with missing keys that have zero length strings, so I think assertEqual() was masking this b/c the whacky PHP type system (and I say that in jest as someone who actually owns the System F book...).

So, it looks like the proper thing to do is to update the expected settings values to match what was done in the patch. Does anything need to be done to the Migrations themselves?

Now that I read this test more closely, it needs to be adjusted for the new display settings.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
36.95 KB
1.86 KB

These Migrate tests are driving me batty. MigrateFieldFormatterSettingsTest only failed when run from MigrateDrupal6Test. I could not get it to fail directly.

I think this takes care of the problem, and I ran MigrateDrupal6Test via run-tests.sh about ten times w/o problems.

The last submitted patch, 93: support_all_options-2399211-93.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 98: support_all_options-2399211-98.patch, failed testing.

The last submitted patch, 98: support_all_options-2399211-98.patch, failed testing.

mpdonadio’s picture

OK, I am throwing in the towel for a bit. I think this needs a fresh set of eyes / hands / brain thing.

I git a `git pull`, applied the patch in #98 and ran MigrateDrupal6Test and DateTimeFieldTest 10 time locally w/o problems.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
3.33 KB
34.14 KB

After applying the patch locally, I was able to reproduce the test failures. This patch just tries to make this green again, without properly addressing the issues.

Status: Needs review » Needs work

The last submitted patch, 104: support_all_options-2399211-104.patch, failed testing.

Anonymous’s picture

Hm. For the migration failures, the expected value seems to differ between #98, #104 and my local installation. It looks like those parameters are randomly there.

The dateformatter issue is odd as well. The same interval is formatted by the same function, so we'd expect the same result. Would it make sense to calculate the expected formatted date as a workaround?
Fwiw, 87654321 should be calculated as 2 years, 9 months, 2 weeks, (0 days,) 12 hours, 25 minutes, 21 seconds by Dateformatter::formatInterval(), which is used by DateTimeTimeAgoFormatter. That means that, if we take into account the granularity, the test in #98 should be green, and the one in #104 should be red.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
37.01 KB
1.92 KB

Looks like the fails on #98 is weird. This patch working on my local.

vijaycs85’s picture

Issue tags: +LONDON_2015_MAY
Anonymous’s picture

Looks like the fails on #98 is weird. This patch working on my local.

That was exactly what I experienced. It seems like the expected variable is sometimes different. I'm not sure why.

Status: Needs review » Needs work

The last submitted patch, 107: 2399211-date-formatters-107.patch, failed testing.

mpdonadio’s picture

I spent a little time experimenting with this, and I think the fails on TestBot are due to concurrency and some of the limitations/quirks with SimpleTest in which context of tests are run. That is the only thing that could explain REQUEST_TIME being different. Since we are not testing the dateformatter itself, just the field foramtter, I think we can refactor the expected values in the test to just use the interval directly and compute it from the dateformatters instead of hardcoding the expected string value. I think that will normalize things. We would need buyin that this isn't cheating the tests, though.

I didn't look at the last batch of failures in MigrateFieldFormatterSettingsTest, but I am wondering if I botched the schema declaration in the YAML, and that is the real problem.

mpdonadio’s picture

Blerg. More timezone problems. Since the date read out of the database will be in UTC and we stripped timezones from the timeago formatter, the timeago tests need to force UTC dates to get a consistent interval. DateTimeFieldTest passes locally for me now, and I think this will be consistent now.

MigrateFieldFormatterSettingsTest still fails individually b/c settings mismatch, but passes when part of MigrateDrupal6Test. I suspect it is because the individual test isn't installing the schema and/or config. Checking that next.

mpdonadio’s picture

Status: Needs work » Needs review

/me grumble

Status: Needs review » Needs work

The last submitted patch, 112: support_all_options-2399211-112.patch, failed testing.

chx’s picture

Currently MigrateDrupal6Test passes because #2176251: Field and FieldInstance default settings are not populated (would you please fix that next? it's just docs). But in the individual test the defaults are populated because we use create so the test expectations which check with identical, fail. You need to add the new defaults to the migration and the test expectations.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

#115 is the summary of an IRC conversation I had with @chx. I am going to tackle this.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
36.88 KB
3.2 KB

Think this should do it. Both MigrateDrupal6Test and MigrateFieldFormatterSettingsTest come up green locally for me.

I reverted MigrateFieldFormatterSettingsTest to HEAD before this to have a clean slate.

jhodgdon’s picture

Status: Needs review » Needs work

I took a careful look at this patch, aside from the tests near the end, where I kind of glazed over, but they look reasonable.

I have a few comments about documentation and UI text, and one that is either a functionality question or a docs question.

  1. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -23,8 +23,19 @@ field.value.datetime:
    +      label: 'Timezone override'
    

    Time zone is two words in English, right? So all docs and labels and UI text with "timezone" in them should be "time zone"

  2. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -23,8 +23,19 @@ field.value.datetime:
    +      label: 'Append timezone'
    

    timezone -> time zone here too

  3. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -32,8 +43,32 @@ field.formatter.settings.datetime_default:
    +field.formatter.settings.datetime_custom:
    +  type: field.formatter.settings.datetime_base
       label: 'Datetime plain display format settings'
    +  mapping:
    

    This label should say "custom" not "plain" in this section.

  4. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -32,8 +43,32 @@ field.formatter.settings.datetime_default:
    +      label: 'Format string'
    

    I don't think we should be using "string" in user-facing text? Should this be maybe just "Format" or maybe "Date/time format"?

  5. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -32,8 +43,32 @@ field.formatter.settings.datetime_default:
    +      label: 'Future format string'
    

    Here too, I wouldn't use "string" on user-facing text. Well I'm not exactly sure if the labels in config schema are user-facing anywhere. If not, you can ignore this but I think they are?

  6. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -32,8 +43,32 @@ field.formatter.settings.datetime_default:
    +      label: 'Past format string'
    

    another "string"

  7. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,97 @@
    +      '#title' => $this->t('Format string'),
    

    This one is definitely user-facing, so I'd say "Date/time format" not "Format string".

  8. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,97 @@
    +      '#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')),
    

    Probabably "date-time" or "date/time" format here?

  9. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,113 @@
    +      '#title' => $this->t('Timezone override'),
    

    Another timezone -> time zone

  10. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,113 @@
    +      '#description' => $this->t('The timezone selected here, will always be used'),
    

    Nitpick: no comma in this, and timezone -> time zone again

  11. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,113 @@
    +      '#title' => $this->t('Append timezone'),
    +      '#description' => $this->t('Append the timezone used to render the date'),
    +      '#default_value' => $this->getSetting('append_timezone'),
    

    Here too, time zone should be two words.

  12. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,113 @@
    +   * Sets the current users or overridden timezone on a date.
    

    This is docs, so timezone -> time zone...

    Also users -> user's

    Also presumably this is a date/time not really a date only?

  13. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,113 @@
    +   * Appends the timezone used to render the date.
    

    timezone -> time zone

    and this is not really just a date?

  14. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,113 @@
    +   * Note: This function will only append the timezone if the 'append_timezone'
    +   * setting is TRUE.
    +   *
    

    I'm not sure this is necessary since the @return also states it? Maybe in the first-line doc say "... if appropriate" and leave it at that?

  15. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,113 @@
    +   * @param string $rendered_date
    +   *   A formatted date string.
    +   *
    +   * @return string
    +   *   A formatted date string with the timezone appended if the
    +   *   'append_timezone' setting is TRUE.
    +   */
    

    date -> date/time

    Also ... shouldn't say "a" in the @return because by that point it's not "any old string" but the one that is passed in, so should be saying "The passed-in string with the time zone appended..."

  16. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,113 @@
    +    $timezone = $this->getSetting('timezone_override');
    +    if ($timezone != '' && $this->getSetting('append_timezone')) {
    +      return SafeMarkup::format('@rendered_date (@timezone)', array(
    

    So, from this code I can see that the time zone is actually only appended if (a) the append_timezone setting is TRUE and (b) the timezone_override setting has a non-empty value.

    This is not documented:
    - in the @return for this function for developers
    - in the Settings form for the user

    Is this really the intention? I'd think that in this function if someone says "append the time zone" we would want to append the time zone that is used for formatting, even if it hasn't been overridden? But this code would not do that I think.

    Anyway, if the current logic is going to stand, it needs to be documented better, but I think this is not the right logic.

  17. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,209 @@
    +   * @param \Drupal\Core\Entity\EntityStorageInterface $date_storage
    +   *   The date storage.
    +   * @param \Symfony\Component\HttpFoundation\Request $request
    

    Hm, this is really the date_format entity storage, if you look at the create() function. So I think that the variable name, the class member variable name, and the documentation here are all misleading.

  18. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,209 @@
    +      '#title' => $this->t('Future format string'),
    +      '#default_value' => $this->getSetting('future_format'),
    +      '#description' => $this->t('The format string for dates in the future. Use <em>@time</em> where you want the time to appear.'),
    +    );
    

    Again, not excited about "string" in user-facing text?

    And this is for dates/times... the #description line is a mixture of date/time, seems a bit confusing.

  19. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,209 @@
    +      '#title' => $this->t('Past format string'),
    +      '#default_value' => $this->getSetting('past_format'),
    +      '#description' => $this->t('The format string for dates in the past. Use <em>@time</em> where you want the time to appear.'),
    +    );
    

    Same comments as the previous block.

  20. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,209 @@
    +      '#description' => $this->t('How many time units should be shown in the "time ago" string.'),
    

    Hm, so... what is the "time ago" string here? I don't see this phrase elsewhere in the form. Probably should not use "string" either. How about something like:

    How many time units should be shown

    or if for some reason you want to be more verbose

    How many time units should be shown in the formatted output

  21. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,209 @@
    +   * @param \Drupal\Core\Datetime\DrupalDateTime|object $date
    +   *   A date object.
    +   * @return string
    +   *   A formatted date string using the chosen format.
    +   */
    

    date -> date time in the 2nd/4th line

    And needs a blank line between @param and @return

    Also in @return don't use "a", it's a definite thing, so "The formatted date/time string..." would be better.

    And... what does the "chosen" format mean here? I suppose it's the past/future format from the settings?

mpdonadio’s picture

Thanks @jhodgdon, that is all great feedback.

Re 16, that was introduced way back in #12. If there is consensus, I'll remove the functionality. Personally, I'm all for simplifying things. \Drupal\views\Plugin\views\field\Date doesn't seem to have this, so it's not a regression (as far as I can tell), but I didn't look to see what the D7 Date module does.

Re 17, that matches what is currently in HEAD for \Drupal\datetime\Plugin\Field\FieldFormatter\DateTimeDefaultFormatter and \Drupal\datetime\Plugin\Field\FieldWidget\DateTimeDefaultWidget; it's just shown in the patch as an addition b/c the refactoring to split things up. To fix it properly would mean touching that portion of the widget code, which may or may not be in scope for this issue?

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
36.88 KB
7 KB

OK, everything except for 16 and 17 from #116 should be addressed. Let me know if I missed something. Didn't add the "if appropriate" suggestion in 14, as I couldn't keep it under 80 cols, and I think using 'DrupalDateTime' is appropriate here (but, am open to suggestions).

Tracing out usages of the settings and ::appendTimezone(), I mostly think that this method is used appropriately (esp since it consolidates some logic that would be repeated though a few classes), but may be badly named. Maybe refactor it to ::appendOptionalTimezone(), and properly document the behavior?

jhodgdon’s picture

Regarding comment #118 item 16, I still do not understand why if someone chooses a setting that says "Append the time zone" that we would only do so if they had also chosen the "Use a custom time zone" setting. The two just don't seem connected to me.

Actually, I guess I don't see why we need the "Append the time zone" setting at all, since you can use "e" in the PHP date function to get the time zone, right? http://php.net/manual/en/function.date.php -- So where did the "append the time zone" setting come from? It's not in #12 as far as I can see.

Regarding #17, even if it was in head, you have code touching those lines, so I think it's within scope to fix it.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

My bad. The timezone override was introduced in #12, the append setting was introduced in #18. Both were before I started working on this, so i just ran with it.

Yes, upon looking at the code, the method is wonky. To work right, it either needs to use the TZ override or use the API to get the one for the current user (the API does fallback to system default). I'll fix this (should be easy), and see if anyone else has an opinion on removing it.

I'll also address #17 in the formatters by changing the property name and docblocks to better reflect what it really is. I think keeping the typehint as EntityStorageInterface is fine, but I'll double check that, too.

jhodgdon’s picture

But... Why do we need the "append" functionality at all, since someone can just use "e" in their format?

I do not see why this was added in #18. It's not in the issue summary (currently at least) and I don't get why we need it at all.

Gábor Hojtsy’s picture

@jhodgdon, @mpdonadio: the goal of this issue is to port the views Date.php field features to general Timestamp fields. Looks like Date.php field in view does not have an append timestamp feature either. As per the issue summary we should port the **existing** features and in fact discuss removing some if they are not needed not add even more. That would make it hard to argue that this is not a feature request.

mpdonadio’s picture

Status: Needs review » Needs work

OK, I'll take that as consensus. I will strip the append timezone setting and functionality.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
37.98 KB
4.44 KB

This just takes care of #118-17. Because of a refactoring that, the timeago formatters don't need that injected anymore, hence just the removed lines.

Next patch will strip the 'append_timezone' setting.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Issue tags: +Needs tests
FileSize
36.27 KB
7.7 KB

I expect this to come up green, but if you look at the interdiff, there are no changes to the tests. It looks like both the timezone_override and append_timezone settings never made it into the tests. So, it looks like they need to be updated. I also did verify that the date override is part of the Views plugin, so that addition was OK.

Taking my name off this in case I don't get to it this weekend.

Status: Needs review » Needs work

The last submitted patch, 127: support_all_options-2399211-127.patch, failed testing.

mpdonadio’s picture

Doing a retrigger b/c

FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.

The test log gave no indication of what went wrong or where.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
39.86 KB
5.69 KB

This is going to fail.

1. It looks like the format methods on the Drupal classes don't handle timezome overrides properly: https://www.drupal.org/node/2497447

2. DateTimeFormatterBase::setTimeZone() actually changes the timezone of the $date object, instead of overriding it on display (which may not work if the above is true). Because objects are pass by reference, really weird things may result at runtime when the timezone of a date object changes unexpectedly.

Status: Needs review » Needs work

The last submitted patch, 131: support_all_options-2399211-131.patch, failed testing.

mpdonadio’s picture

Postponing on #2497447: DrupalDateTime::format() and DateTimePlus::format() ignore the timezone setting. so we can proper override the time zone w/o affecting the date object.

Also adding #2497585: Simpletest should set a system timezone in setUp as a related issue based on some venting in IRC last night.

jhodgdon’s picture

Status: Postponed » Needs work

Looks like we can un-postpone this now.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

#131 currently has a failure in \Drupal\datetime\Tests\DateTimeFieldTest on line 324:

Formatted date field using datetime_custom format displayed as 12/31/2012 9:00:00 AM.

Retesting to see if to see if this works as-is with #2497447: DrupalDateTime::format() and DateTimePlus::format() ignore the timezone setting. committed and to see if this needs a reroll.

Will work on either case later tonight.

mpdonadio’s picture

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

Setting back to Needs Review based on #131 now being green.

jhodgdon’s picture

Status: Needs review » Needs work

Wonderful that the tests pass now!

I think this is very close. I took a close look at all of the code and documentation, and only found a few things to comment on, only two of which are very minor UI text things to fix. Excellent work! Thanks!

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,78 @@
    +      $summary[] = ($override != '') ? $this->t('Timezone: @timezone', $replacements) : '';
    

    This should be "Time zone: ..." (two words for time zone)

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,78 @@
    +      $date->setTimeZone(timezone_open(drupal_get_user_timezone()));
    

    This has interesting implications for page caching. Do we need to think about this more? Because it means that any site with the option set for everyone to have their own time zone cannot cache any formatted dates output on any page. [This is not a "fix this" comment, just an observation.]

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,198 @@
    +class DateTimeTimeAgoFormatter extends FormatterBase implements ContainerFactoryPluginInterface {
    

    I guess that any page using a DateTimeAgoFormatter cannot be cached either, at all, because the output depends on the current time. Another interesting cache implication. [another observation]

  4. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,198 @@
    +      '#description' => $this->t('The format string for dates in the future. Use <em>@time</em> where you want the time to appear.'),
    

    Shouldn't we be saying just "The format" not "The format string" in user-facing text here? (and in the next setting)

jhodgdon’s picture

Regarding the caching, I added a comment to #606840: Enable internal page cache by default about this question.

mpdonadio’s picture

Thanks for the reminder. #2 needs to be fixed. #2497447: DrupalDateTime::format() and DateTimePlus::format() ignore the timezone setting. partially addressed this issue (mainly, the tests). DateTimeFormatterBase::setTimeZone() is bad because it updates the TZ of the object, and not just overrides it for output (side effects! boo!). We need to refactor this to remove that method, and pass the proper setting to the format method (which 2497447 allows now). I'm thinking we add a method to the base class to handle this in one place.

The format methods do set the proper cache context to be time zone aware, but I think #3 is a non issue. If an event happened two hours ago, it doesn't matter whether my local time is 8am or 6pm, 6am or 4pm are still both two hours ago.

jhodgdon’s picture

We do not need to address the caching issues here -- they are being addressed elsewhere. So I think all we need to do on this issue is to address the two very minor UI fixes mentioned in comment #138 (items 1 and 4) and we're ready to go on this one. Thanks!

mpdonadio’s picture

My comment in #140 does need to be done. The current patch changes the date object when it gets rendered if there is a TZ override. This could cause very weird problems should the data object get used afterwards. This should be a pretty easy thing to do, and I am going to hopefully do it tonight.

jhodgdon’s picture

Right, I forgot about that. Sounds good!

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
40.82 KB
7.31 KB

#138 should be addressed, as well as #140. I also made a few small cleanups to try to make the formatters more consistent w/ each other. \Drupal\datetime\Tests\DateTimeFieldTest came up green locally.

Methinks we are good to go.

Status: Needs review » Needs work

The last submitted patch, 144: support_all_options-2399211-144.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
40.78 KB
872 bytes

Ok, I thought that was a harmless cleanup in the new code. Silly me. I don't think that code is 100% correct, but since we have a test for it, the change is out of scope for this issue.

Both \Drupal\datetime\Tests\DateTimeFieldTest and \Drupal\rdf\Tests\Field\DateTimeFieldRdfaTest come up green locally now.

jhodgdon’s picture

FYI, the caching issue for time ago formatting is now its own issue:
#2500525: Time ago/hence date/time formatting breaks caching; needs appropriate max age

I will try to review the latest patch here soon. My minor UI points from the last review did get addressed, thanks! So if someone else wants to review the other code changes... or I'll get to it hopefully today or tomorrow.

dawehner’s picture

Just some nitpicks :(

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,90 @@
    +    $override = $this->getSetting('timezone_override');
    +    if ($override != '') {
    

    You can use if ($override = $this->getSetting('timezone_override)

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,90 @@
    +      $summary[] = ($override != '') ? $this->t('Time zone: @timezone', $replacements) : '';
    

    $override is already not empty.

mpdonadio’s picture

This is just a small rereroll b/c DateTimeDefaultFormatter didn't merge during a rebase.

mpdonadio’s picture

And now, #148 addressed w/ a clean interdiff.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Everything looks good to me, and the comments in #148 have been addressed. So, let's do this!

Note: If #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known is committed first, we'll need to fix one or two lines of this patch to use the new formatDiff() instead of formatInterval(). Or if this is committed first, we'll need to do that in the other issue. So, the race is on!

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-content, +sprint

Bring on D8MI sprint, given that this allows rendering date fields with the right formatter supporting multilingual (ie. rendering the right language variant of the date) :)

dawehner’s picture

Congratulations @mpdonadio!

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

Can you just confirm before we commit this one that on #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter, which I think included this patch in it's latest, the changes to the formatters here were the same as what you had in that latest patch?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Hm. So this patch covers:

core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php [new class]
core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeDefaultFormatter.php [revised]
core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php [new class]
core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php [revised]
core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php [new class]
core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php [revised]

These are all widgets/formatters for date/time fields added to entities using Field UI.

The other patch covers:
core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php

These are the Field UI formatters used for timestamps, like created/changed/etc. -- base fields on entities, as opposed to the Date module's field that you can add to an entity. And then the other issue also changes Views created/changed/etc. types of fields to use Field UI:

+++ b/core/modules/views/src/EntityViewsData.php
@@ -353,7 +353,7 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
       case 'timestamp':
       case 'created':
       case 'changed':
-        $views_field['field']['id'] = 'date';
+        $views_field['field']['id'] = 'field';

OK, so they're separate. That may not be the absolute best separation into two issues, but it's at least a clear separation, and this one is ready to go, so let's proceed. Onward!

jhodgdon’s picture

Title: Support all options from views fields in timestamp formatters » Support all options from views fields in DateTime formatters
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

Actually though this patch doesn't fix the problem in this issue summary at all, which was all about the base field formatters, not the datetime formatters.

So I think we should go back and put the Timestamp fields here. So setting back to Needs Work. This issue was supposed to be about the base timestamp formatters and it got sidetracked to the Datetime formatters. It shouldn't have. I've added Datetime to the issue summary since it's already in the patch.

So let's move the timestamp formatter part of the patch on #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter over here and make sure it's tested (outside of views), and leave #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter for just "use this in views" as was intended.

jhodgdon’s picture

Also #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known just landed, so we should be using formatDiffUntil() and formatDiffSince() instead of formatInterval() here where appropriate. woot!!!

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

I'll start work on this tonight.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
40.61 KB
1.56 KB

Part 1a. This is just the update to the datetime formatter, which is pretty trivial.

Part 1b is updating DateTimeFieldTest, which is proving problematic. The new functions use the request time explicitly. In the old version of the patch, we got around this by always using a consistent interval. We have introduced REQUEST_TIME (which varies) back into the equation in the test, and also have the additional complication that the date-only portions throws away the hh:mm:ss portion of the timestamp when it stuffs it into the database. The net result is that the expected result is a bit unpredictable between test runs when seconds get rendered out. Blerg.

Part 2 will be hand editing the patch from #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter to separate out the views code from the formatter code, and applying it to this, and updating the formatter to use the new functions, and then fixing up any tests (and we may run into the same problems).

Need to sleep on this. I may post the initial part of Part 2 tomorrow night and then worry about the tests together.

mpdonadio’s picture

OTOH, do we want to get this issue in using formatInterval() and then handle the conversions to use formatDiff() as a followup? The Frankenstein's monster of #150 + the formatter portion of #2455125-62: Update EntityViewsData use of generic timestamp to use Field API formatter should be (mostly) easy.

Status: Needs review » Needs work

The last submitted patch, 159: support_all_options-2399211-159.patch, failed testing.

Anonymous’s picture

In the old version of the patch, we got around this by always using a consistent interval. We have introduced REQUEST_TIME (which varies) back into the equation in the test

Assuming we can't fix the REQUEST_TIME, the only way I see this possible is by actually calculating the diff. So I guess we need to call the DateFormatter or use the PHP DateTime::diff() to see what the expected value is. I'd go with the former, because that already calculates weeks which DateTime:diff() does not. Furthermore we are testing if the result is displayed rather than what the exact result is. We have a unit test for that.

do we want to get this issue in using formatInterval() and then handle the conversions to use formatDiff() as a followup

I would do it here, but I would hate to see this patch being held up if it turns out to be tricky.

mpdonadio’s picture

If we lower the granularity to 3 in the tests, we won't see the jitter in the request time. However, I found a bug in DateFormatter::formatDiff(). See #2504211: DateFormatter::formatDiff() ignore granularity in some circumstances.. Technically, that is blocking this, but I am plowing forward. This won't pass.

mpdonadio’s picture

Frankenpatch with formatter related pieces from #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter, but still using formatInterval(), because that we need to rework the changes that #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known made to support the formatter strings w/ the formatDiff() stuff. TimestampFormatterTest passes locally for me, but not sure if we have additional breakage.

The last submitted patch, 163: support_all_options-2399211-163.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 164: support_all_options-2399211-164.patch, failed testing.

Anonymous’s picture

It's great to see this moving along quickly!

Re #163:

+++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
@@ -338,12 +338,12 @@ function testDatetimeField() {
-    $expected = '2 years 9 months 2 weeks 12 hours earlier';
+    $expected = '2 years 9 months 1 week earlier';

Just a sidenote, for if you would intend to revert that change later on. There is an issue #2502571: Date format granularity should only render adjacent units which alters the handling of granularity as part of a caching effort. That would mean that a granularity of 4 would also stop after weeks and not display hours, since days are skipped.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
60.48 KB
3.37 KB

I think this wraps up the work. This will fail until #2504211: DateFormatter::formatDiff() ignore granularity in some circumstances. makes it in. Otherwise, I think this is ready for review.

Status: Needs review » Needs work

The last submitted patch, 168: support_all_options-2399211-168.patch, failed testing.

mpdonadio’s picture

#2504211: DateFormatter::formatDiff() ignore granularity in some circumstances. was committed 5 minutes after I posted #168, and the four fails are in DateTimeFieldTest, which now passes locally. Requeuing.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review

#168 came back green Review away.

Anonymous’s picture

Status: Needs review » Needs work

This looks great. The todo's can be removed now.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -99,4 +170,28 @@ public function viewElements(FieldItemListInterface $items) {
    +    * @todo Refactor when https://www.drupal.org/node/2456521 is committed.
    

    Todo.

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -0,0 +1,86 @@
    +   * @return array
    +   *   The array of settings for the second parameter passed to
    +   *   DrupalDateTime::format().
    

    This description is quite prone to change. I'd make that something like "The settings array that can be passed to..".

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,198 @@
    +   * @todo Refactor when https://www.drupal.org/node/2456521 is committed.
    

    Todo.

mpdonadio’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
60.29 KB
2.12 KB

#173 addressed. IS tweaked.

Status: Needs review » Needs work

The last submitted patch, 174: support_all_options-2399211-174.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review

Got a

PHP Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 13 database or disk is full' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/StatementPrefetch.php:175

Re-testing.

dawehner’s picture

Let's see whether its this time ...

  1. +++ b/core/config/schema/core.entity.schema.yml
    @@ -304,9 +304,33 @@ field.formatter.settings.uri_link:
    +field.formatter.settings.timestamp:
    +  type: mapping
    +  label: 'Timestamp display format settings'
    

    All that feels so common, its almost as if I would read that the 10th time already ;)

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -75,19 +88,77 @@ public static function create(ContainerInterface $container, array $configuratio
    +    $summary[] = t('Future date: %display', array('%display' => $this->formatTimestamp($future_date)));
    +    $summary[] = t('Past date: %display', array('%display' => $this->formatTimestamp($past_date)));
    

    Nitpick: You could use $this->t()

+++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
@@ -0,0 +1,85 @@
+use Drupal\Component\Utility\SafeMarkup;

This is not used

mpdonadio’s picture

jhodgdon’s picture

Status: Needs review » Needs work

Hey, thanks for the patches and reviews! I have been behind in my reviews, sorry.

So I took a careful look through the code and tests, and I think there are a few things to address. Some are minor; some not so minor.

  1. +++ b/core/config/schema/core.entity.schema.yml
    @@ -304,9 +304,33 @@ field.formatter.settings.uri_link:
    +      label: 'Timezone'
    

    Should be "Time zone" (two words) as it is a UI label here.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -75,19 +88,77 @@ public static function create(ContainerInterface $container, array $configuratio
    +      '#description' => $this->t('The format for timestamps in the future. Use <em>@time</em> where you want the time to appear.'),
    

    Do we really want to say "timestamps" in this UI text? What are the field types that this applies to called? We should be consistent.

    Also @time is not really the time, it is the time interval, right? [See below for more on this with similar strings]

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -75,19 +88,77 @@ public static function create(ContainerInterface $container, array $configuratio
    +      '#description' => $this->t('The format for timestamps in the past. Use <em>@time</em> where you want the time to appear.'),
    

    Same here. [still more below]

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -99,4 +170,26 @@ public function viewElements(FieldItemListInterface $items) {
    +    $interval = $this->request->server->get('REQUEST_TIME') - $timestamp;
    +
    +    if ($interval > 0) {
    +      return SafeMarkup::format($this->getSetting('past_format'), ['@time' => $this->dateFormatter->formatTimeDiffSince($timestamp, $options)]);
    

    Since we're not actually using $interval for anything, I think this code would actually be clearer if it said:

    if($timestamp < $this->request->server->get('REQUEST_TIME'))

    When I read the code as it is, I got to if($interval > 0) and then I had to backtrack up a few lines to see which way the difference was written. It's just easier to read with a simple > compare and no local variable, I think?

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -23,7 +29,138 @@
    +      '#title' => $this->t('Custom date format'),
    

    So really this is either the custom date format or the granularity??!? Weird. Granularity would only apply to the "ago" formatter though, right? So maybe that formatter should update the setting name and description so it says granularity, and this one shouldn't mention granularity?

    And why don't we have two different settings anyway? This just doesn't make sense to me.

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -23,7 +29,138 @@
    +      '#description' => $this->t('If "Custom", see <a href="http://us.php.net/manual/en/function.date.php" target="_blank">the PHP docs</a> for date formats. Otherwise, enter the number of different time units to display, which defaults to 2.'),
    

    This is a bit informal for translatable UI text. We shouldn't say "docs", for instance.

    Also, the link text is not Accessible. Link text should always say where the link goes. This one doesn't go to "PHP docs", it goes to the documentation for PHP date formats, so "PHP docs" is not good link text.

  7. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,98 @@
    +      '#description' => $this->t('A user-defined date/time format. See the <a href="@url">PHP manual</a> for available options.', array('@url' => 'http://php.net/manual/function.date.php')),
    

    See previous comment. Same problem with accessibility.

  8. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,196 @@
    +      '#description' => $this->t('The format for dates in the future. Use <em>@time</em> where you want the time to appear.'),
    

    Well this is a bit different from the text used in the other setting for future_format and past_format on the other class.

    Let's make them all the same.

    I still don't like that we call it "dates in the future" and "timestamps in the future" and then @time is "the time", when really it's dates/times in the future and @time is the formatted time/date pieces (which admittedly is not easy to describe) but definitely not "the time".

  9. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,196 @@
    +      '#description' => $this->t('The format for dates in the past. Use <em>@time</em> where you want the time to appear.'),
    

    See previous comments.

  10. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
    @@ -148,13 +172,65 @@ function testDateField() {
    +    $date = DrupalDateTime::createFromTimestamp(REQUEST_TIME - 87654321, 'UTC');
    +    $entity->{$field_name}->value = $date->format($date_format);
    +    $entity->save();
    +
    +    $this->displayOptions['type'] = 'datetime_time_ago';
    +    $this->displayOptions['settings'] = array(
    +      'future_format' => '@time in the future',
    +      'past_format' => '@time in the past',
    +      'granularity' => 3,
    +    );
    +    entity_get_display($this->field->getTargetEntityTypeId(), $this->field->getTargetBundle(), 'full')
    +      ->setComponent($field_name, $this->displayOptions)
    +      ->save();
    +    $expected = '2 years 9 months 1 week in the past';
    +    $this->renderTestEntity($id);
    

    This test may pass today, but I am not convinced it will always pass. I think it will lead to some random test failures in the future.

    The reason I think this is that you are setting $date to REQUEST_TIME minus a fixed number of seconds, and then you are testing that it will format to "2 years 9 months 1 week". So that's fine now but what about in February of the year after a leap year? There will be some times when this will fail, because we're using formatDiff() now to format these things, not formatInterval(), right? So "2 years 9 months 1 week" is not a fixed number of seconds, or even a fixed number of days.

    If you want to make a good test that will continue to pass, you need to use the PHP date add functions to subtract "2 years 9 months 7 days" to REQUEST_TIME, and then it should always work.

    Same with the "ago" tests below.

  11. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
    @@ -214,13 +295,75 @@ function testDatetimeField() {
    +    $date = DrupalDateTime::createFromTimestamp(REQUEST_TIME - 87654321, 'UTC');
    

    Same problem will occur here.

  12. +++ b/core/modules/field/src/Tests/Timestamp/TimestampFormatterTest.php
    @@ -0,0 +1,194 @@
    +      // Test a timestamp in the past
    +      $value = $request_time - 87654321;
    +      $expected = SafeMarkup::format($past_format, ['@time' => \Drupal::service('date.formatter')->formatTimeDiffSince($value, ['granularity' => $granularity])]);
    +
    

    Here too, and the future test just below.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Thanks for the close look. I'll start on these tonight.

Re #2, these are timestamp fields not date/time fields.

Re #5, this is the TimestampFormatter not the TimestampAgoFormatter, so I think this is correct. No granularity; there are two separate formatter classes.

Re #8,9. I thought we had decided on the @time placeholders for DateTimeTimeAgoFormatter several patches ago and gotten to RTBC? The last batch of changes was to fold TimestampFormatter and TimestampAgoFormatter formatters (and related changes to suppor them) In the I can adjust as needed, though. Any suggestions for the actual text between now and tonight (EDT) would be appreciated, otherwise I'll take a stab at these again.

Tests. I'll see if a trip through DrupalDateTime and the add functions will work here.

mpdonadio’s picture

One more thing before I punch in for the day, re #9.

TimestampFormatterTest doesn't use static strings in the timeago tests. The expected strings call out to the formatter service directly, and then the formatter output gets compared to these (we aren't testing whether the date formatter service works, just that the formatter produces what we think it should, which is what the formatter service provides). So, I don't think there will be reliability problems with these.

I think the proper thing for DateTimeFieldTest is to use the same approach instead of relying on static strings. I had originally abandoned this b/c the granularity bug in the date formatter service and the fact that there is something weird going on with REQUEST_TIME in WebTestBase (which will be my text followup), but that will probably work now.

jhodgdon’s picture

RE item #5 in comment #180 - take a look at the #description for that field, which mentions Granularity.

RE item #2, I realize that they are "timestamp" fields from a developer/coder field. But what are they from a User perspective? "timestamp" is not actually a word in English. This is UI text, so we need to think about what these fields are called in the UI.

RE item8, 9: yes, @time is fine, but I don't like the wording saying "the time goes here", when it's not "the time" but something like "1 year 3 months". Maybe this description could be:

The format for dates in the past. Use @time where you want the text representing the interval (such as "1 month 3 days" to appear.

And re #10, the test... well given that your granularity is for a week, you *might* be OK, because probably the diff would only be off by a couple of days, but it would be much much safer to use actual date diffs than a number of seconds instead I think. If you do want to use a number of seconds for the interval, please add a comment saying how many days it is equivalent to in the code.

jhodgdon’s picture

RE #182, yes a comparison to a direct call to formatDiff() instead of a static string would solve the problem. Probably the best approach, since we have a lot of unit tests to make sure formatDiff() is working correctly.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
60.21 KB
8.05 KB

This should address #180, except for the test adjustments. Onto those next which will use the date formatter service instead of static strings, but that may not be tonight.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
FileSize
61.15 KB
7.94 KB

TestBot is borked right now, but I think this will come up green (#185 should fail) when tests start running again.

I think this addresses everything, but I could have overlooked something.

Status: Needs review » Needs work

The last submitted patch, 186: support_all_options-2399211-186.patch, failed testing.

The last submitted patch, 185: support_all_options-2399211-185.patch, failed testing.

mpdonadio’s picture

Weird fail in ConfigTranslationUiTest:408-409

GET http://ec2-54-85-104-252.compute-1.amazonaws.com/checkout/fr/node/add/ri... returned 0 (0 bytes).

Raw "Create fr-.bTH&VQ0" found

Retesting to see if this was a fluke.

jhodgdon’s picture

This is looking good (pending test bot approval)!

Just some cosmetic stuff (comments and UI), and a couple of questions (code may be OK but I wasn't sure).

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -75,19 +88,77 @@ public static function create(ContainerInterface $container, array $configuratio
    +      'future_format' => '@time hence',
    +      'past_format' => '@time ago',
    +      'granularity' => 2,
    

    The description on the past/future fields below says to use @interval not @time. Either that description is wrong, or these default values are wrong.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -75,19 +88,77 @@ public static function create(ContainerInterface $container, array $configuratio
    +      '#description' => $this->t('The format for time intervals in the future. Use <em>@interval</em> where you want the time to appear.'),
    

    Hm.

    "time intervals in the future" doesn't make sense to me. Intervals are differences. Times and dates are in the future.

    So...

    How about this:

    The format for times/dates in the future. Use @interval where you want the formatted interval text to appear.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -75,19 +88,77 @@ public static function create(ContainerInterface $container, array $configuratio
    +      '#description' => $this->t('The format for time intervals in the past. Use <em>@interval</em> where you want the time to appear.'),
    

    See previous comment.

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -75,19 +88,77 @@ public static function create(ContainerInterface $container, array $configuratio
    +      '#description' => $this->t('How many time interval units should be shown in the formatted output.'),
    

    This is good!

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -99,4 +170,25 @@ public function viewElements(FieldItemListInterface $items) {
    +  /**
    +    * Creates a formatted timestamp value as a string.
    +    *
    

    Indentation is 1 character too many in the lines under /* in this doc block.

    And the first line could be even better... How about just:

    Formats a timestamp.

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -23,7 +29,138 @@
    +   * @param \Drupal\Core\Entity\EntityStorageInterface $date_format_storage
    +   *   The date storage.
    +   */
    

    This is date format storage, not date storage.

  7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -23,7 +29,138 @@
    +      '#description' => $this->t('If "Custom", see <a href="http://us.php.net/manual/en/function.date.php" target="_blank">the documentation for PHP date formats</a>.'),
    

    This is much better! Hm. Maybe it should say:

    "Used if Date format is set to Custom. See [the link tag]."

    ?

  8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -23,7 +29,138 @@
    +      '#title' => $this->t('Time zone'),
    +      '#description' => $this->t('Time zone to be used for date output.'),
    +      '#options' => array('' => $this->t('- Default site/user time zone -')) + system_time_zones(FALSE),
    

    Do we need the description here at all? We should always strive to minimize UI text, and this one doesn't seem to tell me much beyond the field title. I mean we're in the context of formatting a date, so if there is a Time Zone field, it should be obvious it will be used to format the date. Right? So let's drop the description. According to our UI guidelines, most fields shouldn't have descriptions.

  9. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,98 @@
    +      '#title' => $this->t('Date/time format'),
    +      '#description' => $this->t('A user-defined date/time format. See <a href="http://us.php.net/manual/en/function.date.php" target="_blank">the documentation for PHP date formats</a>.'),
    

    Let's take out the first sentence in this and just put in the "See ... " part? I think the first part is redundant to the title.

  10. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,98 @@
    +    $summary[] = $date->format($this->getSetting('date_format'), $this->getFormatSettings());
    

    Why does this call $date->format and most of the other classes use the DateFormatter class? Just curious.

  11. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
    @@ -21,30 +20,31 @@
    +        $output = $date->format($format, $this->getFormatSettings());
    

    Here's another one that is using $date->format instead of the DateFormatter. Still not sure why.

  12. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,195 @@
    +      '#description' => $this->t('The format for time intervals in the future. Use <em>@interval</em> where you want the time to appear.'),
    

    See previous comment on similar line in other class.

  13. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,195 @@
    +      '#description' => $this->t('The format for time intervals in the past. Use <em>@interval</em> where you want the time to appear.'),
    

    See previous comment on similar line in other class.

  14. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,195 @@
    +    $form['granularity'] = array(
    +      '#type' => 'number',
    +      '#title' => 'Interval',
    +      '#default_value' => $this->getSetting('granularity'),
    

    Title should be $this->t('Granularity'), not "Interval". Oops.

  15. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,195 @@
    +   * Creates a formatted date value as a string.
    

    How about:

    Formats a date/time as a time interval.

jhodgdon’s picture

Test bot green, woot! :) Those comments in my review should be easy to address. The tests are looking good to me.

mpdonadio’s picture

I'll fix these tonight. Re 10 and 11, I think those were part of the patch before I took this over, but I forget what I turned up when I looked at it. I'll look at it tonight, and comment on it (or fix it if it is easy, but both of those would also involve adding the service to the list that gets injected and one or two more protected properties).

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
65.31 KB
18.14 KB

This should address #191, and I expect it to come up green (unless I introduced a side effect that my local testing didn't catch).

There was no real reason to use `$date->format()` directly. I think this solution is cleaner, too. And, if a review wants to mention it, I think

abstract protected function formatDate($date);

in the base class is the best thing to do. It's not a common function used by the derived classes, and it doesn't have a decent default implementation. I think that the derived classes should be forced to implement a specific version of this for each formatter.

mpdonadio’s picture

If this doesn't get in by 2015/06/24, per #2507899: [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29, I don't think we will need update hooks as the datetime formatters have one to one mappings with the new formatters (default and plain exist in both), and we aren't changing anything with the timestamp formatters (just adding new features). However, we will probably need a change notice as the patch will require a container rebuild. I was unable to make the patch fatal without one, but the new options won't appear (or work) until you do one.

jhodgdon’s picture

Status: Needs review » Needs work

Looking great! A couple of questions and nitpicks:

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -99,4 +170,25 @@ public function viewElements(FieldItemListInterface $items) {
    +    if ($this->request->server->get('REQUEST_TIME') - $timestamp > 0) {
    

    Total nitpick: I would find this easier to read if it was:
    if (request_time > timestamp)
    rather than
    if( request_time - timestamp > 0)

    This appears in another spot in the patch as well.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -23,7 +29,137 @@
    +    $elements['custom_date_format'] = array(
    +      '#type' => 'textfield',
    +      '#title' => $this->t('Custom date format'),
    +      '#description' => $this->t('Used if Date format is set to Custom. See <a href="http://us.php.net/manual/en/function.date.php" target="_blank">the documentation for PHP date formats</a>.'),
    +      '#default_value' => $this->getSetting('custom_date_format') ?: '',
    +    );
    +
    

    Do we need some validation on this field -- what if 'custom' is selected for 'date_format' and this is left empty?

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -23,7 +29,137 @@
    +    $summary[] = $this->t('Date format: @date_format', array('@date_format' => $this->getSetting('date_format')));
    +    if ($custom_date_format = $this->getSetting('custom_date_format')) {
    +      $summary[] = $this->t('Custom date format: @custom_date_format', array('@custom_date_format' => $custom_date_format));
    +    }
    

    Should we only display the custom date format if setting 'date_format' is set to 'custom'?

    Also, shouldn't we be displaying samples of what the format would look like in the output?

    And for the date_format field, we should be showing the human-readable label of the format, not the machine name?

  4. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -0,0 +1,107 @@
    +      '#description' => $this->t('See <a href="http://us.php.net/manual/en/function.date.php" target="_blank">the documentation for PHP date formats</a>.'),
    

    Rather than making translators find the correct URL for their language for this page, we might want to instead pull the URL out of the t() string (put it in as a @url substitution) and use the generic no-language URL of:
    http://php.net/manual/function.date.php

    That URL should redirect people to an appropriate language.

    Same in other places in this patch where this URL is used.

  5. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
    @@ -21,30 +20,29 @@
             if ($this->getFieldSetting('datetime_type') == 'date') {
               // A date without time will pick up the current time, use the default.
               datetime_date_default_time($date);
    -          $format = DATETIME_DATE_STORAGE_FORMAT;
             }
    -        $output = $date->format($format);
    +        else {
    +        }
    +        $this->setTimeZone($date);
    +
    

    I think we need to set the time zone before setting up the default time here?

  6. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -0,0 +1,195 @@
    +    if ($this->request->server->get('REQUEST_TIME') - $timestamp > 0) {
    

    This is the other place with that if() nitpick from above.

mpdonadio’s picture

I'll fix these tonight, which will give me some time to ponder the validation.

Re #196-5, no we need to set the timezone after the default b/c a quirk in how date-only values are stored. The value coming out of the database will be at midnight UTC. The default value function sets this to noon, UTC. When you apply the timezone, you will shift the time forward or backward, but you will land on the same day. If you set the timezone before setting the default time, it is possible to warp the value to the day before.

jhodgdon’s picture

Ah. If I had been writing a default value function, I think I would have made it set the time to noon, local.

In any case, noon UTC is not a guarantee of it not warping the date to the previous or next day. There are a couple of time zones around Alaska/Russia/Pacific that if they are on daylight savings time, are 13 hours plus or minus UTC. So this scheme is not guaranteed to work.

mpdonadio’s picture

You are absolutely correct. For the scope of this issue, I think we can ignore the extreme cases. However, we will need a followup to properly work this out. There is an issue to do general cleanup of datetime.module (can't find it now; link is bookmarked on home machine), which I think need to be split into a meta issue and we can add the zones next to the international date line to it. There are also some issues related to TZ handling in the test framework in general. I'm slowly starting to triage all of issues, a bunch of which are very old, and will hopefully have a gameplan soon for everyone to review.

jhodgdon’s picture

That sounds great! And yes, out of scope for this issue.

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: -sprint +Needs change record
FileSize
65.44 KB
5.32 KB

This addresses #196-1,3,4,6.

#2 - Since timestamps aren't exposed as proper fields on the standard entities, I am hesitant to add an #element_valid w/o even being able to test it manually to see if it is wired up proerly (timestamp formatter is a kerneltest using entity_test and we have coverage of the output). The timestamp formatters are really for views (see #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter) and non-core entities that want to expose them. We could add it to that issue, or as a novice followup to that issue.

#5 - As discussed above, as part of datetime.module cleanup, we will do a followup to see what happens what happens with the extreme timezones.

Also adding the Needs CR tag as part of #2507899: [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29. The CR will just need to say that a container rebuild is needed for this to work when upgrading from beta12.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

OK, I think this is finally ready to go!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

Because it's before July 1st it does not need a CR. Committed 2da1f42 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 2da1f42 on 8.0.x
    Issue #2399211 by mpdonadio, legolasbo, vijaycs85, pjonckiere, rteijeiro...
Gábor Hojtsy’s picture

Yay, thanks all!

Status: Fixed » Closed (fixed)

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