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

Files: 
CommentFileSizeAuthor
#306 1571632_306.patch106.74 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 48,839 pass(es).
[ View ]
#302 1571632-301.patch105.85 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#302 interdiff.txt932 bytesswentel
#301 1571632_301-fixed.patch390.11 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571632_301-fixed.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#301 interdiff.txt930 bytescosmicdreams
#299 1571632-299.patch106.76 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 48,744 pass(es).
[ View ]
#299 interdiff.txt28.23 KBswentel
#290 1571632_290.patch125.35 KBchx
PASSED: [[SimpleTest]]: [MySQL] 48,737 pass(es).
[ View ]
#289 1571632_289.patch123.79 KBchx
FAILED: [[SimpleTest]]: [MySQL] 48,385 pass(es), 45 fail(s), and 52 exception(s).
[ View ]
#285 1571632_285.patch125.53 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571632_285.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#280 1571632_280.patch125.36 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571632_280.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#269 1571632_fresh.patch125.35 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 48,688 pass(es).
[ View ]
#261 1571632_260.patch125.35 KBmarcingy
Test request sent.
Previous result: FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#259 1571632_final.patch256.62 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 48,544 pass(es).
[ View ]
#245 1571632_245.patch125.35 KBchx
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#242 1571632-242.patch125.29 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 48,174 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#242 interdiff.txt3.22 KBswentel
#241 1571632-241.patch123.84 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 48,158 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#241 interdiff.txt5.59 KBswentel
#239 1571632-regional-config-239.patch123.19 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 48,146 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#238 1571632-regional-config-238.patch118.44 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 48,154 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#235 1571632-regional-config-235.patch121.64 KBrvilar
FAILED: [[SimpleTest]]: [MySQL] 47,697 pass(es), 44 fail(s), and 19 exception(s).
[ View ]
#229 1571632-regional-config-229.patch123.19 KBKarenS
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#225 regional-2.patch123.19 KBKarenS
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#225 interdiff.txt15.15 KBKarenS
#223 regional-1.patch115.57 KBKarenS
FAILED: [[SimpleTest]]: [MySQL] 47,822 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#223 regional-interdiff.txt3.11 KBKarenS
#217 regional.patch114.83 KBKarenS
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#217 interdiff.txt12.81 KBKarenS
#210 1571632-210.patch113.34 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 48,016 pass(es), 4 fail(s), and 2,496 exception(s).
[ View ]
#204 1571632-204.patch112.59 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 47,564 pass(es).
[ View ]
#201 1571632-201.patch185.73 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571632-201.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#200 1571632-200.patch373.08 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571632-200.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#198 1571632-198.patch112.91 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 46,392 pass(es).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] 46,436 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#192 1571632-192.patch112.56 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 46,411 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#190 1571632-190.patch112.56 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#188 1571632-188.patch111.16 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/System/DateTimeTest.php.
[ View ]
#181 interdiff.txt3.36 KBdawehner
#181 core-1571632-181.patch113.31 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 46,189 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#171 1571632-172.patch109.95 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 46,176 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#169 1571632-170.patch109.94 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 42,766 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#167 1571632-167.patch106.9 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 42,727 pass(es), 3 fail(s), and 23 exception(s).
[ View ]
#164 1571632-interdiff.txt11.88 KBcosmicdreams
#163 1571632-163.patch91.25 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 42,779 pass(es).
[ View ]
#156 1571632_156.patch89.89 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 42,562 pass(es).
[ View ]
#153 1571632_153.patch89.94 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 42,568 pass(es).
[ View ]
#150 1571632_149.patch89.9 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 42,563 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#146 1571632_146.patch89.21 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 42,200 pass(es), 6 fail(s), and 65 exception(s).
[ View ]
#144 1571632_144.patch89.21 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#142 1571632_142.patch88.8 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 42,016 pass(es), 18 fail(s), and 2,970 exception(s).
[ View ]
#140 1571632_137.patch88.19 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 42,119 pass(es).
[ View ]
#135 1571632_134.patch99.07 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 42,109 pass(es), 12 fail(s), and 2 exception(s).
[ View ]
#133 1571632_133.patch79.55 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 42,113 pass(es), 25 fail(s), and 20 exception(s).
[ View ]
#131 1571632_131.patch77.04 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 42,107 pass(es), 28 fail(s), and 27 exception(s).
[ View ]
#111 1571632_109.patch78.12 KBKarenS
FAILED: [[SimpleTest]]: [MySQL] 41,417 pass(es), 20 fail(s), and 8 exception(s).
[ View ]
#111 interdiff.txt14.78 KBKarenS
#109 1571632_109.patch71 KBchx
FAILED: [[SimpleTest]]: [MySQL] 41,415 pass(es), 33 fail(s), and 32 exception(s).
[ View ]
#106 1571632_104.patch70.56 KBchx
FAILED: [[SimpleTest]]: [MySQL] 40,772 pass(es), 88 fail(s), and 11,520 exception(s).
[ View ]
#104 1571632_104.patch70.56 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#104 interdiff.txt2.49 KBchx
#102 1571632_102.patch70.34 KBchx
FAILED: [[SimpleTest]]: [MySQL] 40,770 pass(es), 88 fail(s), and 11,477 exception(s).
[ View ]
#96 1571632_date_format_changes.patch71.09 KBKarenS
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571632_date_format_changes.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#89 1571632_89.patch66.09 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 41,262 pass(es), 64 fail(s), and 12 exception(s).
[ View ]
#85 d8_date_converge.patch55.15 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#83 drupal-change_to_config_for_tz-1571632-83.patch20.92 KBjustafish
PASSED: [[SimpleTest]]: [MySQL] 41,243 pass(es).
[ View ]
#83 interdiff.txt15.14 KBjustafish
#78 drupal-change_to_config_for_tz-1571632-78.patch21.06 KBjustafish
PASSED: [[SimpleTest]]: [MySQL] 40,362 pass(es).
[ View ]
#78 interdiff.txt2.5 KBjustafish
#70 rename-first_weekday-to-first_day-1571632-70.patch20.48 KBjustafish
FAILED: [[SimpleTest]]: [MySQL] 40,705 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#75 drupal-change_to_config_for_tz-1571632-75.patch21.17 KBjustafish
PASSED: [[SimpleTest]]: [MySQL] 40,716 pass(es).
[ View ]
#75 interdiff.txt2.37 KBjustafish
#63 rename-first_weekday-to-first_day-1571632-63.patch20.53 KBjustafish
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch rename-first_weekday-to-first_day-1571632-63.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#63 interdiff.txt3.07 KBjustafish
#61 drupal-change_to_config_for_tz-1571632-60.patch20.55 KBjustafish
PASSED: [[SimpleTest]]: [MySQL] 40,636 pass(es).
[ View ]
#61 interdiff.txt1.32 KBjustafish
#59 drupal-change_to_config_for_tz-1571632-59.patch20.55 KBjustafish
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#59 interdiff.txt657 bytesjustafish
#56 drupal-change_to_config_for_tz-1571632-56.patch20.25 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 40,253 pass(es).
[ View ]
#54 drupal-change_to_config_for_tz-1571632-54.patch21.87 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 39,804 pass(es).
[ View ]
#50 drupal-change_to_config_for_tz-1571632-50.patch21.93 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 39,762 pass(es).
[ View ]
#40 drupal-change_to_config_for_tz-1571632-40.patch22.07 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 39,762 pass(es).
[ View ]
#35 drupal-change_to_config_for_tz-1571632-35.patch22.07 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#24 drupal-change_to_config_for_tz-1571632-24.patch19.39 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 36,844 pass(es).
[ View ]
#24 interdiff.txt653 bytesdisasm
#22 drupal-change_to_config_for_tz-1571632-21.patch19.4 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 36,837 pass(es).
[ View ]
#22 interdiff.txt3.75 KBdisasm
#21 drupal-change_to_config_for_tz-1571632-21.patch19.4 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 36,861 pass(es).
[ View ]
#21 interdiff.txt3.75 KBdisasm
#19 1571632_19_regional_cmi.patch17.04 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 36,847 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#16 1571632_16_regional_cmi.patch13.99 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 36,845 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#12 1571632_12_regional_cmi.patch18.86 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 36,661 pass(es).
[ View ]
#10 1571632_9_regional_cmi.patch18.98 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 36,600 pass(es).
[ View ]
#8 1571632_8_regional_cmi.patch18.99 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 1571632_8_regional_cmi.patch.
[ View ]
#5 1571632_5_regional_cmi.patch19.02 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 36,598 pass(es).
[ View ]
#3 1571632_3_regional_cmi.patch18.61 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 36,156 pass(es), 152 fail(s), and 11,127 exception(s).
[ View ]
#1 1571632_1_regional_cmi.patch18.64 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.admin.inc.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new18.64 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.admin.inc.
[ View ]

First Try

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new18.61 KB
FAILED: [[SimpleTest]]: [MySQL] 36,156 pass(es), 152 fail(s), and 11,127 exception(s).
[ View ]

Thing I found the typo

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new19.02 KB
PASSED: [[SimpleTest]]: [MySQL] 36,598 pass(es).
[ View ]

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)

Issue tags:+Config novice

tagging

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.

Status:Needs work» Needs review
StatusFileSize
new18.99 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 1571632_8_regional_cmi.patch.
[ View ]

Here's a patch with those revisions

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new18.98 KB
PASSED: [[SimpleTest]]: [MySQL] 36,600 pass(es).
[ View ]

another try

Looks good to me.

StatusFileSize
new18.86 KB
PASSED: [[SimpleTest]]: [MySQL] 36,661 pass(es).
[ View ]

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

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.

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.

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?

StatusFileSize
new13.99 KB
FAILED: [[SimpleTest]]: [MySQL] 36,845 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new17.04 KB
FAILED: [[SimpleTest]]: [MySQL] 36,847 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work
StatusFileSize
new3.75 KB
new19.4 KB
PASSED: [[SimpleTest]]: [MySQL] 36,861 pass(es).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new3.75 KB
new19.4 KB
PASSED: [[SimpleTest]]: [MySQL] 36,837 pass(es).
[ View ]

re-attached with the right status.

+++ 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.

StatusFileSize
new653 bytes
new19.39 KB
PASSED: [[SimpleTest]]: [MySQL] 36,844 pass(es).
[ View ]

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.

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

Issue tags:+Configuration system

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.

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.

Issue tags:+API change

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

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?

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

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

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

Status:Needs work» Needs review
StatusFileSize
new22.07 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new22.07 KB
PASSED: [[SimpleTest]]: [MySQL] 39,762 pass(es).
[ View ]

Should fix th installer

+++ 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.

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!

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

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

@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

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'));

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

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. :)

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new21.93 KB
PASSED: [[SimpleTest]]: [MySQL] 39,762 pass(es).
[ View ]

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."

I prefer
timezone.user.warn_unset

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

Status:Needs work» Needs review
StatusFileSize
new21.87 KB
PASSED: [[SimpleTest]]: [MySQL] 39,804 pass(es).
[ View ]

I'm into #53 lets see what others think

StatusFileSize
new20.25 KB
PASSED: [[SimpleTest]]: [MySQL] 40,253 pass(es).
[ View ]

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.

Needs issue summary

StatusFileSize
new657 bytes
new20.55 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

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.

StatusFileSize
new1.32 KB
new20.55 KB
PASSED: [[SimpleTest]]: [MySQL] 40,636 pass(es).
[ View ]

Bumping up system update hook number.

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.

Status:Needs work» Needs review
StatusFileSize
new3.07 KB
new20.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch rename-first_weekday-to-first_day-1571632-63.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Agreed. Patch changes first_weekday to first_day

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.

Why change it at all? First_day is fine.

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. ;)

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

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.

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new20.48 KB
FAILED: [[SimpleTest]]: [MySQL] 40,705 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
new21.17 KB
PASSED: [[SimpleTest]]: [MySQL] 40,716 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Looks good to me

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).

Status:Needs work» Needs review
StatusFileSize
new2.5 KB
new21.06 KB
PASSED: [[SimpleTest]]: [MySQL] 40,362 pass(es).
[ View ]

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.

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

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.

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

StatusFileSize
new15.14 KB
new20.92 KB
PASSED: [[SimpleTest]]: [MySQL] 41,243 pass(es).
[ View ]

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

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.

StatusFileSize
new55.15 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

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?

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()

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.

Status:Postponed» Needs review
StatusFileSize
new66.09 KB
FAILED: [[SimpleTest]]: [MySQL] 41,262 pass(es), 64 fail(s), and 12 exception(s).
[ View ]

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.

Wow, only 64 fails. I expected more.

Assigned:cosmicdreams» chx

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

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.

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.

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.

StatusFileSize
new71.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571632_date_format_changes.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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.

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.

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.

Status:Needs work» Needs review

Sorry did not mean to change status.

StatusFileSize
new70.34 KB
FAILED: [[SimpleTest]]: [MySQL] 40,770 pass(es), 88 fail(s), and 11,477 exception(s).
[ View ]

Rerolled against HEAD.

Status:Needs review» Needs work

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

Assigned:chx» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.49 KB
new70.56 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new70.56 KB
FAILED: [[SimpleTest]]: [MySQL] 40,772 pass(es), 88 fail(s), and 11,520 exception(s).
[ View ]

Status:Needs review» Needs work

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

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.

Status:Needs work» Needs review
StatusFileSize
new71 KB
FAILED: [[SimpleTest]]: [MySQL] 41,415 pass(es), 33 fail(s), and 32 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new14.78 KB
new78.12 KB
FAILED: [[SimpleTest]]: [MySQL] 41,417 pass(es), 20 fail(s), and 8 exception(s).
[ View ]

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.

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.

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.

Issue tags:-Novice, -Config novice

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

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

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.

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.

Assigned:Unassigned» marcingy

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.

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.

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.

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

Assigned:marcingy» Unassigned

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.

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.

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.

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new77.04 KB
FAILED: [[SimpleTest]]: [MySQL] 42,107 pass(es), 28 fail(s), and 27 exception(s).
[ View ]

here's the latest work for you to review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new79.55 KB
FAILED: [[SimpleTest]]: [MySQL] 42,113 pass(es), 25 fail(s), and 20 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new99.07 KB
FAILED: [[SimpleTest]]: [MySQL] 42,109 pass(es), 12 fail(s), and 2 exception(s).
[ View ]

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.

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

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

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?

Status:Needs review» Needs work
StatusFileSize
new88.19 KB
PASSED: [[SimpleTest]]: [MySQL] 42,119 pass(es).
[ View ]

Doh. Stray elements killed

Status:Needs work» Needs review

Status:Needs work» Needs review
StatusFileSize
new88.8 KB
FAILED: [[SimpleTest]]: [MySQL] 42,016 pass(es), 18 fail(s), and 2,970 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new89.21 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new89.21 KB
FAILED: [[SimpleTest]]: [MySQL] 42,200 pass(es), 6 fail(s), and 65 exception(s).
[ View ]

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.

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new89.9 KB
FAILED: [[SimpleTest]]: [MySQL] 42,563 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new89.94 KB
PASSED: [[SimpleTest]]: [MySQL] 42,568 pass(es).
[ View ]

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.

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.

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.

StatusFileSize
new89.89 KB
PASSED: [[SimpleTest]]: [MySQL] 42,562 pass(es).
[ View ]

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

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.

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.

Yup, my comment was based on

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

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.

> 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.

@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.

Status:Needs work» Needs review
StatusFileSize
new91.25 KB
PASSED: [[SimpleTest]]: [MySQL] 42,779 pass(es).
[ View ]

Ok new version with localisation working as it should.

StatusFileSize
new11.88 KB

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

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.

Also after seeing #1817748: /admin/reports/dblog does not display the results using the correct date format specified for the site 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.

StatusFileSize
new106.9 KB
FAILED: [[SimpleTest]]: [MySQL] 42,727 pass(es), 3 fail(s), and 23 exception(s).
[ View ]

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

Status:Needs review» Needs work
Issue tags:+Needs screenshot

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new109.94 KB
FAILED: [[SimpleTest]]: [MySQL] 42,766 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new109.95 KB
FAILED: [[SimpleTest]]: [MySQL] 46,176 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Wrong key name in my simplified code.

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

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

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

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

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?

"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.

@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.

will do tonight

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

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.

StatusFileSize
new113.31 KB
FAILED: [[SimpleTest]]: [MySQL] 46,189 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new3.36 KB

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

Status:Needs work» Needs review

sending to testbot

Issue summary:View changes

Updated issue summary to relect current work and scope

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.

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.

+++ 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)

I'll try to post an updated patch tonight.

Status:Needs work» Needs review
StatusFileSize
new111.16 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/System/DateTimeTest.php.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new112.56 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

Cleaned up the dirty reroll, send to testbot

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new112.56 KB
FAILED: [[SimpleTest]]: [MySQL] 46,411 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

this fixes the misnamed update function.

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

Status:Needs work» Needs review
StatusFileSize
new112.91 KB
FAILED: [[SimpleTest]]: [MySQL] 46,436 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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.

StatusFileSize
new3.67 KB

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

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.

Status:Needs work» Needs review
StatusFileSize
new112.91 KB
PASSED: [[SimpleTest]]: [MySQL] 46,392 pass(es).
[ View ]

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.

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 ;)

StatusFileSize
new373.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571632-200.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

StatusFileSize
new185.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571632-201.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new112.59 KB
PASSED: [[SimpleTest]]: [MySQL] 47,564 pass(es).
[ View ]

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

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.

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.

Assigned:marcingy» Unassigned

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

Status:Needs review» Needs work

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.

StatusFileSize
new113.34 KB
FAILED: [[SimpleTest]]: [MySQL] 48,016 pass(es), 4 fail(s), and 2,496 exception(s).
[ View ]

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.

Status:Needs work» Needs review

go testbot go

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.

I'll consume this feedback tonight.

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

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?

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

Status:Needs work» Needs review
StatusFileSize
new12.81 KB
new114.83 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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..

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.

+++ 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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new3.11 KB
new115.57 KB
FAILED: [[SimpleTest]]: [MySQL] 47,822 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new15.15 KB
new123.19 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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

<?php
/**
* 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 screenshot, -API change, -d8ux, -Configuration system

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

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 screenshot, +API change, +d8ux, +Configuration system

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

Status:Needs work» Needs review
StatusFileSize
new123.19 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

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 screenshot, -API change, -d8ux, -Configuration system

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

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 screenshot, +API change, +d8ux, +Configuration system

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new121.64 KB
FAILED: [[SimpleTest]]: [MySQL] 47,697 pass(es), 44 fail(s), and 19 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new118.44 KB
FAILED: [[SimpleTest]]: [MySQL] 48,154 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Reroll with yml file

StatusFileSize
new123.19 KB
FAILED: [[SimpleTest]]: [MySQL] 48,146 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new5.59 KB
new123.84 KB
FAILED: [[SimpleTest]]: [MySQL] 48,158 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new3.22 KB
new125.29 KB
FAILED: [[SimpleTest]]: [MySQL] 48,174 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

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

<?php
  $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.

Status:Needs work» Needs review
Issue tags:-Needs issue summary update, -Needs screenshot, -d8ux, -Configuration system
StatusFileSize
new125.35 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

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.

I certainly didnt intend to nuke all these.

Assigned:chx» Unassigned

I am done.

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.

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.

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)

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.

Issue summary:View changes

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

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

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!

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.

Status:Needs review» Reviewed & tested by the community

Oops cross post. Great!!

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs issue summary update, -Needs screenshot, -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.

Status:Reviewed & tested by the community» Needs review
Issue tags:+Needs issue summary update, +Needs screenshot, +API change, +d8ux, +Configuration system
StatusFileSize
new256.62 KB
PASSED: [[SimpleTest]]: [MySQL] 48,544 pass(es).
[ View ]

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

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

StatusFileSize
new125.35 KB
Test request sent.
Previous result: FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Just a re-roll for head.

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

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

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.

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.

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

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

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

StatusFileSize
new125.35 KB
PASSED: [[SimpleTest]]: [MySQL] 48,688 pass(es).
[ View ]

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

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

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

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.

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 screenshot, +API change, +d8ux, +Configuration system

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

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

Status:Needs work» Needs review

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

Status:Needs review» Reviewed & tested by the community

Second try

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.

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?

Issue summary:View changes

update issue description with follow up issues.

StatusFileSize
new125.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571632_280.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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 screenshot, -API change, -d8ux, -Configuration system

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

Status:Needs work» Needs review
Issue tags:-Needs issue summary update, -Needs screenshot, -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.

Status:Needs review» Needs work
Issue tags:+Needs issue summary update, +Needs screenshot, +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.

Status:Needs work» Needs review
StatusFileSize
new125.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1571632_285.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Quick reroll lets get this monster in...

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

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

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 screenshot, +API change, +d8ux, +Configuration system

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

Status:Needs work» Needs review
StatusFileSize
new123.79 KB
FAILED: [[SimpleTest]]: [MySQL] 48,385 pass(es), 45 fail(s), and 52 exception(s).
[ View ]

StatusFileSize
new125.35 KB
PASSED: [[SimpleTest]]: [MySQL] 48,737 pass(es).
[ View ]

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 screenshot, -API change, -d8ux, -Configuration system

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

Status:Needs work» Needs review
Issue tags:+Needs issue summary update, +Needs screenshot, +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.

Status:Needs review» Reviewed & tested by the community

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

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. :(

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.

Status:Reviewed & tested by the community» Needs work

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

Pages