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
Comment | File | Size | Author |
---|---|---|---|
#306 | 1571632_306.patch | 106.74 KB | cosmicdreams |
#302 | 1571632-301.patch | 105.85 KB | swentel |
#302 | interdiff.txt | 932 bytes | swentel |
#301 | 1571632_301-fixed.patch | 390.11 KB | cosmicdreams |
#301 | interdiff.txt | 930 bytes | cosmicdreams |
Comments
Comment #1
cosmicdreams CreditAttribution: cosmicdreams commentedFirst Try
Comment #3
cosmicdreams CreditAttribution: cosmicdreams commentedThing I found the typo
Comment #5
cosmicdreams CreditAttribution: cosmicdreams commentedforgot 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)
Comment #6
cosmicdreams CreditAttribution: cosmicdreams commentedtagging
Comment #7
tim.plunkettThis can be combined.
The first call to config->get still has the default value.
These both still pass a default value
Second call passes a default value.
Might as well remove the call to t() while you're here.
Comment #8
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a patch with those revisions
Comment #10
cosmicdreams CreditAttribution: cosmicdreams commentedanother try
Comment #11
socketwench CreditAttribution: socketwench commentedLooks good to me.
Comment #12
cosmicdreams CreditAttribution: cosmicdreams commentedRerolled the patch so that it uses yml as a the file format for exported config.
Comment #13
dawehnerI scanned the patch and didn't found something which shines out to be wrong, so i consider this patch to be RTBC.
A little out of scope to remove the t() here, but i think that's okay.
Comment #14
dawehnerThat's not really a hook, see system_cron_settings_submit for another example.
Comment #15
cosmicdreams CreditAttribution: cosmicdreams commentedIt 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?
Comment #16
cosmicdreams CreditAttribution: cosmicdreams commentedreroll
#13: I've been told to remove t() whenever I'm working on patches. Just following instructions.
Comment #18
cosmicdreams CreditAttribution: cosmicdreams commentedLooks 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.
Comment #19
aspilicious CreditAttribution: aspilicious commentedComment #20
aspilicious CreditAttribution: aspilicious commentedSomeone in irc asked for a diff so he could see what I did. Here it is...
Conflicted had to reapply
Conflicted reapplied
Conflicted reapplied
Same
change 2
Change 1
15 days to next Drupal core point release.
Comment #21
disasm CreditAttribution: disasm commentedI 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.
Comment #22
disasm CreditAttribution: disasm commentedre-attached with the right status.
Comment #23
aspilicious CreditAttribution: aspilicious commented? This test look strange, it does a variable_get but it's not added to a variable. This test should be looked at closely
Trailing whitespace
Again, testt look strange
Same
15 days to next Drupal core point release.
Comment #24
disasm CreditAttribution: disasm commentedRemoved 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.
Comment #26
disasm CreditAttribution: disasm commented#24: drupal-change_to_config_for_tz-1571632-24.patch queued for re-testing.
Comment #27
sunComment #28
Dries CreditAttribution: Dries commentedI 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.
Comment #29
sunThe keys need to be updated for the new guidelines: http://drupal.org/node/1667896
As also explained in the guidelines, we need to use system_config_form() now:
http://drupal.org/node/1667896
The separate $config variable looks unnecessary in these cases.
Comment #30
sunAll config system conversions are API changes, so tagging as such.
Comment #31
aspilicious CreditAttribution: aspilicious commentedWhat about
I can't see any update function. Did I missed it in my review?
Comment #32
sunAfter reading what this setting is actually for,
timezone.empty_message
sounds a bit misleading:How about this instead?
Comment #33
alexpottHow about
timezone.user.not_set_warning
? Since a user doesn't have an "empty" timezone - they haven't set it :)Comment #34
cosmicdreams CreditAttribution: cosmicdreams commentedLooks like this is still assigned to me. I'll try to incorporate the above feedback on this tonight.
Comment #35
aspilicious CreditAttribution: aspilicious commentedUntested and the weekday stuff needs to be updated
Comment #37
aspilicious CreditAttribution: aspilicious commented#1704196: Remove Config's dependencies on procedural Drupal code in includes/common.inc :)
Comment #38
aspilicious CreditAttribution: aspilicious commented#35: drupal-change_to_config_for_tz-1571632-35.patch queued for re-testing.
Comment #40
aspilicious CreditAttribution: aspilicious commentedShould fix th installer
Comment #41
aspilicious CreditAttribution: aspilicious commentedI 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.
Comment #42
alexpottNeed to remove @see system_settings_submit()
Apart from that nitpick - looks great!
Comment #43
alexpottAh... doesn't look so great...
Typo: Thrusday
Comment #44
aspilicious CreditAttribution: aspilicious commentedWhen config files are translatable we just could use the name :)
But as far I can remember they aren't at the moment
Comment #45
alexpott@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
Comment #46
alexpottI 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 :)
Comment #47
aspilicious CreditAttribution: aspilicious commentedI think that would screw up forms that have automagicly config setting/saving in the future (like the good old system_settings_form)
Comment #48
alexpottHa... didn't mean it like that :)
I meant with your change something along the lines of:
is much easier. :)
Comment #49
sunLet'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.
Comment #50
aspilicious CreditAttribution: aspilicious commentedComment #51
sunNot 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."
Comment #52
aspilicious CreditAttribution: aspilicious commentedI prefer
timezone.user.warn_unset
Comment #53
alexpottHow 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
Comment #54
aspilicious CreditAttribution: aspilicious commentedI'm into #53 lets see what others think
Comment #55
aspilicious CreditAttribution: aspilicious commented#54: drupal-change_to_config_for_tz-1571632-54.patch queued for re-testing.
Comment #56
disasm CreditAttribution: disasm commentedHere'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.
Comment #57
cosmicdreams CreditAttribution: cosmicdreams commented#56: drupal-change_to_config_for_tz-1571632-56.patch queued for re-testing.
Comment #58
BrockBoland CreditAttribution: BrockBoland commentedNeeds issue summary
Comment #59
justafishPatch 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.
Comment #61
justafishBumping up system update hook number.
Comment #62
KarenS CreditAttribution: KarenS commentedThis 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.
Comment #63
justafishAgreed. Patch changes first_weekday to first_day
Comment #64
aspilicious CreditAttribution: aspilicious commentedIf 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.
Comment #65
KarenS CreditAttribution: KarenS commentedWhy change it at all? First_day is fine.
Comment #66
sunI'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. ;)
Comment #67
aspilicious CreditAttribution: aspilicious commentedI think the system update number needs to go up again.
Comment #68
aspilicious CreditAttribution: aspilicious commented#63: rename-first_weekday-to-first_day-1571632-63.patch queued for re-testing.
Comment #70
justafishIt should just need rerolling as stuff around it has changed.
Comment #71
justafishComment #73
disasm CreditAttribution: disasm commented#70: rename-first_weekday-to-first_day-1571632-70.patch queued for re-testing.
Comment #75
justafishLet me paint this moving car some more.. ;)
Comment #76
cosmicdreams CreditAttribution: cosmicdreams commentedLooks good to me
Comment #77
sunThanks @justafish!
I reviewed it once more and only see a few minor issues, which we can hopefully fix very quick:
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]
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:Same as above, but here we can replace each second line with the first (so that the comments appear above).
Comment #78
justafishdate_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.
Comment #80
aspilicious CreditAttribution: aspilicious commented#78: drupal-change_to_config_for_tz-1571632-78.patch queued for re-testing.
Comment #81
KarenS CreditAttribution: KarenS commentedThere 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.
Comment #82
KarenS CreditAttribution: KarenS commentedI created a META issue about date/time configuration at #1763754: [meta] Date and Time configuration
Comment #83
justafishSeems reasonable to me and no one has voiced any objections.
Comment #84
sunThanks 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.
Comment #85
cosmicdreams CreditAttribution: cosmicdreams commentedStill 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
Comment #86
Lars Toomre CreditAttribution: Lars Toomre commentedJust a quick note... recognizing this was not complete.
Maybe use $config = config('system.date') here so two config objects never initiated?
Comment #87
cosmicdreams CreditAttribution: cosmicdreams commentedThanks 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()
Comment #88
cosmicdreams CreditAttribution: cosmicdreams commentedAfter 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.
Comment #89
cosmicdreams CreditAttribution: cosmicdreams commentedForgot the patch, here it is. And just for fun, let's have the tests run.
Comment #91
cosmicdreams CreditAttribution: cosmicdreams commentedWow, only 64 fails. I expected more.
Comment #92
chx CreditAttribution: chx commentedAs per IRC discussion I will finish this. Thanks for the work so far.
Comment #93
KarenS CreditAttribution: KarenS commentedOK, 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.
Comment #94
KarenS CreditAttribution: KarenS commentedWe 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.
Comment #95
cosmicdreams CreditAttribution: cosmicdreams commentedThanks 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.
Comment #96
KarenS CreditAttribution: KarenS commentedOK, I got rid of a bunch of the test failures. There is still more to do :)
Comment #97
KarenS CreditAttribution: KarenS commentedSetting to needs review just to see how badly it fails now. I tried running all the tests locally but ran out of memory.
Comment #99
KarenS CreditAttribution: KarenS commentedAnd 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.
Comment #100
Lars Toomre CreditAttribution: Lars Toomre commentedI 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.
Comment #101
Lars Toomre CreditAttribution: Lars Toomre commentedSorry did not mean to change status.
Comment #102
chx CreditAttribution: chx commentedRerolled against HEAD.
Comment #104
chx CreditAttribution: chx commentedMaybe 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.
Comment #106
chx CreditAttribution: chx commentedComment #108
KarenS CreditAttribution: KarenS commentedYikes! 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.
Comment #109
chx CreditAttribution: chx commentedAh 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.
Comment #111
KarenS CreditAttribution: KarenS commentedMade 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.
Comment #112
KarenS CreditAttribution: KarenS commentedMy interdiff is between my patch and the one in 106, because I uploaded my patch without realizing chx had made a new patch :)
Comment #114
KarenS CreditAttribution: KarenS commentedDown 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.
Comment #115
KarenS CreditAttribution: KarenS commentedAlso, it turns out this issue is absolutely not a novice issue :)
Comment #116
chx CreditAttribution: chx commentedKaren, why localization isnt working? Isn't that what core/modules/locale/lib/Drupal/locale/LocaleConfigSubscriber.php is about?
Comment #117
KarenS CreditAttribution: KarenS commentedThe 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.
Comment #118
chx CreditAttribution: chx commentedI 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.
Comment #119
marcingy CreditAttribution: marcingy commentedComment #120
KarenS CreditAttribution: KarenS commentedThere 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
or
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.
Comment #121
cosmicdreams CreditAttribution: cosmicdreams commentedI 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.
Comment #122
KarenS CreditAttribution: KarenS commentedThe 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.
Comment #123
cosmicdreams CreditAttribution: cosmicdreams commentedforgive the needless bump. I just want folks to know that I plan on working on this issue tonight. (9 hours from now)
My goals:
Comment #124
marcingy CreditAttribution: marcingy commentedComment #125
KarenS CreditAttribution: KarenS commentedI 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.
Comment #126
cosmicdreams CreditAttribution: cosmicdreams commentedAs 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.
Comment #127
marcingy CreditAttribution: marcingy commentedSo 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.
Comment #128
cosmicdreams CreditAttribution: cosmicdreams commentedThat'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.
Comment #129
marcingy CreditAttribution: marcingy commentedSo 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.
Comment #130
cosmicdreams CreditAttribution: cosmicdreams commentedMy 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.
Comment #131
cosmicdreams CreditAttribution: cosmicdreams commentedhere's the latest work for you to review
Comment #133
cosmicdreams CreditAttribution: cosmicdreams commentedThis 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
Comment #135
marcingy CreditAttribution: marcingy commentedOk 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.
Comment #137
cosmicdreams CreditAttribution: cosmicdreams commentedGreat 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
Comment #138
marcingy CreditAttribution: marcingy commented#135: 1571632_134.patch queued for re-testing.
Comment #139
cosmicdreams CreditAttribution: cosmicdreams commentedYea, all of this file stuff, I don't know why this is here. Can you please explain what this is used for?
Comment #140
marcingy CreditAttribution: marcingy commentedDoh. Stray elements killed
Comment #141
marcingy CreditAttribution: marcingy commentedComment #142
cosmicdreams CreditAttribution: cosmicdreams commentedHere'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.
Comment #144
cosmicdreams CreditAttribution: cosmicdreams commentedremoved the intl formats, kept the php formats, and changed the code to use the php formats. Let's see what I missed.
Comment #146
cosmicdreams CreditAttribution: cosmicdreams commentedRan 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.
Comment #147
marcingy CreditAttribution: marcingy commentedSetting 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.
Comment #148
marcingy CreditAttribution: marcingy commentedGrabbing 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.
Comment #149
chx CreditAttribution: chx commentedIf 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
Comment #150
cosmicdreams CreditAttribution: cosmicdreams commentedHere's the most recent patch. I manually went through these pages so I can find what was going wrong.
Comment #151
marcingy CreditAttribution: marcingy commentedBack to needs work because there is still stuff that needs done with this patch.
Comment #152
marcingy CreditAttribution: marcingy commentedComment #153
cosmicdreams CreditAttribution: cosmicdreams commentedI'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.
Comment #154
marcingy CreditAttribution: marcingy commentedPlease 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.
Comment #155
cosmicdreams CreditAttribution: cosmicdreams commentedHello 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.
Comment #156
cosmicdreams CreditAttribution: cosmicdreams commentedThis patch reverts the name change of the "edit" link. It now says edit again. We should keep the dropbutton though.
Comment #157
marcingy CreditAttribution: marcingy commentedCosmicdreams 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.
Comment #158
cosmicdreams CreditAttribution: cosmicdreams commentedWhen 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.
Comment #159
marcingy CreditAttribution: marcingy commentedYup, my comment was based on
Comment #160
cosmicdreams CreditAttribution: cosmicdreams commentedand 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.
Comment #161
chx CreditAttribution: chx commented> 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.
Comment #162
cosmicdreams CreditAttribution: cosmicdreams commented@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.
Comment #163
marcingy CreditAttribution: marcingy commentedOk new version with localisation working as it should.
Comment #164
cosmicdreams CreditAttribution: cosmicdreams commentedHere's an interdiff of 156 and 163, mostly for me to help the review of this go easier.
Comment #165
cosmicdreams CreditAttribution: cosmicdreams commentedsmall code quibbles and documentation critiques
+1 for simplifying system_init and not having to load language interface here.
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?
This break seems to say that we'll stop the foreach once we have a non-empty $date_format.
We should document that better.
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.
Comment #166
marcingy CreditAttribution: marcingy commentedAlso after seeing #1817748: /admin/config/regional/date-time does not show the correct default format for short and medium format I have a feeling there are some places that use format_date where strings for type have not been updated appropriately. Adding to my list of items to check as part of re-roll for review above.
Comment #167
marcingy CreditAttribution: marcingy commentedRe-roll
* Updates format_date calls that we missed before
* Some code simplification as per review in #165.
Comment #168
aspilicious CreditAttribution: aspilicious commentedHere you use double quotes
Here single ones. Use always single ones in these cases.
Trailing whitespace
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.
GetS
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.
Comment #169
marcingy CreditAttribution: marcingy commentedComments 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.
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.
Comment #171
marcingy CreditAttribution: marcingy commentedWrong key name in my simplified code.
Comment #173
marcingy CreditAttribution: marcingy commented#171: 1571632-172.patch queued for re-testing.
Comment #174
Gábor HojtsyI 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?
Comment #175
cosmicdreams CreditAttribution: cosmicdreams commented"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:
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.
Comment #176
Gábor Hojtsy@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.
Comment #177
cosmicdreams CreditAttribution: cosmicdreams commentedwill do tonight
Comment #178
aspilicious CreditAttribution: aspilicious commented#171: 1571632-172.patch queued for re-testing.
Comment #179
aspilicious CreditAttribution: aspilicious commentedIt 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. :)
Comment #181
dawehnerHere is a change which adapts the part in views for the new date formats.
Comment #182
cosmicdreams CreditAttribution: cosmicdreams commentedsending to testbot
Comment #182.0
cosmicdreams CreditAttribution: cosmicdreams commentedUpdated issue summary to relect current work and scope
Comment #183
aspilicious CreditAttribution: aspilicious commentedCan we have screenshot of the adjusted admin pages?
Can we have a *small* upgrade path tests that verifies the settings are converted correctly?
Comment #185
cosmicdreams CreditAttribution: cosmicdreams commentedYea 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.
Comment #186
cosmicdreams CreditAttribution: cosmicdreams commentedcustom should not be prefixed with 'system_'
it's still custom' => format_date($time, 'custom', 'c', $timezone)
Comment #187
cosmicdreams CreditAttribution: cosmicdreams commentedI'll try to post an updated patch tonight.
Comment #188
cosmicdreams CreditAttribution: cosmicdreams commentedwow, I totally thought I had reposted the fixed patch. Well here it is for the testbot.
Comment #190
cosmicdreams CreditAttribution: cosmicdreams commentedCleaned up the dirty reroll, send to testbot
Comment #192
cosmicdreams CreditAttribution: cosmicdreams commentedthis fixes the misnamed update function.
Comment #193
aspilicious CreditAttribution: aspilicious commentedDon't we have a language selector form item now? (I can be wrong.
GetS, check other functions as well
trailing whitespace
Is this going to change? Do we need a todo?
trailing whitespace
Comment #194
cosmicdreams CreditAttribution: cosmicdreams commentedthis patch cleans up whitespace issues. Let me address your questions
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.
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.
Comment #195
cosmicdreams CreditAttribution: cosmicdreams commentedand here's the interdiff (if I did it right)
Comment #196
cosmicdreams CreditAttribution: cosmicdreams commentedsomeone was asking for screenshots for some the date format UI change. Here's a series of screenshots that show the result of this patch
Comment #198
cosmicdreams CreditAttribution: cosmicdreams commentedPrevious 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.
Comment #199
aspilicious CreditAttribution: aspilicious commentedTwo minor things. I couldn't find any other issues.
I think this can be rewritten as
return $config->get('timezone.default') ?: : @date_default_timezone_get();
This is hard to read ;)
Comment #200
cosmicdreams CreditAttribution: cosmicdreams commentedI'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.
Comment #201
cosmicdreams CreditAttribution: cosmicdreams commentedignore 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
Comment #203
cosmicdreams CreditAttribution: cosmicdreams commentedlooks 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.
Comment #204
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a rerolled patch, passes the DateTime tests locally
Comment #205
arlinsandbulte CreditAttribution: arlinsandbulte commentedSo, 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.
Comment #206
cosmicdreams CreditAttribution: cosmicdreams commentedyes, 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.
Comment #207
marcingy CreditAttribution: marcingy commentedUnassigning for now as @cosmicdreams seems to be pushing this along nicely.
Comment #208
arlinsandbulte CreditAttribution: arlinsandbulte commented#1802278: Add a Date component to core landed HOORAY!
Comment #209
cosmicdreams CreditAttribution: cosmicdreams commentedcool, 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.
Comment #210
cosmicdreams CreditAttribution: cosmicdreams commentedI 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.
Comment #211
cosmicdreams CreditAttribution: cosmicdreams commentedgo testbot go
Comment #212
Lars Toomre CreditAttribution: Lars Toomre commentedOverall, 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.
This can be shortened with ?: operator.
This needs to be wrapped for 80 characters. Also the default value needs to be changed to "Defaults to 'system_medium'."
This inline comment needs to start with a Capital letter and needs to be wrapped so that it is not longer than 80 characters.
Think you mean 'to' here instead of 'too'.
This also could use '?: operator.
Change to 'handler: Handles'.
Both of these in-line comments need to end in periods '.'.
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.
Change to 'Checks if'. (active verb tense). Also needs to end in a period.
These sentences need to end in periods and warp at or before 80 characters.
Can we add type hinting here?
Missing a period at the end of the line '.'.
I would rewrite this as "For now, this function defaults to 'php'."
Eliminate the ':' and this line will just fit in 80 characters.
This could be type hinted as well and I would add 'Defaults to NULL' at the end of the description.
I think you meant 'If neither is specified, returns FALSE.' Missing the second part.
Something is wrong with this docblock. The @param variables do not match those in the declaration. Is this variable suppose to be $format_info?
This complete docblock could use type hinting.
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?
Comment #214
cosmicdreams CreditAttribution: cosmicdreams commentedI'll consume this feedback tonight.
A note for this review and future reviewers, we are purposefully not using the ?: shorthand.
Comment #215
Lars Toomre CreditAttribution: Lars Toomre commentedOh 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?
Comment #216
cosmicdreams CreditAttribution: cosmicdreams commentedI can add one to the comments if it is necessary.
Comment #217
KarenS CreditAttribution: KarenS commentedI don't understand changes like:
It seems like this should be reduced to the following:
Or code like this:
Which I'm changing to:
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.
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:
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..
Comment #218
cosmicdreams CreditAttribution: cosmicdreams commentedI 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.
You seem to be hard-coding the form_process_date function to always use the php version of the date format pattern.
Comment #219
KarenS CreditAttribution: KarenS commentedYes, 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.
Comment #220
cosmicdreams CreditAttribution: cosmicdreams commentedAlso, adding a comment would be good.
Your previous patch was marked as postponed by the test bot for some reason.
Comment #222
cosmicdreams CreditAttribution: cosmicdreams commentedLooks like the patch in #217 passed the DateTime tests but blew up on the install / upgrade tests.
Comment #223
KarenS CreditAttribution: KarenS commentedFound 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.
Comment #225
KarenS CreditAttribution: KarenS commentedI think we also need to get rid of the following, which is no longer being used:
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.
Comment #227
justafishThese tests are not failing for me locally.
#225: regional-2.patch queued for re-testing.
Comment #229
KarenS CreditAttribution: KarenS commentedNot 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.
Comment #231
andyceo CreditAttribution: andyceo commented#229: 1571632-regional-config-229.patch queued for re-testing.
Comment #233
cosmicdreams CreditAttribution: cosmicdreams commentedAh, 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.
Comment #234
KarenS CreditAttribution: KarenS commentedThe tests that fail pass locally for me, I'm not sure what is going on.
Comment #235
rvilarThe problem was that function system_update_8035() was redeclared. I change the function name to system_update_8036
Comment #237
KarenS CreditAttribution: KarenS commentedThe 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.
Comment #238
marcingy CreditAttribution: marcingy commentedReroll with yml file
Comment #239
marcingy CreditAttribution: marcingy commentedLast version left date.inc in existence this fixes that situation
Comment #241
swentel CreditAttribution: swentel commentedIt's weird, tests also pass locally for me, will investigate some more. In the mean time, rerolled with a couple of changes, interdiff attached.
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.
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.
Comment #242
swentel CreditAttribution: swentel commentedAdded 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.
Comment #244
chx CreditAttribution: chx commentedWorking 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
creates an intl format of 'j M y' and I have my doubts that's a valid intl pattern.
Comment #245
chx CreditAttribution: chx commentedI dunno why I can't interdiff these two but the big change is in
system_date_time_formats
:format_date()
decides to use intl or php based oncanUseIntl
, the test immediately breaks when you enter some format, it gets saved as intl butformat_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.Comment #246
chx CreditAttribution: chx commentedI certainly didnt intend to nuke all these.
Comment #247
chx CreditAttribution: chx commentedI am done.
Comment #248
KarenS CreditAttribution: KarenS commentedGood 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.
Comment #249
KarenS CreditAttribution: KarenS commentedOf the above issues, the need for tests for the intlFormatter and this comment still need to be addressed:
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.
Comment #251
chx CreditAttribution: chx commentedGiven 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)
Comment #252
KarenS CreditAttribution: KarenS commentedProtecting 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.
Comment #252.0
KarenS CreditAttribution: KarenS commentedUpdated issue summary to include important status updates from the comments.
Comment #253
cosmicdreams CreditAttribution: cosmicdreams commentedUpdated summary to include follow up issues, ready to RTBC? patch looks good to me.
Comment #254
aspilicious CreditAttribution: aspilicious commentedI 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!
Comment #255
KarenS CreditAttribution: KarenS commentedI 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.
Comment #256
KarenS CreditAttribution: KarenS commentedOops cross post. Great!!
Comment #257
cosmicdreams CreditAttribution: cosmicdreams commented#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.
Comment #259
cosmicdreams CreditAttribution: cosmicdreams commentedHopefully the fact that I had to use wordpad to increment the update function's number in this patch won't horribly break it.
Comment #260
cosmicdreams CreditAttribution: cosmicdreams commentedjudging by the size of the file I would assume yes. Can someone please update the system_update_8036 to system_update_8037?
Comment #261
marcingy CreditAttribution: marcingy commentedJust a re-roll for head.
Comment #263
andyceo CreditAttribution: andyceo commented#261: 1571632_260.patch queued for re-testing.
Comment #265
marcingy CreditAttribution: marcingy commented#261: 1571632_260.patch queued for re-testing.
Comment #267
aspilicious CreditAttribution: aspilicious commented#261: 1571632_260.patch queued for re-testing.
Comment #268
marcingy CreditAttribution: marcingy commentedChecking the link directly and it is green but the bot has not responded for some reason
Comment #269
marcingy CreditAttribution: marcingy commentedAs the bot does not want to respond....
Comment #271
marcingy CreditAttribution: marcingy commented#269: 1571632_fresh.patch queued for re-testing.
Comment #273
marcingy CreditAttribution: marcingy commented#269: 1571632_fresh.patch queued for re-testing.
Comment #275
cosmicdreams CreditAttribution: cosmicdreams commentedI have no idea what's going on here. There's no reason why tests should be failing.
Comment #276
marcingy CreditAttribution: marcingy commented#269: 1571632_fresh.patch queued for re-testing.
Comment #277
aspilicious CreditAttribution: aspilicious commentedSecond try
Comment #278
Dries CreditAttribution: Dries commentedI 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.
Comment #279
cosmicdreams CreditAttribution: cosmicdreams commentedHello 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?
Comment #279.0
cosmicdreams CreditAttribution: cosmicdreams commentedupdate issue description with follow up issues.
Comment #280
cosmicdreams CreditAttribution: cosmicdreams commentedadvanced the system.install's new update function's number.
Comment #282
cosmicdreams CreditAttribution: cosmicdreams commented#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.
Comment #284
KarenS CreditAttribution: KarenS commentedThis part of the code looks suspicious:
Notice the odd "- $config". That line shouldn't be there.
Comment #285
aspilicious CreditAttribution: aspilicious commentedQuick reroll lets get this monster in...
Comment #287
cosmicdreams CreditAttribution: cosmicdreams commented#285: 1571632_285.patch queued for re-testing.
I don't understand why the patch doesn't even apply anymore.
Comment #289
chx CreditAttribution: chx commentedComment #290
chx CreditAttribution: chx commentedComment #291
cosmicdreams CreditAttribution: cosmicdreams commentedAssuming 290 passes, RTBC again.
Comment #293
cosmicdreams CreditAttribution: cosmicdreams commented#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.
Comment #294
chx CreditAttribution: chx commentedComment #295
marcingy CreditAttribution: marcingy commentedSo glad to see this green again +1 for rtbc from me.
Comment #296
webchickWhile I appreciate that this patch is a huge PITA to re-roll, I'm sorry, but I'm with Dries here. :\
"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. :(
Comment #297
webchick"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.
Comment #298
webchickSorry. Cross-post. With myself! :D Too many tabs. ;)
Comment #299
swentel CreditAttribution: swentel commentedAssuming this will turn green again.
Comment #300
webchickLooks like there are still some remaining hunks?
Comment #301
cosmicdreams CreditAttribution: cosmicdreams commentedupdated the issue to use the proper date format names.
Comment #302
swentel CreditAttribution: swentel commentedI think I (or rather I can trust my ide now) have got them all now.
Comment #303
cosmicdreams CreditAttribution: cosmicdreams commentedsigh, the patch size is way off. must be line endings again. will fix.
EDIT: Cool swentel posted a new one in 301. Let me see if he found the improperly named date formats as well.
Comment #305
KarenS CreditAttribution: KarenS commented#302 should be OK, let's get it tested.
Comment #306
cosmicdreams CreditAttribution: cosmicdreams commentedthis patch slightly alters 302 so that the proper date format names are used. previous patch used 'small' and 'large'. Those date formats don't appear to exist.
Will need to create a follow up issue to look at these tests and improve their logic.
Comment #308
marcingy CreditAttribution: marcingy commented#306: 1571632_306.patch queued for re-testing.
Comment #309
marcingy CreditAttribution: marcingy commentedComment #310
webchickOk, YAY!!! Still applies, so I'm taking the plunge quick before it changes its mind. :)
Committed and pushed to 8.x. WOOHOO. Thanks so much for all the hard work here, folks!
Comment #311
BerdirI just noticed #1858984: Split system date formats from timezone settings did not require any upgrade test changes. So we're missing upgrade path tests for that. And it looks like we're actually completely missing an upgrade path for date formats? we didn't even delete the tables...?
This was mentioned like 5 times in the reviews, but we still forgot it :(
I think that needs a critical follow-up...
Comment #312
cosmicdreams CreditAttribution: cosmicdreams commentedCreated follow up issue: #1860778: Provide an upgrade path for date formats
Comment #314
webchickWe never got a change notice for this. Specifically, #501428: Date and time field type in core is trying to call system_get_date_types() and can't.
Comment #315
chx CreditAttribution: chx commentedhttp://drupal.org/node/1876852 Needs review.
Comment #316
BerdirChange notice looks good.
Noticed this because of the change notice. Can we get a follow-up issue for this?
Shouldn't this be
t('@name format: @date')
, then it is properly translatable and the check_plain() isn't necessary.Comment #317
BerdirComment #319
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.
Comment #319.0
xjmAdd an questions and answers section.