Settings that interact with regional settings and date formats should be converted to the new configuration system.

With this issue we seek to modify all system_date variables and other variables that can be found on the admin/config/regional/* pages.

Tasks:

  • Create the system.date.yml file
  • Convert system_regional_settings in system.admin.inc to use these configuration settings.
  • Search through core for any other place that use these variables and convert the CRUD syntax to use the new config system
  • Revamp the handling of date formats so that it uses the config system.
  • By using config files we no long need seperate type data handling functions
  • As a result remove all date_type functions
  • Remove database tables that previous persisted date format information.
  • Ensure no feature loss through new handling of date formats
  • Ensure translation of date formats is not regressed.

One feature that's added is that we're using CMI to store the configuration for data formats. As a result, we're getting rid of the tables that previously handled that task. That's a pretty important feature as developers can now modify a configuration file when they want to provide new date formats.

We also provide a place where future work can provide custom date format patterns (specifically thinking about IntlDateFormatter). We namespace the current date format patterns to php so that we can store multiple patterns for the same format. For example:

formats:
  system_short:
    name: 'Default Short Date'
    pattern:
      php: 'm/d/Y - H:i'
    locked: 0

In addtion, we introduce system_get_date_format_pattern to allow the system-driven logic to retrieve the right pattern. This allows us to rework that function and this config file in the future if we want to support IntlDateFormatter.

Questions and Answers

What does the "system_" namespacing of certain date format patterns mean to the developer?

Using 'system_' to namespace the date formats patterns for short, medium, and long provides developers who are reading the configuration a clear separation between date formats that were created by hand or through the UI and the ones that were initially installed.

True, there is nothing within our implementation that demands these configurations to be named in this way. It's a carry-over from the previous generation of this system where short, medium, and long date formats where hidden by the system so that developers would have to work harder to change them. Screwing up these formats results in system instability as they are used throughout a default Drupal installation to output dates.

So, it's one part tradition and one part a name-spacing the config to indicate that it belongs to the system.

Also, if it's still in the patch, I modified the admin pages for date formats to use the new dropbutton.

Previous Summary

The maintenance mode settings at admin/config/development/maintenance need to be converted to use the new configuration system.

Tasks

  • Create a default config file and add it to the system module.
  • Convert the system_regional_settings in system.admin.inc to read to/write from the appropriate config.
  • Convert any code that currently accesses the variables used to use the config system.
  • Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.

This would be a good task for someone wanting to get up to speed on how the new config system works. Feel free to ping me on IRC if you need help.

Follow up issues

#1844196: Ensure the medium (or more?) formats are always there
#1844614: Add tests for the IntlDateFormatter formats

CommentFileSizeAuthor
#306 1571632_306.patch106.74 KBcosmicdreams
#302 1571632-301.patch105.85 KBswentel
#302 interdiff.txt932 bytesswentel
#301 1571632_301-fixed.patch390.11 KBcosmicdreams
#301 interdiff.txt930 bytescosmicdreams
#299 1571632-299.patch106.76 KBswentel
#299 interdiff.txt28.23 KBswentel
#290 1571632_290.patch125.35 KBchx
#289 1571632_289.patch123.79 KBchx
#285 1571632_285.patch125.53 KBaspilicious
#280 1571632_280.patch125.36 KBcosmicdreams
#269 1571632_fresh.patch125.35 KBmarcingy
#261 1571632_260.patch125.35 KBmarcingy
#259 1571632_final.patch256.62 KBcosmicdreams
#245 1571632_245.patch125.35 KBchx
#242 1571632-242.patch125.29 KBswentel
#242 interdiff.txt3.22 KBswentel
#241 1571632-241.patch123.84 KBswentel
#241 interdiff.txt5.59 KBswentel
#239 1571632-regional-config-239.patch123.19 KBmarcingy
#238 1571632-regional-config-238.patch118.44 KBmarcingy
#235 1571632-regional-config-235.patch121.64 KBrvilar
#229 1571632-regional-config-229.patch123.19 KBKarenS
#225 regional-2.patch123.19 KBKarenS
#225 interdiff.txt15.15 KBKarenS
#223 regional-1.patch115.57 KBKarenS
#223 regional-interdiff.txt3.11 KBKarenS
#217 regional.patch114.83 KBKarenS
#217 interdiff.txt12.81 KBKarenS
#210 1571632-210.patch113.34 KBcosmicdreams
#204 1571632-204.patch112.59 KBcosmicdreams
#201 1571632-201.patch185.73 KBcosmicdreams
#200 1571632-200.patch373.08 KBcosmicdreams
#198 1571632-198.patch112.91 KBcosmicdreams
#196 date_format_change_1.PNG86.33 KBcosmicdreams
#196 date_format_change_add.PNG92.33 KBcosmicdreams
#196 date_format_change_delete.PNG17.46 KBcosmicdreams
#196 date_format_change_edit.PNG91.46 KBcosmicdreams
#195 1571632-192-interdiff.txt3.67 KBcosmicdreams
#194 1571632-194-3.patch112.91 KBcosmicdreams
#192 1571632-192.patch112.56 KBcosmicdreams
#190 1571632-190.patch112.56 KBcosmicdreams
#188 1571632-188.patch111.16 KBcosmicdreams
#181 interdiff.txt3.36 KBdawehner
#181 core-1571632-181.patch113.31 KBdawehner
#171 1571632-172.patch109.95 KBmarcingy
#169 1571632-170.patch109.94 KBmarcingy
#167 1571632-167.patch106.9 KBmarcingy
#164 1571632-interdiff.txt11.88 KBcosmicdreams
#163 1571632-163.patch91.25 KBmarcingy
#156 1571632_156.patch89.89 KBcosmicdreams
#153 1571632_153.patch89.94 KBcosmicdreams
#150 1571632_149.patch89.9 KBcosmicdreams
#146 1571632_146.patch89.21 KBcosmicdreams
#144 1571632_144.patch89.21 KBcosmicdreams
#142 1571632_142.patch88.8 KBcosmicdreams
#140 1571632_137.patch88.19 KBmarcingy
#135 1571632_134.patch99.07 KBmarcingy
#133 1571632_133.patch79.55 KBcosmicdreams
#131 1571632_131.patch77.04 KBcosmicdreams
#111 1571632_109.patch78.12 KBKarenS
#111 interdiff.txt14.78 KBKarenS
#109 1571632_109.patch71 KBchx
#106 1571632_104.patch70.56 KBchx
#104 1571632_104.patch70.56 KBchx
#104 interdiff.txt2.49 KBchx
#102 1571632_102.patch70.34 KBchx
#96 1571632_date_format_changes.patch71.09 KBKarenS
#89 1571632_89.patch66.09 KBcosmicdreams
#85 d8_date_converge.patch55.15 KBcosmicdreams
#83 drupal-change_to_config_for_tz-1571632-83.patch20.92 KBjustafish
#83 interdiff.txt15.14 KBjustafish
#78 drupal-change_to_config_for_tz-1571632-78.patch21.06 KBjustafish
#78 interdiff.txt2.5 KBjustafish
#70 rename-first_weekday-to-first_day-1571632-70.patch20.48 KBjustafish
#75 drupal-change_to_config_for_tz-1571632-75.patch21.17 KBjustafish
#75 interdiff.txt2.37 KBjustafish
#63 rename-first_weekday-to-first_day-1571632-63.patch20.53 KBjustafish
#63 interdiff.txt3.07 KBjustafish
#61 drupal-change_to_config_for_tz-1571632-60.patch20.55 KBjustafish
#61 interdiff.txt1.32 KBjustafish
#59 drupal-change_to_config_for_tz-1571632-59.patch20.55 KBjustafish
#59 interdiff.txt657 bytesjustafish
#56 drupal-change_to_config_for_tz-1571632-56.patch20.25 KBdisasm
#54 drupal-change_to_config_for_tz-1571632-54.patch21.87 KBaspilicious
#50 drupal-change_to_config_for_tz-1571632-50.patch21.93 KBaspilicious
#40 drupal-change_to_config_for_tz-1571632-40.patch22.07 KBaspilicious
#35 drupal-change_to_config_for_tz-1571632-35.patch22.07 KBaspilicious
#24 drupal-change_to_config_for_tz-1571632-24.patch19.39 KBdisasm
#24 interdiff.txt653 bytesdisasm
#22 drupal-change_to_config_for_tz-1571632-21.patch19.4 KBdisasm
#22 interdiff.txt3.75 KBdisasm
#21 drupal-change_to_config_for_tz-1571632-21.patch19.4 KBdisasm
#21 interdiff.txt3.75 KBdisasm
#19 1571632_19_regional_cmi.patch17.04 KBaspilicious
#16 1571632_16_regional_cmi.patch13.99 KBcosmicdreams
#12 1571632_12_regional_cmi.patch18.86 KBcosmicdreams
#10 1571632_9_regional_cmi.patch18.98 KBcosmicdreams
#8 1571632_8_regional_cmi.patch18.99 KBcosmicdreams
#5 1571632_5_regional_cmi.patch19.02 KBcosmicdreams
#3 1571632_3_regional_cmi.patch18.61 KBcosmicdreams
#1 1571632_1_regional_cmi.patch18.64 KBcosmicdreams
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cosmicdreams’s picture

Status: Active » Needs review
FileSize
18.64 KB

First Try

Status: Needs review » Needs work

The last submitted patch, 1571632_1_regional_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
18.61 KB

Thing I found the typo

Status: Needs review » Needs work

The last submitted patch, 1571632_3_regional_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
19.02 KB

forgot a few ->save()'s and I'm trying to faithfull reproduce what's going on the the install.core.inc and bootstrap as I'm pretty sure if I don't do it exactly that way it will lead to lots of fails. (as the above shows)

cosmicdreams’s picture

Issue tags: +Config novice

tagging

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/includes/bootstrap.incundefined
@@ -2095,13 +2095,16 @@ function drupal_bootstrap($phase = NULL, $new_phase = TRUE) {
+    $value = isset($config_data_default_timezone) ? $config_data_default_timezone : @date_default_timezone_get();
+    return $value;

This can be combined.

+++ b/core/modules/system/system.moduleundefined
@@ -2042,8 +2043,9 @@ function system_form_user_register_form_alter(&$form, &$form_state) {
+  if ($config->get('configurable_timezones', 1) && empty($account->timezone) && !$config->get('user_default_timezone')) {

The first call to config->get still has the default value.

+++ b/core/modules/system/system.moduleundefined
@@ -2051,8 +2053,9 @@ function system_user_presave($account) {
+  if (!$account->timezone && $config->get('configurable_timezones', 1) && $config->get('empty_timezone_message', 0)) {

These both still pass a default value

+++ b/core/modules/user/user.api.phpundefined
@@ -295,8 +295,9 @@ function hook_user_update($account) {
+  if (!$account->timezone && $config->get('configurable_timezones') && $config->get('empty_timezone_message', 0)) {

Second call passes a default value.

+++ b/core/modules/user/user.testundefined
@@ -167,7 +169,7 @@ class UserRegistrationTestCase extends WebTestBase {
+    $this->assertEqual($new_user->timezone, config('system.regional')->get('date_default_timezone'), t('Correct time zone field.'));

Might as well remove the call to t() while you're here.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
18.99 KB

Here's a patch with those revisions

Status: Needs review » Needs work

The last submitted patch, 1571632_8_regional_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
18.98 KB

another try

socketwench’s picture

Looks good to me.

cosmicdreams’s picture

Rerolled the patch so that it uses yml as a the file format for exported config.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I scanned the patch and didn't found something which shines out to be wrong, so i consider this patch to be RTBC.

+++ b/core/modules/user/user.testundefined
@@ -167,7 +169,7 @@ class UserRegistrationTestCase extends WebTestBase {
+    $this->assertEqual($new_user->timezone, config('system.regional')->get('date_default_timezone'), 'Correct time zone field.');

A little out of scope to remove the t() here, but i think that's okay.

dawehner’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/system.admin.incundefined
@@ -2009,10 +2011,26 @@ function system_regional_settings() {
 /**
+ * Implements hook_form_submit.

That's not really a hook, see system_cron_settings_submit for another example.

cosmicdreams’s picture

Status: Needs work » Needs review

It has frequently been advocated to remove t() on tests whenever possible the comment in question will go away once #1324618: Implement automated config saving for simple settings forms goes in. Should this patch really be held back until then?

cosmicdreams’s picture

reroll

#13: I've been told to remove t() whenever I'm working on patches. Just following instructions.

Status: Needs review » Needs work

The last submitted patch, 1571632_16_regional_cmi.patch, failed testing.

cosmicdreams’s picture

Issue tags: +Novice

Looks like I have fails in UserRegistrationTest and UserTimeZoneTest to cleanup. Most likely because these tests aren't pulling the value from the config object.

I'll try to get to this tonight.

Meanwhile, I'll mark this as novice since all a person would need to do is see how previous patches pull values from the config and hunt down any use of variable_get in the tests I mention here and replace them.

If someone can beat me to it that'll be fine.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
17.04 KB
aspilicious’s picture

Someone in irc asked for a diff so he could see what I did. Here it is...

+++ b/core/modules/locale/locale.moduleundefined
@@ -660,7 +660,7 @@ function locale_library_info_alter(&$libraries, $module) {
-              'firstDay' => variable_get('date_first_day', 0),
+              'firstDay' => config('system.regional')->get('date_first_day'),
             ),

Conflicted had to reapply

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/FormatDateTest.phpundefined
@@ -29,7 +29,7 @@ class FormatDateTest extends WebTestBase {
-    variable_set('configurable_timezones', 1);
+    config('system.regional')->set('configurable_timezones', 1)->save();

Conflicted reapplied

+++ b/core/modules/system/lib/Drupal/system/Tests/System/DateTimeTest.phpundefined
@@ -35,8 +35,11 @@ class DateTimeTest extends WebTestBase {
-    variable_set('date_default_timezone', 'Pacific/Honolulu');
-    variable_set('configurable_timezones', 0);
+    $config = config('system.regional');
+    $config
+      ->set('date_default_timezone', 'Pacific/Honolulu')
+      ->set('configurable_timezones', 0)
+      ->save();

Conflicted reapplied

+++ b/core/modules/system/lib/Drupal/system/Tests/System/DateTimeTest.phpundefined
@@ -52,7 +55,7 @@ class DateTimeTest extends WebTestBase {
-    variable_set('date_default_timezone', 'America/Los_Angeles');
+    $config->set('date_default_timezone', 'America/Los_Angeles')->save();

Same

+++ b/core/modules/user/lib/Drupal/user/Tests/UserRegistrationTest.phpundefined
@@ -145,8 +145,11 @@ class UserRegistrationTest extends WebTestBase {
-    variable_set('configurable_timezones', 1);
-    variable_set('date_default_timezone', 'Europe/Brussels');
+    $config = config('system.regional');
+    $config
+      ->set('configurable_timezones', 1)
+      ->set('date_default_timezone', 'Europe/Brussels')
+      ->save();

change 2

+++ b/core/modules/user/lib/Drupal/user/Tests/UserTimeZoneTest.phpundefined
@@ -26,8 +26,11 @@ class UserTimeZoneTest extends WebTestBase {
-    variable_set('date_default_timezone', 'America/Los_Angeles');
-    variable_set('configurable_timezones', 1);
+    $config = config('system.regional');
+    $config
+      ->set('configurable_timezones', 1)
+      ->set('date_default_timezone', 'America/Los_Angeles')
+      ->save();
     variable_set('date_format_medium', 'Y-m-d H:i T');

Change 1

15 days to next Drupal core point release.

disasm’s picture

Status: Needs review » Needs work
FileSize
3.75 KB
19.4 KB

I found some more spots that were missed. One was the UserRegistration class was not reading from config in the actual test. Others were in the OpenID test classes.

disasm’s picture

Status: Needs work » Needs review
FileSize
3.75 KB
19.4 KB

re-attached with the right status.

aspilicious’s picture

+++ b/core/modules/openid/lib/Drupal/openid/Tests/OpenIDRegistrationTest.phpundefined
@@ -30,8 +30,12 @@ class OpenIDRegistrationTest extends OpenIDTestBase {
-    variable_get('configurable_timezones', 1);
-    variable_set('date_default_timezone', 'Europe/Brussels');
+
+    config('system.regional')
+      ->set('configurable_timezones', 1)
+      ->set('date_default_timezone', 'Europe/Brussels')
+      ->save();

? This test look strange, it does a variable_get but it's not added to a variable. This test should be looked at closely

+++ b/core/modules/openid/lib/Drupal/openid/Tests/OpenIDRegistrationTest.phpundefined
@@ -30,8 +30,12 @@ class OpenIDRegistrationTest extends OpenIDTestBase {
+    ¶

Trailing whitespace

+++ b/core/modules/openid/lib/Drupal/openid/Tests/OpenIDRegistrationTest.phpundefined
@@ -87,8 +91,11 @@ class OpenIDRegistrationTest extends OpenIDTestBase {
-    variable_get('configurable_timezones', 1);
-    variable_set('date_default_timezone', 'Europe/Brussels');
+
+    config('system.regional')
+      ->set('configurable_timezones', 1)
+      ->set('date_default_timezone', 'Europe/Brussels')

Again, testt look strange

+++ b/core/modules/openid/lib/Drupal/openid/Tests/OpenIDRegistrationTest.phpundefined
@@ -128,8 +135,10 @@ class OpenIDRegistrationTest extends OpenIDTestBase {
-    variable_get('configurable_timezones', 1);
-    variable_set('date_default_timezone', 'Europe/Brussels');
+    config('system.regional')
+      ->set('configurable_timezones', 1)
+      ->set('date_default_timezone', 'Europe/Brussels')
+      ->save();

Same

15 days to next Drupal core point release.

disasm’s picture

Removed the trailing whitespace (oops). I thought the test looked weird as well, hence why I changed it to set. That was the way the other tests had been written for other parts of the system. I agree, someone working on the openid stuff should probably take a look at it and make sure I didn't break any functionality. The tests do pass though the way I rewrote them.

Status: Needs review » Needs work
Issue tags: -Novice, -Config novice

The last submitted patch, drupal-change_to_config_for_tz-1571632-24.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Config novice
sun’s picture

Issue tags: +Configuration system
Dries’s picture

Status: Needs review » Reviewed & tested by the community

I looked at this patch and it looks good to me. It is a straight-forward port to the new configuration system. I'm marking it RTBC for now.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/config/system.regional.yml
@@ -0,0 +1,6 @@
+site_default_country: ''
+date_first_day: '0'
+date_default_timezone: ''
+configurable_timezones: '1'
+empty_timezone_message: '0'
+user_default_timezone: '0'

The keys need to be updated for the new guidelines: http://drupal.org/node/1667896

+++ b/core/modules/system/system.admin.inc
@@ -2012,10 +2014,29 @@ function system_regional_settings() {
+  $form['#submit'][] = 'system_regional_settings_submit';
+
   return system_settings_form($form);
...
+ * @see system_settings_form()

As also explained in the guidelines, we need to use system_config_form() now:
http://drupal.org/node/1667896

+++ b/core/modules/user/lib/Drupal/user/Tests/UserRegistrationTest.php
@@ -145,8 +145,11 @@ class UserRegistrationTest extends WebTestBase {
+    $config = config('system.regional');
+    $config

+++ b/core/modules/user/lib/Drupal/user/Tests/UserTimeZoneTest.php
@@ -26,8 +26,11 @@ class UserTimeZoneTest extends WebTestBase {
+    $config = config('system.regional');
+    $config

The separate $config variable looks unnecessary in these cases.

sun’s picture

Issue tags: +API change

All config system conversions are API changes, so tagging as such.

aspilicious’s picture

What about

default_country
first_day
timezone.default
timezone.configurable
timezone.empty_message
timezone.user_default

I can't see any update function. Did I missed it in my review?

sun’s picture

After reading what this setting is actually for, timezone.empty_message sounds a bit misleading:

+++ b/core/modules/system/system.admin.inc
@@ -1969,14 +1970,15 @@ function system_regional_settings() {
   $form['timezone']['configurable_timezones'] = array(
     '#type' => 'checkbox',
     '#title' => t('Users may set their own time zone.'),

@@ -1996,14 +1998,14 @@ function system_regional_settings() {
     '#type' => 'checkbox',
     '#title' => t('Remind users at login if their time zone is not set.'),
-    '#default_value' => variable_get('empty_timezone_message', 0),
+    '#default_value' => $config->get('empty_timezone_message'),
     '#description' => t('Only applied if users may set their own time zone.')
...
     '#type' => 'radios',
     '#title' => t('Time zone for new users'),
-    '#default_value' => variable_get('user_default_timezone', DRUPAL_USER_TIMEZONE_DEFAULT),
+    '#default_value' => $config->get('user_default_timezone'),
     '#options' => array(
       DRUPAL_USER_TIMEZONE_DEFAULT => t('Default time zone.'),
       DRUPAL_USER_TIMEZONE_EMPTY   => t('Empty time zone.'),

How about this instead?

country.default
date.first_weekday
timezone.default
timezone.user.configurable
timezone.user.default
timezone.user.empty_warning
alexpott’s picture

How about timezone.user.not_set_warning? Since a user doesn't have an "empty" timezone - they haven't set it :)

cosmicdreams’s picture

Looks like this is still assigned to me. I'll try to incorporate the above feedback on this tonight.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
22.07 KB

Untested and the weekday stuff needs to be updated

Status: Needs review » Needs work

The last submitted patch, drupal-change_to_config_for_tz-1571632-35.patch, failed testing.

aspilicious’s picture

aspilicious’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +API change, +Configuration system, +Config novice

The last submitted patch, drupal-change_to_config_for_tz-1571632-35.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
22.07 KB

Should fix th installer

aspilicious’s picture

+++ b/core/modules/system/system.admin.incundefined
@@ -1967,8 +1968,8 @@ function system_regional_settings() {
+    '#options' => array('Sunday' => t('Sunday'), 'Monday' => t('Monday'), 'Tuesday' => t('Tuesday'), 'Wednesday' => t('Wednesday'), 'Thrusday' => t('Thursday'), 'Friday' => t('Friday'), 'Saturday' => t('Saturday')),

I changed this to use usable names in the config file: needs review and an extra update step.

28 days to next Drupal core point release.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.admin.incundefined
@@ -1944,8 +1944,9 @@ function system_rss_feeds_settings_submit($form, &$form_state) {
  * @see system_settings_form()
  * @see system_regional_settings_submit()
  */
-function system_regional_settings() {
+function system_regional_settings($form, &$form_state) {

Need to remove @see system_settings_submit()

Apart from that nitpick - looks great!

alexpott’s picture

Ah... doesn't look so great...

+++ b/core/modules/system/system.admin.incundefined
@@ -1967,8 +1968,8 @@ function system_regional_settings() {
+    '#options' => array('Sunday' => t('Sunday'), 'Monday' => t('Monday'), 'Tuesday' => t('Tuesday'), 'Wednesday' => t('Wednesday'), 'Thrusday' => t('Thursday'), 'Friday' => t('Friday'), 'Saturday' => t('Saturday')),

Typo: Thrusday

aspilicious’s picture

When config files are translatable we just could use the name :)
But as far I can remember they aren't at the moment

alexpott’s picture

@aspilicious: I like the change to have meaningful values in system.regional:date.first_weekday - readable and meaningful config files is an aim of CMI

alexpott’s picture

I don't think system.regional:date.first_weekday will be translated as that will result in some really interesting effects when you change the configuration language (whatever changing the configuration language actually means).

However, I think this looks nice :)

t(config('system.regional')->get('date.first_weekday'));
aspilicious’s picture

I think that would screw up forms that have automagicly config setting/saving in the future (like the good old system_settings_form)

alexpott’s picture

Ha... didn't mean it like that :)

I meant with your change something along the lines of:

  drupal_set_message(
    t("The site's first day of the week is set to: %first_weekday",
    t(config('system.regional')->get('date.first_weekday')))
  );

is much easier. :)

sun’s picture

+++ b/core/modules/locale/locale.module
@@ -438,7 +438,7 @@ function locale_library_info_alter(&$libraries, $module) {
           'ui' => array(
             'datepicker' => array(
...
-              'firstDay' => variable_get('date_first_day', 0),
+              'firstDay' => config('system.regional')->get('date.first_weekday'),

+++ b/core/modules/system/system.admin.inc
@@ -1967,8 +1968,8 @@ function system_regional_settings() {
-    '#options' => array(0 => t('Sunday'), 1 => t('Monday'), 2 => t('Tuesday'), 3 => t('Wednesday'), 4 => t('Thursday'), 5 => t('Friday'), 6 => t('Saturday')),
...
+    '#options' => array('Sunday' => t('Sunday'), 'Monday' => t('Monday'), 'Tuesday' => t('Tuesday'), 'Wednesday' => t('Wednesday'), 'Thrusday' => t('Thursday'), 'Friday' => t('Friday'), 'Saturday' => t('Saturday')),

Let's revert that change to date.first_weekday. The value is used directly and passed to other libraries and functions. It is actually very common for both native PHP and JS functions (as well as *nix in general) to express the first weekday as an integer, ranging from 0 to 6.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
21.93 KB
sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/config/system.regional.yml
@@ -0,0 +1,10 @@
+        not_set_warning: '0'

+++ b/core/modules/system/system.module
@@ -2241,8 +2243,9 @@ function system_user_presave($account) {
 function system_user_login(&$edit, $account) {
+  $config = config('system.regional');
   // If the user has a NULL time zone, notify them to set a time zone.
-  if (!$account->timezone && variable_get('configurable_timezones', 1) && variable_get('empty_timezone_message', 0)) {
+  if (!$account->timezone && $config->get('timezone.user.configurable') && $config->get('timezone.user.not_set_warning')) {
     drupal_set_message(t('Configure your <a href="@user-edit">account time zone setting</a>.', array('@user-edit' => url("user/$account->uid/edit", array('query' => drupal_get_destination(), 'fragment' => 'edit-timezone')))));
   }

Not fully happy with the new timezone.user.not_set_warning key yet. It is quite verbose and somehow does not seem to be very concise and self-descriptive.

Let's try to find a better one.

E.g.:

timezone.user.warn_unset
timezone.user.unset_warning
timezone.user.warn_empty
timezone.user.verify_non_empty
timezone.user.verify_custom
timezone.user.verify

There might be much better ones! But out of those, the last looks most appealing, as it keeps the actual/exact implementation (a message) out of the config key and just tells the system "You shall verify user timezones are set."

aspilicious’s picture

I prefer
timezone.user.warn_unset

alexpott’s picture

How about just
timezone.user.warn or timezone.user.warning

And then if in the future there are other warnings
timezone.user.warn.unset and timezone.user.warn.somecondition

aspilicious’s picture

Status: Needs work » Needs review
FileSize
21.87 KB

I'm into #53 lets see what others think

aspilicious’s picture

disasm’s picture

Here's a reroll, fixing some conflicts in doc tags as well as update hook numbers.

I looked over the patch and it looks really good. I ran update.php, which ran as expected from a fresh drupal install. I ran git grep on all the variables you were replacing and I didn't see any that were missed.

Since I rerolled the patch, I don't feel comfortable marking this RTBC, so lets get another person to review it and verify I didn't break anything in my reroll.

cosmicdreams’s picture

BrockBoland’s picture

Needs issue summary

justafish’s picture

Patch looks good. I've read through it, applied it and all seems well. I did notice a stray variable_set in OpenIDRegistrationTest, although it doesn't seem to actually have been doing anything in the first place.

Status: Needs review » Needs work

The last submitted patch, drupal-change_to_config_for_tz-1571632-59.patch, failed testing.

justafish’s picture

Bumping up system update hook number.

KarenS’s picture

This is a nit, but why are we re-naming first_day to first_weekday? The original value is perfectly understandable and that name has been used as long as I have been using Drupal. This just becomes a needless change for all other modules that use this value. If it is going to be changed, the change that maps better to the PHP date functions would be to call it first_day_of_week or first_dow, rather than to arbitrarily create a new and different name for that value.

I also agree that the keys for the days of the weeks need to retain the numeric values, which map to values used by PHP date functions for those days of the week, so I'm glad to see that change got pulled back out.

justafish’s picture

Status: Needs work » Needs review
FileSize
3.07 KB
20.53 KB

Agreed. Patch changes first_weekday to first_day

aspilicious’s picture

Status: Needs review » Needs work

If first_weekday is a *bad idea* it should be first_day_of_week OR first_dow (for the reasons mentioned in #62)
But first_dow is not ok (its to cryptic). So lets go with first_day_of_week if we need to change it.

KarenS’s picture

Why change it at all? First_day is fine.

sun’s picture

Status: Needs work » Needs review

I'm also fine with date.first_day.

Don't have time to review the full patch right now, but will do so later, unless others beat me to it. ;)

aspilicious’s picture

I think the system update number needs to go up again.

aspilicious’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Novice, +API change, +Configuration system, +Config novice

The last submitted patch, rename-first_weekday-to-first_day-1571632-63.patch, failed testing.

justafish’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
20.48 KB

It should just need rerolling as stuff around it has changed.

justafish’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -Novice, -API change, -Configuration system, -Config novice

The last submitted patch, rename-first_weekday-to-first_day-1571632-70.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Novice, +API change, +Configuration system, +Config novice

The last submitted patch, rename-first_weekday-to-first_day-1571632-70.patch, failed testing.

justafish’s picture

Status: Needs work » Needs review
FileSize
2.37 KB
21.17 KB

Let me paint this moving car some more.. ;)

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

sun’s picture

Thanks @justafish!

I reviewed it once more and only see a few minor issues, which we can hopefully fix very quick:

+++ b/core/includes/bootstrap.inc
@@ -2162,13 +2162,15 @@ function drupal_bootstrap($phase = NULL, $new_phase = TRUE) {
 function drupal_get_user_timezone() {
...
   else {
     // Ignore PHP strict notice if time zone has not yet been set in the php.ini
     // configuration.
-    return variable_get('date_default_timezone', @date_default_timezone_get());
+    $config_data_default_timezone = $config->get('timezone.default');
+    return isset($config_data_default_timezone) ? $config_data_default_timezone : @date_default_timezone_get();
   }

I actually wonder whether

1) we shouldn't rely on a system.regional:timezone.default being set (during installation) and remove the default value handling entirely here,

and/or

2) plain simply default to 'UTC'

However, that's a functional change, so let's better do that in a separate issue. Does anyone want to take that on? :) [if so, please link to the new issue from here]

+++ b/core/modules/system/lib/Drupal/system/Tests/System/DateTimeTest.php
@@ -44,8 +44,11 @@ class DateTimeTest extends WebTestBase {
   function testTimeZoneHandling() {
...
+    $config = config('system.regional');
+    $config

+++ b/core/modules/user/lib/Drupal/user/Tests/UserTimeZoneTest.php
@@ -26,8 +26,11 @@ class UserTimeZoneTest extends WebTestBase {
   function testUserTimeZone() {
...
+    $config = config('system.regional');
+    $config

The second $config line is unnecessary here, because the methods on the Config object are chainable (except for the ->get() method, which obviously returns a value) - so this works just fine:

$config = config('foo.bar')
  ->set('bar', 'beer')
  ->save();

$config->get('bar'); // hands 'beer'
+++ b/core/modules/user/lib/Drupal/user/Tests/UserRegistrationTest.php
@@ -149,17 +149,20 @@ class UserRegistrationTest extends WebTestBase {
   function testRegistrationDefaultValues() {
-    $config = config('user.settings');
+    $config_user_settings = config('user.settings');
     // Don't require e-mail verification and allow registration by site visitors
     // without administrator approval.
-    $config
+    $config_user_settings
       ->set('verify_mail', FALSE)
...
+    $config_system_regional = config('system.regional');
     // Set the default timezone to Brussels.
-    variable_set('configurable_timezones', 1);
-    variable_set('date_default_timezone', 'Europe/Brussels');
+    $config_system_regional = config('system.regional')
+      ->set('timezone.user.configurable', 1)

Same as above, but here we can replace each second line with the first (so that the comments appear above).

justafish’s picture

Status: Needs work » Needs review
FileSize
2.5 KB
21.06 KB

date_default_timezone_get is going to return UTC as a last resort anyway and setting the timezone could have been disabled in an installation profile.

Patch incorporates other suggestions.

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -Novice, -API change, -Configuration system, -Config novice

The last submitted patch, drupal-change_to_config_for_tz-1571632-78.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +Novice, +API change, +Configuration system, +Config novice
KarenS’s picture

There is a parallel discussion in #1708542: Convert systems date and time admin form to configuration system about other date configuration. I'm really wondering if it makes sense to move all the date-related configuration into its own namespace ('system.date' rather than 'system.regional'). So timezone and first day would become 'system.date.timezone' and 'system.date.first_day'. When combined with other date configuration like, like date format settings and options, you could create a config object using the system.date prefix and get everything that is date-related. The separation between 'regional' and 'date' that we have now is only because of where they are placed in the configuration form.

On the fallback for the timezone. The original reason for the way this is handled is that we wanted to be sure a timezone got set. If we default it to 'UTC' we don't know if it is set that way on purpose or because it wasn't set at all. If it is left empty, we can create warning messages that things won't work right until it is set. If it defaults to 'UTC', the user may not realize what happened so it can be fixed. There may be other ways to fix the problems, but that's the rational for not automatically setting the timezone.

I'm wondering about creating a meta issue about date/time configuration to encompass all these issues but still keep the separate issues moving forward.

KarenS’s picture

I created a META issue about date/time configuration at #1763754: [meta] Date and Time configuration

justafish’s picture

Seems reasonable to me and no one has voiced any objections.

sun’s picture

Status: Needs review » Postponed

Thanks for keeping this alive! :)

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

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

cosmicdreams’s picture

FileSize
55.15 KB

Still incomplete, but here's the result's of tonight's work. This is currently broken:

Also, this issue still may be postponed due to #1616594: META: Implement multilingual CMI

Lars Toomre’s picture

Just a quick note... recognizing this was not complete.

+++ b/core/includes/common.incundefined
@@ -1918,58 +1918,14 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
+  if ($type != 'custom') {

Maybe use $config = config('system.date') here so two config objects never initiated?

cosmicdreams’s picture

Thanks for the call out Lars, I'll make sure I don't forget that.

For now, I'm leaving format_date for last as the work cafuego is thinking about could have a larger impact on that function than my work. See the current discussion here: #1733316: Switch to IntlDateFormatter in format_date()

cosmicdreams’s picture

After some clean up and only addressing some of the tests I was able to get to this patch.

Unfortunately is this still incomplete as a result of not having a way forward yet with handling multilingual config files. But once that's available this work can be continued. In the end we'll be able to persist all of the information we need about date formats without the use of tables.

cosmicdreams’s picture

Status: Postponed » Needs review
FileSize
66.09 KB

Forgot the patch, here it is. And just for fun, let's have the tests run.

Status: Needs review » Needs work

The last submitted patch, 1571632_89.patch, failed testing.

cosmicdreams’s picture

Wow, only 64 fails. I expected more.

chx’s picture

Assigned: cosmicdreams » chx

As per IRC discussion I will finish this. Thanks for the work so far.

KarenS’s picture

OK, here are some of the things that are broken:

- Minor, we lost the javascript that previews the format you are choosing when you add a new format in the date/time administration screen. A small thing, but very helpful since date format strings are not very user-friendly.
- Major, format_date() is totally broken since it still expects a format type. That in turn leave us with a blank value for the $submitted function on nodes, etc. This probably broke a ton of tests.
- Major, we have lost the ability for the administrator to control the format of the created date on the node. We now have no way to choose how to display the created date.

Other notes:
- The only place core really uses the date format type is in the $submitted string. There is no way to see the real impact of this change using only core. It will show up more obviously when using Views and Date modules or when writing custom code.
- I can't tell how localization will work with this change.

KarenS’s picture

We are getting no date formats because config('system.date')->get('format.medium') doesn't produce anything. The yaml file is there, but it uses a pattern like 'formats: system_medium'.

So config('system.date')->get('format.medium') is used all over the place in this patch, but it looks like it needs to be config('system.date')->get('formats.system_medium.pattern') instead. Which is a mouthful, but seems to work better.

cosmicdreams’s picture

Thanks for the review Karen. This work is still on my radar. I think I'll have time tonight to dive back in on fixes. This is likely what's causing so many errors.

In the previous work, I had left format_date alone since cafuego's work to use IntlDateFormatter would bring about more change that my work does. I should stop procrastinating on that work, since it's needed for this patch to succeed.

KarenS’s picture

OK, I got rid of a bunch of the test failures. There is still more to do :)

KarenS’s picture

Status: Needs work » Needs review

Setting to needs review just to see how badly it fails now. I tried running all the tests locally but ran out of memory.

Status: Needs review » Needs work

The last submitted patch, 1571632_date_format_changes.patch, failed testing.

KarenS’s picture

Status: Needs work » Needs review

And just to summarize the main differences between 89 and 96:

- I changed all the references to config('system.date')->get('format.medium') to config('system.date')->get('formats.system_medium.pattern'), as noted above.
- I found one of the problems that is keeping the javascript helper from working (the element name changed), but that wasn't enough to get it working again.
- I fixed some of the broken tests. I removed the test for format types and altered the test for formats to match the new screens, and fixed other tests to use the right new config values.

Lars Toomre’s picture

Status: Needs review » Needs work

I was just reading through your work in progress @KarenS. I am now understanding more about how this new config system is supposed to work.

As you proceed, I would ask that you continue to add type hinting to the @param/@return directives in all of the docblocks you touch. The other request would be to remove t() around assertion text messages which is an on-going process with all core patches. Thanks.

Lars Toomre’s picture

Status: Needs work » Needs review

Sorry did not mean to change status.

chx’s picture

FileSize
70.34 KB

Rerolled against HEAD.

Status: Needs review » Needs work

The last submitted patch, 1571632_102.patch, failed testing.

chx’s picture

Assigned: chx » Unassigned
Status: Needs work » Needs review
FileSize
2.49 KB
70.56 KB

Maybe this has fewer exceptions. Very likely confg('locale.config.' . $language->langcode . '.system.date') needs a wrapper in locale.module.

I am unassigning only because it seems others are making awesome progress as well and I do not want to stop anyone. I am just trying to help a little.

Status: Needs review » Needs work

The last submitted patch, 1571632_104.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
70.56 KB

Status: Needs review » Needs work

The last submitted patch, 1571632_104.patch, failed testing.

KarenS’s picture

Yikes! 88 fails. I guess this shows how widely-used date formats are. And it also shows that I don't know how to get a new file in a patch, because the yaml file is missing from that patch :)

Some more things we need to fix (in addition to the test failures):

- We need to add a 'locked' option to the yaml file, then mark the html_ formats as locked. These should not be displayed as formats that can be edited.
- We need to make the format edit form more user-friendly, probably by providing a drop-down list of options to select from, along with 'other' or 'custom', where 'custom' gives you the textfield where you can type in a custom format. I don't want to attempt that change until we figure out how to fix the javascript that should be displaying your format as you type. We might have one more yaml item for the values in the option list (which we can grab from the current code). By making that a configuration item, other modules could add other options to the list of format suggestions.
- We should add more descriptive text to the format list page to explain how these are being used.
- We should document what goes in the yaml file -- how is this being handled elsewhere?
- We need to add more to the upgrade path, we need to move any custom formats in the formats table into configuration. Currently there is only an upgrade hook for the variables.

chx’s picture

Status: Needs work » Needs review
FileSize
71 KB

Ah yes it's not system.install, it's a new yaml file. I just run git add core/modules/system/config/system.date.yml and it doesn't matter whether i commit or not whether i branch or not because I roll all my files with git diff 8.x > patchfile and so as long as git is aware it'll do the right thing. Handy.

And yes, 88 fails :( I am just uploading this , hoping for less exceptions.

Status: Needs review » Needs work

The last submitted patch, 1571632_109.patch, failed testing.

KarenS’s picture

Status: Needs work » Needs review
FileSize
14.78 KB
78.12 KB

Made some more changes and this time hopefully I actually got the yml file in the patch.

- I added the 'locked' option to the yml file and excluded locked formats from the format list.
- I removed a bunch of code in the api that refers to the format functions and hooks, because none of that works like this any more.
- I had trouble with the localization changes made by chx when trying to spin up a new site, so I just commented that part out for now and return the regular format list.
- I had trouble on a new installation with the default timezone, since it is set to empty by the yml file, but it tries to create a timezone from an empty value, which you can't do. So I changed that a bit.
- chx added an install hook to create the formats, but that isn't needed when the yml file is provided.

I'm curious how much progress has been made on passing tests, so I'm going to try to test this patch.

KarenS’s picture

My interdiff is between my patch and the one in 106, because I uploaded my patch without realizing chx had made a new patch :)

Status: Needs review » Needs work

The last submitted patch, 1571632_109.patch, failed testing.

KarenS’s picture

Down to 20 fails, and over half of them are related to localization, which we know isn't working, so it's getting close :)

I have to pull away and do client work now. If anyone else wants to dig into this, you're welcome to push it along. Otherwise I'll try to look at it again tonight.

Still to do: fix the remaining failures (perhaps by commenting out localization tests until localization is working), and try to do the rest of the things listed in #108.

KarenS’s picture

Issue tags: -Novice, -Config novice

Also, it turns out this issue is absolutely not a novice issue :)

chx’s picture

Karen, why localization isnt working? Isn't that what core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php is about?

KarenS’s picture

The localized date formats have not been converted to CMI yet. That is waiting on patches like #1648930: Introduce configuration schema and use for translation. At the same time, we have destroyed the old system. So all the localization tests for date formats will fail.

chx’s picture

I would like to do a straight port and not wait on huge issues like that. If the old system worked then we can port it without changes like that.

marcingy’s picture

Assigned: Unassigned » marcingy
KarenS’s picture

There is a discussion going on at #1733316: Switch to IntlDateFormatter in format_date() about the benefits of using the new IntlDateFormatter. That affects this patch because it affects the date format strings. The IntlDateFormatter uses a different format string than the regular PHP format strings we use now. We probably cannot completely switch to that because it's not available widely enough, so we may need to support either possibility.

By making a small change I think we can do that. Either

formats:
  system_long:
    name: "Default Long Date"
    pattern: 'l, F j, Y - H:i'
    type: 'php'
    locked: 0
  system_medium:
    name: "Default Medium Date"
    pattern: 'D, m/d/Y - H:i'
    type: 'php'
    locked: 0
  system_short:
    name: "Default Short Date"
    pattern: 'm/d/Y - H:i'
    type: 'php'
    locked: 0

or

formats:
  system_long:
    name: "Default Long Date"
    pattern: 
      php: 'l, F j, Y - H:i'
      intl: 'yyyy-MM-dd\'T\'kk:mm:ssZZ"'
    locked: 0
  system_medium:
    name: "Default Medium Date"
    pattern:
      php: 'D, m/d/Y - H:i'
      intl: FULL
    locked: 0
  system_short:
    name: "Default Short Date"
    pattern: 
      php: 'm/d/Y - H:i'
      intl: 'yyyy-MM-dd kk:mm'
    locked: 0

As to the question about why localized formats are broken, my understanding is that there will be a separate patch to transform the localized formats to config because they require much more complex logic. That being the case we probably should just alter this patch to remove the remaining vestiges of the localized formats because they won't work any more, and let that patch add them back.

cosmicdreams’s picture

I had a chance to pop into Gabor's initiative meeting and found out that work is nearing completion on being able to use localized config. I proposed that we could get all the non-localization tests to pass for now and then when that feature is available add in the additional syntax that will allow us to use localized config, then make the remainder of the tests pass. Gabor said that sounded like a good plan for now.

A few questions:

What is the locked config setting for? Is it necessary?

My personal preference is to favor the first set of config, as I don't know if there is a use case for having one environment (dev, staging, etc) having the intl library enabled and the other environment (production) not have the intl library enabled.

If we wanted to optimize our config for such a circumstance, then I would prefer the second. That would allow us to associate both format strings to one date_format pattern. A poor-man's 1-to-many relation for patterns, keyed by type.

KarenS’s picture

The reason for 'locked' is explained in http://drupal.org/node/1571632#comment-6518110, but maybe that needs more clarification. The current code does not display all the 'html_' formats in the UI because they are not configurable and are confusing. The 'locked' value tells the UI not to display them.

cosmicdreams’s picture

forgive the needless bump. I just want folks to know that I plan on working on this issue tonight. (9 hours from now)

My goals:

  1. Implement #120's plan B
  2. Get as many tests to pass as I can
marcingy’s picture

Assigned: marcingy » Unassigned
KarenS’s picture

I agree with the idea of implementing #120 B. That's the only way core can provide default values for both types of systems when we don't know how widely available the IntlDateFormatter is.

cosmicdreams’s picture

As discussed on IRC, I'm assigning this issue to marcingy so that work can conclude on all config values that aren't related to date formats. I've also created #1808246: Convert internal handling of date formats to CMI so that the work we are doing to change internal handling of date formats does not hold up this issue.

Edit: it appears I cannot assign this to someone other than myself (duh). marcingy feel free to push this forward.

marcingy’s picture

So my interest in this patch was related to killing the date format tables, I can easily take care of the stuff in the variable table but that has a clear migration path. The dates side of the patch is the part that really needs to be moved a long and the part that needs to being as simple of port as possible.

cosmicdreams’s picture

That's excellent. Please push on. I'm attempting a larger impact patch because I see date types as unnecessary now. Any work you do in your straight port will likely attempt to resolve a lot of the same tests breakages that we're currently facing.

So even though our plans are different I see the work you're planning to do as mutually beneficial. Please press on.

marcingy’s picture

So why not focus your efforts here rather that diluting efforts. There really is no point in me working on a patch to deal with date formats if there is a second patch doing exactly the same thing that is duplication of effort. The issue you are working on should be a follow up to this one and marked as postponed for now.

cosmicdreams’s picture

My main point in my previous responses holds true, I'm not in your way. If you have patches you want to upload that represent your ideas, please upload them. We can further our discussion with code.

However, please do not try to delay me. Have faith that I will be reasonable when negotiating a proper solution to this issue. I'm currently pursuing a solution that removes date types in addition to the date_types table and the date_formats table. To me the change in the data model is a natural consequence of how our new architecture supports this feature.

But my mind is open to alternative solutions. Let's take a look at your solution and talk this out.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
77.04 KB

here's the latest work for you to review

Status: Needs review » Needs work

The last submitted patch, 1571632_131.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
79.55 KB

This should clean up a number of the exceptions

I found that the arguments for system_date_format_save were reversed
also there were some syntactical errors

Status: Needs review » Needs work

The last submitted patch, 1571632_133.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
99.07 KB

Ok new version which passes all date tests locally, hopefully I haven't broken anything else where.

This adds but in localization of a form although it might be a bit rough around the edges.

Note this does not tweak the formatting as per #120 yet but that should be a simple task once we have the basics in place.

Status: Needs review » Needs work

The last submitted patch, 1571632_134.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review

Great work, I hope it turns out green.

Reading through this patch reminds me of a question I was too tired to ask last night. Should we revise the logic where we check to see if a date format has a locale property?

It seems that the conditional logic we have that accesses that property will always be false

marcingy’s picture

#135: 1571632_134.patch queued for re-testing.

cosmicdreams’s picture

Status: Needs review » Needs work
-- lots of file usage stuff cut out for issue readibility

Yea, all of this file stuff, I don't know why this is here. Can you please explain what this is used for?

marcingy’s picture

Status: Needs review » Needs work
FileSize
88.19 KB

Doh. Stray elements killed

marcingy’s picture

Status: Needs work » Needs review
cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
88.8 KB

Here's a patch that builds off of 140 and implements both the data model from 120b and a helper function to retrieve the appropriate date format pattern based on whether they have the intl library enabled or not.

Status: Needs review » Needs work

The last submitted patch, 1571632_142.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
89.21 KB

removed the intl formats, kept the php formats, and changed the code to use the php formats. Let's see what I missed.

Status: Needs review » Needs work

The last submitted patch, 1571632_144.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
89.21 KB

Ran this locally and in at least the CommerPreview's case the exceptions and the errors have been resolved.

* Fixed the name of the out of date update function in system.install.

marcingy’s picture

Status: Needs review » Needs work

Setting to need work as I now understand how the current localisation system works with regards determining if a localised date format exists or not. That will allow use to load into config as per now and avoid the extra call to config in format_date (). Note to self this is hook_init() in system.

marcingy’s picture

Assigned: Unassigned » marcingy

Grabbing this back, I am busy the next 2 nights but will get a chance to look at this on Thursday, and I have a plan of attack.

Couple of things that need done
* Fix the test regressions in #146.
* Make sure the GUI itself doesn't show up any errors beyond those in the tests, if it does add more tests for coverage (likely initially as an @todo)
* Work on removing the peformance regression that the current patch introduces.
** Revert change in format date
** Fix up logic in system_init so the variables continue to be overriden at a $config level which will allow use to reduce the level of queries and check once per page (as d7 does) if localisation is in play.

chx’s picture

If you need overrides on $config beyond what locale does, I heartily recommend writing a subscriber for it. Check drupal_container and core/lib/Drupal/Core/EventSubscriber/ConfigGlobalOverrideSubscriber.php

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
89.9 KB

Here's the most recent patch. I manually went through these pages so I can find what was going wrong.

marcingy’s picture

Status: Needs review » Needs work

Back to needs work because there is still stuff that needs done with this patch.

marcingy’s picture

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
89.94 KB

I've changed the link title from "edit" to "configure" in order to be more consistent with the other link I've seen. Oh and I'm using dropbuttons too.

I've run the DateTimeTest locally and it passed. Let's see if Testbot agrees.

marcingy’s picture

Please don't add additional features or change strings those should be follow ups except where absolutely necessary - lets keep this change as small in scope as possible. I think the simple answer is to start back from #140 which was green and passed that is going to be my approach on Thursday.

cosmicdreams’s picture

Hello marcingy,

The last patch I uploaded is likely to pass the tests. Either you or I could revert the change of the name of the link pretty quickly. I'm ready for code reviews if you have the time.

cosmicdreams’s picture

FileSize
89.89 KB

This patch reverts the name change of the "edit" link. It now says edit again. We should keep the dropbutton though.

marcingy’s picture

Status: Needs review » Needs work

Cosmicdreams the patch is not ready for reviews yet in my opinion it needs work as per the list #148, one of which items has been ticked off as I say but the patch most definently lacks test coverage (I fixed a typo that passed all tests in the #140 patch) and needs deal with localisation a more performant way. I spoke to chx today and between us we have a plan to go forwards after he gave me some guadiance about how a a d8 solution should look.

And we should have to be reverting stuff we should simply not be adding scope creep in the first place.

cosmicdreams’s picture

When I mark this issues as needs review I do that because I'm trying to get the testbot to test the patches I'm uploading.

marcingy’s picture

Yup, my comment was based on

I'm ready for code reviews if you have the time.

cosmicdreams’s picture

and my reason for saying that is to point out that if you have time to review my code and let me know what you think I'm doing wrong, then please provide that criticism. Otherwise I'll leave you to this.

chx’s picture

> I've changed the link title from "edit" to "configure" in order to be more consistent with the other link I've seen. Oh and I'm using dropbuttons too.

Please don't. We need to stay focused to get the first example of actual multilingual CMI objects into core as soon as possible. A straight port is key. I would, however, welcome if you'd file followups and link them from the issue summary. Also, copying 120B into the issue summary with explaining why (intl -- not core but contrib -- but still leave space for it) would be helpfiul. Let marcingy finish this -- he has a solid roadmap.

cosmicdreams’s picture

@chx & marcingy: That sounds good to me. Let me know when you're looking for a code review / help with getting stuff to pass tests.

Also, you'll see in my most recent patch that I reverted the change to the name of the administrative link.

marcingy’s picture

Status: Needs work » Needs review
FileSize
91.25 KB

Ok new version with localisation working as it should.

cosmicdreams’s picture

FileSize
11.88 KB

Here's an interdiff of 156 and 163, mostly for me to help the review of this go easier.

cosmicdreams’s picture

small code quibbles and documentation critiques

+++ b/core/modules/system/system.moduleundefined
@@ -2242,24 +2242,6 @@ function system_filetransfer_info() {
-  global $conf;
-
-  $language_interface = language(LANGUAGE_TYPE_INTERFACE);
-  // For each date type (e.g. long, short), get the localized date format
-  // for the user's current language and override the default setting for it
-  // in $conf. This should happen on all pages except the date and time formats
-  // settings page, where we want to display the site default and not the
-  // localized version.
-  if (strpos(current_path(), 'admin/config/regional/date-time/formats') !== 0) {
-    $languages = array($language_interface->langcode);
-
-    // Setup appropriate date formats for this locale.
-    $formats = system_get_localized_date_format($languages);
-    foreach ($formats as $format_type => $format) {
-      $conf[$format_type] = $format;
-    }
-  }

+1 for simplifying system_init and not having to load language interface here.

+++ b/core/modules/system/system.moduleundefined
@@ -2303,40 +2285,22 @@ function system_init() {
+      foreach ($date_formats as $dfid => $data) {
+        // If a localized format exists for this language, use it.
+        $formats[$dfid] = $date_formats[$dfid];

I think this can be simplified even further.

The logic here seems to be saying that if we find that we have configuration for date_formats stored for a specific language then return that. Am I wrong?

+++ b/core/modules/system/system.moduleundefined
@@ -2303,40 +2285,22 @@ function system_init() {
+      break;

This break seems to say that we'll stop the foreach once we have a non-empty $date_format.

We should document that better.

+++ b/core/modules/system/system.moduleundefined
@@ -3666,33 +3630,30 @@ function system_get_date_formats($dfid = NULL) {
 function system_get_date_format_pattern($format_info) {

If this is how we want to code this function, then we should call the parameter $pattern instead of $format_info, because it's not all of the information of a pattern.

marcingy’s picture

Also after seeing #1817748: /admin/config/regional/date-time does not show the correct default format for short and medium format I have a feeling there are some places that use format_date where strings for type have not been updated appropriately. Adding to my list of items to check as part of re-roll for review above.

marcingy’s picture

FileSize
106.9 KB

Re-roll
* Updates format_date calls that we missed before
* Some code simplification as per review in #165.

aspilicious’s picture

Status: Needs review » Needs work
Issue tags: +Needs screenshots
+++ b/core/modules/system/config/system.date.ymlundefined
@@ -0,0 +1,60 @@
+    name: "HTML Date"

Here you use double quotes

+++ b/core/modules/system/config/system.date.ymlundefined
@@ -0,0 +1,60 @@
+    name: 'HTML Time'

Here single ones. Use always single ones in these cases.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/DateTimeTest.phpundefined
@@ -136,80 +108,37 @@ function testDateFormatConfiguration() {
+    ¶

Trailing whitespace

+++ b/core/modules/system/system.admin.incundefined
@@ -2659,16 +2544,19 @@ function theme_system_themes_page($variables) {
+ * @param string $dfid

Can we make this human readable. In general we are moving away from this kinda names. Why not simple use $date_format_id. Same as we want to change $pid to $parent_id in other places.

+++ b/core/modules/system/system.moduleundefined
@@ -3735,73 +3602,50 @@ function system_date_formats() {
+ * Get the appropriate date format pattern type.

GetS

+++ b/core/modules/system/system.moduleundefined
@@ -3845,244 +3682,39 @@ function system_date_format_locale($langcode = NULL, $type = NULL) {
+  ¶

trailing whitespace

This also needs a screenshot from the new UI (with the machina name component).
I also think we don't have any tests for the date format UI. So a simple tests that create a date format in the UI should be sufficient.

AND we don't have any upgrade tests. A small tests that checks if a date format is still present with the correct settings would be wonderfull.

marcingy’s picture

Status: Needs work » Needs review
FileSize
109.94 KB

Comments above fixed up including the rename of the variable.

I have been looking at the moment we don't actually seem to have full upgrade path for custom date formats or formats that are already localised so the upgrade path needs some further love.

I also think we don't have any tests for the date format UI. So a simple tests that create a date format in the UI should be sufficient.

We do as I had to create a format for localisation test.

I'll get a screenshot together later.

Also adds a test to make sure that reseting localisations reverts dates back to default.

Status: Needs review » Needs work

The last submitted patch, 1571632-170.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
109.95 KB

Wrong key name in my simplified code.

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -Needs screenshots, -API change, -Configuration system

The last submitted patch, 1571632-172.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +Needs screenshots, +API change, +Configuration system

#171: 1571632-172.patch queued for re-testing.

Gábor Hojtsy’s picture

I was asked to review this patch. On a cursory review, I did not find anything wrong. I also read the issue summary, and there was no indication of features lost or added. Is that true?

cosmicdreams’s picture

"No features lost or added"
Not completely the case. One feature that's added is that we're using CMI to store the configuration for data formats. As a result, we're getting rid of the tables that previously handled that task. That's a pretty important feature as developers can now modify a configuration file when they want to provide new date formats.

We also provide a place where future work can provide custom date format patterns (specifically thinking about IntlDateFormatter). We namespace the current date format patterns to php so that we can store multiple patterns for the same format. For example:

formats:
  system_short:
    name: 'Default Short Date'
    pattern:
      php: 'm/d/Y - H:i'
    locked: 0

and we provide system_get_date_format_pattern to allow the system-driven logic to retrieve the right pattern. This allows us to rework that function and this config file in the future if we want to support IntlDateFormatter.

Also, if it's still in the patch, I modified the admin pages for date formats to use the new dropbutton.

Gábor Hojtsy’s picture

@cosmicdreams: sounds like it would be best to update the issue summary for the benefit of reviewers then :) Looks like it was not kept up to date.

cosmicdreams’s picture

will do tonight

aspilicious’s picture

#171: 1571632-172.patch queued for re-testing.

aspilicious’s picture

It is possible that views was using one of these variables. Please verify that with a grep. If tests are green and that is the case I'll warn the vdc team. :)

Status: Needs review » Needs work

The last submitted patch, 1571632-172.patch, failed testing.

dawehner’s picture

FileSize
113.31 KB
3.36 KB

Here is a change which adapts the part in views for the new date formats.

cosmicdreams’s picture

Status: Needs work » Needs review

sending to testbot

cosmicdreams’s picture

Issue summary: View changes

Updated issue summary to relect current work and scope

aspilicious’s picture

Can we have screenshot of the adjusted admin pages?
Can we have a *small* upgrade path tests that verifies the settings are converted correctly?

Status: Needs review » Needs work

The last submitted patch, core-1571632-181.patch, failed testing.

cosmicdreams’s picture

Yea dawehner when I reviewed your patch I figured those tests were going to fail. There is no specific "custom" format anymore. Custom formats each get their own machine name and are there for retrieved by that machine name.

We could alter that test to create a new date format and then retrieve it.

@aspilicious
I'll be able to post screenshots soon (perhaps tomorrow night or over the weekend). Anyone can apply this patch and go to the regional pages and do the same.

cosmicdreams’s picture

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/FieldDateTest.phpundefined
@@ -56,10 +56,10 @@ public function testFieldDate() {
+        'custom' => format_date($time, 'system_custom', 'c', $timezone),

custom should not be prefixed with 'system_'

it's still custom' => format_date($time, 'custom', 'c', $timezone)

cosmicdreams’s picture

I'll try to post an updated patch tonight.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
111.16 KB

wow, I totally thought I had reposted the fixed patch. Well here it is for the testbot.

Status: Needs review » Needs work

The last submitted patch, 1571632-188.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
112.56 KB

Cleaned up the dirty reroll, send to testbot

Status: Needs review » Needs work

The last submitted patch, 1571632-190.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
112.56 KB

this fixes the misnamed update function.

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.admin.incundefined
@@ -2806,10 +2695,27 @@ function system_configure_date_formats_form($form, &$form_state, $dfid = 0) {
+  $languages = language_list();
+
+  $options = array();
+  foreach ($languages as $langcode => $data) {
+    $options[$langcode] = $data->name;
+  }
+
+  if (!empty($options)) {
+    $form['date_langcode'] = array(
+      '#title' => t('Select localizations'),
+      '#type' => 'select',
+      '#options' => $options,
+      '#multiple' => TRUE,
+      '#default_value' => empty($format_info['locales']) ? '' : $format_info['locales']
+    );

Don't we have a language selector form item now? (I can be wrong.

+++ b/core/modules/system/system.moduleundefined
@@ -3747,109 +3606,79 @@ function system_date_formats() {
+ * Get the list of defined date formats and attributes.

GetS, check other functions as well

+++ b/core/modules/system/system.moduleundefined
@@ -3747,109 +3606,79 @@ function system_date_formats() {
+ *   given machine name are returned, in a single associative array keyed by ¶

trailing whitespace

+++ b/core/modules/system/system.moduleundefined
@@ -3747,109 +3606,79 @@ function system_date_formats() {
+ * For now this function defaults to php.

Is this going to change? Do we need a todo?

+++ b/core/modules/system/system.moduleundefined
@@ -3747,109 +3606,79 @@ function system_date_formats() {
+ *   If $date_format_id and $langcode are specified, returns the corresponding ¶
+ *   date format string. If only $langcode is specified, returns an array of ¶
+ *   all date format strings for that locale, keyed by the date type. If ¶
+ *   neither is specified.

trailing whitespace

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
112.91 KB

this patch cleans up whitespace issues. Let me address your questions

Don't we have a language selector form item now? (I can be wrong.

If you can point me to it I'll see what it will take to include it and whether that is in scope or not.

Is this going to change? Do we need a todo?

It will if there is time. I'll add a todo here and we can remove that comment when the new functionality lands or when we find we don't have time to add it.

cosmicdreams’s picture

FileSize
3.67 KB

and here's the interdiff (if I did it right)

cosmicdreams’s picture

someone was asking for screenshots for some the date format UI change. Here's a series of screenshots that show the result of this patch

using dropbuttons

Status: Needs review » Needs work

The last submitted patch, 1571632-194-3.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
112.91 KB

Previous change removed the DrupalGet from the test that is needed to kick off the rest the test that broke. Ran the test locally to prove I caught everything. The test ran fine locally.

So here's the patch.

Note: I did get some exceptions but that seems to be related to not being able to delete the directory that the compiled service_container is located in. That shouldn't be related to this patch, just my machine.

aspilicious’s picture

Two minor things. I couldn't find any other issues.

+++ b/core/includes/bootstrap.incundefined
@@ -2172,13 +2172,16 @@ function drupal_bootstrap($phase = NULL, $new_phase = TRUE) {
-    return variable_get('date_default_timezone', @date_default_timezone_get());
+    $config_data_default_timezone = $config->get('timezone.default');
+    return !empty($config_data_default_timezone) ? $config_data_default_timezone : @date_default_timezone_get();

I think this can be rewritten as
return $config->get('timezone.default') ?: : @date_default_timezone_get();

+++ b/core/includes/common.incundefined
@@ -1943,58 +1943,15 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
+  // if we have a custom use the format use the user provided values.

This is hard to read ;)

cosmicdreams’s picture

FileSize
373.08 KB

I'll fix the comment in common.inc. However, in bootstrap.inc, we are intentionally not using the :? shorthand as this version is easier to read.

cosmicdreams’s picture

FileSize
185.73 KB

ignore the last patch as started writing this before a lot of code was apparently committed and my git diff 8.x created a patch about twice the size this should be. Here's the proper patch

Status: Needs review » Needs work

The last submitted patch, 1571632-201.patch, failed testing.

cosmicdreams’s picture

looks like I need to reroll

Edit: looks like some recently landed Ajax refactorings have impacted this patch. I've done the merge of the two approaches, and am testing locally before I upload a patch here.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
112.59 KB

Here's a rerolled patch, passes the DateTime tests locally

arlinsandbulte’s picture

Issue tags: +d8ux

So, if I understand correctly from #196, this patch consolidates 'custom format strings' & 'format types' ???
If so that is a HUGE UX improvement! cosmicdreams++

Previously, in D7 and below:
The user would first need to create a custom format string.
Then they would create & name a 'Format Type' to use a certain format string.

I never totally understood why this was a two step process, and lots of other users would get confused when trying to create a custom date format.

cosmicdreams’s picture

yes, this patch does consolidate date types and date formats. As this is a natural result of using YAML's associative array storage for handling this config (instead of the database). Which basically boils down to, we don't need to store types separately since the date type can be a key to an array that defines the date format pattern. As a result, we've removed functions that handled types and removed a whole page from the admin that handled types.

I should add a note here that the patch in #1802278: Add a Date component to core provides a DateTime component that would handle that actual formatting of dates from a custom string and is built to use the IntlDateFormatter as well as the previous date formatting functionality. If that patch lands before this one we'll have to rework the parts that actually format dates so that we can use the persisted values appropriately. That component seems to provide a better functionality than the system_get_date_format_pattern function we're introducing in this patch.

I'm tempted to mark this patch postponed until #1802278: Add a Date component to core gets in.

marcingy’s picture

Assigned: marcingy » Unassigned

Unassigning for now as @cosmicdreams seems to be pushing this along nicely.

arlinsandbulte’s picture

Status: Needs review » Needs work
cosmicdreams’s picture

cool, I was looking forward to doing a preemptive reroll of this patch with the date component in place. Looks like I can just do that now.

The main things we need to work out is how to interact with the Date Component's handling of date formats. The component is smart enough to use IntlDateFormatter when it's available. We'll need to send the right settings at the right time. It seems to me that a lot of the functions that handle formatting date formats that are in system.module need to be reworked / revised / ripped out.

cosmicdreams’s picture

FileSize
113.34 KB

I ran the DateTime tests locally and found that this patch passes the tests but had a handful of exceptions.

So, here's the patch so everyone can see how it merged.

cosmicdreams’s picture

Status: Needs work » Needs review

go testbot go

Lars Toomre’s picture

Overall, this patch is pretty good. Here are some note from reading through all of the proposed changes. Most are nitpicks. However, we definitely need to address the docblock for system_format_date_save(), which does not match order of variables in the function declaration.

+++ b/core/includes/bootstrap.incundefined
@@ -2172,13 +2172,16 @@ function drupal_bootstrap($phase = NULL, $new_phase = TRUE) {
+    $config_data_default_timezone = $config->get('timezone.default');
+    return !empty($config_data_default_timezone) ? $config_data_default_timezone : @date_default_timezone_get();

This can be shortened with ?: operator.

+++ b/core/includes/common.incundefined
@@ -1900,7 +1900,7 @@ function format_interval($interval, $granularity = 2, $langcode = NULL) {
- *   - One of the built-in formats: 'short', 'medium', 'long', 'html_datetime',
+ *   - One of the built-in formats: 'system_short', 'system_medium', 'system_long', 'html_datetime',
  *     'html_date', 'html_time', 'html_yearless_date', 'html_week',
  *     'html_month', 'html_year'.
  *   - The name of a date type defined by a module in hook_date_format_types(),

This needs to be wrapped for 80 characters. Also the default value needs to be changed to "Defaults to 'system_medium'."

+++ b/core/includes/common.incundefined
@@ -1944,58 +1944,15 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
-      break;
+  // if we have an incoming custom date format use the provided date format pattern.
+  if ($type != 'custom') {

This inline comment needs to start with a Capital letter and needs to be wrapped so that it is not longer than 80 characters.

+++ b/core/includes/common.incundefined
@@ -1944,58 +1944,15 @@ function format_date($timestamp, $type = 'medium', $format = '', $timezone = NUL
+    $format = config('system.date')->get('formats.' . $type . '.pattern');
+    // Fall back too system_medium if a value was not found.
+    if (!isset($format)) {

Think you mean 'to' here instead of 'too'.

+++ b/core/modules/system/system.admin.incundefined
@@ -1890,14 +1892,16 @@ function system_regional_settings() {
-    '#default_value' => variable_get('date_default_timezone', date_default_timezone_get()),
+    '#default_value' => isset($date_default_timezone) ? $date_default_timezone : date_default_timezone_get(),
     '#options' => $zones,

This also could use '?: operator.

+++ b/core/modules/system/system.admin.incundefined
@@ -1933,7 +1937,24 @@ function system_regional_settings() {
+/**
+ * Form builder submit handler; Handle submission for regional settings.

Change to 'handler: Handles'.

+++ b/core/modules/system/system.admin.incundefined
@@ -2678,76 +2568,53 @@ function system_date_delete_format_form($form, &$form_state, $dfid) {
+    foreach ($formats as $date_format_id => $format_info) {
+      // Do not display date formats that are locked
+      if (empty($format_info['locked'])) {
+        $row = array();
+        $row[] = array('data' => $date_format_id);
+        $row[] = array('data' => $format_info['name']);
+        $row[] = array('data' => format_date(REQUEST_TIME, 'custom', system_get_date_format_pattern($format_info['pattern'])));
+
+        // Prepare Operational links

Both of these in-line comments need to end in periods '.'.

+++ b/core/modules/system/system.admin.incundefined
@@ -2763,24 +2630,48 @@ function system_date_time_formats() {
+ * @param string $date_format_id (optional)
+ *   When present, provides the machine name of the date format that is being
+ *   modified.

This should be:
@param string $date_format
(optional) When present, provides the machine anme of the date format that is being modified. Defaults to an empty string.

+++ b/core/modules/system/system.admin.incundefined
@@ -2803,12 +2711,25 @@ function system_configure_date_formats_form($form, &$form_state, $dfid = 0) {
+ * Check if the chosen machine_name exists or not

Change to 'Checks if'. (active verb tense). Also needs to end in a period.

+++ b/core/modules/system/system.admin.incundefined
@@ -2803,12 +2711,25 @@ function system_configure_date_formats_form($form, &$form_state, $dfid = 0) {
+
+  // The machine name field should already check to see if the requested machine name is available
+  // Regardless of machine_name or human readable name, check to see if the provided pattern exists

These sentences need to end in periods and warp at or before 80 characters.

+++ b/core/modules/system/system.moduleundefined
@@ -3762,73 +3621,52 @@ function system_date_formats() {
- * @param $type
+ * @param $date_format_id
  *   (optional) The date type name.
  *
  * @return
- *   An associative array of date formats. The top-level keys are the names of

Can we add type hinting here?

+++ b/core/modules/system/system.moduleundefined
@@ -3762,73 +3621,52 @@ function system_date_formats() {
+ *   - name: The human readable name of the date format.
+ *   - pattern: The pattern that will modify the format of the date

Missing a period at the end of the line '.'.

+++ b/core/modules/system/system.moduleundefined
@@ -3762,73 +3621,52 @@ function system_date_formats() {
+ *
+ * For now this function defaults to php.

I would rewrite this as "For now, this function defaults to 'php'."

+++ b/core/modules/system/system.moduleundefined
@@ -3762,73 +3621,52 @@ function system_date_formats() {
+ *
+ * @todo: Decide if this function is needed or if Date Component can handle this.
  */

Eliminate the ':' and this line will just fit in 80 characters.

+++ b/core/modules/system/system.moduleundefined
@@ -3838,31 +3676,24 @@ function system_date_formats_rebuild() {
+ * @param $date_format_id
+ *   (optional) The machine name for the date format.

This could be type hinted as well and I would add 'Defaults to NULL' at the end of the description.

+++ b/core/modules/system/system.moduleundefined
@@ -3838,31 +3676,24 @@ function system_date_formats_rebuild() {
+ *   all date format strings for that locale, keyed by the date type. If
+ *   neither is specified.

I think you meant 'If neither is specified, returns FALSE.' Missing the second part.

+++ b/core/modules/system/system.moduleundefined
@@ -3872,244 +3703,39 @@ function system_date_format_locale($langcode = NULL, $type = NULL) {
  *
  * @param $date_format
  *   A date format array containing the following keys:
- *   - type: The name of the date type this format is associated with.

Something is wrong with this docblock. The @param variables do not match those in the declaration. Is this variable suppose to be $format_info?

+++ b/core/modules/system/system.moduleundefined
@@ -3872,244 +3703,39 @@ function system_date_format_locale($langcode = NULL, $type = NULL) {
- * @param $dfid
+ * @param $date_format_id
  *   If set, replace the existing date format having this ID with the

This complete docblock could use type hinting.

+++ b/core/modules/system/system.moduleundefined
@@ -4118,13 +3744,20 @@ function system_date_format_save($date_format, $dfid = 0) {
- * @param $dfid
+ * @param $date_format_id
  *   The date format ID.

Add type hint here please. It is unclear whether this should be a string or an integer.

Can we also add type hinting to all of the @param and @return directives that are changed in this patch?

Status: Needs review » Needs work

The last submitted patch, 1571632-210.patch, failed testing.

cosmicdreams’s picture

I'll consume this feedback tonight.

A note for this review and future reviewers, we are purposefully not using the ?: shorthand.

Lars Toomre’s picture

Oh good to know... I have seen a number of other patches use it. Is there a reference node that documents the '?:" purposefully not used shorthand?

cosmicdreams’s picture

I can add one to the comments if it is necessary.

KarenS’s picture

Status: Needs work » Needs review
FileSize
12.81 KB
114.83 KB

I don't understand changes like:

-    $variables['source_date'] = format_date($item->timestamp, 'custom', variable_get('date_format_medium', 'D, m/d/Y - H:i'));
+    $format = config('system.date')->get('formats.system_medium.pattern');
+    $variables['source_date'] = format_date($item->timestamp, 'custom', system_get_date_format_pattern($format));

It seems like this should be reduced to the following:

$variables['source_date'] = format_date($item->timestamp, 'medium');

Or code like this:

    foreach ($locale_formats as $date_format_id => $format_info) {
      $pattern = system_get_date_format_pattern($format_info['pattern']);
      $choices[$date_format_id] = format_date(REQUEST_TIME, 'custom', $pattern);
    }

Which I'm changing to:

     foreach ($locale_formats as $date_format_id => $format_info) {
      $choices[$date_format_id] = format_date(REQUEST_TIME, $date_format_id);
    }

The only 'custom' formats should be those that don't exist in the format config, everything else has a type we can retrieve. If we get rid of all these 'custom' formats that are actually known format types, it would then make it possible to do all the IntlDateFormatter handling within the format_date() function, which is much simpler and easier to understand.

And the following code is not right, we wouldn't use the intl format here, this would always be the php format. The format string is just being used to figure out the position of the month, day, and year. Using the intl format in that spot wouldn't even work right.

diff --git a/core/includes/form.inc b/core/includes/form.inc
index 6956f04..9a73342 100644
--- a/core/includes/form.inc
+++ b/core/includes/form.inc
@@ -2963,7 +2963,8 @@ function form_process_date($element) {
   $element['#tree'] = TRUE;
 
   // Determine the order of day, month, year in the site's chosen date format.
-  $format = variable_get('date_format_short', 'm/d/Y - H:i');
+  $format = config('system.date')->get('formats.system_short.pattern');
+  $format = system_get_date_format_pattern($format);

With those changes I can get rid of system_get_date_format_pattern() altogether.

Also the reason for all the exceptions in the previous patch is from this line, which is a mistake:

-function system_user_login($edit, $account) {
+function system_user_login(&$edit, $account) {

I've worked up a new patch with these changes. I also added the intl formats to the yaml file so they are available and altered the description in the formatter form to point to the page for intl formats if on a system that uses them.

No idea what I may have done to tests, so we'll see..

cosmicdreams’s picture

I think I understand, It's still a good idea to breakout date format config (php vs intl), but we don't need to use the 'custom' flag in the format_date function anymore since we're using ids to explicitly declare each date format.

And with new date component we can allow the system to pull the intl date formats instead of the php ones.

We'll see if the patch goes green but I have one concern.

+++ b/core/includes/form.incundefined
@@ -2964,7 +2964,7 @@ function form_process_date($element) {
+  $format = $format['php'];

You seem to be hard-coding the form_process_date function to always use the php version of the date format pattern.

KarenS’s picture

+++ b/core/includes/form.incundefined
@@ -2964,7 +2964,7 @@ function form_process_date($element) {
+ $format = $format['php'];

Yes, that one only makes sense as a php format string. It's not being used to display a date, it's being used to see what order the date parts are in for the selector. If you read the code you'll see that grabbing an intl string there wouldn't even work. It probably should use the constant instead of 'php', so it should be DrupalDateTime::PHP instead. And actually that function goes away altogether in my date field patch anyway, so it doesn't matter that much.

cosmicdreams’s picture

Also, adding a comment would be good.

Your previous patch was marked as postponed by the test bot for some reason.

Status: Needs review » Needs work

The last submitted patch, regional.patch, failed testing.

cosmicdreams’s picture

Looks like the patch in #217 passed the DateTime tests but blew up on the install / upgrade tests.

KarenS’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
115.57 KB

Found the reason for the test failures -- the intl formats use a single quote as an escape character so there were single quotes embedded in the yaml settings that needed to be wrapped with double quotes. I also figured out how to fix the javascript helper that shows you what your format looks like when editing a format.

Status: Needs review » Needs work

The last submitted patch, regional-1.patch, failed testing.

KarenS’s picture

Status: Needs work » Needs review
FileSize
15.15 KB
123.19 KB

I think we also need to get rid of the following, which is no longer being used:

/**
 * Implements hook_date_formats().
 */
function system_date_formats() {
  include_once DRUPAL_ROOT . '/core/includes/date.inc';
  return system_default_date_formats();
}

We also need to get rid of the date.inc file, which is no longer being used and no longer matches the expected patterns anyway.

I'm also incorporating the code comment suggestions from #212 and fixing the remaining tests (i.e. DrupalDateTime test was using the old variables and needed to be updated for CMI).

This still needs an upgrade path for the existing formats to convert them to CMI.

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -Needs screenshots, -API change, -d8ux, -Configuration system

The last submitted patch, regional-2.patch, failed testing.

justafish’s picture

Status: Needs work » Needs review

These tests are not failing for me locally.

#225: regional-2.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs screenshots, +API change, +d8ux, +Configuration system

The last submitted patch, regional-2.patch, failed testing.

KarenS’s picture

Status: Needs work » Needs review
FileSize
123.19 KB

Not sure what's going on with tests. The previous test used to say there were 3 failures, none of which fail locally, just as reported by justafish. Now the test is a total fail. This is just a re-roll to catch up to head, no changes.

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -Needs screenshots, -API change, -d8ux, -Configuration system

The last submitted patch, 1571632-regional-config-229.patch, failed testing.

andyceo’s picture

Status: Needs work » Needs review

#229: 1571632-regional-config-229.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs screenshots, +API change, +d8ux, +Configuration system

The last submitted patch, 1571632-regional-config-229.patch, failed testing.

cosmicdreams’s picture

Ah, I suspect a function has a duplicative name in that file, as that has typically been the issue. Likely, the tests that have failed in the past are still failing. Need to looking deeper into this.

KarenS’s picture

The tests that fail pass locally for me, I'm not sure what is going on.

rvilar’s picture

Status: Needs work » Needs review
FileSize
121.64 KB

The problem was that function system_update_8035() was redeclared. I change the function name to system_update_8036

Status: Needs review » Needs work

The last submitted patch, 1571632-regional-config-235.patch, failed testing.

KarenS’s picture

The yml file was present in #229 but missing in #235, which is the cause of all those failures. I don't have time to re-roll it, but someone needs to add that back in.

marcingy’s picture

Status: Needs work » Needs review
FileSize
118.44 KB

Reroll with yml file

marcingy’s picture

Last version left date.inc in existence this fixes that situation

Status: Needs review » Needs work

The last submitted patch, 1571632-regional-config-239.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
5.59 KB
123.84 KB

It's weird, tests also pass locally for me, will investigate some more. In the mean time, rerolled with a couple of changes, interdiff attached.

  • Removed leftover code to system_date_time_settings().
  • Change the logic of the machine name, it now actually works as the machine_name FAPI element was designed for.
  • Rename 'Machine Name' to 'Machine name' in the table.

Some remarks, but I haven't touched the code yet, as it doesn't bother me *that* much that I wanted to actually change them.

  • I'm not sure about showing the machine name in the table. We don't use that pattern in core. Compare with text formats which only shows the human name. Then again, Content types seems to make an exception, but it's also a compeletely different table, and it's also less in the face.
  • Why adding a sort on the table headers, especially on pattern, it seems to me a default sort on the Human name is fine enough ?
  • You can delete the 'system_x' date formats. The result is that the submitted date on a node detail won't work anymore. I'm not sure about the right path here.
  • I'd remove the 'Default' from the system date formats name, it seems really redundant to me.
  • Rename the menu local task to 'Add format' to 'Add date format', or the other way around ? That would make it consistent with the 'Add date format' link in case there are no custom date formats.

Also, we'll have to move the date formats to config entity, but we should probably try to get this in first and do that conversion in a follow up.

swentel’s picture

FileSize
3.22 KB
125.29 KB

Added a menu loader function for the edit and delete callbacks as it was possible to edit/delete locked date formats. Also renamed validate and submit functions because 'add' is a little confusing since the same form callback is used for adding and editing.

Status: Needs review » Needs work

The last submitted patch, 1571632-242.patch, failed testing.

chx’s picture

Assigned: Unassigned » chx

Working on this. I can already tell that the reason this passes on everyone's machine and not on the testbot because the bot has intl enabled and

  $pattern_type = class_exists('intlDateFormatter') ? DrupalDateTime::INTL : DrupalDateTime::PHP;
  $format['pattern'][$pattern_type] = trim($form_state['values']['date_format_pattern']);

creates an intl format of 'j M y' and I have my doubts that's a valid intl pattern.

chx’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs screenshots, -d8ux, -Configuration system
FileSize
125.35 KB

I dunno why I can't interdiff these two but the big change is in system_date_time_formats:

1581c1582,1583
< +  $pattern = class_exists('IntlDateFormatter') ? $patterns[DrupalDateTime::INTL] : $patterns[DrupalDateTime::PHP];
---
> +  $date = new DrupalDateTime;
> +  $pattern = $date->canUseIntl() ? $patterns[DrupalDateTime::INTL] : $patterns[DrupalDateTime::PHP];

format_date() decides to use intl or php based on canUseIntl , the test immediately breaks when you enter some format, it gets saved as intl but format_date() decides to use php. And, to top it, the test enters a php format anyways as most every other test does, so after fixing system.admin.inc, seemingly nothing else needs to change. I also removed an unused $admin_date_format from testAdminDefinedFormatDate but that's immaterial.

chx’s picture

I certainly didnt intend to nuke all these.

chx’s picture

Assigned: chx » Unassigned

I am done.

KarenS’s picture

Good debugging chx! That makes sense to me. It looks like we should add some tests that specifically test the intlFormatter as well, but I think that could be a follow up issue.

KarenS’s picture

Status: Needs review » Needs work

Of the above issues, the need for tests for the intlFormatter and this comment still need to be addressed:

You can delete the 'system_x' date formats. The result is that the submitted date on a node detail won't work anymore. I'm not sure about the right path here.

The system definitely expects that there will always be the basic format types (long, medium, short), as do tests, so it seems like we need to keep them from being deleted. The former process wouldn't allow you do delete the 'medium' type altogether, just change the format it was using. We need to have something equivalent here. That's critical I think, so marking back to needs work for that. It don't know if we have to protect all three, but at least we have to protect the ones used in tests.

chx’s picture

Status: Needs work » Needs review

Given how complicated this can I strongly recommend not to block , I filed #1844196: Ensure the medium (or more?) formats are always there. (The user can edit the config file, removing the whole medium or perhaps just the php and just the intl pattern and it's not sure whether we need to restore both, edit the php and the intl pattern to be contradictory and so on)

KarenS’s picture

Protecting the basic formats in a follow up issue is OK with me, I don't want to block this either. We also need a follow up to add tests for the IntlDateFormatter formats, so I added #1844614: Add tests for the IntlDateFormatter formats.

KarenS’s picture

Issue summary: View changes

Updated issue summary to include important status updates from the comments.

cosmicdreams’s picture

Updated summary to include follow up issues, ready to RTBC? patch looks good to me.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

I read through the patch again and some of the last comments. The patch looks great and I feel it's time to commit this as this is becomming a huge thread. There are already some followup issues defined.

LETS DO THIS! GO TEAM!

KarenS’s picture

Status: Reviewed & tested by the community » Needs review

I don't have time to try out the latest patch. Someone beside chx should run it through its paces to be sure nothing got lost along the way. I won't have time to do that until tomorrow probably, so if someone else can do it first they can mark it RTBC.

KarenS’s picture

Status: Needs review » Reviewed & tested by the community

Oops cross post. Great!!

cosmicdreams’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update, -Needs screenshots, -API change, -d8ux, -Configuration system

#245: 1571632_245.patch queued for re-testing.

Lots of code changes recently, Since this is RTBC it could be visited upon a core committer any day now. Let's make sure it still passes.

EDIT: looks like we need to escalate the update function's number. I'll see if I can do that on this old PC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1571632_245.patch, failed testing.

cosmicdreams’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update, +Needs screenshots, +API change, +d8ux, +Configuration system
FileSize
256.62 KB

Hopefully the fact that I had to use wordpad to increment the update function's number in this patch won't horribly break it.

cosmicdreams’s picture

judging by the size of the file I would assume yes. Can someone please update the system_update_8036 to system_update_8037?

marcingy’s picture

FileSize
125.35 KB

Just a re-roll for head.

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -Needs screenshots, -API change, -d8ux, -Configuration system

The last submitted patch, 1571632_260.patch, failed testing.

andyceo’s picture

Status: Needs work » Needs review

#261: 1571632_260.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1571632_260.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review

#261: 1571632_260.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1571632_260.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +Needs screenshots, +API change, +d8ux, +Configuration system

#261: 1571632_260.patch queued for re-testing.

marcingy’s picture

Checking the link directly and it is green but the bot has not responded for some reason

marcingy’s picture

FileSize
125.35 KB

As the bot does not want to respond....

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -Needs screenshots, -API change, -d8ux, -Configuration system

The last submitted patch, 1571632_fresh.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review

#269: 1571632_fresh.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1571632_fresh.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review

#269: 1571632_fresh.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs screenshots, +API change, +d8ux, +Configuration system

The last submitted patch, 1571632_fresh.patch, failed testing.

cosmicdreams’s picture

I have no idea what's going on here. There's no reason why tests should be failing.

marcingy’s picture

Status: Needs work » Needs review

#269: 1571632_fresh.patch queued for re-testing.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Second try

Dries’s picture

I don't understand why we prefixed the formats with 'system_', and can't seem to find an explanation in the comments above.

It seems unnecessary and less developer friendly. What does 'system_' mean to a developer? I don't understand what it means.

cosmicdreams’s picture

Hello Dries, thank you for stopping by. I'll add a note about the system_ namespacing for the date format configurations. Using 'system_' to namespace the date formats patterns for short, medium, and long provides developers who are reading the configuration a clear separation between date formats that were created by hand or through the UI and the ones that were initially installed.

True, there is nothing within our implementation that demands these configurations to be named in this way. It's a carry-over from the previous generation of this system where short, medium, and long date formats where hidden by the system so that developers would have to work harder to change them. Screwing up these formats results in system instability as they are used throughout a default Drupal installation to output dates.

So, it's one part tradition and one part a name-spacing the config to indicate that it belongs to the system.

Edit: in order to get this patch in, are you requesting additional comments within the code or simply a revised summary at the top of the issue?

cosmicdreams’s picture

Issue summary: View changes

update issue description with follow up issues.

cosmicdreams’s picture

FileSize
125.36 KB

advanced the system.install's new update function's number.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs issue summary update, -Needs screenshots, -API change, -d8ux, -Configuration system

The last submitted patch, 1571632_280.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs screenshots, -API change, -d8ux, -Configuration system

#280: 1571632_280.patch queued for re-testing.

Not this again, The test that fails isn't a file we've changed, however a case could be made that it's using the yml file that was slightly changed. Don't know the root cause.

The last submitted patch, 1571632_280.patch, failed testing.

KarenS’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs screenshots, +API change, +d8ux, +Configuration system

This part of the code looks suspicious:

diff --git a/core/modules/user/lib/Drupal/user/Tests/UserRegistrationTest.php b/core/modules/user/lib/Drupal/user/Tests/UserRegistrationTest.php
index 73f3396..f27ee5a 100644
--- a/core/modules/user/lib/Drupal/user/Tests/UserRegistrationTest.php
+++ b/core/modules/user/lib/Drupal/user/Tests/UserRegistrationTest.php
@@ -149,17 +149,18 @@ function testRegistrationEmailDuplicates() {
   }
 
   function testRegistrationDefaultValues() {
-    $config = config('user.settings');
     // Don't require e-mail verification and allow registration by site visitors
     // without administrator approval.
-    $config
+    $config_user_settings = config('user.settings')

Notice the odd "- $config". That line shouldn't be there.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
125.53 KB

Quick reroll lets get this monster in...

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -Needs screenshots, -API change, -d8ux, -Configuration system

The last submitted patch, 1571632_285.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review

#285: 1571632_285.patch queued for re-testing.

I don't understand why the patch doesn't even apply anymore.

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs screenshots, +API change, +d8ux, +Configuration system

The last submitted patch, 1571632_285.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
123.79 KB
chx’s picture

FileSize
125.35 KB
cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

Assuming 290 passes, RTBC again.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs issue summary update, -Needs screenshots, -API change, -d8ux, -Configuration system

The last submitted patch, 1571632_290.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +Needs screenshots, +API change, +d8ux, +Configuration system

#290: 1571632_290.patch queued for re-testing.

Retesting based on two assumptions:
1. The test in 290 failed because of the BROKEN HEAD that we had earlier today
2. The patch in 259 had failures because the system.date.yml file wasn't included.

Hopefully we get a good result now.

chx’s picture

Status: Needs review » Reviewed & tested by the community
marcingy’s picture

So glad to see this green again +1 for rtbc from me.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Avoid commit conflicts

While I appreciate that this patch is a huge PITA to re-roll, I'm sorry, but I'm with Dries here. :\

+++ b/core/modules/views/config/views.view.glossary.ymlundefined
@@ -47,7 +47,7 @@ display:
-          date_format: large
+          date_format: system_long

"system_long" totally means nothing to me; the "after" code here is much harder to grok. This is specifically about a date format, so why is it not "system_date_format_long" or similar?

I read in the issue summary (thanks!) about this prefix being used to separate date formats created by hand vs. initially installed. But the ones created by hand are ultimately also created via system module, so this prefix seems like a false distinction. If we want to be clear about that, we should probably do as core normally does and use CONSTANT_NAMES to easily pick out system-defined things (such as DRUPAL_ANONYMOUS_RID in bootstrap.inc). However, I notice that, say, Image Styles don't do this. Image's pre-defined styles are just called .large, ,medium, and .thumbnail. So why are date formats special here?

I'm actually wondering if this system date format renaming actually has anything to do with the CMI conversion and whether it could be moved to a separate issue in order to make this one easier to review/commit. If I'm wrong about that, happy to hear pushback. I tried ctrl+Fing for variations of "prefix" "system_" etc. and couldn't find where and why this originally got introduced.

I'd love to just commit this and push this feedback to a follow-up, but it seems like it requires a patch nearly as big to adjust, so it makes more sense to me to just do this in the initial commit. I'm tagging "Avoid commit conflicts" in exchange, so hopefully that lessens the re-roll burden some. Sorry. :(

webchick’s picture

Status: Needs work » Reviewed & tested by the community

"However, I notice that, say, Image Styles don't do this. Image's pre-defined styles are just called .large, ,medium, and .thumbnail. So why are date formats special here?"

Also worth pointing out, neither does User. "DRUPAL_ANONYMOUS_RID" maps to "anonymous" not "system_anonymous" or "drupal_anonymous" or "user_anonymous." So I really am thinking that stuff shouldn't be part of this patch (and possibly at all) unless there's a compelling reason that I'm missing.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Sorry. Cross-post. With myself! :D Too many tabs. ;)

swentel’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
28.23 KB
106.76 KB

Assuming this will turn green again.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/FieldDateTest.phpundefined
@@ -56,9 +56,9 @@ public function testFieldDate() {
-        'small' => format_date($time, 'small', '', $timezone),
+        'small' => format_date($time, 'system_small', '', $timezone),
...
-        'large' => format_date($time, 'large', '', $timezone),
+        'large' => format_date($time, 'system_large', '', $timezone),

Looks like there are still some remaining hunks?

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
930 bytes
390.11 KB

updated the issue to use the proper date format names.

swentel’s picture

FileSize
932 bytes
105.85 KB

I think I (or rather I can trust my ide now) have got them all now.

cosmicdreams’s picture

Status: Needs work » Needs review

sigh, the patch size is way off. must be line endings again. will fix.

EDIT: Cool swentel posted a new one in 301. Let me see if he found the improperly named date formats as well.

Status: Needs review » Needs work

The last submitted patch, 1571632_301-fixed.patch, failed testing.

KarenS’s picture

#302 should be OK, let's get it tested.

cosmicdreams’s picture

FileSize
106.74 KB

this patch slightly alters 302 so that the proper date format names are used. previous patch used 'small' and 'large'. Those date formats don't appear to exist.

Will need to create a follow up issue to look at these tests and improve their logic.

The last submitted patch, 1571632_306.patch, failed testing.

marcingy’s picture

#306: 1571632_306.patch queued for re-testing.

marcingy’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, YAY!!! Still applies, so I'm taking the plunge quick before it changes its mind. :)

Committed and pushed to 8.x. WOOHOO. Thanks so much for all the hard work here, folks!

Berdir’s picture

I just noticed #1858984: Split system date formats from timezone settings did not require any upgrade test changes. So we're missing upgrade path tests for that. And it looks like we're actually completely missing an upgrade path for date formats? we didn't even delete the tables...?

This was mentioned like 5 times in the reviews, but we still forgot it :(

I think that needs a critical follow-up...

cosmicdreams’s picture

Status: Fixed » Closed (fixed)

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

webchick’s picture

Title: Convert regional settings to configuration system » Change notice: Convert regional settings to configuration system
Priority: Normal » Critical
Status: Closed (fixed) » Active
Issue tags: +Needs change record

We never got a change notice for this. Specifically, #501428: Date and time field type in core is trying to call system_get_date_types() and can't.

chx’s picture

Status: Active » Needs review
Issue tags: -Avoid commit conflicts
Berdir’s picture

Status: Needs review » Fixed

Change notice looks good.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/Date.phpundefined
@@ -33,9 +33,9 @@ protected function defineOptions() {
     $date_formats = array();
-    $date_types = system_get_date_types();
-    foreach ($date_types as $key => $value) {
-      $date_formats[$value['type']] = check_plain(t($value['title'] . ' format')) . ': ' . format_date(REQUEST_TIME, $value['type']);
+    $date_types = system_get_date_formats();
+    foreach ($date_types as $machine_name => $value) {
+      $date_formats[$machine_name] = check_plain(t('@name format', array('@name' => $value['name'])) . ': ' . format_date(REQUEST_TIME, $machine_name));

Noticed this because of the change notice. Can we get a follow-up issue for this?

Shouldn't this be t('@name format: @date'), then it is properly translatable and the check_plain() isn't necessary.

Berdir’s picture

Title: Change notice: Convert regional settings to configuration system » Convert regional settings to configuration system
Priority: Critical » Normal

Status: Fixed » Closed (fixed)

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

xjm’s picture

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

xjm’s picture

Issue summary: View changes

Add an questions and answers section.