Convert systems date and time admin form to configuration system

Is this a duplicate of #1571632: Convert regional settings to configuration system?

Jumped the proverbial gun. This issue will convert system_date_time_settings().

#1571632: Convert regional settings to configuration system converts system_regional_settings(). Not sure that this will be a config novice task though as this will involve the conversion of system_add_date_format_type_form() as well.

+++ b/core/modules/system/system.moduleundefined
@@ -2130,10 +2130,10 @@ function system_init() {
+  $short_default = $config->get("format_type.short.default");

why double quotes?

26 days to next Drupal core point release.

+++ b/core/modules/system/config/system.date_time.ymlundefined
@@ -0,0 +1,7 @@
+format_type:
+    long:
+        default: 'l, F j, Y - H:i'
+    medium:
+        default: 'D, m/d/Y - H:i'
+    short:

Why not

format:
    long: 'l, F j, Y - H:i'
    medium: 'D, m/d/Y - H:i'
    ... 

26 days to next Drupal core point release.

And don't we have to convert the stuff written in common.inc. (the format_date() function)

See http://drupal.org/coding-standards/#quotes

Drupal does not have a hard standard for the use of single quotes vs. double quotes. Where possible, keep consistency within each module, and respect the personal style of other developers.

There seems to be a mix of single and double quote usage. Whilst not exactly against the coding standards it look a little strange to me.

+++ b/core/includes/common.incundefined
@@ -1906,11 +1906,8 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
+      $format = config('system.date_time')->get("format.$type");

This could be something like $format = config('system.date_time')->get('format.' . $type)

+++ b/core/includes/common.incundefined
@@ -1949,11 +1946,12 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
+        $default = config('system.date_time')->get("format.$type");

As above

+++ b/core/modules/system/system.admin.incundefined
@@ -2064,7 +2064,7 @@ function system_date_time_settings() {
+      $default = $config->get("format.$type");

As above

+++ b/core/modules/system/system.admin.incundefined
@@ -2215,7 +2230,9 @@ function system_add_date_format_type_form_submit($form, &$form_state) {
+    ->set("format.$machine_name", $form_state['values']['date_format'])

As above

+++ b/core/modules/system/system.admin.incundefined
@@ -3249,7 +3266,7 @@ function system_date_format_localize_form($form, &$form_state, $langcode) {
+      $default = config('system.date_time')->get("format.$type");

As above

+++ b/core/modules/system/system.api.phpundefined
@@ -3516,7 +3516,7 @@ function hook_archiver_info_alter(&$info) {
+ * @code config('system.date_time')->set("format.$type", $format); @endcode

As above

+++ b/core/modules/system/system.api.phpundefined
@@ -3586,7 +3586,7 @@ function hook_date_format_types_alter(&$types) {
- * call @code variable_set('date_format_' . $type, $format); @endcode

As above

+++ b/core/modules/system/system.installundefined
@@ -1998,6 +2000,20 @@ function system_update_8014() {
+    $variables["date_format_$type"] = "format.$type";

As above

+++ b/core/modules/system/system.moduleundefined
@@ -2150,7 +2152,8 @@ function system_get_localized_date_format($languages) {
+          $default = $config->get("format.$type");

As above

+++ b/core/modules/system/system.moduleundefined
@@ -2163,14 +2166,15 @@ function system_get_localized_date_format($languages) {
+  $formats['date_format_medium'] = $config->get("format.medium");
+  $formats['date_format_long'] = $config->get("format.long");

Unnecessary double quotes

+++ b/core/modules/system/system.moduleundefined
@@ -2163,14 +2166,15 @@ function system_get_localized_date_format($languages) {
+      $default = $config->get("format.$type");

As above

+++ b/core/includes/common.incundefined
@@ -1949,11 +1946,12 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
       if ($type != 'medium') {
-        $format = variable_get('date_format_' . $type, '');
+        $default = config('system.date_time')->get("format.$type");
+        $format = $default ? $default : '';
       }
       // Fall back to 'medium'.
       if ($format === '') {
-        $format = variable_get('date_format_medium', 'D, m/d/Y - H:i');
+        $format = config('system.date_time')->get('format.medium');
       }

I'm sure this could be nicer. Maybe something like:

$format = config('system.date_time')->get('format.' . $type);

// Fall back to 'medium'.
if (empty($format)) {
  $format = config('system.date_time')->get('format.medium');
}

I think the double quotes are needed if you add a variable inside the quotes. I prefer

$format = config('system.date_time')->get('format.' . $type);

instead of
$format = config('system.date_time')->get("format.$type");

+++ b/core/includes/common.incundefined
@@ -1949,11 +1946,11 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
+        $format = config('system.date_time')->get('format' . $type);

should be ->get('format.' . $type);

the first dot sepeartes the keys, the second one concats the strings

24 days to next Drupal core point release.

In common.inc I still can find the following

    case 'html_datetime':
      $format = variable_get('date_format_html_datetime', 'Y-m-d\TH:i:sO');
      break;

    case 'html_date':
      $format = variable_get('date_format_html_date', 'Y-m-d');
      break;

    case 'html_time':
      $format = variable_get('date_format_html_time', 'H:i:s');
      break;

    case 'html_yearless_date':
      $format = variable_get('date_format_html_yearless_date', 'm-d');
      break;

    case 'html_week':
      $format = variable_get('date_format_html_week', 'Y-\WW');
      break;

    case 'html_month':
      $format = variable_get('date_format_html_month', 'Y-m');
      break;

    case 'html_year':
      $format = variable_get('date_format_html_year', 'Y');

These defaults should be added to the yml file and updated in the update function.

I also find this in the same function:

      // Retrieve the format of the custom $type passed.
      if ($type != 'medium') {
        $format = variable_get('date_format_' . $type, '');
      }

this should be converted to a $config->get() / $config_set() mechanism

Ok there probably is something wrong with "system_update_8015" the real error is hidden. If you enable the language module before calling that language_negotation function you'll see that other errors will popup.

Marked #1431292: Migrate date format configuration to CMI which was 6 months older. And I was wondering where the action is happening. Go, go!

fixed a typo and do a complete retest.

Some problems in this patch:

- The system update number needs to be changed because there now is another update 8017.
- On a fresh install, if I go to the admin/config/regional/date-time page and do nothing but submit the page, I get the following error three times "Notice: Undefined variable: type in system_date_time_settings_submit() (line 2123 of core/modules/system/system.admin.inc).", one for each of the format types in that page.
- If I add a new date format to the page, it is saved correctly but not added to the yml file.
- If I try to change a date from the format defined in the yml file, it is not saved, and it reverts to the value in the yml file.

Also, my understanding was that each variable would have its own yml file, but this puts all the variables in a single file. I don't know which is right.

Ah, I picked up the wrong patch, the update hook is correct. The yml files are not supposed to update, so that was my mistake too. With the latest change the other problems I was having saving the values on the form are fixed. I tried this both on a clean install and on a site that needed to run the update hook, and both worked fine. Just waiting for the testbot to return the test and we can mark it RTBC.

And the test finally came back green, so marking RTBC.

Probably should add a test, it might be OK to make that a follow-up issue, but we can see what the maintainers think.

Awesome work here! Thanks for moving this forward! :)

A few final issues:

+++ b/core/includes/common.inc
@@ -1911,55 +1911,17 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
+        $format = config('system.date_time')->get('format.' . $type);
...
+        $format = config('system.date_time')->get('format.medium');

The resulting logic of this switch is not a switch, but a very simple if/else condition ;)

Essentially, if $type is 'custom', use it directly. Otherwise, retrieve the configured format via:

$date_config = config('system.date');

So let's

1) Kill the switch.

2) Instantiate the config object only once and assign it to $date_config.

3) Rename the object/file to just system.date.

+++ b/core/modules/system/system.admin.inc
@@ -2039,12 +2039,13 @@ function system_regional_settings() {
+function system_date_time_settings($form, $form_state) {

&$form_state always needs to be taken by reference.

+++ b/core/modules/system/system.install
@@ -2062,6 +2062,24 @@ function system_update_8017() {
+function system_update_8018() {
+  $format_types = system_get_date_types();
+  $not_gui_format_types = drupal_map_assoc(array('html_datetime', 'html_date', 'html_time',
+    'html_yearless_date', 'html_week', 'html_month', 'html_year',));
+  $format_types = array_merge($not_gui_format_types, $format_types);
+  $variables = array();
+  foreach ($format_types as $type => $format_type) {
+    $variables['date_format_' . $type] = 'format.' . $type;
+  }
+
+  update_variables_to_config('system.date_time', $variables);

Update functions cannot and must not use API functions that are triggering module hooks. system_get_date_types() invokes hook_date_format_types().

We need to change this update to only care for the date types and formats being registered and known to Drupal core.

This essentially means that we can use the regular update_variables_to_config() code for the hard-coded types.

We can most likely also convert the custom formats being contained in the {date_format_type} table by querying it directly -- although the conversion of types seems to be a separate issue and not contained at all in here?

We can most likely also convert the custom formats being contained in the {date_format_type} table by querying it directly -- although the conversion of types seems to be a separate issue and not contained at all in here?

Looks like enough back and forth itself for these ones. The DB based formats can come in another related issue AFAIS. We don't need to do everything at once.

I'd like to take another stab at this before it goes in, both to consider sun's suggestions and to be sure the custom formats are behaving correctly (I'm not sure I tested that as completely as I should). And maybe get a couple of tests in to be sure they don't get broken again as things continue to progress. I will be traveling all day tomorrow, so I won't get back to this until after I return, but I plan to do it in the next couple of days.

In the Drupal 8 Multilingual Initiative, we would love to get going with this as one of our example data pieces to make multilingual, so this landing is really just a start of many other layers of work happening :)

I've attempted to fix everything sun mentioned, except the update hook. Interdiff also attached.

There are a number of things I'd like to change, after further reflection:

- Code like $pos = strpos($key, 'date_format_'); is messy, it's because we are still storing format types as 'date_format_TYPE', which was done because we needed the namespacing when these were turned into variables. We can clean all the date_format arrays up to use the format types without any prefixing or namespacing, which gets rid of messy bits of code like that.
- I want to change the name of the configuration from 'date_time' to 'date_format_type'. I plan to introduce other date and time changes and this is definitely not the most important big of configuration for a date/time system. There are also other bits of the date_format system that still need to be converted to CMI and I don't think all those tables are going into a single CMI file, so we should be specific that *this* item is just the default settings for each date format type, not everything about date formats, let alone everything about date_time. And that name corresponds with the variable names we are replacing here, so it feels like it will be more intuitive.
- After the above two changes, we can simplify the configuration to remove the 'format.' prefixes and flatten the values somewhat. The only configuration here is the date formats, so we don't need to add a prefix for 'format' as well.
- I also have been looking at the very messy needs of the date format locale table in light of the translation system to try to see how it could fit into this. Not to mention the fact that currently other modules can declare their own formats and format types and we need a system that makes that possible and I'm not sure at this point how that could happen. That's another reason to keep this configuration object focused on only replacing the format variables into a configuration object that isn't going to try to solve everything about date formats -- because I don't thing a single configuration object is going to be able to do all that.

Patch forthcoming.

OK, here's a patch that tries to do fundamentally the same thing as the previous one, with a few exceptions:

- I changed the name of the config to date_format_type and got rid of the 'format' prefix on the values.
- I fixed the handling of date format option arrays to get rid of the 'date_format_' prefixing in general.
- I fixed the update hook by using a db_query() instead of an API function.
- I ran into a test failure that required that I add the 'locale' module to the DateFormat test module list. Before I did that I had fatal errors about missing locale tables. I can't believe that hasn't created a problem before

This cleans up the config handling a bit, and leaves the door open for other config values in the future. So instead of:

$format = config('system.date')->get('format.medium'); 

We do:

$format = config('system.date_format_type')->get('medium');

This also allows me to get rid of code like this:

    $pos = strpos($key, 'date_format_');
    if ($pos !== FALSE) {
      $format = substr($key, 12);
      $config->set('format.' . $format, $value);
    }

Passes tests locally, hopefully the bot will agree.

I'm still concerned that we haven't removed any of the old tables or the table handling, so I'm not quite sure how good a test this is of the config management itself, but it moves things forward, gets rid of the variables, and hopefully doesn't break anything.

#45: 1708542-system.date_time.variable_to_config.patch queued for re-testing.

#45 seems to be missing the default config in system.date_format_type.yml?

Yes seems the .yml file is missing.

Why not just name the yml file:
system.date.format.yml

If that works...

@KarenS: Thanks, that makes a lot of sense :)

On the config object/file name though, I was proposing system.date earlier, because my actual concern is whether these settings shouldn't be merged into the system.regional config object being introduced by #1571632: Convert regional settings to configuration system (which, in total, could be system.locale in the end).

That is, because it appears to me that whenever you'd need system.date, you'd also need system.regional (because it contains the timezone configuration and so on). I'm only 90% sure about this, so I'd appreciate feedback everyone. At least the code spots that these both issues are touching look very similar.

(Note: The issues do not need to be merged because of that. update_variables_to_config() has built-in support for being called multiple times to merge more variables into the same config object.)

Yeah, I'm wondering about the right namespace for this. Taking it in a slightly different direction, I'm wondering if we should pull a bunch of date-related configuration into a date module and namespace. I'm going to be proposing to add a date module to core anyway, but I'm not sure of the best plan for configuration naming.

On the question about calling this system.date.formats, I'm not sure what the benefit is of adding an extra level into the configuration. I guess it would be nice if we think we would want to combine all the system.date configuration at once, but I'm not sure how that would work. Even then, I don't want to call this 'formats'. We have a bunch of configuration that has not been converted to CMI yet, including the date formats table, so I'm thinking we should leave that term open for now. This is not actually date formats, it is the identification of which formats to use with each format type. The formats table contains a list of all the possible formats across all format types, including custom ones created by the site owner in the UI. We also have a table of format types, including custom ones created by the site owner in the UI. These variables are neither of those, they are the mapping of a format to a format type. I suppose even using the term format_type could be a problem later when we bring that table in.

Anyway, I was indeed missing the yml file. New patch with that added too see if the bot likes it. The interdiff is not showing the difference in the yml files for some reason, but everything else shows up.

Don't let the fatal trick you. It's related to a failing cmi upgrade path.

Assigning myself to this.

For the naming of this and the rest of the configuration that needs to be updated, how about this:

date format variables - system.date.format_value
date format table - system.date.formats
date format type table - system.date.format_types
date format locale table - system.date.format_locale

Then change the two date-related settings in the regional config:

date_timezone - system.date.timezone
date_first_day - system.date.first_day

That gets all the date-related configuration into a configuration object that could be called using the 'system.date' prefix. Scratch my earlier idea about moving this into a date module. The date module I'm thinking of (to create a date field) would be optional. And a date.inc file that has values that are always included can't have config attached to it, so I think this has to be handled by the system module so date values are always available. At least that's the way I understand the system.

Or maybe this, so we have a way to create a config object that has only date format configuration:

date format variables - system.date.format.value
date format table - system.date.format.options
date format type table - system.date.format.types
date format locale table - system.date.format.locale

Created a META issue at #1763754: [meta] Date and Time configuration

I'm going to try a re-roll.

patch attached using system.date.format.value namespace for date formats as suggested in #56.

The breakage is in update_variables_to_config , it needs to bug out immediately if an empty variable_map is thrown at it. Add if (!$variable_map) return to the top.

patch attached incorporating feedback in #62.

I think we should add a check before we call the upgrade function. But this is just an upgrade function so I don't rly care that much.

Thanks for keeping this alive! :)

The country doesn't belong into the date configuration though :-/

EDIT: Sorry, copypaste mistake. Anyway:

Let's postpone this on #1763754: [meta] Date and Time configuration until we have an agreed-on way forward.

#63: 1708542-system.date_time.variable_to_config-63.patch queued for re-testing.

Tested local and Variable migratio test is not failing

I think the work in #1571632: Convert regional settings to configuration system makes this a duplicate. That issue seems to encompass the things discussed here, and has a newer patch that is coming along.

If there is anything in this issue not addressed in the other one, let's discuss it there.