I was looking at the datetime object this morning and I noticed that there are many calls to dateFormat() here and throughout the system that triggers the inspection, "Method 'getPattern' not found in class". I did a little digging and I found that dateFormat is used outside of Drupal\Core\Datetime\Date as method that delivers a string (of a date format). But internally it is expected to deliver a DateFormat object so that the getPattern method be invoked on it in an effort to finally deliver a date format.

The begs the question, which is right? And what should the "fix" be?

I quote "fix" because technically all the tests are passing and there really isn't anything broke. "If it ain't broke..."

In total there are only 2 uses of Drupal\Core\Datetime\Date's dateFormat() in all of core. There are 2 other uses of dateFormat in Drupal\datetime\Plugin\Field\FieldFormatter\DateTimeDefaultFormatter but these are calls to it's own implementation of dateFormat

I'm generally confused as to why nothing seems to be using Date's dateFormat method and why is was typed property. I'm hoping someone who's reading this issue can explain that.

Until then, here's a patch to fix the typing.

CommentFileSizeAuthor
#1 2197931_2.patch844 bytescosmicdreams

Comments

cosmicdreams’s picture

cosmicdreams commented
Status: Active » Needs review
StatusFileSize
new 2197931_2.patch844 bytes
cosmicdreams’s picture

cosmicdreams commented

Reading the code more closely reveals that Drupal\Core\Datetime\Date's dateFormat() function is protected, meaning that it's only for that class and any class that extends it to use. Drupal\Core\Datetime\Date does provide a public function named format() which formats date by using it's internal dateFormat() function.

Drupal\datetime\Plugin\Field\FieldFormatter\DateTimeDefaultFormatter's dateFormat() function looks like it's wrapping Drupal\Core\Datetime\Date's public format() function and providing it with parameters.

That's the source of my confusion. Here we have two functions that are named the same but exist in different classes yet they fundamentally do different things. Drupal\Core\Datetime\Date's dateFormat() returns a date format and Drupal\datetime\Plugin\Field\FieldFormatter\DateTimeDefaultFormatter's dateFormat() returns a formatted date. It's a fine point but an important one. Those are different things.

We should probably change Drupal\datetime\Plugin\Field\FieldFormatter\DateTimeDefaultFormatter's dateFormat() function's name to format(). That's the simplest change I can think of.

alansaviolobo queued 1: 2197931_2.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2197931_2.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs work » Closed (works as designed)

Reviewing the code (it is now \Drupal\Core\Datetime\DateFormatter::formatDate(), it looks like the string parameter type is correct:

  protected function dateFormat($format, $langcode) {
    if (!isset($this->dateFormats[$format][$langcode])) {
      $original_language = $this->languageManager->getConfigOverrideLanguage();
      $this->languageManager->setConfigOverrideLanguage(new Language(array('id' => $langcode)));
      $this->dateFormats[$format][$langcode] = $this->dateFormatStorage->load($format);
      $this->languageManager->setConfigOverrideLanguage($original_language);
    }
    return $this->dateFormats[$format][$langcode];