Problem/Motivation
The date field handler in views has a lot of options (see Drupal\views\Plugin\views\field\Date
).
This includes:
- selecting a date format
- specify a custom date format
- select the timezone which should be used to render the field (useful for some rss feeds for example)
- Use both a date as well as a time ago rendered format.
Sadly the timestamp field handler doesn't provide those at all, nor do the Datetime module's field formatters.
Proposed resolution
- Port over the existing features into the views field handlers that are currently being used for base timestamp entity fields
- Both the Datetime field formatters and the base Timestamp formatters need these features.
- Discuss which of those could be dropped
Remaining tasks
Features to port
selecting a date format- Core featurespecify a custom date formatselect the timezone which should be used to render the field (useful for some rss feeds for example)Use both a date as well as a time ago rendered format.Drop timezone support in timeagoSimplify timeago to use a format string instead of umpteen format choices
Still to do: get these into the Timestamp formatters, which are currently missing from the patch (it just covers Datetime formatters so far).
User interface changes
New settings form options for DateTime fields and Timestamp fields.
API changes
Display setting keys have changed for places manually rendering entity fields.
Beta phase evaluation
Issue category | Task because it was outlined as such by the parent META, #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality |
---|---|
Issue priority | Major because while the parent is now down to a major; this isn't critical as it won't lead to a SA down the road, it does restore/add back desired functionality for 8.0.0 |
Prioritized changes | The main goal of this issue is improved user and developer experience; views output will be identical to entity output. See also the rationale outlined in the parent meta. |
Disruption | Minimal. |
Comment | File | Size | Author |
---|---|---|---|
#201 | interdiff-194-201.txt | 5.32 KB | mpdonadio |
#201 | support_all_options-2399211-201.patch | 65.44 KB | mpdonadio |
#194 | interdiff-186-194.txt | 18.14 KB | mpdonadio |
#194 | support_all_options-2399211-194.patch | 65.31 KB | mpdonadio |
#186 | interdiff-185-186.txt | 7.94 KB | mpdonadio |
Comments
Comment #1
BerdirTime ago sounds like a different formatter to me, not a setting.
Not sure about supporting custom date formats, supporting proper localization for them won't be easy.
Comment #2
dawehner@Berdir
Agreed about the time ago.
Regarding the custom one: Isn't it enough to simply specify those to be translatable and be done?
Comment #3
Gábor HojtsyIf those date format settings are stored in configuration, they can be marked translatable. For shipped config, their default configs can be translated and even get a proper string context to aid translators :) From core.data_types.schema.yml:
Comment #4
legolasboI am working on this
Comment #5
legolasboI've created a new field formatter which is a straight port of views' time ago formatter.
Comment #6
legolasboComment #7
yched CreditAttribution: yched commentedThanks @legolasbo !
Just a review of what the code does, not a review of whether it adresses the issue :-)
We can remove the empty lines after function() and foreach()
Did field_load() ever existed ? At any rate, that comment is a bit stale now.
Also, that code block could simply be moved to a ternary ?
$elements[$delta] = array('#markup' => ($item->date ? $this->dateFormat($item->date) : '');
As a user, I don't think I would know what to input here. Can we add some #description ?
Method lacks visibility - protected, I guess ?
Also, the param and its phpdoc should typehint to the right interface
Not good :-)
Comment #8
legolasboThanks for your feedback @yched,
Attached patch fixes the issues you mentioned and adds another formatter "Custom" which enables the user to display a date using a custom format string.
Comment #9
yched CreditAttribution: yched commentedDid not review the new Custom formatter from #8 yet
Still lacks "protected" visibility :-)
Also, method name should be formatDate() - verb first.
Comment #10
yched CreditAttribution: yched commentedNitpick - Lose the empty line ?
There should be an empty line between the closing } of the last method and the closing } of the class.
Methods should be defined after properties
(and typically after the __construct() too - even though I guess that could be debated for static methods)
Same as above
Comment #12
legolasbo@yched some of your comments also applied to the original formatter. I've also corrected them there. Besides that I've also added a timezone override setting to force the rendering in a certain timezone. This works for all formatters except the time ago formatter because a timespan doesn't change if you change the timezone and it seems illogical to me to compare two different timezones.
I think the current patch at least adds all the features mentioned in the issue summary, but still needs some work regarding UX when timezones are forced. I think there should be an option to append the rendered date with the timezone used, in order to alert the user to the fact that the rendered datetime is in a different timezone than expected.. Something like this:
17 jan 2015 - 21:41 (Europe/Amsterdam)
Comment #13
legolasboComment #15
legolasboOops, missed a use statement
Comment #16
legolasboComment #18
legolasboFixed (at least part of) the failing tests by appending the configuration schema and added the option to append the timezone used.
Comment #22
legolasboI could not get the failing tests to pass/fail consistently while trying to find out why the tests fail. When i step trough them using the debugger every assertEqual returns true. The test does fail most of the time, but sometimes it passes miraculously.
I'm at a loss here and welcome anyone to take a stab at this.
The new functionality also needs to have some tests written. Tagged the issue accordingly
Comment #23
dawehner.
Comment #24
legolasboI found out that a difference in the order of the array keys caused the tests to fail. Since the order doesn't influence any functionality I resorted to sorting the arrays before assertEqual() which makes the tests pass again.
Comment #25
vijaycs85Reroll of #24. Still needs test for new formatters.
Comment #26
jhodgdonThere's a related issue, just want to point out. Its issue summary is bare. Will update it.
Comment #27
jhodgdonSo... Shouldn't this patch also make the base EntityViewsData class use the 'field' plugin for the timestamp fields, here:
That last should then be:
which would make the entity views use the Field formatter for their date fields. Either that or we need to make that change on the parent issue. Or it needs to be a new issue? I'm not sure which, but right now it's not done on either issue.
Comment #28
dawehner@jhodgdon
Please don't, this issue should be in scope ... converting all the places, fixing potential issues, is IMHO part of a new issue.
Comment #29
jhodgdonOK. Well we'll need to either do the update on the patch on #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency then or create another issue.
Comment #30
jhodgdonAnd by the way I'm now keeping track of what still needs to be done in the "convert the Entity base fields to use Field rendering" effort on #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality. so it won't fail to happen. ;)
Comment #31
dawehnerIn general, I'm glad we made progress here!
First question: The timestamp handler in views is part of views module itself, so it could be applied to for example the node.created field. At the same time, the
TimestampItem
is in core, so I kinda think this should belong into core .I like the separation of datetime_custom and datetime_time_ago.
I'm curious, can we apply any kind of validation here?
Nitpick: Two empty lines
Comment #32
Gábor Hojtsy@vijaycs85: still working on this one?
Elevating to major being a subissue of #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality.
Comment #33
vijaycs85I will try to address #31 and some test.
Comment #34
vijaycs85here is a reroll.
#31.1 - the field is from datetime module, so the schema.
#31.3 - Doesn't look like we do any validation - core/modules/system/src/Form/DateFormatFormBase::dateTimeLookup
#31.4 - Fixed.
P.S: didn't manage to update the tests.
Comment #35
jhodgdonNitpicks:
a)
Should be contains ... DateTimeCustomFormatter
b) I do not think @ingroup field_formatter should be on the base formatter class. If you look at https://api.drupal.org/api/drupal/core!modules!field!field.api.php/group... the only things listed there are the base classes/interfaces for the entire formatter system. DateTimeBase doesn't belong there.
c) In the base formatter class:
Needs blank line between param and return. Also the param needs a type (string) in the @param line.
d)
Really, we're doing this??!? How about using formatString()? Putting EM tags in directly and substituting variables like that seems wrong.
e) In the Ago formatter:
I had to go look up DateFormatter::formatInterval() and read the code to figure out what "Granularity" and "The number of different time units to display" meant. I think this needs better/more documentation. What is happening in the formatInterval function is:
- It goes down a list of units: year, month, week, day, hour, minute, second -- starting at year.
- It checks to see if the interval is bigger than that number of seconds. If it is, that counts as one "unit to display". The output is formatted and the interval is subtracted off.
- It continues until the required granularity is reached (a certain different number of time units have been added to the display).
- This whole way of doing things in the formatInterval() function is also wrong -- I filed #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known. Just wrong. Can't believe this is in Core.
f) I guess this all needs tests.
Comment #36
xjmComment #37
mpdonadioI'll bring this home. Is the feedback in #35, and some unit tests, the only outstanding work?
Comment #38
mpdonadio#34 doesn't apply anymore. I'll post a reroll tonight before I start work on this.
Comment #39
mpdonadioRebase w/ choosing the patch over HEAD. All of the conflicts were in the Migrate tests.
Comment #40
mpdonadioNow onto the feedback.
Comment #41
mpdonadioI read through the issue a few times, and I think all feedback has been addressed, other than test coverage for the new formatters. If I missed anything, let me know.
I also did a visual pass through the three new files (DateTimeCustomFormatter.php, DateTimeFormatterBase.php, and DateTimeTimeAgoFormatter.php) and fixed some coding standard things as well as a few things PhpStorm was complaining about.
OK, let's see if I broke anything with the existing tests before moving onto new tests.
Comment #42
mpdonadioWhy does DateTimeFieldTest rely on the system timezone (not the test system it is running in) being UTC?
Comment #43
mpdonadioFirst pass at test coverage for the new formatters.
Comment #44
jhodgdonThanks! Changes in #41 addressed most of my comments from #35. I still think that the settings form for Granularity may need more documentation for the user though:
Do you think they'll actually understand what this means?
Comment #45
mpdonadio#44 May bad; I forgot about that one. That description pretty much matches the docblock DateFormatter::formatInterval(), which is also obtuse.
Comment #46
jhodgdonWell that is even worse, because really what happens if you say granularity is 2, and say your time interval is 3 hrs, 20 mins, and 22 seconds, you'll get "3 hours 20 minutes" as output, whereas that doc in #44 would imply that you'd get years and months.
Comment #47
dawehnerWhat about not adding the granularity for now? Isn't that a feature which did not existed in views at all?
That line of documentation isn't helpful
Comment #48
mpdonadio#47-1: It is called Interval in the Date module for Drupal 7, and is exposed to Views 3 when you add a Date field to the row output (I double checked on a D7 site). The description there is "How many time units should be shown in the 'time ago' string.". See date_interval_formatter_settings_form()
#47-2: Unhelpful, but consistent all of the formatters in \Drupal\Core\Field\Plugin\Field\FieldFormatter and the core modules that define field formatters in HEAD.
Comment #49
dawehnerRight, but in order to resolve the critical, we just have to get the options which are currently supported by views, just saying.
Well, I'd prefer sanity over consistency.
Comment #50
mpdonadioOK, @dawehner and I had a quick IRC conversation this morning. Since we can't think of better language, we are going to go with the Drupal 7 label / description for the granularity (see my link in #48). We can create a followup to address this, though, as it is confusing but not critical for the patch.
We also decided that the short description on the class was more better in this patch, even though it isn't consistent with the other field formatters.
I think this is good to go.
Comment #51
dawehnerI guess we can make it protected on the longrun?
Let's replace the usage of REQUEST_TIME, see #2459155: Remove REQUEST_TIME from bootstrap.php
Note: We usually don't use those kind of style of comments.
Let's not use format_string anymore, but
SafeMarkup::format()
+1 for not using assertIdentical as more strict checking is not necessarily a better idea, due to its costs during changes of code.
Comment #52
mpdonadioOK, #51 should be addressed. Per @alexpott, REQUEST_TIME is still in simpletest, so I didn't change that.
I have no idea if this will pass. There are a few things really wrong. Unfortunately, I think HEAD and the patch have problems, and both the formatters and the tests have problems.
One, a Date-Only field renders out with the time. HEAD does this, too. This is a regression from Drupal 7. Haven't dug into this.
Two, with HEAD, the rendered time for a Date-Only field is always 12:00. With the patch (tested back to #39, it is 12:00 adjusted for the user's system timezone (but not adjusted for DST). I'm UTC-5 w/o DST, so it prints out as 07:00 for me.
Oddly enough, the Date+Time seems to work as expected, but I haven't actually looked at storage to see the values there.
I think the timeago formatters are OK, but I didn't adjust the test expectation when I started chasing my tail with timezone problems. I'm not 100% sure they are immune to the TZ problem mentioned above.
I wanted to get others thoughts on this before just make the patch pass in case there is actually a problem with the code, especially since I jumped in here halfway.
Comment #53
dawehnerThank you for addressing my points of the review! I'll read your comment later today. Are you interested in working on #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter once this issue is in?
Comment #54
mpdonadioAlso, elevating this to Critical as the parent is a critical.
Comment #56
effulgentsia CreditAttribution: effulgentsia commentedThanks for the great work on this. I don't think it's a release blocker though, so back to Major. The proposed resolution in #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality distinguishes between the children that are required for respecting access control (critical) vs. those that add/restore all the desired options (important, but not critical), and I think this child is the latter.
Comment #57
effulgentsia CreditAttribution: effulgentsia commentedApologies. I see now that #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter, which is critical due to access control, is postponed on this, which does make this critical. Sorry for the noise.
Comment #58
mpdonadioI'll try to catch @dawehner on IRC tomorrow and chat about this. If we think we can proceed on #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter w/o this patch in final form, I will unpostpone and demote back to Major.
Comment #59
effulgentsia CreditAttribution: effulgentsia commentedPer #2393339-57: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality, tagging for critical office hours, but if there's a reason to not have this particular child issue in that list (e.g., if it's already almost done and the people already working on this don't want new help), please untag it.
Comment #60
dawehnerThe date field in D7 is quite elaborated, as it takes into account both the user and the server timezone.
@mpdonadio
I'm curious whether we could defer that kind of work into a follow up and enable you to work on the views issue at the same time?
Comment #61
mpdonadioThrowing this at TestBot. DateTimeFieldTest and all PhpUnit work locally for me.
I think I may have made an out of scope change, but DateTimeFieldTest wouldn't pass w/o it. I will explain all when I have time, but reading the interdiff will show the general problems.
Nobody is listed in MAINTAINERS for the DateTime module. Who unofficially owns it?
Oh, I hate timezones.
Comment #62
YesCT CreditAttribution: YesCT commented.
Comment #63
mpdonadioOK, explanation.
WebTestBase doesn't set a timezone, so tests will run in your default timezone per PHP. This patch explicitly sets one for predictable results, and picks one near the extreme end.
Storage uses UTC.
As far as I can tell, Date+Time gets handled properly through UI -> DB -> UI roundtrip. Objects get created w/ the UTC timezone, and then you be can set whatever timezone you need. Yay.
Storage for DateOnly just stores the date. This is where things get weird. When you read from storage, just the Y/m/d get read. The time portion is set to the local time. There is a helper function, datetime_date_default_time(), to set this back to noon. The problem is if you set the timezone before doing this, the day may leap forward or backward. Blech. So, this version of the patch sets the default time before adjusting the timezone in all of the formatters. As I write this out, I think we can move this logic into DateTimeFormatterBase::setTimeZone(), but it still needs to be done for timeago, even though a timezone isn't needed for that (I need to play with this a little).
The above also happened in the field widget. I fixed it there, too.
Comment #64
mpdonadioOK, I am really confused now that I have read some related issues. The IS mentions the "timestamp" handler, but the first patch (and what I continued to work on) updated the "datetime" handlers. #27 mentions this, but #28 vetos it.
On the remaining tasks, "select the timezone ..." was struck as a remaining task, but wasn't in the patch that was posted when it was struck out.
So, at the least this needs an IS update and a Beta Eval. Should the IS just be updated to reflect what the current patch does and we will handle the timezone selection for "datetime" and "timestamp" fields as a followup?
Comment #65
mpdonadioThis doesn't actually block #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter. They are related, but this does not need to be in for work to begin on that. I don't think this needs to be critical, either, since it is only adding formatters and not directly making the change to use field formatters instead of views plugins.
Comment #66
dawehnerDon't we want to pick up the timezone from the current user?
I guess we need the timezone cache contexts added here.
Comment #67
mpdonadio#66-1, this is already done. `$this->setTimeZone($date)` calls `DateTimeFormatterBase::setTimeZone()`, which takes into account the timezone override setting and the system/user timezone logic.
#66-2, added the cache context to the render array.
Comment #68
dawehnerIt looks really great so far! We also have additional test coverage.
I'm curious whether we have to pass along the langcode somehow, similar to the discussions, which happen around #2449597: Number formatters: Make it possible to configure format_plural on the formatter level (comment 30 for example)
Nitpick: Using $form feels just more natural here as variable name.
Comment #69
mpdonadio#68-1: the constructor for DrupalDateTime() already sets up the language for the $date object:
I guess the option we would have is to replace all of the formatter calls with
or maybe
and then add the proper cache context tag to the render array.
Is the current language ever different than the language of the view? As a stupid American :) I am not terribly familiar with this aspect of Drupal.
#68-2: Changed to $form. Poked through core, and it isn't terribly consistent with $element vs $form for the settings forms in the plugins, but I think you are right here.
Also cleaned up some PhpStorm warnings, and add calls to parent methods where necessary.
Comment #70
rteijeiro CreditAttribution: rteijeiro commentedCode looks good. Just fixed a few nitpicks.
Comment #71
jhodgdonSo... I've also been following/reviewing #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known, already marked related by the way, and I thought I had commented here before about this, but I guess I hadn't.
The question I have is whether all of these options on the Ago formatter really make sense. I realize they've been adopted from the existing Views formatter, but... we have the opportunity here to make things better since this Time Ago formatter didn't exist before in Core.
So my concerns are about the options:
Time zones for "ago" formats don't actually make sense.
So here I think we should get rid of the time zone pieces...
And here also I think we should remove the time zone settings.
Shouldn't the past date be just as interesting as the future date for showing the settings? Past dates are *way* more common (e.g., the date a node was created/updated).
I don't get why we really need all these options, or how a user is really going to understand the differences? I mean, they're called things like "Raw this" and "span" and "hence", are they really going to understand which option does what? I think this would be a *lot* better if we just let them put in a formatting string for past and a formatting string for future. So the settings form would say:
Format for past: [text box, defaulting to "@time ago", with description saying to use @time to indicate the formatted interval]
Format for future: [same, defaulting to "@time hence"]
And then the code would just say
if( it's in the future) {
return format_string($future_setting, array(@time => formatInterval(...)));
}
etc.
These past/future strings could be translated with Config Translate and are general enough to cover all the possibilities.
Thoughts?
Comment #72
jhodgdonSee also #2456521-62: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known
Comment #73
jhodgdonThe comment I thought I had left here is actually on #2455125-42: Update EntityViewsData use of generic timestamp to use Field API formatter, but then I realized on #2456521-62: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known that prefix/suffix wasn't easily translatable. So I'm advocating making a @time ago type of string for all the cases.
Comment #74
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedSee #71.
As for #71.5. I've given this some thought, and while you are absolutely right, this issue is about porting the existing features. IMO, changing the formatters (even if it's simplifying and good for UX) is out of scope here.
I looked at the patch, and I was wondering if we have sufficient test coverage? It seems like we are not testing all formatters properly. Do we have that already?
Comment #75
mpdonadioOK, had brief IRC conversation with @dawehner and he is for simplifying the timeago stuff. We should move forward with the idea from #44 to use the prefix/suffix fields when a user want timeago. Since https://www.drupal.org/node/2456521 is frozen, we aren't going to use the new method that it introduces; that can be done as a followup.
If anyone wants to work on this feel free; I'll assign myself if I can actually block out some time for it.
Comment #76
mpdonadioOK, just a baby step here to address #71:1-4. Didn't touch any of the tests.
I did notice that the settings summary for this and #70 both output 0 sec for timeago dates in the future. Not sure if this is the problem, if if the problem is in the dateformatter itself.
Next patch will have the reworked prefix/suffix options based on [#9831765]
Comment #77
mpdonadioOK, more better test coverage. DateTimeFieldTest passes locally. Ready for review.
Comment #78
mpdonadioI think IS accurately reflects the patch now. The siblings don't seem to have beta evals, so I am not adding one.
Comment #79
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedThis looks great already!
Looking at the tests, I think we don't have test coverage for DateTimeDefaultFormatter?
Some more feedback:
You could just use $this->setTimeZone($date) once and drop the else clause.
Here that can be changed as well.
This description confuses me. The timezone_override is of type select, but this description make me think I want to have an answer with yes/no.
If I understand this correctly, it could say: "The timezone selected here, will always be used."
It also holds time, so "A date and time object." might be more suitable?
This naming confuses me. Even though the visibility is not public, I think it looks quite a bit like a setter for a timezone on the object you call it with. Instead this is a setter for the object you pass along. I'm not sure how this is handled in core elsewhere, I just wanted to note it.
"Appends the timezone to a rendered date string."?
I also think we should note here (possibly in a new paragraph) that it is possible that a timezone is not appended.
This comment also is not accurate, since the timezone is not always appended.
Again the double setTimeZone thing.
Could you explain why we need the request object here?Found it's usage in formatDate(), but I think it could be removed, and by extension this variable as well.
Coding standards want a trailing comma. See https://www.drupal.org/coding-standards#array
You could use "(int) $_SERVER['REQUEST_TIME'])" here.
Suggestion: Build up a date in the UTC timezone.
Some lines earlier it was set to "UTC", so what was the use of that?
Shouldn't this (and several other occurrences) use t() instead of SafeMarkup::format()?
Comment #80
mpdonadioThanks. I fix this up today or tomorrow. However, re #79-11, using the request object to get the request time is the better thing to do, as the superglobal should be avoided. Globals are being phased out so we can move away from SimpleTest in the future.
Comment #81
mpdonadioI added debug and the default formatter is indeed being tested.
#79 should be addressed, except
9,11: Using the $_SERVER superglobal should be avoided. It is bad practice, and hampers testability; DI should be used instead. The proper thing is to use the request stack from the container, and then use the current request object to get the REQUEST_TIME.
10: This is a constructor call using late static binding, not an array, Adding a trailing comma to the last parameter is a syntax error.
13: The date-only objects use a time of noon (that is what datetime_date_default_time does), which we want on the UTC date before we adjust for the system timezone. This just does everything with the API instead of magic values.
14. Assertion messages in SimpleTest should not be translated per the documentation (look at the docblock on everything in AssertContentTrait).
Comment #82
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedThanks mpdonadio!
Re #81:
9,11: I stumbled upon this several times now, and we always ended up using the superglobal (or the constant if it was a refactor of existing code). Thanks for explaining the proper way to do this!
10. Oh dear. You are totally right.
13. Aha, I see. That makes sense.
14: Ok, I did not know that.
Comment #83
dawehnerIt looks great for me now! Thank you @mpdonadio for your long term effort here!!
Comment #85
mpdonadioThis needed to be adjusted now that #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods landed.
Comment #86
dawehnerThat is pretty sensible!
Comment #87
alexpottGiven that timezone_override and append_timezone come from an abstract base class it is really nice when the schema inheritance follows the same structure. That way if the base class changes you only have one place to change things.
This is missing the timezone_override and append_timezone schema - would be solved it is used the new type proposed above.
committed is misspelt.
This all looks out-of-scope.
Why are these changes necessary?
Comment #88
mpdonadioI'll try to address everything and get a new patch posted soon.
1: I will defer to @dawehner on this, but I am pretty sure we are keeping all of the Views code for sitebuilders who need the plugins for hook_views_data()
2: I'm happy to add a beta eval, but I thought this and all of the siblings were already triaged by the core maintainers as part of #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality? (None of the siblings that have been committed seem to have beta evals, either).
3+4: In #76, we removed the timezones from the timeago formatters, so I am not sure how to handle this with the schemas?
6: In #51, @dawehner requested Safemarkup::format(). Should I just use it in the assertions that were added, and leave the rest as format_string()?
7. This was explained in #24, but I will double check this (it was before I got involved in the patch).
Comment #89
alexpottSo I missed...
What is the point of extending the class if we are unsetting this?
Comment #90
mpdonadioOK, this just addresses 3, 4, and 5. DateTimeTimeAgoFormatter does not need to extend DateTimeFormatterBase. I think I did the schema inheritance properly. I'll look at the rest later.
Comment #91
dawehnerWel yeah for tables, like the tracker ones, we can't use those entity field/formatters.
Comment #92
mpdonadioOK, took a stab at a beta eval. I didn't find a sibling with one after more searching, but feel free to tweak it.
I have everything else addressed, but want to do another once over of the patch to double check it before posting it.
Comment #93
mpdonadioOK, I think this is good.
#87 should all be addressed. In particular,
1. No views code is being removed; there is still use for those plugins.
6. I reverted those, as those tests do not cover portions of the code that changed.
7. I reverted this to HEAD, and it passes locally for me. I suspect something else was wrong that was causing this to fail. However, I do agree with the spirt of that hunk, which could be a worthwhile followup.
I uploaded two interdiffs to make this easier to review, since there was an intermediate patch to double check the change to the parent class of the timeago formatter.
Comment #94
dawehnerI agree we should not convert existing format_string() calls but not introduce new ones.
Comment #96
mpdonadioI swear I ran that test locally before posting this, and it passed. I fails locally for me after I did a full site rebuild.
The patch from #24 makes this pass, but I think it is hiding problems. HEAD uses assertIdentical(); that patch uses assertEquals()
If I expand out the error messages:
you will see that the differences are with missing keys that have zero length strings, so I think assertEqual() was masking this b/c the whacky PHP type system (and I say that in jest as someone who actually owns the System F book...).
So, it looks like the proper thing to do is to update the expected settings values to match what was done in the patch. Does anything need to be done to the Migrations themselves?
Now that I read this test more closely, it needs to be adjusted for the new display settings.
Comment #98
mpdonadioThese Migrate tests are driving me batty. MigrateFieldFormatterSettingsTest only failed when run from MigrateDrupal6Test. I could not get it to fail directly.
I think this takes care of the problem, and I ran MigrateDrupal6Test via run-tests.sh about ten times w/o problems.
Comment #103
mpdonadioOK, I am throwing in the towel for a bit. I think this needs a fresh set of eyes / hands / brain thing.
I git a `git pull`, applied the patch in #98 and ran MigrateDrupal6Test and DateTimeFieldTest 10 time locally w/o problems.
Comment #104
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedAfter applying the patch locally, I was able to reproduce the test failures. This patch just tries to make this green again, without properly addressing the issues.
Comment #106
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedHm. For the migration failures, the expected value seems to differ between #98, #104 and my local installation. It looks like those parameters are randomly there.
The dateformatter issue is odd as well. The same interval is formatted by the same function, so we'd expect the same result. Would it make sense to calculate the expected formatted date as a workaround?
Fwiw, 87654321 should be calculated as 2 years, 9 months, 2 weeks, (0 days,) 12 hours, 25 minutes, 21 seconds by Dateformatter::formatInterval(), which is used by DateTimeTimeAgoFormatter. That means that, if we take into account the granularity, the test in #98 should be green, and the one in #104 should be red.
Comment #107
vijaycs85Looks like the fails on #98 is weird. This patch working on my local.
Comment #108
vijaycs85Comment #109
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedThat was exactly what I experienced. It seems like the expected variable is sometimes different. I'm not sure why.
Comment #111
mpdonadioI spent a little time experimenting with this, and I think the fails on TestBot are due to concurrency and some of the limitations/quirks with SimpleTest in which context of tests are run. That is the only thing that could explain REQUEST_TIME being different. Since we are not testing the dateformatter itself, just the field foramtter, I think we can refactor the expected values in the test to just use the interval directly and compute it from the dateformatters instead of hardcoding the expected string value. I think that will normalize things. We would need buyin that this isn't cheating the tests, though.
I didn't look at the last batch of failures in MigrateFieldFormatterSettingsTest, but I am wondering if I botched the schema declaration in the YAML, and that is the real problem.
Comment #112
mpdonadioBlerg. More timezone problems. Since the date read out of the database will be in UTC and we stripped timezones from the timeago formatter, the timeago tests need to force UTC dates to get a consistent interval. DateTimeFieldTest passes locally for me now, and I think this will be consistent now.
MigrateFieldFormatterSettingsTest still fails individually b/c settings mismatch, but passes when part of MigrateDrupal6Test. I suspect it is because the individual test isn't installing the schema and/or config. Checking that next.
Comment #113
mpdonadio/me grumble
Comment #115
chx CreditAttribution: chx commentedCurrently MigrateDrupal6Test passes because #2176251: Field and FieldInstance default settings are not populated (would you please fix that next? it's just docs). But in the individual test the defaults are populated because we use
create
so the test expectations which check with identical, fail. You need to add the new defaults to the migration and the test expectations.Comment #116
mpdonadio#115 is the summary of an IRC conversation I had with @chx. I am going to tackle this.
Comment #117
mpdonadioThink this should do it. Both MigrateDrupal6Test and MigrateFieldFormatterSettingsTest come up green locally for me.
I reverted MigrateFieldFormatterSettingsTest to HEAD before this to have a clean slate.
Comment #118
jhodgdonI took a careful look at this patch, aside from the tests near the end, where I kind of glazed over, but they look reasonable.
I have a few comments about documentation and UI text, and one that is either a functionality question or a docs question.
Time zone is two words in English, right? So all docs and labels and UI text with "timezone" in them should be "time zone"
timezone -> time zone here too
This label should say "custom" not "plain" in this section.
I don't think we should be using "string" in user-facing text? Should this be maybe just "Format" or maybe "Date/time format"?
Here too, I wouldn't use "string" on user-facing text. Well I'm not exactly sure if the labels in config schema are user-facing anywhere. If not, you can ignore this but I think they are?
another "string"
This one is definitely user-facing, so I'd say "Date/time format" not "Format string".
Probabably "date-time" or "date/time" format here?
Another timezone -> time zone
Nitpick: no comma in this, and timezone -> time zone again
Here too, time zone should be two words.
This is docs, so timezone -> time zone...
Also users -> user's
Also presumably this is a date/time not really a date only?
timezone -> time zone
and this is not really just a date?
I'm not sure this is necessary since the @return also states it? Maybe in the first-line doc say "... if appropriate" and leave it at that?
date -> date/time
Also ... shouldn't say "a" in the @return because by that point it's not "any old string" but the one that is passed in, so should be saying "The passed-in string with the time zone appended..."
So, from this code I can see that the time zone is actually only appended if (a) the append_timezone setting is TRUE and (b) the timezone_override setting has a non-empty value.
This is not documented:
- in the @return for this function for developers
- in the Settings form for the user
Is this really the intention? I'd think that in this function if someone says "append the time zone" we would want to append the time zone that is used for formatting, even if it hasn't been overridden? But this code would not do that I think.
Anyway, if the current logic is going to stand, it needs to be documented better, but I think this is not the right logic.
Hm, this is really the date_format entity storage, if you look at the create() function. So I think that the variable name, the class member variable name, and the documentation here are all misleading.
Again, not excited about "string" in user-facing text?
And this is for dates/times... the #description line is a mixture of date/time, seems a bit confusing.
Same comments as the previous block.
Hm, so... what is the "time ago" string here? I don't see this phrase elsewhere in the form. Probably should not use "string" either. How about something like:
How many time units should be shown
or if for some reason you want to be more verbose
How many time units should be shown in the formatted output
date -> date time in the 2nd/4th line
And needs a blank line between @param and @return
Also in @return don't use "a", it's a definite thing, so "The formatted date/time string..." would be better.
And... what does the "chosen" format mean here? I suppose it's the past/future format from the settings?
Comment #119
mpdonadioThanks @jhodgdon, that is all great feedback.
Re 16, that was introduced way back in #12. If there is consensus, I'll remove the functionality. Personally, I'm all for simplifying things. \Drupal\views\Plugin\views\field\Date doesn't seem to have this, so it's not a regression (as far as I can tell), but I didn't look to see what the D7 Date module does.
Re 17, that matches what is currently in HEAD for \Drupal\datetime\Plugin\Field\FieldFormatter\DateTimeDefaultFormatter and \Drupal\datetime\Plugin\Field\FieldWidget\DateTimeDefaultWidget; it's just shown in the patch as an addition b/c the refactoring to split things up. To fix it properly would mean touching that portion of the widget code, which may or may not be in scope for this issue?
Comment #120
mpdonadioOK, everything except for 16 and 17 from #116 should be addressed. Let me know if I missed something. Didn't add the "if appropriate" suggestion in 14, as I couldn't keep it under 80 cols, and I think using 'DrupalDateTime' is appropriate here (but, am open to suggestions).
Tracing out usages of the settings and ::appendTimezone(), I mostly think that this method is used appropriately (esp since it consolidates some logic that would be repeated though a few classes), but may be badly named. Maybe refactor it to ::appendOptionalTimezone(), and properly document the behavior?
Comment #121
jhodgdonRegarding comment #118 item 16, I still do not understand why if someone chooses a setting that says "Append the time zone" that we would only do so if they had also chosen the "Use a custom time zone" setting. The two just don't seem connected to me.
Actually, I guess I don't see why we need the "Append the time zone" setting at all, since you can use "e" in the PHP date function to get the time zone, right? http://php.net/manual/en/function.date.php -- So where did the "append the time zone" setting come from? It's not in #12 as far as I can see.
Regarding #17, even if it was in head, you have code touching those lines, so I think it's within scope to fix it.
Comment #122
mpdonadioMy bad. The timezone override was introduced in #12, the append setting was introduced in #18. Both were before I started working on this, so i just ran with it.
Yes, upon looking at the code, the method is wonky. To work right, it either needs to use the TZ override or use the API to get the one for the current user (the API does fallback to system default). I'll fix this (should be easy), and see if anyone else has an opinion on removing it.
I'll also address #17 in the formatters by changing the property name and docblocks to better reflect what it really is. I think keeping the typehint as EntityStorageInterface is fine, but I'll double check that, too.
Comment #123
jhodgdonBut... Why do we need the "append" functionality at all, since someone can just use "e" in their format?
I do not see why this was added in #18. It's not in the issue summary (currently at least) and I don't get why we need it at all.
Comment #124
Gábor Hojtsy@jhodgdon, @mpdonadio: the goal of this issue is to port the views Date.php field features to general Timestamp fields. Looks like Date.php field in view does not have an append timestamp feature either. As per the issue summary we should port the **existing** features and in fact discuss removing some if they are not needed not add even more. That would make it hard to argue that this is not a feature request.
Comment #125
mpdonadioOK, I'll take that as consensus. I will strip the append timezone setting and functionality.
Comment #126
mpdonadioThis just takes care of #118-17. Because of a refactoring that, the timeago formatters don't need that injected anymore, hence just the removed lines.
Next patch will strip the 'append_timezone' setting.
Comment #127
mpdonadioI expect this to come up green, but if you look at the interdiff, there are no changes to the tests. It looks like both the timezone_override and append_timezone settings never made it into the tests. So, it looks like they need to be updated. I also did verify that the date override is part of the Views plugin, so that addition was OK.
Taking my name off this in case I don't get to it this weekend.
Comment #129
mpdonadioDoing a retrigger b/c
The test log gave no indication of what went wrong or where.
Comment #131
mpdonadioThis is going to fail.
1. It looks like the format methods on the Drupal classes don't handle timezome overrides properly: https://www.drupal.org/node/2497447
2. DateTimeFormatterBase::setTimeZone() actually changes the timezone of the $date object, instead of overriding it on display (which may not work if the above is true). Because objects are pass by reference, really weird things may result at runtime when the timezone of a date object changes unexpectedly.
Comment #133
mpdonadioPostponing on #2497447: DrupalDateTime::format() and DateTimePlus::format() ignore the timezone setting. so we can proper override the time zone w/o affecting the date object.
Also adding #2497585: Simpletest should set a system timezone in setUp as a related issue based on some venting in IRC last night.
Comment #134
jhodgdonLooks like we can un-postpone this now.
Comment #135
mpdonadio#131 currently has a failure in \Drupal\datetime\Tests\DateTimeFieldTest on line 324:
Retesting to see if to see if this works as-is with #2497447: DrupalDateTime::format() and DateTimePlus::format() ignore the timezone setting. committed and to see if this needs a reroll.
Will work on either case later tonight.
Comment #137
mpdonadioSetting back to Needs Review based on #131 now being green.
Comment #138
jhodgdonWonderful that the tests pass now!
I think this is very close. I took a close look at all of the code and documentation, and only found a few things to comment on, only two of which are very minor UI text things to fix. Excellent work! Thanks!
This should be "Time zone: ..." (two words for time zone)
This has interesting implications for page caching. Do we need to think about this more? Because it means that any site with the option set for everyone to have their own time zone cannot cache any formatted dates output on any page. [This is not a "fix this" comment, just an observation.]
I guess that any page using a DateTimeAgoFormatter cannot be cached either, at all, because the output depends on the current time. Another interesting cache implication. [another observation]
Shouldn't we be saying just "The format" not "The format string" in user-facing text here? (and in the next setting)
Comment #139
jhodgdonRegarding the caching, I added a comment to #606840: Enable internal page cache by default about this question.
Comment #140
mpdonadioThanks for the reminder. #2 needs to be fixed. #2497447: DrupalDateTime::format() and DateTimePlus::format() ignore the timezone setting. partially addressed this issue (mainly, the tests). DateTimeFormatterBase::setTimeZone() is bad because it updates the TZ of the object, and not just overrides it for output (side effects! boo!). We need to refactor this to remove that method, and pass the proper setting to the format method (which 2497447 allows now). I'm thinking we add a method to the base class to handle this in one place.
The format methods do set the proper cache context to be time zone aware, but I think #3 is a non issue. If an event happened two hours ago, it doesn't matter whether my local time is 8am or 6pm, 6am or 4pm are still both two hours ago.
Comment #141
jhodgdonWe do not need to address the caching issues here -- they are being addressed elsewhere. So I think all we need to do on this issue is to address the two very minor UI fixes mentioned in comment #138 (items 1 and 4) and we're ready to go on this one. Thanks!
Comment #142
mpdonadioMy comment in #140 does need to be done. The current patch changes the date object when it gets rendered if there is a TZ override. This could cause very weird problems should the data object get used afterwards. This should be a pretty easy thing to do, and I am going to hopefully do it tonight.
Comment #143
jhodgdonRight, I forgot about that. Sounds good!
Comment #144
mpdonadio#138 should be addressed, as well as #140. I also made a few small cleanups to try to make the formatters more consistent w/ each other. \Drupal\datetime\Tests\DateTimeFieldTest came up green locally.
Methinks we are good to go.
Comment #146
mpdonadioOk, I thought that was a harmless cleanup in the new code. Silly me. I don't think that code is 100% correct, but since we have a test for it, the change is out of scope for this issue.
Both \Drupal\datetime\Tests\DateTimeFieldTest and \Drupal\rdf\Tests\Field\DateTimeFieldRdfaTest come up green locally now.
Comment #147
jhodgdonFYI, the caching issue for time ago formatting is now its own issue:
#2500525: Time ago/hence date/time formatting breaks caching; needs appropriate max age
I will try to review the latest patch here soon. My minor UI points from the last review did get addressed, thanks! So if someone else wants to review the other code changes... or I'll get to it hopefully today or tomorrow.
Comment #148
dawehnerJust some nitpicks :(
You can use if ($override = $this->getSetting('timezone_override)
$override is already not empty.
Comment #149
mpdonadioThis is just a small rereroll b/c DateTimeDefaultFormatter didn't merge during a rebase.
Comment #150
mpdonadioAnd now, #148 addressed w/ a clean interdiff.
Comment #151
jhodgdonEverything looks good to me, and the comments in #148 have been addressed. So, let's do this!
Note: If #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known is committed first, we'll need to fix one or two lines of this patch to use the new formatDiff() instead of formatInterval(). Or if this is committed first, we'll need to do that in the other issue. So, the race is on!
Comment #152
Gábor HojtsyBring on D8MI sprint, given that this allows rendering date fields with the right formatter supporting multilingual (ie. rendering the right language variant of the date) :)
Comment #153
dawehnerCongratulations @mpdonadio!
Comment #154
jhodgdonCan you just confirm before we commit this one that on #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter, which I think included this patch in it's latest, the changes to the formatters here were the same as what you had in that latest patch?
Comment #155
jhodgdonHm. So this patch covers:
core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php [new class]
core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeDefaultFormatter.php [revised]
core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php [new class]
core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php [revised]
core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php [new class]
core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php [revised]
These are all widgets/formatters for date/time fields added to entities using Field UI.
The other patch covers:
core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
These are the Field UI formatters used for timestamps, like created/changed/etc. -- base fields on entities, as opposed to the Date module's field that you can add to an entity. And then the other issue also changes Views created/changed/etc. types of fields to use Field UI:
OK, so they're separate. That may not be the absolute best separation into two issues, but it's at least a clear separation, and this one is ready to go, so let's proceed. Onward!
Comment #156
jhodgdonActually though this patch doesn't fix the problem in this issue summary at all, which was all about the base field formatters, not the datetime formatters.
So I think we should go back and put the Timestamp fields here. So setting back to Needs Work. This issue was supposed to be about the base timestamp formatters and it got sidetracked to the Datetime formatters. It shouldn't have. I've added Datetime to the issue summary since it's already in the patch.
So let's move the timestamp formatter part of the patch on #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter over here and make sure it's tested (outside of views), and leave #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter for just "use this in views" as was intended.
Comment #157
jhodgdonAlso #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known just landed, so we should be using formatDiffUntil() and formatDiffSince() instead of formatInterval() here where appropriate. woot!!!
Comment #158
mpdonadioI'll start work on this tonight.
Comment #159
mpdonadioPart 1a. This is just the update to the datetime formatter, which is pretty trivial.
Part 1b is updating DateTimeFieldTest, which is proving problematic. The new functions use the request time explicitly. In the old version of the patch, we got around this by always using a consistent interval. We have introduced REQUEST_TIME (which varies) back into the equation in the test, and also have the additional complication that the date-only portions throws away the hh:mm:ss portion of the timestamp when it stuffs it into the database. The net result is that the expected result is a bit unpredictable between test runs when seconds get rendered out. Blerg.
Part 2 will be hand editing the patch from #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter to separate out the views code from the formatter code, and applying it to this, and updating the formatter to use the new functions, and then fixing up any tests (and we may run into the same problems).
Need to sleep on this. I may post the initial part of Part 2 tomorrow night and then worry about the tests together.
Comment #160
mpdonadioOTOH, do we want to get this issue in using formatInterval() and then handle the conversions to use formatDiff() as a followup? The Frankenstein's monster of #150 + the formatter portion of #2455125-62: Update EntityViewsData use of generic timestamp to use Field API formatter should be (mostly) easy.
Comment #162
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedAssuming we can't fix the REQUEST_TIME, the only way I see this possible is by actually calculating the diff. So I guess we need to call the DateFormatter or use the PHP DateTime::diff() to see what the expected value is. I'd go with the former, because that already calculates weeks which DateTime:diff() does not. Furthermore we are testing if the result is displayed rather than what the exact result is. We have a unit test for that.
I would do it here, but I would hate to see this patch being held up if it turns out to be tricky.
Comment #163
mpdonadioIf we lower the granularity to 3 in the tests, we won't see the jitter in the request time. However, I found a bug in DateFormatter::formatDiff(). See #2504211: DateFormatter::formatDiff() ignore granularity in some circumstances.. Technically, that is blocking this, but I am plowing forward. This won't pass.
Comment #164
mpdonadioFrankenpatch with formatter related pieces from #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter, but still using formatInterval(), because that we need to rework the changes that #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known made to support the formatter strings w/ the formatDiff() stuff. TimestampFormatterTest passes locally for me, but not sure if we have additional breakage.
Comment #167
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedIt's great to see this moving along quickly!
Re #163:
Just a sidenote, for if you would intend to revert that change later on. There is an issue #2502571: Date format granularity should only render adjacent units which alters the handling of granularity as part of a caching effort. That would mean that a granularity of 4 would also stop after weeks and not display hours, since days are skipped.
Comment #168
mpdonadioI think this wraps up the work. This will fail until #2504211: DateFormatter::formatDiff() ignore granularity in some circumstances. makes it in. Otherwise, I think this is ready for review.
Comment #170
mpdonadio#2504211: DateFormatter::formatDiff() ignore granularity in some circumstances. was committed 5 minutes after I posted #168, and the four fails are in DateTimeFieldTest, which now passes locally. Requeuing.
Comment #172
mpdonadio#168 came back green Review away.
Comment #173
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedThis looks great. The todo's can be removed now.
Todo.
This description is quite prone to change. I'd make that something like "The settings array that can be passed to..".
Todo.
Comment #174
mpdonadio#173 addressed. IS tweaked.
Comment #176
mpdonadioGot a
Re-testing.
Comment #178
dawehnerLet's see whether its this time ...
All that feels so common, its almost as if I would read that the 10th time already ;)
Nitpick: You could use
$this->t()
This is not used
Comment #179
mpdonadio#178 addressed.
Comment #180
jhodgdonHey, thanks for the patches and reviews! I have been behind in my reviews, sorry.
So I took a careful look through the code and tests, and I think there are a few things to address. Some are minor; some not so minor.
Should be "Time zone" (two words) as it is a UI label here.
Do we really want to say "timestamps" in this UI text? What are the field types that this applies to called? We should be consistent.
Also @time is not really the time, it is the time interval, right? [See below for more on this with similar strings]
Same here. [still more below]
Since we're not actually using $interval for anything, I think this code would actually be clearer if it said:
if($timestamp < $this->request->server->get('REQUEST_TIME'))
When I read the code as it is, I got to if($interval > 0) and then I had to backtrack up a few lines to see which way the difference was written. It's just easier to read with a simple > compare and no local variable, I think?
So really this is either the custom date format or the granularity??!? Weird. Granularity would only apply to the "ago" formatter though, right? So maybe that formatter should update the setting name and description so it says granularity, and this one shouldn't mention granularity?
And why don't we have two different settings anyway? This just doesn't make sense to me.
This is a bit informal for translatable UI text. We shouldn't say "docs", for instance.
Also, the link text is not Accessible. Link text should always say where the link goes. This one doesn't go to "PHP docs", it goes to the documentation for PHP date formats, so "PHP docs" is not good link text.
See previous comment. Same problem with accessibility.
Well this is a bit different from the text used in the other setting for future_format and past_format on the other class.
Let's make them all the same.
I still don't like that we call it "dates in the future" and "timestamps in the future" and then @time is "the time", when really it's dates/times in the future and @time is the formatted time/date pieces (which admittedly is not easy to describe) but definitely not "the time".
See previous comments.
This test may pass today, but I am not convinced it will always pass. I think it will lead to some random test failures in the future.
The reason I think this is that you are setting $date to REQUEST_TIME minus a fixed number of seconds, and then you are testing that it will format to "2 years 9 months 1 week". So that's fine now but what about in February of the year after a leap year? There will be some times when this will fail, because we're using formatDiff() now to format these things, not formatInterval(), right? So "2 years 9 months 1 week" is not a fixed number of seconds, or even a fixed number of days.
If you want to make a good test that will continue to pass, you need to use the PHP date add functions to subtract "2 years 9 months 7 days" to REQUEST_TIME, and then it should always work.
Same with the "ago" tests below.
Same problem will occur here.
Here too, and the future test just below.
Comment #181
mpdonadioThanks for the close look. I'll start on these tonight.
Re #2, these are timestamp fields not date/time fields.
Re #5, this is the TimestampFormatter not the TimestampAgoFormatter, so I think this is correct. No granularity; there are two separate formatter classes.
Re #8,9. I thought we had decided on the @time placeholders for DateTimeTimeAgoFormatter several patches ago and gotten to RTBC? The last batch of changes was to fold TimestampFormatter and TimestampAgoFormatter formatters (and related changes to suppor them) In the I can adjust as needed, though. Any suggestions for the actual text between now and tonight (EDT) would be appreciated, otherwise I'll take a stab at these again.
Tests. I'll see if a trip through DrupalDateTime and the add functions will work here.
Comment #182
mpdonadioOne more thing before I punch in for the day, re #9.
TimestampFormatterTest doesn't use static strings in the timeago tests. The expected strings call out to the formatter service directly, and then the formatter output gets compared to these (we aren't testing whether the date formatter service works, just that the formatter produces what we think it should, which is what the formatter service provides). So, I don't think there will be reliability problems with these.
I think the proper thing for DateTimeFieldTest is to use the same approach instead of relying on static strings. I had originally abandoned this b/c the granularity bug in the date formatter service and the fact that there is something weird going on with REQUEST_TIME in WebTestBase (which will be my text followup), but that will probably work now.
Comment #183
jhodgdonRE item #5 in comment #180 - take a look at the #description for that field, which mentions Granularity.
RE item #2, I realize that they are "timestamp" fields from a developer/coder field. But what are they from a User perspective? "timestamp" is not actually a word in English. This is UI text, so we need to think about what these fields are called in the UI.
RE item8, 9: yes, @time is fine, but I don't like the wording saying "the time goes here", when it's not "the time" but something like "1 year 3 months". Maybe this description could be:
The format for dates in the past. Use @time where you want the text representing the interval (such as "1 month 3 days" to appear.
And re #10, the test... well given that your granularity is for a week, you *might* be OK, because probably the diff would only be off by a couple of days, but it would be much much safer to use actual date diffs than a number of seconds instead I think. If you do want to use a number of seconds for the interval, please add a comment saying how many days it is equivalent to in the code.
Comment #184
jhodgdonRE #182, yes a comparison to a direct call to formatDiff() instead of a static string would solve the problem. Probably the best approach, since we have a lot of unit tests to make sure formatDiff() is working correctly.
Comment #185
mpdonadioThis should address #180, except for the test adjustments. Onto those next which will use the date formatter service instead of static strings, but that may not be tonight.
Comment #186
mpdonadioTestBot is borked right now, but I think this will come up green (#185 should fail) when tests start running again.
I think this addresses everything, but I could have overlooked something.
Comment #189
mpdonadioWeird fail in ConfigTranslationUiTest:408-409
Comment #191
jhodgdonThis is looking good (pending test bot approval)!
Just some cosmetic stuff (comments and UI), and a couple of questions (code may be OK but I wasn't sure).
The description on the past/future fields below says to use @interval not @time. Either that description is wrong, or these default values are wrong.
Hm.
"time intervals in the future" doesn't make sense to me. Intervals are differences. Times and dates are in the future.
So...
How about this:
The format for times/dates in the future. Use @interval where you want the formatted interval text to appear.
See previous comment.
This is good!
Indentation is 1 character too many in the lines under /* in this doc block.
And the first line could be even better... How about just:
Formats a timestamp.
This is date format storage, not date storage.
This is much better! Hm. Maybe it should say:
"Used if Date format is set to Custom. See [the link tag]."
?
Do we need the description here at all? We should always strive to minimize UI text, and this one doesn't seem to tell me much beyond the field title. I mean we're in the context of formatting a date, so if there is a Time Zone field, it should be obvious it will be used to format the date. Right? So let's drop the description. According to our UI guidelines, most fields shouldn't have descriptions.
Let's take out the first sentence in this and just put in the "See ... " part? I think the first part is redundant to the title.
Why does this call $date->format and most of the other classes use the DateFormatter class? Just curious.
Here's another one that is using $date->format instead of the DateFormatter. Still not sure why.
See previous comment on similar line in other class.
See previous comment on similar line in other class.
Title should be $this->t('Granularity'), not "Interval". Oops.
How about:
Formats a date/time as a time interval.
Comment #192
jhodgdonTest bot green, woot! :) Those comments in my review should be easy to address. The tests are looking good to me.
Comment #193
mpdonadioI'll fix these tonight. Re 10 and 11, I think those were part of the patch before I took this over, but I forget what I turned up when I looked at it. I'll look at it tonight, and comment on it (or fix it if it is easy, but both of those would also involve adding the service to the list that gets injected and one or two more protected properties).
Comment #194
mpdonadioThis should address #191, and I expect it to come up green (unless I introduced a side effect that my local testing didn't catch).
There was no real reason to use `$date->format()` directly. I think this solution is cleaner, too. And, if a review wants to mention it, I think
in the base class is the best thing to do. It's not a common function used by the derived classes, and it doesn't have a decent default implementation. I think that the derived classes should be forced to implement a specific version of this for each formatter.
Comment #195
mpdonadioIf this doesn't get in by 2015/06/24, per #2507899: [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29, I don't think we will need update hooks as the datetime formatters have one to one mappings with the new formatters (default and plain exist in both), and we aren't changing anything with the timestamp formatters (just adding new features). However, we will probably need a change notice as the patch will require a container rebuild. I was unable to make the patch fatal without one, but the new options won't appear (or work) until you do one.
Comment #196
jhodgdonLooking great! A couple of questions and nitpicks:
Total nitpick: I would find this easier to read if it was:
if (request_time > timestamp)
rather than
if( request_time - timestamp > 0)
This appears in another spot in the patch as well.
Do we need some validation on this field -- what if 'custom' is selected for 'date_format' and this is left empty?
Should we only display the custom date format if setting 'date_format' is set to 'custom'?
Also, shouldn't we be displaying samples of what the format would look like in the output?
And for the date_format field, we should be showing the human-readable label of the format, not the machine name?
Rather than making translators find the correct URL for their language for this page, we might want to instead pull the URL out of the t() string (put it in as a @url substitution) and use the generic no-language URL of:
http://php.net/manual/function.date.php
That URL should redirect people to an appropriate language.
Same in other places in this patch where this URL is used.
I think we need to set the time zone before setting up the default time here?
This is the other place with that if() nitpick from above.
Comment #197
mpdonadioI'll fix these tonight, which will give me some time to ponder the validation.
Re #196-5, no we need to set the timezone after the default b/c a quirk in how date-only values are stored. The value coming out of the database will be at midnight UTC. The default value function sets this to noon, UTC. When you apply the timezone, you will shift the time forward or backward, but you will land on the same day. If you set the timezone before setting the default time, it is possible to warp the value to the day before.
Comment #198
jhodgdonAh. If I had been writing a default value function, I think I would have made it set the time to noon, local.
In any case, noon UTC is not a guarantee of it not warping the date to the previous or next day. There are a couple of time zones around Alaska/Russia/Pacific that if they are on daylight savings time, are 13 hours plus or minus UTC. So this scheme is not guaranteed to work.
Comment #199
mpdonadioYou are absolutely correct. For the scope of this issue, I think we can ignore the extreme cases. However, we will need a followup to properly work this out. There is an issue to do general cleanup of datetime.module (can't find it now; link is bookmarked on home machine), which I think need to be split into a meta issue and we can add the zones next to the international date line to it. There are also some issues related to TZ handling in the test framework in general. I'm slowly starting to triage all of issues, a bunch of which are very old, and will hopefully have a gameplan soon for everyone to review.
Comment #200
jhodgdonThat sounds great! And yes, out of scope for this issue.
Comment #201
mpdonadioThis addresses #196-1,3,4,6.
#2 - Since timestamps aren't exposed as proper fields on the standard entities, I am hesitant to add an #element_valid w/o even being able to test it manually to see if it is wired up proerly (timestamp formatter is a kerneltest using entity_test and we have coverage of the output). The timestamp formatters are really for views (see #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter) and non-core entities that want to expose them. We could add it to that issue, or as a novice followup to that issue.
#5 - As discussed above, as part of datetime.module cleanup, we will do a followup to see what happens what happens with the extreme timezones.
Also adding the Needs CR tag as part of #2507899: [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29. The CR will just need to say that a container rebuild is needed for this to work when upgrading from beta12.
Comment #202
jhodgdonOK, I think this is finally ready to go!
Comment #203
alexpottBecause it's before July 1st it does not need a CR. Committed 2da1f42 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #205
Gábor HojtsyYay, thanks all!