Problem/Motivation

\Drupal\Core\Datetime\DateFormatter should implement an interface, DateFormatterInterface, to allow it to be swappable.

Proposed resolution

Make interface.

Remaining tasks

Do it.

User interface changes

None.

API changes

\Drupal\Core\Datetime\DateFormatter will implement an interface instead of being a bare class.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it adds an interface to the codebase and doesn't address a bug or add new functionality
Issue priority Normal because this isn't a pressing need
Prioritized changes The main goal of this issue is improving developer experience due to the decision made in #2568613: Remove HTML support from date formats and replace remaining !placeholder where format_date() and 'date.formatter' => format() are used to escape HTML in user supplied format strings. The change will allow users to change this behavior in an easier fashion, if needed.
Disruption None. Existing code that typehints to DateFormatter will continue to work, though changing to DateFormatterInterface is recommended.
CommentFileSizeAuthor
#29 interdiff-25-29.txt569 bytesmpdonadio
#29 create-2571647-29.patch59.4 KBmpdonadio
#25 create-2571647-25.patch59.95 KBmpdonadio
#21 create-2571647-21.patch59.93 KBmpdonadio
#17 interdiff-12-17.txt2.58 KBmpdonadio
#17 create-2571647-17.patch59.93 KBmpdonadio
#12 interdiff-07-12.txt569 bytesmpdonadio
#12 create-2571647-12.patch59.86 KBmpdonadio
#7 create-2571647-7.patch59.3 KBmpdonadio
#4 interdiff-02-04.txt42.55 KBmpdonadio
#4 create-2571647-4.patch59.3 KBmpdonadio
#3 create-2571647-2.patch16.75 KBmpdonadio
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Status: Active » Needs review

Quick patch that just creates the interface, and updates the DateFormatter class to use it.

Next patch later tonight will replace DateFormatter with DateFormatterInterface where appropriate.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
FileSize
create-2571647-2.patch16.75 KB

I think DrupalCon is making the website cry. Got a 404 when I posted that...

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Assigned: mpdonadio » Unassigned
FileSize
create-2571647-4.patch59.3 KB
interdiff-02-04.txt42.55 KB

Think I swapped all of the correct usages according to PhpStorm. Should be ready for review.

Status: Needs review » Needs work

The last submitted patch, 4: create-2571647-4.patch, failed testing.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Issue tags: +Needs reroll

The fun of patches touching lot pf parts during sprints. This needs a reroll. If someone wants to do this at the sprint, feel free. Otherwise, I will reroll it tonight.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
create-2571647-7.patch59.3 KB

Straightforward rebase:

$ git rebase 8.0.x
First, rewinding head to replay your work on top of it...
Applying: Created DateFormatterInterface.
Applying: Swap DateFormatter for DateFormatterInterface.
Using index info to reconstruct a base tree...
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
M	core/modules/aggregator/src/Controller/AggregatorController.php
M	core/modules/aggregator/src/Plugin/aggregator/processor/DefaultProcessor.php
M	core/modules/config_translation/src/FormElement/DateFormat.php
M	core/modules/system/src/Form/CronForm.php
M	core/modules/system/src/Form/DateFormatFormBase.php
M	core/modules/system/src/Form/PerformanceForm.php
M	core/modules/user/src/Controller/UserController.php
M	core/modules/views_ui/src/ViewEditForm.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/views_ui/src/ViewEditForm.php
Auto-merging core/modules/user/src/Controller/UserController.php
Auto-merging core/modules/system/src/Form/PerformanceForm.php
Auto-merging core/modules/system/src/Form/DateFormatFormBase.php
Auto-merging core/modules/system/src/Form/CronForm.php
Auto-merging core/modules/config_translation/src/FormElement/DateFormat.php
Auto-merging core/modules/aggregator/src/Plugin/aggregator/processor/DefaultProcessor.php
Auto-merging core/modules/aggregator/src/Controller/AggregatorController.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@mpdonadio dumb question but how does making this an interface make it swappable? Is the plan to make it a service?

Otherwise this looks fine.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented

#8 I was only tangentially involved in the discussion in #2568613: Remove HTML support from date formats and replace remaining !placeholder where format_date() and 'date.formatter' => format() are used that led to this followup.

I assume what @alexpott meant in #39, is that by introducing the interface and using that instead of the concrete class in the injections, someone will be able to create their own formatter that implements DateFormatterInterface and ServiceProviderBase, they can swap it out in the container (see https://www.drupal.org/node/2026959). If we didn't have the interface, to swap out the service the new class would be forced to extend DateFormatter and implement ServiceProviderBase, which may not be desirable.

As a side note, if we are going to add the interface here, should we also add a proper service locator helper on \Drupal() for 'date.formatter' as part of this that @returns DateFormatterInterface?

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented

Oh, I jut re-read this. DateFormatter is already exposed as a service, http://cgit.drupalcode.org/drupal/tree/core/core.services.yml#n1247, but we don't have a helper on \Drupal for it.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue tags: +Needs beta evaluation

Thanks, I should have known that since I was working with it's service yesterday:p

Not sure about the side note in #9 but I'd assume so. Maybe just add it and see what people say?

Because this a normal task it will need a beta eval.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
FileSize
create-2571647-12.patch59.86 KB
interdiff-07-12.txt569 bytes

Added service helper. Do we have test coverage for these anywhere? Couldn't find any?

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Issue summary: View changes
Issue tags: -Needs beta evaluation +Needs change record

Added beta eval. Should probably have a short CR for this.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Issue tags: -Needs change record

Created CR draft. Not sure if anything else is needed.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented

DOUBLE SUBMIT

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented
Status: Needs review » Needs work

The patch looks good. I didn't find any additional occurrences.

  1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatterInterface.php
@@ -0,0 +1,178 @@
+   * @see \Drupal\Core\Datetime\DateFormatter::formatDiff()
+   * @see \Drupal\Core\Datetime\DateFormatter::formatTimeDiffSince()
...
+   * @see \Drupal\Core\Datetime\DateFormatter::formatDiff()
+   * @see \Drupal\Core\Datetime\DateFormatter::formatTimeDiffUntil()
...
+   * @see \Drupal\Core\Datetime\DateFormatter::formatInterval()
+   * @see \Drupal\Core\Datetime\DateFormatter::formatTimeDiffSince()
+   * @see \Drupal\Core\Datetime\DateFormatter::formatTimeDiffUntil()

    Shouldn't these be changed to point to the DateFormatterInterface as well? 

  2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatterInterface.php
@@ -0,0 +1,178 @@
+   *   - One of the built-in formats: 'short', 'medium',
+   *     'long', 'html_datetime', 'html_date', 'html_time',
+   *     'html_yearless_date', 'html_week', 'html_month', 'html_year'.

    I know this is copy-pasted, but we might want to fix the indentation while we're at it.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Status: Needs work » Needs review
FileSize
create-2571647-17.patch59.93 KB
interdiff-12-17.txt2.58 KB

#16-1 is fixed

#16-2 being dense here, but I don't see the indentation problem? Everything looks OK per https://www.drupal.org/node/1354 ?

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

I don't see the code violation in that 16.2 either so I'm going to to go with RTBC on this one. Thanks for the Beta eval.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented

Oh, I thought that it should be wrapped as close to 80 chars as possible, but it was a nitpick anyway.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: create-2571647-17.patch, failed testing.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Status: Needs work » Needs review
FileSize
create-2571647-21.patch59.93 KB

Reroll I think b/c #2571673: Convert Views t() usage where it is used as an attribute value

$ git rebase 8.0.x
First, rewinding head to replay your work on top of it...
Applying: Created DateFormatterInterface.
Applying: Swap DateFormatter for DateFormatterInterface.
Using index info to reconstruct a base tree...
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
M	core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
M	core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
M	core/modules/user/src/Controller/UserController.php
M	core/modules/views_ui/src/ViewEditForm.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/views_ui/src/ViewEditForm.php
CONFLICT (content): Merge conflict in core/modules/views_ui/src/ViewEditForm.php
Auto-merging core/modules/user/src/Controller/UserController.php
Auto-merging core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
Auto-merging core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
Failed to merge in the changes.
Patch failed at 0002 Swap DateFormatter for DateFormatterInterface.

Conflict was in the use section for ViewEditForm. Chose HEAD and just updated the use to be DateFormatterInterface instead of DateFormatter

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented
Status: Needs review » Reviewed & tested by the community

That reroll looks ok to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: create-2571647-21.patch, failed testing.

The last submitted patch, 21: create-2571647-21.patch, failed testing.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
FileSize
create-2571647-25.patch59.95 KB

Just needed a simple rebase:

$ git rebase 8.0.x
First, rewinding head to replay your work on top of it...
Applying: Created DateFormatterInterface.
Applying: Swap DateFormatter for DateFormatterInterface.
Using index info to reconstruct a base tree...
M	core/modules/node/src/Controller/NodeController.php
M	core/modules/system/src/Form/FileSystemForm.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/system/src/Form/FileSystemForm.php
Auto-merging core/modules/node/src/Controller/NodeController.php
Applying: Added service helper.
Applying: Updated @see references.
mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Status: Needs work » Needs review
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal.php
@@ -693,4 +693,14 @@ public static function entityDefinitionUpdateManager() {
+  /**
+   * Returns the date formatter service.
+   *
+   * @return \Drupal\Core\Datetime\DateFormatterInterface
+   *   The date formatter service.
+   */
+  public static function dateFormatter() {
+    return static::getContainer()->get('date.formatter');
+  }

This seems unused and unnecessary to complete the aims of the patch.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Status: Needs work » Needs review
FileSize
create-2571647-29.patch59.4 KB
interdiff-25-29.txt569 bytes

Reverted core/lib/Drupal.php to HEAD to remove that hunk.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

Manually compared in Kaleidoscope the interface doc moves and there are no changes.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Whilst this is a large patch it is not disruptive since anywhere in contrib or custom that typehints on DateFormatter will still work. This became more important after we stopped supporting HTML in data formats as one of the things we said is that someone could swap out the service to add support back in. Unfortunately without an interface that is almost impossible. In fact we should have a rule that all services must implement a single interface that covers their public methods that are used as part of the service (that interface can of course extend other interfaces).

Committed 641eb3f and pushed to 8.0.x. Thanks!

  • alexpott committed 641eb3f on 8.0.x 
    Issue #2571647 by mpdonadio, joelpittet, pjonckiere: Create...
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

+1 to having that rule! :)

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented

Published CR. Created #2580739: [META] All core services should implement an interface.

Status: Fixed » Closed (fixed)

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