system_get_date_formats() mentions that it returns the following fields:

 *   - dfid: The date format ID.
 *   - format: The format string.
 *   - type: The machine-readable name of the date type.
 *   - locales: An array of language codes. This can include both 2 character
 *     language codes like 'en and 'fr' and 5 character language codes like
 *     'en-gb' and 'en-us'.
 *   - locked: A boolean indicating whether or not this date type should be
 *     configurable from the user interface.
 *   - module: The name of the module that defined this date format in its
 *     hook_date_formats(). An empty string if the format was user-defined.
 *   - is_new: A boolean indicating whether or not this date type is as of yet
 *     unsaved in the database.

yet a call to system_get_date_formats('short') returns

Array
(
    [Y-m-d H:i] => Array
        (
            [type] => short
            [format] => Y-m-d H:i
            [locales] => Array
                (
                )
 
            [locked] => 1
            [is_new] => 
            [dfid] => 1
            [language] => 
        )
...

This has an extra 'language' field and is missing the 'module' field.

Either the documentation or the code will need to be fixed. Due to a lack of tests (see #1013980: Date types/formats need proper tests), it's hard to know what's correct.

CommentFileSizeAuthor
#10 1015236-10.patch5.63 KBbfroehle
#4 1015236-4.patch5.12 KBbfroehle
#2 1015236b.patch2.96 KBjhodgdon
#1 1015236.patch3.04 KBjhodgdon

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Title: Missing 'module' and extra 'language' field returned by system_get_date_formats » system_get_date_formats return value has extraneous information in it, and doc is wrong
Priority: Major » Normal
Status: Active » Needs review
StatusFileSize
new 1015236.patch3.04 KB

Good catch! Here's the function that actually builds the list returned by this function:
_system_date_formats_build()
http://api.drupal.org/api/drupal/modules--system--system.module/function...

- It appears that [language] being there is an artifact of how the list is generated, so I don't think we should document it. [locales] is the return component to use. What is happening is that individual $format items are being read from the database and each one may have a language value, but the individual languages are being merged into the locales array. So the fact that language is left on there is just an artifact. We should probably remove it so it doesn't confuse anyone again.
- It appears that [module] was supposed to be added to the return value, but it's only being added as '' for formats that were in the database, so it's pretty much useless. For module-provided types, it's not being defined at all, which pretty much defeats the purpose. My opinion is that we should remove it from the doc and remove the line that adds '' for the database-provided types.

Here's a patch, which removes the doc for module, removes the code that was adding an empty string to module to items from the database, and unsets the misleading language element. Also, when I was tinkering with this and figuring out what [language] was, I also couldn't resist rewriting the locales doc just a bit for these two functions. Hope that's OK.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
StatusFileSize
new 1015236b.patch2.96 KB

Whoops. Use this one. Last patch had a bad copy/paste artifact in it.

bfroehle’s picture

bfroehle commented

After the patch in #2, system_get_date_formats('short') returns


Array
(
    [Y-m-d H:i] => Array
        (
            [type] => short
            [format] => Y-m-d H:i
            [locales] => Array
                (
                )
 
            [locked] => 1
            [is_new] => 
            [dfid] => 1
        )
 ...

This makes the doc-block consistent with the code, which is good. However is this correct, or should we be changing the code to return 'module'?

bfroehle’s picture

bfroehle commented
Title: system_get_date_formats return value has extraneous information in it, and doc is wrong » system_get_date_formats return and doc information is inconsistent
StatusFileSize
new 1015236-4.patch5.12 KB

Here's an alternative patch, which instead of altering the doc alters the function to return 'module' as promised. This is very different than the patch in #2, although some of the doc wording changes were incorporated.

The operation of _system_date_formats_build() in this patch is very similar to that for _system_date_format_types_build() --- in fact the changes required were relatively trivial, just replace

  // Get formats supplied by various contrib modules.
  $module_formats = module_invoke_all('date_formats');

with

  // Get list of modules that implement hook_date_formats().
  $modules = module_implements('date_formats');

  foreach ($modules as $module) {
    $module_formats = module_invoke($module, 'date_formats');
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I'm not sure what we need the 'module' component for. It was obviously not being used for anything.

bfroehle’s picture

bfroehle commented

I skimmed through the issue that created the code -- it's all in #11623: Localize date formats -- but didn't find any reference to the use of the 'module' field. I also looked through the date module in contrib and didn't notice any ['module'] or ->module strings.

One reason for keeping it is consistency with system_get_date_types which also returns the 'module' field.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I'd like @plach to review + confirm this patch.

+++ modules/system/system.module
@@ -3712,31 +3714,35 @@ function _system_date_formats_build() {
+  $modules = module_implements('date_formats');
...
+  foreach ($modules as $module) {

Looks like we don't need the $modules variable.

+++ modules/system/system.module
@@ -3748,6 +3754,7 @@ function _system_date_formats_build() {
     $format = (array) $record;
+    unset($format['language']);
     $format['is_new'] = FALSE; // It's in the db, so override this setting.

This unset() could use a code comment.

Powered by Dreditor.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Maybe sun or platch could comment on which approach (#1 explanation/#2 patch, or #4) would be better. I personally don't think we need 'module', and 'language' clearly needs to go...

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

subscribing while I find the time to have a look

bfroehle’s picture

bfroehle commented
Title: system_get_date_formats return and doc information is inconsistent » system_get_date_formats() return and doc information is inconsistent
StatusFileSize
new 1015236-10.patch5.63 KB

Improvements to #4 based upon sun's comments in #7.

Added a comment:

+    // The language is later saved as an element of the locales field.
+    unset($format['language']);

Switched to not use $modules:

+  foreach (module_implements('date_formats') as $module) {

and

-  // Get list of modules that implement hook_date_format_types().
-  $modules = module_implements('date_format_types');
-
-  foreach ($modules as $module) {
+  foreach (module_implements('date_format_types') as $module) {

This still doesn't address jhodgdon's concern in #8

Maybe sun or platch could comment on which approach (#1 explanation/#2 patch, or #4) would be better. I personally don't think we need 'module', and 'language' clearly needs to go...

It's completely trivial to populate the 'module' field and brings consistency with system_get_date_types() which also returns a 'module' field. However, it's probably not used by anything so... it'd be good to have somebody else weigh in.

See #1013980: Date types/formats need proper tests for test routines.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

populate the 'module' field and brings consistency with system_get_date_types() which also returns a 'module' field. However, it's probably not used by anything so...

Normally, when collecting data from modules and exposing them as configurable items, an important use-case of storing the originating 'module' is to clean up and delete the configuration data in hook_modules_uninstalled().

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

RE #11 - that isn't being done here, and I'm pretty sure we don't want to. These hooks are really just for getting date formats and date types into the UI. Once they've been set in the UI, they can be used elsewhere on the site in the UI (at least, I think so -- for instance as a format choice for a date field), so you wouldn't want to delete things that were originally defined by a module.

bfroehle’s picture

bfroehle commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Version: 8.x-dev » 7.x-dev
Component: system.module » documentation
Issue tags: -Needs backport to D7

This function no longer seems to exist in D8, and I can't find any reference to the string in question in D8 code base. (Though I didn't search overly long.)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Component: documentation » system.module

Oops. My mistake; I didn't read far enough down the patch, and thought this was just a docs thing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Derp. This is about system_get_date_formats(), not system_get_date_types().

So the function does exist in D8, but looks like the return value docs have already been addressed there.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 7.x-dev » 8.x-dev
Component: system.module » documentation

actually I think the current 8.x docs are exactly what are shown in the original report?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Component: documentation » system.module

Ugh. I agree that the current patch goes way beyond documentation. Changing component back. Sorry for the noise.

alansaviolobo queued 10: 1015236-10.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 10: 1015236-10.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.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

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

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

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

quietone’s picture

quietone commented
Version: 8.9.x-dev » 7.x-dev
Issue tags: +Bug Smash Initiative

system_get_date_formats was removed in #2003892: Convert date formats to config entities. I checked the Drupal 7 api page and this issue is still valid. Changing version.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.