Clean up redundant methods in datetime field formatters

Will be done as part of #2161337: Add a Date Range field type with support for end date.

Agree that there should be no test changes. We also need to look at the #293 patch and see what refactoring needs to get back in, so we can use that in the DareRange followups. Both should be able to be handled in this issue.

Re #7, that would almost make Default, Plain, and Custom identical...

I could see custom having the time element + RDFa. The default pulls from the date formats you have defined in config, and then custom lets you specify your own format. They are two different use cases.

Took a quick look look at this. Do we want to start from the DateTime formatter changes from https://www.drupal.org/files/issues/2161337-293.patch and see what can be improved there? I'm wondering if moving logic to the buildDate() function (which can be reused by the range formatters) outweighs trying to consolidate viewElements().

This needs a reroll b/c #2811725: Error when render Datetime Range field: Error: Unsupported operand types. I was hoping some other patches would land before addressing this, but we should get this in as a step towards other work that needs to be done. Assuming a good reroll, and passing tests, I am OK with this.

@mpdonadio Here is the patch that i rerolled.

Ok, so in this issue #2811725: Error when render Datetime Range field: Error: Unsupported operand types the way viewElements() work was also changed. Lets try the current patch

Reroll looks good; the merge conflict looks like it is taken care of properly, plus we have test coverage of that portion.

I think this is good to go.

This issue is blocking #2632040: [PP-1] Add ability to select a timezone for datetime field. I reviewed it as well and agree it looks good.

Looks like it passed to me?

https://www.drupal.org/pift-ci-job/581855 seems to be a known random, #2829040-31: [meta] Known intermittent, random, and environment-specific test failures

Back b/c a CI error, https://www.drupal.org/pift-ci-job/582322

1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
   @@ -32,30 +32,16 @@ public static function defaultSettings() {
   +    // @todo Remove this method in https://www.drupal.org/node/2793143.
   
   +++ b/core/modules/datetime_range/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
   @@ -38,6 +38,7 @@ public static function defaultSettings() {
   +    // @todo Remove this method in https://www.drupal.org/node/2793143.
   

   This @todo looks incorrect. Remove the method? At most it should be deprecated, or I guess the implementation removed and... I dunno.

   Does this mean something like "Remove the foobar strategy in https://www.drupal.org/node/2793143"? Because #2793143: The datetime and daterange formatters (except plain) should use the HTML time element looks like it changes the code but does not remove the implementation of the method.

2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeDefaultFormatter.php
   @@ -31,59 +31,6 @@ public static function defaultSettings() {
   -  public function viewElements(FieldItemListInterface $items, $langcode) {
   

   So if this implementation is removed, is it falling back to a base implementation?...

3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
   @@ -118,6 +119,30 @@ public function settingsSummary() {
   +  public function viewElements(FieldItemListInterface $items, $langcode) {
   

   ...Ah yes, this base implementation that is being added. I get it now. That's kind of the point. :)

NW for the first point though. Also queuing updated test runs against the correct environment.

Bumping to major since it is marked as blocking #2632040: [PP-1] Add ability to select a timezone for datetime field which is major.

For those following along, the DrEditor context isn't correct in the paste in #39. We are discussing the comment on DateTimeCustomFormatter::viewElements and DateRangeCustomFormatter::viewElements():

--- a/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
+++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
@@ -32,30 +32,16 @@ public static function defaultSettings() {
    * {@inheritdoc}
    */
   public function viewElements(FieldItemListInterface $items, $langcode) {
+    // @todo Remove this method in https://www.drupal.org/node/2793143.

I disagree with that comment being wrong, but we can remove it if desired.

This issue is about cleaning up some redundant methods in the formatters, and moving code onto the base class (share all the code!).

DateTimeCustomFormatter::viewElements and DateRangeCustomFormatter::viewElements cannot really go away now and inherit the base methods because the field markup is different. #2793143: The datetime and daterange formatters (except plain) should use the HTML time element will result in all of the datetime/daterange fields using the same markup, except for the plain formatters: plain fields are um, plain, and the others get wrapped in a <time> element. So, in the final version of that, the plain formatters will have their own viewElements, and the other formatters will use the one from the base class, which is what that comment is talking about.

So, #2793143: The datetime and daterange formatters (except plain) should use the HTML time element is really soft blocked on this issue, which is why that issue wasn't postponed at the time (we could still make progress on it), and more important work was being done on other issues. I just postponed it.

Doesn't really need RM review specifically.

I don't really follow #41. Can we just improve the comment to say what the intended followup is, rather than "Remove this method" which is ambiguous and seems to contradict the proposed patch on the issue referenced in the @todo?

Good idea. My suggestion is

@todo Evaluate removing this method in https://www.drupal.org/node/2793143 to determine if the behavior and markup from the base class can be used instead.

Or something along those lines that reads better. Updated the other issue to better reflect the scope and solve.

+++ b/core/modules/datetime_range/src/DateTimeRangeTrait.php
@@ -11,68 +12,40 @@
+        if ($start_date->format('U') !== $end_date->format('U')) {
+          $elements[$delta] = [

Can we change this is ->getTimestamp() to match #2851902: Replace $date->format('U') with $date->getTimestamp()?

Otherwise RTBC.

@mpdonadio Here is the new patch.

Looks good to me. Thanks.

So I am reviewing this patch carefully doing word diffs on the refactored lines. I've looked at all but the date range trait so far. One question has come up:

+++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
-        $output = $this->formatDate($date);
...
-        '#markup' => $output,

+++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
+      '#plain_text' => $this->formatDate($date),

+++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
-        $output = $this->formatDate($date);
...
-        '#markup' => $output,

So this appears to be changed from #markup to #plain_text. That's probably a good security hardening, but could there be any formatting regressions or double-escaping depending on the format? What is the expected return format of formatDate()? Do we have test coverage for date formats that include restricted characters, and how is it sanitized in HEAD?

Okay and my only other question is, DateTimeRangeTrait::viewElements() seems to be added in this patch, but not used anywhere that I can tell?

#49, crap er I mean nice catch; we had a bad reroll somewhere and removed hunks got back in somewhere between #27 and #46.

#48, that is likely b/c the daterange formatters use '#plain_text', so this was done for consistency. The comment on DateFormatterInterface::format() is

   * @return string
   *   A translated date string in the requested format. Since the format may
   *   contain user input, this value should be escaped when output.

And see also \Drupal\KernelTests\Core\Datetime\FormatDateTest(). We don't explicitly test this in the field tests, but I just manually tested this and didn't get XSS in 8.4.x HEAD (even when I hardcode the string in the formatter itself) with the datetime custom formatter and get double escaping with the daterange custom formatter. Crap.

I'm punting this directly to jhedstrom for a look, since the majority of the work on this was his.

+++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
@@ -167,4 +192,69 @@ protected function getFormatSettings() {
+      '#plain_text' => $this->formatDate($date),

I agree with @xjm that this is going to be a regression. I think we should

- Change this to '#markup' to preserve how the datetime formatters work
- Possible CR to announce that daterange custom will now be '#markup'
- Add assertions to datetime and daterange to custom formatters to make sure normal tags (eg, bold) get preserved
- Add assertions to datetime and daterange to make sure an XSS gets stripped

This will partially be undone by #2793143: The datetime and daterange formatters (except plain) should use the HTML time element, but at least the test coverage will be there to make sure we don't break anything.

Yeah, it changes how the daterange custom formatter works. We don't need a CR for the bugfix, but we would need one to announce that a render array is changing, in case anyone is monkeying with it directly.

Need to look at this closer tomorrow, but

+++ b/core/modules/datetime_range/src/Tests/DateRangeFieldTest.php
@@ -165,6 +165,16 @@ public function testDateRangeField() {
+      $this->displayOptions['settings']['date_format'] = '\\<\\s\\t\\r\\o\\n\\g\\>m/d/Y\\<\\/\\s\\t\\r\\o\\n\\g\\>\\<\\s\\c\\r\\i\\p\\t\\ \\s\\r\\c\\=\\"\\a\\l\\e\\r\\t\\(\\\'\\s\\o\\m\\e\\ \\x\\s\\s\\\'\\)\\;\\" \\/\\>';

I think we want this to have the equivalent of <script>alert("XSS")</script> instead.

Looks good. Here are those two lines w/o the crazy escaping (did search/replace)

-      $this->displayOptions['settings']['date_format'] = '<strong>m/d/Y</strong><script src="alert('some xss');" />';
+      $this->displayOptions['settings']['date_format'] = '<strong>m/d/Y</strong><script>alert("XSS")</script>';

I think this is good to go. I would much prefer #1918994: Improve Datetime and Daterange Widget accessibility land before, this, though in case we get into reroll-purgatory. That patch is much more important than this one right now.

Thanks for adding tests for XSS sanitization!

Can we use a quoteless XSS exploit like in the first comment of #2506021: When testing for XSS (cross-site scripting) with assembled HTML, use a test exploit that does not contain quotes in the tests? Given the string manipulation here it seems like that would be a better choice.

I definitely agree #1918994: Improve Datetime and Daterange Widget accessibility is more important, but this patch is easier to commit so long as they don't break each other. I haven't had an opportunity to do a full review of the other.

$ git checkout 8.4.x
$ git pull
$ git apply --index 2775669-58.patch
$ git apply --index 1918994-163.patch

No fuzz / merge warnings. DateTimeFieldTest, DateRangeFieldTest, DateTimeItemTest, and DateRangeItemTest all pass locally.

Is that valid JS?

<script>alert(String.fromCharCode(88,83,83))</script> with all of the whacky encoding will probably work.

Interdiff looks good ; took the whacky XSS strings and searched/replaced to make sure they are valid JS when decoded.

Checking the full interdiff since the last time I reviewed...

Some unused use. I can fix these on commit later when my airplane is not landing, or someone else can clean them up in the interim. Leaving RTBC.

PHPCS: core/modules/datetime/src/Tests/DateTimeFieldTest.php passed

FILE: ...aintainer/core/modules/datetime_range/src/DateTimeRangeTrait.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 5 | WARNING | [x] Unused use statement
 7 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 37ms; Memory: 4Mb

PHPCS: core/modules/datetime_range/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php passed

FILE: ...ge/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Committed to 8.4.x with the unused uses fixed. Thanks!