Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
The datetime field does not support end dates, which makes it impossible to create a date range. This is a very important feature for a calendar.
Proposed resolution
Add new field field type to represent a data range, which will allow collection of end date.
Remaining tasks
Basic FieldTypeBasic FieldWidgetsBasic FieldFormattersFieldWidgets validationFieldItem constraint to enforce start <= end?(going to defer this)Polish theming of widgets(defer to #1918994: Improve Datetime and Daterange Widget accessibility)Test coverage of the above.Committer review on whether the extensive partial code duplication is acceptable (see #226).Entity/Field/Framework review on schema key names (see #73, #146, and #229).- Fix layout issues in DateRangeDefaultWidget (see #305) and/or add touchups due to #1918994: Improve Datetime and Daterange Widget accessibility
User interface changes
New field widgets.
API changes
New field type and associated goodness.
Data model changes
New schema for date ranges.
Comment | File | Size | Author |
---|---|---|---|
#367 | interdiff-348-367.txt | 13.07 KB | mpdonadio |
#367 | 2161337-367.patch | 114.39 KB | mpdonadio |
#213 | 2161337-213.patch | 109.57 KB | mpdonadio |
#213 | interdiff-206-213.txt | 829 bytes | mpdonadio |
#236 | 2161337-date_range-235.patch | 110.19 KB | tim.plunkett |
Comments
Comment #1
MarkusDBX CreditAttribution: MarkusDBX commentedI'm also curious of this, any updates? Is end date functionality supposed to go into core in the first release of Drupal 8? Or are you supposed to create two fields in Drupal 8, for this kind of functionality?
Comment #2
kiwad CreditAttribution: kiwad commentedWas looking for more info about this and found a comment by @KarenS in a D8 core issue
https://www.drupal.org/node/501428#comment-6699586
Although, i'm not sure if this means that we need to look for a DateRange project or the current contrib...
Is it better to start with 2 core date fields to manage Start-End dates for now in D8 ? However that might be a pain to restructure data if a DateRange module eventually happens...
Comment #3
kclarkson CreditAttribution: kclarkson commentedI am "very" surprised that this was not in the initial core discussions. I am assuming most people what an end date when using the date module.
A couple of questions:
-At this point are we forced to use two fields?
-What are the advantages to disadvantages to two separate fields?
Comment #4
jhedstromThe core 'datetime' module also does not support the storage of timezones with the date, so that is another feature the contrib module will need to take on.
Comment #5
colanIs there a core issue for getting an end date into core 8.1.x?
If not, perhaps we should open one? It would be better to work upstream if possible.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedRe #3:
Kind of a late reply, but using only what exists at this point, two fields is the only way to go other than custom coding. However, it will be hard (and unsupported) to migrate that data once we get the functionality in.
Re #5:
Afaik there is no specific request, but the more general discussion can be found here: #2543958: [META] DateTime Module Improvements.
Comment #7
swentel CreditAttribution: swentel commentedFirst stab at this (currently not a different field type, see https://www.drupal.org/node/2632040#comment-10676660 for more background.
Tests should be green.
Widget also shows up - but it's not 'nice' with the inline floating labels :/
Comment #8
swentel CreditAttribution: swentel commentedholy crap - I just realized that this is still in the date queue, moving to core
Comment #9
swentel CreditAttribution: swentel commentedreuploading to get a test run ....
Comment #10
swentel CreditAttribution: swentel commentedAlso, this is major and a blocker for contribs - and even relatively simple sitebuilder tasks too ..
Comment #12
colanComment #13
swentel CreditAttribution: swentel commentedFix tests - interesting here - since that this /could/ potentially mean the upgrade path can be easy (or at least less annoying).
Comment #15
SKAUGHTin terms of some UX.. just adding and end date makes the user fill out a second date popup as well. perhaps instead of using the 'collect an end date' checkbox we can use better options on the select list of the field settings:
'#options' => array(
static::DATETIME_TYPE_DATE => t('Date (all day)'),
static::DATETIME_TYPE_DATETIME => t('Date and time'),
static::DATETIME_TYPE_DATETIMERANGE => t('Date and time with range'),
),
this leaves the issue of 'spanning days' of course. then maybe use the checkbox for the option to add the date popup. Under the idea that most people will be collecting a time on the same day.
Comment #16
SKAUGHTComment #18
jhedstromRather than
enddate_get
, can we go with something likecollect_enddate
? I didn't fully understand the meaning ofenddate_get
until I read further through the patch.We should add some small random time to the sample enddate so they aren't identical to the start date.
Comment #19
jhedstromCross-post. The review in #18 is for the patch in #13. The patch in #16 seems to be an interdiff, rather than a full patch?
Comment #20
swentel CreditAttribution: swentel commentedI based my patch on the functionality that exists in the date project, variables/keys are also coming from there. All up for better names indeed :)
Comment #21
mpdonadioDo we want to postpone #2632040: [PP-1] Add ability to select a timezone for datetime field on this since they touch the exact same portions of code, and likely also want TZ support for both? Seems logical to do this first.
This is looking good.
Comment #22
swentel CreditAttribution: swentel commentedI'm fine with postponing - interesting that you mention, do we want different timezone per date, or is the timezone for all dates that are stored per delta ? Will have to look how the date project currently does this.
Comment #23
SKAUGHTMyself, I do think this functionality is more important than a timezone subfield. Given that right now i can't use drupal8 to make a basic event content type stating 10am-4:30pm..
i'ld say Timezone is more clear with 'a Location of event' Field to an end user.. in most situations.
Surely, coordination with #2632040 will need to occur regardless of which ever issue may make it into core first..
Comment #25
darrick CreditAttribution: darrick commentedHopefully this helps. Reran the patches from #13 and #16 to fix the testing errors. Noting that one missing piece is setting the default enddate.
Comment #26
colanLet's see if the errors go away before changing the state back to "Needs work".
Comment #29
mpdonadio#25, thanks for the patch. Can we get an interdiff? They really help review progress on issues.
Comment #30
darrick CreditAttribution: darrick commented@mpdonadio I'm not familiar with the interdiff process but will look that up while I'm waiting for this one to process. I added a datetime.install file to update the schema for the enddate_get field. Bit of learning curve on that so bear with me :)
Comment #31
darrick CreditAttribution: darrick commentedHere is the interdiff. Assumed 15 was an interdiff. So this is between 13+15 and 30.
Comment #32
mpdonadioInitial thoughts. Looking at the fatal that the code mentions.
String change would be a BC break, so we need to either keep the old ones, or set them conditionally based on whether the end date is there.
Ditto on string change.
String changes.
Comment #33
swentel CreditAttribution: swentel commentedStrings can be changed in minors if I read https://www.drupal.org/core/d8-allowed-changes#minor right.
Comment #34
mpdonadioStill working on this...
Comment #35
mpdonadioPunting this one back.
The main problem is DateTimeItem::propertyDefinitions(). Because of the way the Field API works, I don't see how we can use this logic here to add properties based on the enddate_get setting, since the schema has already been created, but not updated when the settings form gets saved, which is leading to an exception because of a null value.
Comment #36
mpdonadioComment #37
DanieleN CreditAttribution: DanieleN at Previon Plus AG commentedNice Patch, thanks!
Actually, i've a datefield only with a date (All day), also without a time field.
Seems the end_date field don't support this.
Actually i've disabled the time field in hook_form_FORM_ID_alter().
My code support the AddMore Buttons.
The
$count_addmore = $form['field_offer_blocked']['widget']['#max_delta'];
is the actually count of field-date.So the user see only the date field without the time field.
Complete Code:
Better way: This would be in the patch (..or in the core)
Comment #38
DanieleN CreditAttribution: DanieleN at Previon Plus AG commentedComment #39
swentel CreditAttribution: swentel commentedDon't change the version number please.
Comment #40
mpdonadioWorking on this again.
Comment #41
mpdonadioFirst pass at formatter updates. Need to figure what to do with timeago.
Comment #42
mpdonadioFirst pass at end date with timeago. Temperature check in sprint room was that if the end date is present, base the timeago on that. We really want to avoid a setting here, to not get into the situation of Date does everything again. It is easy enough to write a new formatter if you need different behavior.
Onto checking how the default values work.
Also think we want to get this patch in w/o Views support, and handle those plugins as followup after we get the TZ at entry support in.
Comment #44
tim.plunkettNot sure that local variables are needed here, but if you keep them, might as well use it consistently
Comment #46
mpdonadio#44, that is consistent with how the rest of that method is written. IOW, sometimes the $item property is used, other places the local is used. The patch does it's best to try to keep the code style consistent (for better or worse)...
Default values seem to work OK w/ manual testing.
Manual review of the UI and general approach would be appreciated, so we can begin updating the webtests to cover these new cases.
Starting on upgrade path in the meantime.
Comment #47
mpdonadioI've been reading the issues about the schema upgrades, and I don't quite understand how to add the columns/indexes to all of the field instances in this case. Help here would be appreciated.
Going to start on some test coverage, but unassigning myself if anyone else wants to jump in.
Comment #48
darrick CreditAttribution: darrick commentedTime ago in D7 is based on the start datetime. Which makes sense to me. I.e. if you have an event from 6-7pm I would say that event happened x time ago based on when it started not when it ended.
@mpdonadio what issues about schema upgrades are you referring to?
Comment #49
mpdonadio#48, I'm not wed to using the start date. I am not sure if the D7 behavior is intentional, or it just works that way. A bunch of us thought that basing off of the end date made more sense. Let's see what others say. It's an easy change.
Schema change. In the patch, DateTimeItem::schema() defined a new column. Since https://www.drupal.org/node/2554097, schema changes that add columns require update hooks. The storage setting is added in datetime_update_8001() in the patch, but adding the new column to add datetime field instances isn't. One, I'm not sure the best way to get the list off all of the datetime field instances, and two, I'm not sure if we need to do a copy/update/copy update, or whether we can just add the column and the index to each table. If you try a manual update, you see the failure, and you can also see the problem in the last batch of test results. Hoping @swentel can chime in here; I'm not familiar with this aspect of the Field API :)
Comment #50
darrick CreditAttribution: darrick commentedThis patch updates the widget for setting the default value for the end date. Also modified the default formatter to only show the time if the start and end dates are the same. Also formatter no longer shows end datetime if equal to start datetime.
Comment #51
swentel CreditAttribution: swentel commentedI can look at the upgrade path on thurday, unless someone beats me to it before it.
Comment #52
mpdonadio#50, thanks for the updates; they look like good changes. We should do another pass to think about other edge cases.
It looks like some formatting changes crept in. We need to try to avoid them, as they are considered out-of-scope changes for the patch (a committer will likely reject the patch with these).
#51, thanks, b/c the way storage works, I wasn't able to figure out a way to make the schema dynamic based on the options (the tables get created before the settings form is presented).
Comment #54
darrick CreditAttribution: darrick commentedOkay. I removed the formatting changes. Also fixed some errors in how the default end date was being set.
Comment #56
nkraftWatching the this. Was just about to set up my own Event content type and discovered the lack of a decent widget and the lack of the end date. Thanks for taking this on. Will this potentially be in the next core update, or is it destined for a contrib module?
Comment #57
mpdonadio#56, we decided end dates and timezone-upon-entry are going to be in core, and we are doing our best to try to get them into 8.1 in April. Manual testing would be appreciated so we can start on the integration tests, and hopefully we can figure out this upgrade path (and tests).
Comment #58
colanI think the feature freeze is February 24th so that gives us 2 weeks, correct? I hope to have some time on Friday to take a closer look at this (e.g. testing, etc.).
Comment #59
nkraftThat's fantastic @mpdonadio -- so I should hold off on creating end dates for now;
You mentioned manual testing. What does that entail, exactly? I'm wondering if its something I can help with, or if its beyond my backend skills...
Comment #60
mpdonadio#59, we need people to try out the patch on systems and make sure the UX is decent and that we didn't miss anything from field creation through rendering. Then we can add the integration test coverage where needed.
Comment #61
ongdesign CreditAttribution: ongdesign commentedMy two cents, testing the patch on a clean install:
The current patch does not support null values for end dates if the start date is populated. However, the start date can be null if a valid end date is specified. I think I understand the rationale given in this thread, but it seems odd to me that the end date is the one presented as optional (an explicit user action is required when adding a Date field to "collect end date"), yet it's the required field. There's also no real documentation of the fact that the end date will become the required field, if it's added.
The interval formatting also seems a little wonky to me (e.g., "2/17/16 to 25/16") -- I've never seen intervals denoted like that.
Testing the patch on an existing install, I can't get drush updb to complete without deleting all existing instances of date fields first.
Comment #62
mpdonadio#61, thanks for the manual testing; those all sound like bugs we need to iron out.
The last part about the `drush updb` has to do with the upgrade path that we still need to write.
Comment #63
darrick CreditAttribution: darrick commentedI've been looking into fixing the bugs mentioned by #61. But also into the schema update. I'm repurposing some of the code from the EntityDefinitionUpdateManager::getChangeList function to dig up the fields which need updating.
Any tips on how I should go about updating the schema? The updateFieldStorageDefinition doesn't work. Currently my only option seems to be to do it directly:
Also, is someone else already working on this. If not I could probably get a patch done later today.
Comment #64
darrick CreditAttribution: darrick commentedHopefully this will get us closer.
Comment #66
darrick CreditAttribution: darrick commentedExcuse the noise. Was working off the wrong branch.
Comment #68
darrick CreditAttribution: darrick commentedFixed the last testing error.
Also poking around the D7 date_api found the date_limit_format function which would help clean up the formatting when the end date is the same day as the start date. But not sure where to put that function. Possibly might be useful in the DateHelper class.
Referring to make this code in the DateTimeDefaultFormatter class more robust for handling different date formats:
Comment #69
mpdonadioDon't have time to review the patch, but the I think end date logic you describe should be part of DateTimeFormatterBase so derived formatters have a chance to override/change the functionality.
I am also really wondering if special handling of the formatters for same date/month/year are really needed. I'm leaning towards core providing outputting both dates if they aren't equal, and leave it at that. Contrib can handle fancy handling, and so could a custom module for a site.
This issue is holding back a lot, and we don't want to get bogged down in minutiae. The upgrade path is key here, and also making sure we have good test coverage over everything we are adding.
Comment #70
darrick CreditAttribution: darrick commented#69 Okay. I don't want to hold anything up. Maybe once the dust settles we can revisit it or roll it into a separate issue.
As for the tests and upgrade paths. I'd be willing to help. But have no clue about what tests are needed and for the upgrade path I'm assuming you mean D6 to D8 and D7 to D8 as I believe I've already provided for a 8.0.x to 8.1.x database update.
Comment #71
BerdirThat seems pretty complicated and risky for a minor update.
I'm not sure if it was discussed but wouldn't it be a lot less problematic to just have two different field types? There was some discussion above about a Date Range (although I'm not sure that name makes sense to me.. )
We did similar changes with text fields with/without a format.
With everything being classes, it's actually pretty easy to extend one field type from another and add additional properties/schema, without impacting anything that already exists.
Comment #72
mpdonadio#71, I think the main concern I had was that what will we do when we need recurrences (needed for proper calendar support) and timezone support (see the relate issue, but we haven't ironed how how this would be implemented)? Splitting this out will add two more field types, and adding those would potentially add more field types:
- date only
- date only with end date
- date+time
- date+time with end date+time
- date only w/ custom timezone
- date only w/ recurrence
- ...
That could potentially end up being a lot of field types for someone to choose. And making the wrong choice would mean a site owner would have to figure out a way to get data from one field type into another.
Comment #73
BerdirDate only and date+time is already a single type, so date with end date could easily support with and without time too.
But yes, I'm not sure where to draw the line with recurrence, timezone etc. But do you really want to maintain a single field type that has 6 different properties/columns and supports all kind of imaginable combinations of those?
That said, schema can also be flexible, although we only support changing it when it's empty. We already have cases like this with entity reference fields for example, that have different schema depending on whether they reference a config or content entity. So we could also make it so that value2 (which find a horrible name, fwiw) is only defined when the feature is enabled and not allow to change it for fields with data. Then at least we avoid the upgrade path complexity and unnecessary columns when they're not needed.
Comment #74
swentel CreditAttribution: swentel commented@Berdir did some reasoning about splitting here: https://www.drupal.org/node/2632040#comment-10716892
I suggested it too in that issue to split (a bit above) - I'm constantly shifting on the idea. One the concerns would be the explosion of formatters and widgets, potentially leading to a lot of duplicate code, but not entirely sure. Only a patch could prove if it's worth or not.
Comment #75
BerdirI see. What about making the schema dynamic then? I'm worried about that upgrade path, I think it's missing quite a few things?
Specifically, it doesn't yet update the stored definitions and schemas so that the storage knows that the database was updated. That's quite complicated, see https://www.drupal.org/node/2641828#comment-10711058 where I changed a base field from one type to the other. It also only seems to update base field tables, doesn't yet update the data and data revision tables and then there's the complexity with non-sql backends, which would have to implement all this too? Oh, and test coverage :)
Comment #76
mpdonadioThe patches before #35 attempted to do the dynamic schema. See my comments there. We worked on this at DCNJ, but couldn't get around
Yes, this is complicated... I am not sure which is the least bad situation here.
Comment #77
BerdirAh, I think I see the problem that you mentioned. Not sure about a proper fix, but as a workaround, this patch seems to work. Wondering if the link field has similar issues with required not required, but possibly not.
I also had to remove a duplicated use in one test and Drupal\datetime\Tests\DateTimeFieldTest is currently failing for me, with and without my changes.
Either way, we definitely need UI tests here for creating a date field, so we test coverage for that.
Comment #78
darrick CreditAttribution: darrick commentedPatch in #77 seems to works as advertised. I created both a single date field and a start and end date field on a content type. The table schema was correctly created for both. I then edited the fields to make the first single date include a end date and the other to only collect a start date. The schema was correctly updated (i.e. date_value2 was added to the first schema and removed from the second.)
I don't have a time to update the patch but created an interdiff to remove the schema update code in datetime.install and also remove the formatting code mentioned in #69
Comment #79
BerdirNote that updating will only work as long as there is no data. We will need to add code to prevent that those settings are changed if there is data, similar to how \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::storageSettingsForm() does it, just disable those form elements.
Comment #80
darrick CreditAttribution: darrick commented#79 During my tests I wasn't able to change the storage settings if the field has data.
Comment #81
jhedstromCan we normalize and either use
value2
ordate2
consistently throughout? The db column name isvalue2
but elsewhere in code we're usingdate2
.I'd also prefer better naming of
enddate_get
as mentioned in #18, but if that doesn't bug anybody else, I won't insist :)Comment #83
ryantollefson CreditAttribution: ryantollefson commentedBased on #82, does this mean we will be waiting until October to get this in core?
Is it possible to get this as a separate module as a stop-gap for the next 7 months?
Comment #84
mpdonadioSorry I have been a bit AWOL on this. Been dealing with something that has been occupying most of my free time.
Do we think we have this at a place with the widget and formatter where we can start on these tests and bring this to completion.
#83, we can see if we can get this into 8.1 as an exception to the policy.
Comment #85
mpdonadioReroll (think b/c #2684095: Convert field kernel tests to KernelTestBaseTNG).
Comment #86
mpdonadioIf this comes back with fails in Migrate, we can ignore: #2685281: Spurious fails in 8.2.x in MigrateBlockTest. We can still work on this; you just need to go into the queued test and click the "run anyway" button.
Comment #88
darrick CreditAttribution: darrick commentedI'd really like to see this in 8.1. Would it be helpful if I re-rolled a patch with my interdiff in #78, audit for the inconsistencies mentioned in #81 and better naming of the variables as mentioned in #18.
That would leave tests. And as for variable names I would change enddate_get to collect_enddate and enddate_require to require_enddate.
I can do this over the weekend.
Comment #89
mpdonadioOn mobile, so being brief. Need time to look at 78. Rest look ok to do.
Comment #90
darrick CreditAttribution: darrick commentedNew patch as mentioned in #88. Will wait for tests before uploading interdiff and summary.
Comment #91
darrick CreditAttribution: darrick commented#81 I believe date2 is used instead of value2 because it's a computed value in a format which the datetime element prefers. So, didn't make any changes.
#18 I renamed enddate_get to collect_enddate and enddate_require ot reguire_enddate throughtout.
#69 I removed the code for special formatting of start and end dates if the start and end date was the same. #61 seemed to have some trouble with this. So that should be fixed now.
#77 Seemed to negate the need to provide a update path for the field storage tables so I removed that code.
#37 Current patch supports start and end with date only.
I've been following this issue from the beginning and AFAICT we need tests, 6.x - 8.x, 7.x - 8.x upgrade path and also some testing and feedback from the community.
BTW: Found two minor bugs during testing:
#2686407: Datetime type should be disabled on storage setting form if field has data
#2686409: Time Ago summary does not render on Manage Display for Timestamp and Datetime fields
Comment #92
mpdonadioThanks darrick! This patch doesn't have to have upgrade paths from 6.x and 7.x; those would be part of Migrate (and probably contrib since the source is contrib). So, we need to make sure the UI for the changes is good, and then we can bang out that test coverage.
Comment #93
dawehnerI really like this feature!
Yeah this is more like a decision between a description that follows the technical hierarchy vs. one which follows english sentences. At least I don't care that much.
It is weird that we calculate this twice here.
I'm wondering whether it would be better to use
#prefix
on the date2 element instead.What is the reason what we remove this here?
I'm wondering whether this should rather calculate the middlevalue between these 2 dates.
Any reason why this method is public?
Comment #94
13jupiters CreditAttribution: 13jupiters commentedThanks all for the hard work on this. My testing effort was unsuccessful but before I litter the thread with problems, can someone verify just what version of Core the patch applies to now? I applied patch in #90 to 8.1.0-beta1 - pre-install, post-install - and to 8.0.5 (post-install). When manually updating the core Date module and running updb, hook_install_8001 appeared to fire successfully.
When attempting to add new Date fields to a node type, the form had the proper/expected options ("collect end date?" "require end date?"), but failed to save - no node__field_mydatefield tables, complaints about missing "value2", objects not existing, etc.
So the question is - have others manually patched 8.0.5 or 8.1.0-beta with #90 successfully? Should I be trying against 8.2-dev? Did I miss another required patch?
Edit: a single "date(all day)" - with no end date - still functions properly.
By the way - is there any progress in getting an exception made that would allow end date into 8.1?
Comment #95
mpdonadioThe patch is against 8.2, but there is little difference between 8.1 and 8.2 now, so it should apply cleanly to both, and I suspect it will apply cleanly to 8.1-beta1.
Hopefully I will have time to move this forward again.
I haven't been able to catch a framework manager to ask about getting this into 8.1.
Comment #96
13jupiters CreditAttribution: 13jupiters commentedPatch in #90 applied cleanly to 8.1.0-beta2 and end dates are working quite well. Very encouraging, great work everyone! A fully functioning calendar now within sight - something a lot of regular users are going to assume D8 has...
Comment #97
jhedstromI just spent some time manually testing #90. It is looking really close. A few issues I noticed:
These are pretty minor and I would be fine following those up in separate issues since this is already such a large change.
Comment #98
jhedstromThinking about this a bit more, I'm now thinking the suggestion above in #71 to have a separate field type (Date range) will make the most sense. If we pursue an all-in-one field as the D7 version did, the code will be needlessly complex (especially once we add on timezone support). Further, anybody with an existing date field that contains data will not be able to use the end date anyway (w/o custom programming or some sort of migration), so a separate field would not have a negative impact on folks with existing data.
Comment #99
mpdonadio#98, how would you address the concerns in #72 if we do the split? Or, are you thinking that we do date range as its own field, and then add timezone support to all of the field types?
Comment #100
jhedstromre #99 yeah, I was thinking timezone would be added for all dates, but the logic and formatters for a range could be isolated to its own plugin class, making things a bit simpler and more testable.
Comment #101
mpdonadioOK, talked with some people, mulled this over, and I think we need to retreat a bit to move forward.
Despite all of our efforts, the approach to add this to the existing fields has hit a brick wall. We need to redo this patch as two new fields, date+end and datetime+end. We can figure out better ranges, but we need clone the two existing fields and make verions w/ end date support.
One, this will be a lot easier. Two, it eliminates the upgrade path. C, we're stuck.
It will also allow us work on the timezone patch in parallel. If that lands first, it will be easy to add that into the new fields.
If anyone objects, please let us know.
Comment #102
heddnI think having a separate field makesa ton of sense. +1 from me.
Comment #103
darrick CreditAttribution: darrick commented#101 Doesn't the current date field support both date and datetime? Are those also going to be split?
Comment #104
mpdonadio#103, the existing types will not be split.
Comment #105
darrick CreditAttribution: darrick commentedSo six months ago I created a date_combo module (http://cgit.drupalcode.org/merci/tree/modules/date_combo?h=8.x-2.x). It's missing some widgets and formatters. But I could roll it into a patch.
Is date_combo a good name for the field?
And should it handle both date and date w/time, like the single date field?
Comment #106
ryantollefson CreditAttribution: ryantollefson commentedI'm certainly not a coder, so I admit that I don't understand all of the nuances behind these decisions; however, from the website designer/end-user perspective, fewer fields seems more intuitive.
Think about how date selection works in something like Google Calendar... everything has start+date+time and end+date+time. You check the box for all-day and the time fields disappear. Everything has a default end-date that is the same as the start-date, you change it only if needed. This type of setup seems pretty intuitive for the end-user.
Comment #107
heddnThe thing is we aren't just dealing with calendar based dates. We also have things like created date, date of birth, etc. Or maybe we have a re-occurring calendar invite. With no end, date. To support all of these, you need more flexibility than just a single date field that stores single dates as deltas.
So adding a date field that has a start/end on it makes sense, in some cases.
Comment #108
natedillon CreditAttribution: natedillon as a volunteer commentedIs there going to be a way to migrate from the existing date field to the new one? I'm currently building a Drupal 8 site that needs to use the date field for events, but I want to do this in a way in which I will be able to easily transition to the new one when it's available. I'm guessing the only thing I can do for now is set up fields for start date/time, end date/time, all day, etc., but is there anything I should be thinking about doing now that will make this an easier transition for me later?
Comment #109
heddnRe 108, migrate is in core. Moving data from/to fields is very simple.
For me, I'm using Delta 0 as my start date and Delta 1 as end. With my custom admin theme that is based on Seven, I can even easily make the ui read start/end for data entry using some very basic twig magic.
Comment #110
teek CreditAttribution: teek commentedHi All,
I just installed 8.1.0. Was a bit disappointed by the date/time entry field. The date field gives a nice popup where I can select a date (perfect!) but the time field next to it requires a very precisely formatted time string (like 18:20:00) and does not give a popup. There is also no way to add an end date.
I gather from the large amount of text above that there is "some" discussion regarding how to tackle this... I just would like to know, what are the time scales? When can I expect a nice time picker and a way to add an end-date/time? in 8.2.0?
Are there any modules you recommend that have this functionality right now?
Thanx in advance.
Comment #111
heddnThe timepicker is another issue. If one doesn't already exist for it, it should be created. The end date/time will be done when someone writes it up.
In the mean time, you can use two fields (for start/end) or use a single field with cardinality of 2 and use twig override in the admin theme to adjust the label text of delta 0 & delta 1.
Comment #112
mpdonadio#110, if your browser supports HTML5 date elements, then the time will use an appropriate time entry. Chrome has this, Firefox doesn't. There is an issue to add a polyfill for this in #1838234: Add jQuery Timepicker for the Time element of the datetime field.
At this point, both end date and the polyfill are targeted for 8.2.x.
Comment #113
mpdonadioOk, just reviewed the outstanding issues. I am going to tackle this, so @jhedstrom can focus on #2632040: [PP-1] Add ability to select a timezone for datetime field.
For the sake of sanity, this will focus on the fields and we will handle the views integration as a separate issue (esp since we have one or two views quirks remaining).
Comment #114
mpdonadioWork in progress to start to show approach. Not worth testing yet.
Comment #115
kaizerking CreditAttribution: kaizerking commentedworked for me
Comment #116
mpdonadioThis almost works. The widgets need some TLC, and the date-only version doesn't work (likely a stupid copy/pasta error). Too tired to debug now.
And then tests...
Unassigning myself in case someone wants to work on this tomorrow. Will pick back up around 7:00pm EDT (2016-05-13T19:00:00-04:00).
Comment #118
mpdonadioOh, yeah. The schema additions, too. That would help.
Comment #119
mpdonadioActively working on this tonight.
Comment #120
mpdonadioCan't figure out what I botched with the schema. Field formatter settings work, but the field settings don't (datetime vs date).
Still needs polishing, better validation, and tests.
Comment #121
mpdonadioTagging out for a bit.
Comment #123
mpdonadioThis essentially works. Updated IS with todo list.
Would appreciate prelim feedback, help with the Constraints, and help with the theming.
Then more polishing and tests.
Comment #125
mpdonadioElement validation. Not sure if we still want/need a Constraint on this, too?
Comment #127
jhedstromI'd say we do, since those would fire from the API, not just from the UI.
Comment #128
DuneBL#125 : Very nice patch; I would love to have it in core.
Here is a small feedback:
1-After saving a node containing several date_range fields, I got the following warnings/notices:
Warning: DateTime::diff() expects parameter 1 to be DateTimeInterface, object given in Drupal\Component\Datetime\DateTimePlus->__call() (line 294 of core/lib/Drupal/Component/Datetime/DateTimePlus.php).
Notice: Trying to get property of non-object in Drupal\datetime\Plugin\Field\FieldWidget\DateRangeWidgetBase->validateStartEnd() (line 144 of core/modules/datetime/src/Plugin/Field/FieldWidget/DateRangeWidgetBase.php).
Notice: Undefined index: group in datetime_field_views_data() (line 25 of core/modules/datetime/datetime.views.inc).
2-Edit Form:
In the Edit form, the "Start" label is inline with the field label, it should be on the next line. See the attached screenshot.
3-Feature request:
I really love the "Separator" setting because it allows us to output something like "StartDate to EndDate" if we choose " to " as a separator. What do you think to add another setting "Prefix" to get something like "From StartDate to EndDate" if we choose "From" as Prefix?
FYI, I am using 8.1...
Comment #129
edysmpApplied #125 and works. Missing view filter for date(I believe that now is for text)... so useless for me :(
Comment #130
mpdonadio#128, I am going to replicate that tonight and post a new patch before moving onto tests.
#129, views filters for this field will be a followup. It is too much to do in one patch, especially with the views related bugs we are currently trying to squash.
Comment #131
FluxSauce CreditAttribution: FluxSauce at Four Kitchens commentedDrupal 8.1.2 with #125 applied, added a date range field, then viewed the form and got the following Notice:
Comment #132
mpdonadioThis should fix the warning.
Will handle theme issue later on.
Prefer not to add too many options for the formatters. 1, we will bikeshed them forever and 2, they are easy to extend yourself for customization.
Going to start on the tests. We can do the constraint as a followup (I also think #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken complicates matters here).
Comment #133
mpdonadioThis should address the warning in #131, but note this patch is technically against 8.2.x, even if it cleanly applies to 8.1.x.
Comment #136
FluxSauce CreditAttribution: FluxSauce at Four Kitchens commentedWow, that was a fast one! Unfortunately, the bug still exists, and actually manifests a second bug (pretty much the same) when editing the field.
Something I neglected to mention and may be causing it - Default value: Default dates: Current date. Removing the default eliminates the error.
Comment #137
FluxSauce CreditAttribution: FluxSauce at Four Kitchens commentedComment #138
mpdonadio#136, I was literally working on this when you posted that. Did you try again with the patch in #133? The string in the error message doesn't appear anywhere in 8.2.x anymore with that applied, and I don't get a notice and default dates work fine for me. This is the hunk in the patch now:
Comment #139
darrick CreditAttribution: darrick commented#136: Just tested the patch from #133 and not able to reproduce your issue.
Testing Date Range with Date AND Time:
Testing Date Range with Date ONLY (same as above except with following issues):
Edited to add regarding#128: The labels on the widgets could use a little work. I.e. When using multiple values you have a header with the field name ("Date Range") then below that inputs for the multiple values. With the field name repeated again (value x) start. Would be better to strip out the both the field name and value x. For single value fields I agree with the start label should be below the field name with the field name having in separate div.
Comment #140
FluxSauce CreditAttribution: FluxSauce at Four Kitchens commented133 fixes the PHP notice, I hesitated because of the 8.2.x. Sincere thank you!
For completeness (I'm migrating from a D7 site so I'm doing constant comparisons):
* Defaults - should have the ability to set distinct defaults for start and end. Here's the D7 interface, values are same, blank, now, strtotime:
* User Interface: Checkbox for Show End Date - show_todate
Comment #141
FluxSauce CreditAttribution: FluxSauce at Four Kitchens commentedComment #142
johannez CreditAttribution: johannez as a volunteer commentedHello guys,
I'm working currently on a custom field that includes start and end date plus has the option to repeat weekly/monthly. I'm happy to help out and convert my work into a patch, if interested.
One thing I noticed though was that the value for the core Datetime module is saved as an ISO standard string in the database. All the other dates in the system (created, changed, etc.) are saved as UNIX timestamps. Is there a strong argument to save it as that string instead of the common timestamp? If there was a previous discussion about that, please let me know. I couldn't find anything here.
Comment #143
mpdonadio#142, thanks for the offer to help. Work on the current patch would be appreciated, especially test coverage; it's nearly ready other than that. I am not sure if we want to go down the road with a third restart. The current work is also keeping in mind views integration and how it will tie into some other parallel work (like the timezone mess).
You are strong encouraged, though, to publish your work as a contrib module, and then people have a choice. It may end up that recurrences stay in contrib anyway.
ISO dates where chosen a long time ago because they are easy to manipulate as strings, especially for database backends that lack date processing, for extracting the various parts.
Comment #144
johannez CreditAttribution: johannez as a volunteer commentedThank you, Matt, for the explanations.
I understand that ISO dates are easier to manipulate with db commands. However, not using timestamps is probably the reason we have that timezone mess ;)
However, I think it's manageable and I'll stay with saving datetime as ISO UTCs into the database.
I have to finish my client project first until the end of this month. Then I'll clean up the code and see, if I can provide a patch here or make it separate contrib module.
Once we got the end date down, it'll be easier to build the repeating date functionality as we can then re-use this module.
Regarding views integration, I noticed that the exposed filter for datetime only has a text field and not a pretty date selector widget. I'll look into that too.
Comment #145
13jupiters CreditAttribution: 13jupiters commented@johannez in #144, I think you'll find that the datetime widget for exposed filters has been handled in #2648950: [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter
Comment #146
jhedstromWith these new plugins, is there any way they can share a common base that provides more value than just
FieldItemBase
? While not ideal, thevalue
andvalue2
db columns used by D7 worked, and if we continued that here, then the range plugin could just extend the base fields of the single-date plugin. We also wouldn't need new type constants, and it might simplify views integration since at least one of the columns (value
) would be shared between the field types.Comment #147
Jaesin CreditAttribution: Jaesin at Chapter Three commentedIf that were the case, the end data wouldn't need to be a separate field at all. Just a different widget or widget option like a "Collect an end date?" check box. Like what we see in #77.
Comment #148
mpdonadio#90 probably represents the last real attempt to do this in the existing fields. Thoughts about the split start in #97 or so.
I don't have objection to using value, value2; I just chose new names b/c of the comments earlier in the issue about how bad these were :)
Comment #149
DuneBL#132: Many thanks for the patch; here is my feedback
1_The warnings showed in #128 are gone in 8.2 (but still there in 8.1)
2_There is still a problem with the Start Label in Edit form (Single value):
=>the "Start" label is inline with the field label, it should be on the next line.
Comment #150
mpdonadioHopping back on this. I have time to put another dent in this the next few days.
Going to continue forward with the idea of a new field for this, and start out with changing the schema back to 'value' and 'value2', and then adding an all-day option.
Comment #151
jonathanshawIf I interpret #146 right, the suggestion is about architecture (2 columns of one field), not the naming of the fields/columns. So do continue to use better names than 'value' and 'value2' if you think that's right. I really appreciate your work here!
Comment #152
mpdonadioRenamed schema keys.
Comment #154
mpdonadioFirst pass at all day.
Comment #156
jhedstromSince now 1 out of 2 columns are shared between the field types (
value
), this need no longer be conditional. A follow-up issue will need to add support forvalue2
.Comment #157
mpdonadio#146, so I renamed the schema keys; that will help with the views integration. I think I want to get the tests in place first (and passing), then see if we can refactor to leverage the existing classes. Mainly, I think we are pretty much at the point where we need the tests to make sure we don't start going backwards (especially with some of other patches that are getting close, and will likely rebase/merge against).
Starting on tests...
Comment #158
mpdonadioGot about half the tests done; this is mostly copy-pasta from the old ones. Still need to go through all the deprecated functions in them...
Comment #160
Jaesin CreditAttribution: Jaesin at Chapter Three commentedMaybe this has already been brought up but If a customer decides they need to collect an end date on an existing datetime field, we will need to create a migration. That's a significant con for a new field type IMHO.
Comment #161
mpdonadio#108 and #109 discuss migration with a new field, and is one of the reasons we are going down this road.
Hoping to post the first pass at the tests w/in the next 30 min. We are in the home stretch.
Comment #162
mpdonadioThe all-date option has a timezone bug that I need to look into. The value in storage ends up midnight-to-midnight UTC instead of in the user's local time. I'll look at that tomorrow. Rest of the features and tests should be good for review, other than layout issues. Want to get a front-end dev to help out with this to do it right.
Comment #164
KarenS CreditAttribution: KarenS at Lullabot commentedNice work! Just a note that I have found that storing an all-day date as midnight can be confusing because doing a timezone conversion on it would shift the date in many cases. What I did was to store all-day dates as noon instead. That is less confusing, the date stored in the database always looks right and will in fact be right even if someone applies the wrong timezone to it.
Also, I totally concur with the idea of creating a new field for this purpose. It seems wrong to apply all this overhead to every place that simple dates are used, which is why I didn't add it to the base field. My intention was always to have a simple field for simple purposes and a more complex field only when all this overhead is actually required.
Comment #165
mpdonadioThanks Karen.
This is somewhat scattered across a few issues and some IRC, but once this lands we will have two day options. One is based on the day only from the existing field, and that still uses noon as it's time. However, we discovered that people in NZ are having problems with date rollover with this (see #2739290: UTC+12 is broken on date only fields). In that patch, we are removing timezone support for that option only, so the date is just a date.
This patch introduces all-day, which is really a date+time, where the time is always midnight to midnight using timezone handling (and we have another issue for zone selection upon input). So, there will be both uses cases available for people.
Comment #166
mpdonadioShould be green. Manually test and review away.
Going to see a FE dev at DevDays wants to clean up the widget form; we know that needs TLC.
Comment #167
mpdonadioAdded some issues that will impact this, or will impact them depending on commit order.
Comment #168
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedThis is great, thank you for all the work!
I did some manual testing with date, datetime, and an all day field. Overall, it works very good.
For the allday field (and possibly the other ones, didn't check), I did find a warning in the logs:
and
To reproduce, just create a new content type and add an allday field. Then create one item of your new type, filling in the allday field, and save.
Some nitpicks:
That should be $end_date in the comment.
And here again.
The last part of this description was really confusing me until I tried all options. So the issue is that a date-only field will also display a time value if the format defines it? Shouldn't we just be saying that in the description?
Comment #169
pguillard CreditAttribution: pguillard commentedFrom #168
Comment #170
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedApparently, the branch I was working on didn't start from 8.2.x. Judging from the comments above, this is probably the cause of that warning, so we don't need to look into that. Sorry for the noise.
Comment #172
mpdonadioPutting this back to NR b/c of an apparent TestBot fluke.
Re #168-3, that is nearly identical to the text in DateTimeDefaultFormatter. Since all of these fields are in memory as DrupalDateTime objects, the date-only values do have a time associated with them. And, the same formatters are used for both datetime and date-only variants. I'm open to suggestions here, but struggling for anything better.
Comment #174
jonathanshawRe #168.3 / 173 I agree the message is not great, but if it's the same as the default formatter, let's make it a separate issue. That helps keep this issue moving forward.
Comment #175
gambry#169 works for me, just a note:
It should probably use the constant DateRangeItem::DATERANGE_TYPE_DATE on every condition you check
$this->getFieldSetting('daterange_type')
, but I can see constants are not used on the DateTime formatters too so I can see consistency on leaving everything as it is.Great work!
Comment #176
DuneBLI have created a date range fields before patch #169
If I run drush entup (after applying it), I receive a message to update the field:
Then I get some errors:
Exception thrown while performing a schema update. SQLSTATE[42S22]: Column not found: 1054 Unknown column 'field_my_date_range_value'
Comment #177
mpdonadio#176, do you know when you made these? I think this may be because renamed the columns back to value/value2 to be consistent with the singular date and the Drupal 7 Date module.
Comment #178
DuneBLIt was very early in the patch live...
I assume I will have to recreate this field
Comment #179
jhedstromI started reviewing but ran out of time. I think this is very close.
Note several of these items are to pick up fixes from #2739290: UTC+12 is broken on date only fields, so if it becomes too burdensome, we could postpone on that (probably not necessary now, but if it goes back to needs work, then perhaps).
Coding standards nit pick: Should capitalize first letter, and add a period.
Needs a space between
*
and)
.I think this will inherit the fix over in #2739290: UTC+12 is broken on date only fields, so hopefully no follow-up work will be needed when that lands.
This should be re-worked as the fix over in #2739290: UTC+12 is broken on date only fields does (basically avoid setting timezone to null here, and instead inherit the date timezone if there isn't an override)
Same change here as noted above.
Rather than use the global
REQUEST_TIME
constant, in other formatters (and views plugins) in the datetime module, we've injected the request (or in some cases the request stack) service to the formatter, and then used:$this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME')
I think that will be preferable here.
Same as above.
This needs a fix similar to the one over in #2739290: UTC+12 is broken on date only fields:
Spacing nit here.
Comment #180
mpdonadioNW to backport #2739290: UTC+12 is broken on date only fields to this patch. Unassigning myself in case I can't get to this in the next few days.
Comment #181
mpdonadioNits, before moving on to the date-only timezome change, so we can look at a cleaner interdiff.
Comment #182
mpdonadioTZ change w/o test change. Still comes up green for me. Onto the test change itself.
Comment #184
mpdonadioPossible spurious in #182?
This is green for me locally. If same, I can has reveiw?
Comment #185
mpdonadioComment #186
jhedstromQuick review here.
Nit, I think this comment went in accidentally with the UTC timezone fix, so shouldn't be copied here.
This one appears to be missing the UTC timezone fix.
Comment #187
dkre CreditAttribution: dkre commented#184 works perfectly (almost) applied to 8.1.7
The only issue I have found is when you have a multiple value field with a select list formatter. If the ampm value is unselected/unset an error message isn't displayed when 'add another item' is clicked but a validation error is shown on form submission.
Really great work.
Comment #188
jhedstrom@dkre.one Does this happen with multivalue date fields without end dates (eg, without this patch)? Meaning, is that a bug introduced in this patch, or an existing bug? If the latter, then we should open a new issue to address that.
Comment #189
jonathanshawCan we plan a strategy for getting this into 8.2 before window closes august 3?
How about:
1) fix #186
2) investigate #187
3) move widget theming to a follow-up (may be eligible to go in during 8.2 beta period?)
4) mark rtbc (or is more in depth code review needed?)
5) create documentation follow ups?
Comment #190
mpdonadioAnyone feel free to tackle #186 and #187, and/or the styling thing. I won't have time until Saturday. @jhedstrom shouldn't work on this, so he can review. I will also ping people at DrupalGovCon.
Comment #191
jonathanshawI cannot replicate #187 on a simplytest.me clean install of 8.2 with this patch. What I did:
Add date field, unlimited, to article content type
Select "select list" as widget for the field, choose "12 hour time" as time type, update, save.
Add a new article, select values for year, month, day, hour and minute but leave am/pm unselected.
Press "add another".
Then I see a validation error on the AM/PM, just as I do without this patch.
Possibly #187 arises because of applying patch #184 to 8.1.7 which does not have #2739290: UTC+12 is broken on date only fields.
I suggest ignore #187.
Comment #192
jonathanshawI have tested widget/field combinations systematically and can see no theming issues with the widgets that are not also basically present with regular date fields. See screenshots below for a comparison.
Given this, and that theming can easily be done in a follow-up, and that theming can be done during beta period, and that theming should be postponed on #1918994: Improve Datetime and Daterange Widget accessibility which looks unlikely to make 8.2, I suggest we leave theming out of this issue.
If there are theming problems we should fix in this issue, please would someone specify them exactly.
Comment #193
mpdonadioWill have #186 as soon as I get back. Expect patch tonight.
Comment #194
mpdonadio#184 addressed.
Comment #195
jhedstromI am trying to rally a few more folks to review.
Let's open that issue and reference it here.
Comment #196
mpdonadioInstead of a followup, I think #195, I think we can revive #1918994: Improve Datetime and Daterange Widget accessibility as the solution for any theming issues, as that is the better for the long term and a11y. That is in the IS right now.
Comment #197
generalredneckJust a note... it appears that there has been some back and forth on which (if any) should allow NULL. Currently from my testing neither start or end date are allowed to be NULL.
Comment #198
mpdonadio#197, that is expected now that this is a separate field.
Comment #199
jhedstromI spent some time going over this very closely. It is looking good.
One tiny nit, and one architectural question/comment:
Tiny nit, missing a space between the '*' and the ')'.
Code comment is out of whack with the class name.
Looking at this, does it make sense to extend
FormatterBase
and replicate all of the code inDateTimeFormatterBase
?It looks like if that were extended, much of this code wouldn't need to be duplicated...
Extending the existing formatter base will make follow-up easier (for instance the timezone support issue).
Comment #200
mpdonadio#199 addressed.
Architecture. A while ago, we decided for expediency (ha!) to keep these on separate tracks w/o a common base in case we ran into problem or rebase hell with the other issues, get it in, and then refactor for commonality. I think at this point we just need to get this done, and then create a followup.
Comment #201
dawehnerThis is looking really great, IMHO. I think most points from below can be totally solved in follow ups.
Follow up work: Use the request stack twice
Possible follow up: A lot of this code is really similar, I could imagine this could be refactored a bit.
For future reference, IMHO it could be helpful that these documentations explain what is special about those formatters ...
This could be moved to a possible follow up
Possible follow up/future reference: Describe why we need this class. What is special about date ranges, that it needs its own field item class.
Possible followup: IMHO we shouldn't rely on IDs for the
#states
API, as they might change in ajax requests. Use name instead?Shouldn't this class override
mainPropertyName()
? Otherwise this method returns 'value' which isn't right, that's for sureFollow up, nitpick: Use DateTimeComputed::class
Nitpick followup: Add public to those methods
It almost seems to be that we should better have some foreach loop over
['year', 'month' ...]
and['value', 'value2']
Comment #202
mpdonadioNon f/up items from #201.
Comment #204
mpdonadioNot sure what proper fix is for the FieldTypePluginManagerTest::testMainProperty() fail? DateRangeItem is a compound value, so it doesn't have a main value column?
Comment #205
swentel CreditAttribution: swentel commented@mpdonadio just leave out the mainPropertyName() method. DateTimeItem doesn't implement it either (and thus falls back to 'value'). Should be fine for this one too, you can argue that 'value' is the 'mainest' property here :)
Comment #206
mpdonadioThanks @swentel.
Comment #207
mpdonadioInterdiff didn't attach first time around.
Comment #209
dawehnerBut isn't the problem that we don't store any value in the first place in 'value' and by that would always return NULL?
Comment #210
mpdonadio#209, we reverted back to 'value' and 'value2' for the actual schema to make ports easier, and to potentially allow some base classes between datetime and daterange. We use better variable names in PHP instead.
Comment #211
mpdonadioLooks like some spurious update fails. Back to NR.
Comment #212
jhedstromI think this is ready to go!
Could somebody add follow up issues for #201?
Thanks everybody!
I love this refactor!
Comment #213
mpdonadioJust found one oopsie in the last refactor. Passes locally.
Comment #214
xjmGreat work on this issue; very clean and good test coverage. A few comments scanning through the patch.
I think this should say "field type" rather than file type?
"Daterange" is not an English word, so I think it should be two words everywhere in strings. (I'm fine with daterange as a machine name though. An underscore might be better but I have no strong opinion.)
I think these changes are out of scope.
Comment #215
xjm@mpdonadio reached out asking me whether this could have some exception to the beta deadline. We're discussing whether that would be okay, but in the meanwhile, adding the beta deadline tag for visibility so that this gets prioritized in the RTBC queue once it is back to RTBC.
Comment #216
mpdonadio#214 addressed.
Comment #217
jhedstromBack to RTBC assuming #216 goes green.
Comment #218
dawehnerI'm also really happy. It would be great if someone take the time to look at some of the follow ups I propose, but IMHO this totally don't block the RTBC.
Comment #219
dawehnerI didn't meant to change things!
Comment #220
generalredneckSo the major schema change was a big headache for sites that already have data and using this... But I know that's what happens when using patches :P
With that said, This is working pretty well minus the comments I (and others) have made above about the inconsistancies of features between this and D7.
One other Nit Pick... Because of the schema change, Views now shows the following:
http://content.screencast.com/users/allan4k/folders/Jing/media/e15d5481-...
but again... none of this stoping me from saying SHIP IT :P (and please no more schema changes without updates)
Comment #221
colanAdditional follow-ups can be created for missing D7-like things.
Adding tag so that we remember to add the follow-ups we know we need, discussed earlier.
Comment #222
jhedstromI added a few follow-up issues.
Comment #223
xjmI cannot reproduce the Views duplication @generalredneck indicates. Does this only occur on a site where an earlier version of the patch has been applied? (Also, there is not, and will not ever be, any expectation that patch authors need to provide an upgrade path between different versions of a patch. If you want an upgrade path for that, write one.) It also does not make sense to provide an upgrade path from an existing field type to a new one, because we have no way of knowing whether the site author wants an end date. So I think we are all good on that front.
I did a bit of manual testing and it works well! I am first presented with these options in the field selection:
One concern I had about the approach in this patch was that adding an additional field type would make the already overwhelming field selection interaction more overwhelming, but I don't think that is the case. It's easy for me to understand the difference between "Date" and "Date range".
Then I configured the default value:
All good so far. Next I tested the field:
This is the first thing that is a little funky. "Release window Start" is goofy capitalization for English. This makes me suspect either a string like
@foo Start
that should be@foo start
for the English default, or that two strings might be being concatenated together in a less translation-friendly way. (I am not sure which because I haven't done a code review yet.) I also couldn't figure out any way to configure these labels either; it didn't seem to be an option on (e.g.) the form display settings for the field.Next I tested the validation for an end date before a start date:
Worked as I expected. Edit: Except the validation message could be a little better. How about "The @label end date cannot be before the start date"?
I entered a valid date range and then looked at the default formatter:
Looks good to me!
Finally I tested changing the formatter:
I found the differences between "Plain", "Custom", and "Default" non-intuitive; "Default" provided a lot more options than I expected a default to; and it seemed to me like Default and Custom were partly redundant. But I checked and confirmed these things are all true of the existing Date field type as well, so not introduced here.
Leaving at RTBC for now until I've had a chance to do a code review, but the bit about the start date label (edit: and the validation message) might be actionable.
Comment #224
xjm(Uploading correct screenshot.)
Comment #225
xjmOne other thing I've just noticed. The UI label in the screenshot above when choosing the field type appears to be "Date Range" (title case) but our text standards specify sentence case (like "Taxonomy term" in the screenshot).
Comment #226
xjmHmm, so, I've started a code review of the patch, and I'm a bit concerned about the amount of code duplication between this and the existing Date field type and its widgets/formatters. I see there is #2775669: Clean up redundant methods in datetime field formatters and #2775671: Datetime field items can extend from a single base class but I'm not sure I'm comfortable with those as followups given the sheer scope of the duplication, especially to add this as a stable feature. Every time we have a bug, we have to remember to check whether it needs to be fixed in both places, etc. This adds a lot of potential technical debt.
My feeling at the moment is that we would need to take care of the bulk of this code duplication now, before commit. In other circumstances we might commit something as an experimental module instead and make the refactoring a required part of the path to stable, but I don't think that is a good fit in this case, because then we are painting ourselves into a corner in terms of moving it back into datetime once it's stable. It's not like a UI that can just be moved into another module later without too much disruption; it includes a storage and so we'd get into the same situation needing an update path.
I'll get a second opinion from another committer on whether the issues to refactor away the code duplication can be followups.
Comment #227
mpdonadioThanks for taking the time to look at this issue.
My biggest concern is that the refactoring may not be as straightforward as it appears because of potential subtleties between the two types. For example, if the date range FieldItem extends the existing one instead of FieldItem, then the range item will have both sets of constants. Internally, there are naming convention differences (date vs start_date/end_date). There may be other things. This could potentially spill over into needing test changes, and then we are back to chasing our own tail with refactoring and fixing tests at the same time. I'm not saying this shouldn't be explored, but I think it may be a bigger exercise than we think to do all in one step to get into 8.2.0 on time; smaller refactors through separate issues may be safer.
However, I will defer to committers on what is best here.
Comment #228
generalredneck@xjm
Negative
First you misunderstood the issue. There isn't a "views duplication". I was simply stating that the views field names were not... super which is why I didn't make a big deal out of it.
To reproduce:
In my test I have a daterange field called "Date" with the machine name of "field_date"
Notice that it shows the following when searching for handlers as shown in the screenshot above:
Date (field_date)
Date (field_date:value2)
What would be more correct would be something along the lines of
Date:Start Date (field_date)
Date:End Date (field_date:value2)
or something to indicate that "value2" is in fact the end date as it would take a developer to understand this.
This is my mistake for not giving the proper format for reporting
Understood
Comment #229
xjmThanks for clarifying @generalredneck; I was able to reproduce that too. Because of the schema itself for the field:
I agree that
value
andvalue2
are not sufficiently clear key names. (I see also that this was raised earlier on the issue, e.g. by @Berdir.)So I think this patch needs work at least for that point as well as the string changes suggested in #223. Still waiting for a second opinion on whether the refactor should also be blocking.
Comment #230
mpdonadioOk, string updates are in this version.
Re the schema, we decided to go back to value/value2 for a few reasons (see #146). That is consistent with how the Drupal 7 Date module did things, so that may may migrations easier. Using value/value2 instead of start_date/end_date (a few patches had this) will allow for potential refactoring to allow reuse between DateTime and DateRange classes. It will also likely allow the same thing to happen when we do real views integration (which was decided to not be part of this because of the added complexity, some views related work that is still ongoing, and the potential desire to wrap in the timezone support into views at the same time). I do think this is the proper decision here, so I did not redo that.
Views. Nice catch. See #156. For the time being, I put that hunk back in, so the handlers don't try to get wired in, but you can at least display the field value in a view (since field values default to use the formatters and not plugins).
Comment #231
xjmThanks @mpdonadio.
I'm not sure I agree with #146. For me, what contrib did in D7 is not a reason to introduce a similar vagueness in the schema in D8. Let's get an entity or framework manager's review on that too. I'll add these two things to the remaining tasks in the summary.
Meanwhile, a couple of very small nits with the latest interdiff:
These should use FormattableMarkup instead of
t()
.Comment #232
mpdonadioOops, thanks for the reminder. Fixed, and passes locally.
I don't think parity with Drupal 7 is the primary factor with the schema key names, but more with the DateTime classes to allow extension, where possible (eg, inherit the value behavior from the the DateTime class and then add support for the value2 in the DateRange class, where possible). That said, I do agree value2 is a bad name, but maybe not as bad as using value/end_value because that option wouldn't be symmetrically descriptive.
Comment #234
mpdonadioBack to NR; spurious fails in FieldHandlersUpdateTest.
Comment #235
catchWhat's wrong with start_value and end_value?
Comment #236
tim.plunkettFixed a bunch of code style issues, no logic changes.
As someone who spent a lot of time dealing with D7 Date module, I think that keeping value/value2 is beneficial. For example, when handling a large set of dates with and without ranges, you can rely on
->value
always being there (also when doing Views or direct DB access).I'm going to tentatively set this back to RTBC on those grounds.
Comment #237
xjmThanks @tim.plunkett.
Even if
value
is the name for the start date, I still thinkvalue2
is not a good name for the end date.Comment #238
xjmAnother difference AFAIK is that with this field both start and end values are required. Neither is allowed to be NULL.
Comment #239
mpdonadioKeeping 'value' and picking something new for 'value2' would (1) alleviate my concerns with a refactor (2) be mostly easy to implement. Open to suggestions; think 'end_value' would be fine. If nobody else come up with some better, I can implement that tonight.
Comment #240
xjmDiscussed further with @effulgentsia and @catch.
We would like to see refactoring the date field, widget, and formatter code to possibly use some base classes and/or traits explored before we commit this. In turn, we agreed to make the issue a beta target, since it is RTBC now otherwise. That means we'd like to consider adding this feature during the beta if it is ready within the next few weeks, ideally before we create a second beta.
That way, if the refactor turns out to be problematic, we can also go back to this version of the path.
If the issue turns out to not be RTBC within the beta phase, it can still be one of the first features committed to 8.3.x this month or next, which will also be beneficial to site owners and contrib authors looking forward to this feature.
Thanks @mpdonadio, @jhedstrom, @tim.plunkett et al!
Comment #241
mpdonadioMany thanks to the various managers and others who have been looking into this. I really appreciate it.
I have a plan worked out for this in my head. Will be posting incremental patches, starting with the schema key change (value2 => end_value), then the field items, then widgets, then formatters (there is a tricky part here that I want to eventually brainstorm). And we will consider #236 the reference patch.
Timing should be OK.
Comment #242
mpdonadioOK. Schema key change. value2 => end_value. DateRangeFieldTest passes locally.
Comment #243
mpdonadioOk, here are refactors of DateRangeItem and DateRangeFieldItemList. DateRangeFieldTest passes locally. This is best read applied.
Changes to DateRangeItem are mostly straightforward. The constants are inherited, and the new one is added in the same style. It also changes the settings key to the inherited value, which should make more of the widgets and formatters reusable.
Changes to DateRangeFieldItemList are a balance between proper reuse and forcing things to be reusable. For example, the form elements can be inherited w/ minor alterations, but using the parent submit and validate functions would require shenanigans. I also uncovered a bug in the default values, where you could not set only one; that logic made this not a good candidate for reuse.
Comments appreciated. Will start widgets next, likely tomorrow night.
Comment #245
joelpittetRandom test failure, setting status back to needs review.
Comment #246
mpdonadioOk, here are some small additional changes to the field items, and a pass at some refactoring the widget bases.
The widgets themselves are a conundrum. The default widget is different enough b/c all-day that any real changes seem forced. The list widget has common code, but it this gets moved to a trait, then you lose the connection to WidgetBase. Ideas?
Note that in the process of doing this, I uncovered #2778083: Default value for Date-Only fields is broken when UTC date is different than user current date.
This passes locally. Will start formatters next, likely tomorrow night but may not finish until Thursday or Friday. The plan for those is
- ditch DateRangeFormatterBase
- refactor the DateTime formatters to have a method that builds the render array for a single item, and use this in viewElements
- have the DateRange formatters extend the equivalent DateTime
- the DateRange viewElements would then call the method to build a single item (which would be inherited), and then just glue them together with the separator
I think that approach will work, and will be good in the long run.
As always, comments appreciated.
Comment #247
mpdonadioOK, I think we are cooking with gas now. This includes some small cleanups of previous patches, plus the refactored formatters. I really like how these turned out. I had to make some changes to the DateTime formatters to make this possible, but no changes were made to DateTimeFieldTest.
DateTimeFieldTest and DateRangeFieldTest pass locally.
Tagging out. Think this is ready for review. Interdiffs against the #246 and #236 are included. This is probably best reviewed applied, as the interdiffs are pretty messy.
Comment #248
Berdirthis confused me a bit, non-explicit array keys are a bit uncommon with render arrays. with the cache contexts on the top element. There is also a risk that cache contexts are lost, if people print out the dates separately for customized output.
Might make sense to have this as its own template so it is easier to customize. Also, what about RTL languages?
is there a case where the cache context is *not* added? maybe just do it in buildDate(), the range fields have it twice then but that's not really an issue. Skipping them as described above should also not be an issue then.
that means the start date is only stored if there is an end date. is that not a supported use case (I think not filling out the end date if it is the same is pretty common).
maybe both fields are required in the widget anyway, didn't get there yet.
hm, not seeing any special logic. how does this work, also with a required vs non-reqiured field exactly?
Comment #249
mpdonadioThanks @Berdir, I'll fix up 1,2 with the keys and contexts. I went back and forth on them anyway. Have to think about templates and RTL support.
3, We have both as mandatory here since they are separate fields now; widgets enforce this (later patches have test coverage for this). If you don't want an end date, then use a Datetime field instead.
4. That is a near lift from the way the existing DateTimeDatelistWidget works. The top of the method calls DateRangeWidgetBase, the top of which calls DateTimeWidgetBase. If you trace through those, you will see how the wrapper, required, values, etc, all get set up. That particular hunk is just merging in the parts to make the end date work list a select list. The rest of the $element has been taken care of through the parent calls.
Comment #250
markabur CreditAttribution: markabur commentedIn my experience, something like an event calendar will have a mix of single-day and multi-day events. In the past I've always been able to use one date field for this, and my clients have never needed to fill out the end date for all-day single-day events.
Would it be possible for the widget to work like date module for D7, where the end date is optional and there is a "Collect an end date" checkbox to show/hide it? If the checkbox is left unchecked, the end date widget would be hidden, but behind the scenes maybe it could automatically use the same value as the start date?
Comment #251
mpdonadio#250, it is definitely possible, but I am hesitant to add this if we want to hit the 8.2.0 deadline, especially after having an RTBC version of this that we are just refactoring now.
Both widgets would need to be updated, along with test coverage and test changes for how end date being required is handled. If the start date and end date are the same, then the formatters already only show one (mostly, all-day is really a date+time from 00:00 to 23:59, so it is always range in storage).
This is something that could definitely be tackled in https://www.drupal.org/project/datetime_extras, especially since we aren't trying to exactly replicate the Drupal 7 Date module 1-for-1 feature-wise in core. One of the reasons we made that module is so we could have a place for special formatters, widgets, and things that are going to be tough (like proper recurrence support).
Comment #252
colanAgreed. Let's make it optional in that contrib module or in another follow-up so we can make the deadline here. It won't change the data model so we shouldn't run into too many problems.
Comment #253
mpdonadioThis should take care of #248 1 and 2, but not using a separate template; 3 and 4 were answered earlier.
Going to play the Stupid American card, and ask what the proper way to handle RTL is? Inject LanguageManager, then check $languageManager->getCurrentLanguage()->getDirection() and change the element order if the result is DIRECTION_RTL? Not really seeing anything in core as an example (may not be tracing this out right or fully).
Comment #254
jhedstromThis seems like a sound approach. In
ckeditor.admin.inc
there is some use of this, but it didn't look like it was swapping the order of anything, but rather using a different image button based on RTL.I will be out most of next week, so am going to say this is again RTBC from my perspective at this point.
Comment #255
mpdonadio#254 do you think RTL is a RTBC blocker here, or can we do that in a followup? I would want to make sure we do that properly and add test coverage if it is a blocker.
Comment #256
jhedstromI don't see it as a blocker, so let's add a follow-up. Back to RTBC!
Comment #258
mpdonadioBack to RTBC; sporadic fail up updates tests, https://www.drupal.org/pift-ci-job/408095.
Comment #260
mpdonadioSetting this back to 8.2.x since @xjm added this as Beta Target.
Comment #264
mpdonadioSent a msg to @Mixlogic to try to see whether the latest fails are patch related or bot related. Not sure why we have passes, and multiple versions of fails here.
Comment #265
mpdonadioPer @Mixlogic, this looks like a general problem with those tests (happening elsewhere). Will set this back to RTBC when we know the nightly test runs will be OK.
Comment #266
mpdonadio@Mixlogic suggested resetting the status and this looks like a problem with the migrate tests in the CI environment and may take some time to hash out.
Comment #267
sylus CreditAttribution: sylus commentedJust attaching a 8.x-1.x compatible backport for composer / drush make / testing purposes.
Comment #268
mpdonadioSince #2739290: UTC+12 is broken on date only fields did not get a backport to 8.1.x, I am not sure the patch in #267 will work reliably.
Comment #269
sylus CreditAttribution: sylus commentedThanks for the tip! I haven't run across that issue during my prototyping yet but good to know!
While looking at this code I noticed I needed to make a slight tweak to get date_ranges to properly work with arguments in views. Since date_range is essentially a datetime when the two fields are taken into account the simple fix for me seems to work.
I was wondering if this should instead be:
Comment #270
mpdonadio#269, thank hunk of code is to disable the views plugins for daterange fields, but allow them for datetime fields. The plugins somewhat work with views right now, but not fully enough to warrant not having that code (see #228 and the few comments that follow it). We will handle proper support in a followup. The original patch for this was a bear (the refactoring we did should help this), and we may want to wait for #2627512: Datetime Views plugins don't support timezones to be committed and #2632040: [PP-1] Add ability to select a timezone for datetime field to be wrapped up before tacking them for datetime ranges. That is why we are pushing to get this into 8.2.x, as it does block a lot of planned work.
Comment #275
Zythyr CreditAttribution: Zythyr commentedI have been following this issue and it is my understanding you guys have a possible solution but it will take some time to test and make it stable. I am not a developer so I don't completely everything that was discussed. I have a question regarding implementation of a date field on my live D8 site.
I plan to launch a live D8 site which has an event content type with a date field. I want to require users to input an end date. The current stable version of D8 doesn't have an option to input an end date. Shall I create another date field for the end date as a temporary solution? If I use a second date field to collect the end date, how will this affect my site when a stable solution is published for collecting an end date in a single date field? Will there be a way to merge two separate date fields (start and end) into one?
I want to thank you guys for working hard on finding a solution for the end date in D8.
Comment #276
dkre CreditAttribution: dkre commented@Zythyr
I'm in a similar position with a project currently in development which requires this feature. Even though I have quite a bit of experience with D7 and some basic coding skills, it took quite a bit upskilling to get my project on track with the changes that came with D8. Not the coding but the development environment that is required to work effectively with D8 which is really what you need.
Git (or some sort of version control) is critical now with the new release schedule of minor/patch (8.minor.patch, 8.1.8). While learning to use git, branching, patching etc might be a hassle it has too many benefits to overlook. Using git you can branch (make copies) of your site prior to testing any new patches/updates while working with the beta releases of 8.2.x and patches. It makes the whole process easy and stress free.
This feature/issue has been promoted to 8.2.x beta (see #260) so this means that we know when this will be released which is fantastic, so the approach I've adopted is to work with core 8.2.x and look to go live after the 8.2.0 release date (Oct 8, from memory) to ensure I'm going live with stable releases.
You won't be able to merge the field data between Date Time field and this Date Time Range fields at a later date because they are distinctly different field types. You could migrate the data through an import/export process but this requires a bit of coding. Once the data has been merged you will need to then alter your views etc to work with the new field however.
Hope this helps.
Comment #277
xjmHmm, I am concerned that the repeated failed tests on this issue do not actually appear to be happening in HEAD in the same way at the moment. Compare:
https://www.drupal.org/pift-ci-job/422159
https://www.drupal.org/pift-ci-job/415739
I'm going to reupload the patch and retest it against the three databases. If it is somehow exacerbating an existing random fail that becomes a blocker. If all the test runs pass, we are probably okay.
Comment #279
xjmThat's not likely to be happening because of this issue. Let's try rerunning the tests attached to the patch above once CI is stable.
Comment #280
mpdonadio@xjm, I added PHP 5.6 and PHP 7 against MySQL to the list in case this gives more clues, but I will refrain from retesting. @Mixlogic found another test that had the same fail (PHPUnit_Framework_Exception: zend_mm_heap corrupted), and was investigating. Unfortunately, I don't have the IRC log saved from when we were talking about this.
This may shed some light on this, though: http://stackoverflow.com/questions/14597468/how-to-diagnose-these-php-co...
Comment #281
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI've been reviewing this patch, and overall, I really like it a lot. I'm not totally done reviewing it yet, but here's the only things I've found so far:
Here and similar, I think we should make the 'type' hierarchy match the class hierarchy. I did so in this patch.
I don't like this function. For one, its name is misleading, since what can be returned includes anything within '#cache', not just contexts, and for that matter, other top-level properties, not just #cache. If we really want to separate out cache information from the rest of buildDate(), then it would be more appropriate for the function to return a CacheableMetadata object instead and have the caller use the applyTo() method, or something like that, but that can be a followup. In the meantime, I just inlined this into the callers. Those callers already inline, and therefore amongst themselves duplicate, the call to
->setTimeZone()
, and the addition of the cache context is required due to that, so having both lines of code near each other has its advantages.Leaving at RTBC since these are minor changes.
Comment #282
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAlso, I reviewed @xjm's points in #223 and confirmed they're all addressed except this one:
The reason for this is that "Release window" is the label of the element representing the field item overall, and "Start" is the label of the element for the start date property. They just appear near each other due to I presume some CSS that inlines the two
h4
elements. See screenshot:I'm fine with punting any improvements of this to a followup.
Comment #283
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFrom #281's interdiff:
Oops. That can't be removed. This patch fixes that, and cleans up the date range formatter defaultSettings() functions to only include what is different from the parent class, which makes it easier to compare the classes to the config schema.
That I messed up on that schema change signals that #281's interdiff, and this one, needs another pair of eyes on it, so setting to "Needs review" for that. I'm still reviewing the full patch as well.
Comment #284
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhat's this code accomplishing? I skimmed through the issue comments about punting Views integration to a follow-up, but I'm not clear on what this code is actually solving. In HEAD, views_views_data() currently does this:
So, by making datetime_field_views_data() return empty for daterange fields, we're not actually removing those daterange fields from anything: not from fields, nor filters, nor sorts, etc. Seems to me all we're doing is forcing those filters/sorts/etc. to use string-based comparison instead of datetime-based ones. What am I missing?
Comment #285
mpdonadio#284, if that hook runs for both datetime and daterange fields, the daterange fields get the plugins that datetime.module defines for argument/sort/filter, instead of the default ones provided by views.
The biggest problem is that the end value doesn't get picked up properly (see around #228) and the plugins behave oddly. Doing the field + the proper views plugins w/ proper testing is too big of an issue to tackle at once. Part of the refactor was to make this a little easier, but there is still a decent amount of work to make this work as expected. We can add the plugins with datetime_extras once we have something mostly stable, and then get into core for 8.3.0.
Comment #287
xjmCan we add an inline comment for #284?
Comment #288
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI filed #2786577: The Views integration Datetime Range fields should extend the views integration for regular Datetime fields as a child issue and here's a code comment for it.
I'm still looking for a review/RTBC of my changes from #281 on :)
Comment #289
mpdonadioThanks for the followup; forgot about adding one explicitly to the issue. Nagging^H^H^H^H^H^H^Hpinging @jhedstrom for a review. He is most familiar with this w/o actually touching it. Attaching an interdiff against the last RTBC patch to make review easier.
Comment #290
effulgentsia CreditAttribution: effulgentsia at Acquia commentedVery nice test coverage in this class. I opened #2786583: Create a base class for DateTimeFieldTest and DateRangeFieldTest and move common code into it as a follow-up issue, and here's trivial docs/whitespace fixes. Since this interdiff is trivial, #289 is still the interdiff that needs real review.
Comment #291
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI ran a
coder
check on the patch and it found a couple coding standards violations. Here's a fix for them.Comment #292
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIs this comment correct? Looks to me from both reading the code and clicking around in the UI that both the start date and the end date are allowed to have a default.
Other than that, I'd feel great committing this once #289 is reviewed. Or if another committer beats me to it, that's fine too.
Comment #294
mpdonadioWhat was in my brain, and what I ended up typing were not the same thing. There is coverage for this in DateRangeFieldTest::testDefaultValue(), towards the end of the method.
Comment #295
effulgentsia CreditAttribution: effulgentsia at Acquia commentedA review from him would be awesome if he's available, but I don't think we should block this on him if he's not. We want to tag beta2 soon, so it would be really ideal for this to land today. To assist with either him or someone else reviewing the "253-293" interdiff in #294, here's some info about it:
See #281.2 for the rationale.
These duplicate what is in the parent method, so not needed. Removing the duplication helps maintain a parallel with the config schema definitions.
This is what is most substantial in the interdiff. I'll post a longer comment shortly explaining it.
Other than the above, the interdiff just contains some docs and coding standards cleanup.
Comment #296
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think the best way to review the
datetime.schema.yml
interdiff is to actually review its complete changes in the full patch. The key for me was, per #281.1, matching up the schema type hierarchy with the PHP class hierarchy. That's not a code-enforced requirement, but I think it makes things much less error-prone both in the present and in keeping up with future changes.Here's a section-by-section explanation:
In the patch, class
DateRangeItem
extendsDateTimeItem
and does not implement its owndefaultStorageSettings()
ordefaultFieldSettings()
.In the patch, class
DateRangeFieldItemList
extendsDateTimeFieldItemList
and overrides thedefaultValuesForm()
method. The overridden method invokes the parent one, so I was tempted to make the abovefield.value.daterange
set itstype
tofield.value.datetime
instead of tomapping
. However, the two child properties that would be inherited by doing so,default_date_type
anddefault_date
, both have theirlabel
overridden, and I don't know if config schema lets you extend a child property without explicitly redefining the child property'stype
as well. I'm open to changing the above to be:to match what's happening in
DateRangeFieldItemList::defaultValuesForm()
if whoever reviews this thinks that would be an improvement and knows it (i.e., not specifyingtype
ondefault_date_type
anddefault_date
in order for it to inherit that from thefield.value.datetime
definition) to be an allowed thing that's supported by config schema.This corresponds to the following code for these formatters from the patch:
This corresponds to:
Note that the
type
is set tomapping
, notfield.widget.settings.datetime_datelist
, because the class does not extendDateTimeDatelistWidget
. As a result, the schema definition mostly duplicates the one forfield.widget.settings.datetime_datelist
, just as the defaultSettings() method above mostly duplicates the one inDateTimeDatelistWidget
. There is no technical requirement for config schema type hierarchy to follow PHP class hierarchy, so we could reduce the duplication by setting thetype
tofield.widget.settings.datetime_datelist
, but I suspect that it's better for long-term maintainability to keep the config schema hierarchy parallel to the PHP class one.And this corresponds to class
DateRangeDefaultWidget
extendingDateRangeWidgetBase
without implementing anydefaultSettings()
.I hope this helps whoever reviews this.
Comment #297
YesCT CreditAttribution: YesCT commentedI can also look at this right now.
Comment #298
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIn terms of testing the config schema changes, rather than just relying on human review, here's some information:
Comment #299
jhedstromThese updates look great. I also like that we've made the separator configurable.
I've filed #2787105: Add RTL support for daterange formatters which is a follow-up needed from #255.
I see a few other folks are actively reviewing this now, but I think it is RTBC if they concur :)
Comment #300
Gábor Hojtsy@jhedstrom: uhm, well, @effulgentsia raised some concerns with the config schema, so unless that is responded to at least (if not adressed), I don't see how this is RTBC?
Comment #301
Gábor HojtsyRe the config inspector, you install the module and then
admin/config/development/configuration/inspect
lists all the config files in active storage and also validates them for this screen. So any errors are instantly called out. Then the List inspection mode is a good way to review the concrete errors (if any). I installed the patch with config inspector and set up a sample date range field with default settings. That did not result in any schema errors in the field and storage settings or the display or form modes either. (Not an exhaustive test by any measure, but a spot check at least).Comment #302
jhedstromre #300
I guess I didn't see further suggestions for changing it though? Setting back to NR so more folks can further review the specific interdiff in #294 for those schema changes. I followed through the detailed reasoning and explanations in #296 and the current patch's schema made sense to me following those. If folks using the config inspector do find errors (I haven't had a chance myself), those should probably be addressed here if severe enough (if not, follow up issues seem sufficient).
Comment #303
jhedstromI think the remaining question is #296.2? I don't know the answer to that myself.
#296.4's suggestion of maintaining the class hierarchy makes sense, unless somebody thinks it won't be more maintainable long term.
Comment #304
YesCT CreditAttribution: YesCT commentedI also installed the config inspector, made a date field and a date range field, I dont see any errors, and looking at the config inspector form tab things look good. Still looking more.
@effulgentsia thanks for pointing out #254621 in #298 that not translating all the config is not the fault of this patch.
I (also) dont know the answer to 296.2
Comment #305
YesCT CreditAttribution: YesCT commentedTranslation of date range config, and date range content worked.
It would be nice to add "context" to the start and end strings so that someone translating those interface strings would know they have to deal with dates.
I tested multivalue date range (and compared it to multivalue date) and there didn't seem to be any data problems.
I made content. Here is a screenshot.
Note Birth day is a date field, and Party is a date range field.
Visually, in the content creations, the labels on start and end are a bit concerning, especially that there doesn't seem to be an association with the field name for the "end".
So I'm done testing for now unless someone pings me.
Comment #306
xjmThanks @effulgentsia, @jhedstrom, @mpdonadio, @Gábor Hojtsy, and @YesCT.
I am really glad to hear that the schema is in order and that translation works correctly. That is a relief!
Unfortunately, the additional presentational/usability issues in #305 combined with the fact that we are well into the beta phase (And already have postponed beta2 more than two days) still make me uncomfortable committing this to 8.2.x. I am going to go ahead and move this to 8.3.x. I do think it's really close.
The current structure also compounds the accessibility issue from #1918994: Improve Datetime and Daterange Widget accessibility. In addition to the
h4
for the field label being separate from the semantic label, the "Start" and "End" are separate from the field label. (Fixing #1918994: Improve Datetime and Daterange Widget accessibility will provide us a pattern that can help fix this as well, but is not a hard blocker.)Comment #308
xjm(Adding credit for @miwayha who helped with the accessibility testing here at MWDS.)
Comment #309
xjm(Also some reviewer credit; not complete though.)
Comment #310
mpdonadioOK, this needs works for the field widget problem (ie, the way DateRangeDefaultWidget renders out).
However, I am postponing this on #1918994: Improve Datetime and Daterange Widget accessibility. Since we refactored DateRangeDefaultWidget -> DateRangeWidgetBase -> DateTimeWidgetBase, the two issues are now tightly coupled with the way ::formElement() gets built up, so I see it as a hard blocker. We need to see how that shakes out to see what the impact will be on this widget (which may require us to rethink how we refactored DateRangeDefaultWidget and DateRangeWidgetBase).
Comment #311
swentel CreditAttribution: swentel commentedReally ... #1918994: Improve Datetime and Daterange Widget accessibility has been open for 3 years ... I don't think it's fair to use that issue to postpone committing this patch on that, given all the work that has been done here. To be clear, I'm not saying that I don't care about presentational/usability, but these exist in the D7 version of date as well, and it's working out fine for many many people.
I strongly suggest won't fixing this and moving it to https://www.drupal.org/project/datetime_extras and come back in a year (or maybe never) while the community can move on laying foundations for good date support in various ways: range, timezone, multi day event views, calendar (really, calendars, multi day events are a pain without this field type (or a self maintained variant)). Committing this to 8.3.x, even early would basically just kill any momentum that people might have gained by getting things forward for better date/time/calendar support in D8 core. The problem space is just to hard and I feel this is a perfect example of something that should and could get a new and better momentum by keeping it it in contrib.
Comment #312
rdworianyn CreditAttribution: rdworianyn commentedI agree with swentel, this should not be blocking, especially since it has been working like this in 7. This is a huge setback for many people if it gets pushed to 8.3...
Comment #313
mpdonadioWe can unpostpone this particular issue and work independently of #1918994: Improve Datetime and Daterange Widget accessibility, but it would still have to goto Needs Work, b/c #306, which also removed beta target and moved it to 8.3.x.
Comment #314
mpdonadioComment #315
gambryThat's a shame.
I've been waiting for this feature for months. Planning projects releases according with the targets in here... and now for a label issue, fixable in a follow up in probably a couple of days and as a bug being committable at any point, I see the great work done so far been postponed to the 5th of April 2017 AKA one year.
I understand the decision but personaly I agree with #311.
Comment #316
heddnI've been doing bad things for my D8 sites since December of 2015; like telling them they cannot have events that have an end date. Then justifying it (to myself) by knowing this was coming. But now that this is punted out by another 9 months, I can only hope that datetime_extras get's this functionality sooner, rather than later.
Any chance on revisiting the decision and still including this in 8.2? Pretty please? My conscience will be feel much better. I don't think I'm the only one with a vested interest. With 125+ followers, there's a few others that want to see this brought into core, or somewhere.
Comment #317
b0b CreditAttribution: b0b commentedIt's kind of ironic that the date and calendar functionality was what brought me to Drupal way back in version 4.6 and then took my career down the Drupal path. Now it's the only module keeping me from moving that very same site to D8. This is very important functionality.
Comment #318
dalinI plan on just applying this as a patch for future D8 builds. The remaining work seems confined enough that running it as a patch isn't likely to be a problem.
Or are other people seeing something that I'm missing???
Comment #319
mpdonadioOk, let me preface this by saying that front-end work in Drupal 8 is not my strongest skill. Nonetheless, I am pushing forward on this whether it is for 8.2.x or 8.3.x.
The biggest problem is datetime_wrapper appears to be used for each of the input sets, and then again for the pair (turn on twig debug and take a look, it gets output three times). So, three h4 elements get rendered, and two of them happen to be near each other because they are inlined. I think the fix may just be to introduce a daterange_wrapper and make sure it renders well. The odd thing about this solution is that datetime_wrapper is part of core, and not the module because it is part of the datetime render element and not the datetime field. Maybe just a fieldset?
The problem that I see is that a datetime field actually gets wrapped twice, once by the widget and again by the datetime render element. It isn't evident, though, because the inner one doesn't have a title, so there is only one h4 element. This bug is carried over to the daterange widget b/c the common code.
A solution for #1918994: Improve Datetime and Daterange Widget accessibility is going to complicate matters here, probably for the worse. Per #306 this is still Needs Work, though.
Attached is a start of a fix based on a fieldset. TBH, though, the proper thing to do is fix it in the DateTime widget first...
Comment #320
xjmThanks @heddn, @gambry, @swentel, @b0b, and @ryantollefson for the feedback.
This issue was given 18 days of special consideration beyond the documented deadlines for the minor release, as well as an extensive time investment from multiple core committers. This was precisely because of the value of this feature and how long contrib authors and site owners have been anticipating it. Only two other issues ultimately received the same consideration, and both of those issues introduce optional experimental features (as compared to this, which is being added to an essential stable core module already installed on most sites).
When I made the case for this to be a beta target issue (when @mpdonadio reached out to me shortly before the announced deadline three weeks ago), other core committers rightfully pushed back and said the feature could live in contrib until 8.3.0. I advocated to the other committers to avoid that because of the need for an upgrade path, but it is still an option.
If you need this functionality, help the datetime maintainers resolve the outstanding issues here, and consider a contrib module that will provide an upgrade path to 8.3.x core. The best way to advocate for a feature is to test it, document the results of the testing, and assist the patch authors with resolving issues large and small that are uncovered.
I do believe this important and basic functionality does belong in Drupal core, once it is ready.
Comment #322
mpdonadioWas pretty much expecting that run to fail, but it also shows that we are missing similar coverage in the DateRangeFieldTest::testDatetimeRangeField() method (and also DateTimeFieldTest::testDatetimeRangeField)
Would appreciate some front-end expertise to figure out how we can wrap up this for 8.3.x ASAP, potentially taking into consideration that we may need a side issue to resolve the double datetime_wrapper bug in the datetime widget(s) first.
Also, if someone wants to add one of these patches to datetime_extras asa sub-module and do the update path / migration to the core field, I can grant maintainer access.
@xjm, would you entertain committing the patch in #294 to 8.3.x, front-end warts and all, so we can continue the bugfixes in followups, and also move forward on rerolling the in-progress patches to take into account the new field (like the ones with the timezone support and fixes) and move forward on the views plugins?
Comment #323
xjmAnother option to consider at this point (in addition to those suggested by swentel and mine above) is whether putting this field in a separate Core 8.3.x experimental module would be beneficial. swentel pointed out the risks to innovation for this functionality being added to core before the hard problems are fully solved. If we add it as an experimental module, however, we can commit it to 8.3.x even with outstanding accessibility bugs, as well as make improvements to it iteratively not only before but also after the April 5 release of 8.3.0. It would require some contained changes to the current patch.
Experimental modules can receive significant changes even in patch releases, providing something closer to the contrib rate of innovation but with the added assurance of a much wider testing audience, core review and QA, and core test coverage to reduce the risk of regressions due to other core changes.
Comment #324
xjmI crossposted with #322. @mpdonadio, this would be a completely viable 8.3.x experimental module as-is. Then the followup issues become part of the experimental module roadmap, which is sort of a contract between maintainers and the community to make the module stable within two minor releases. That also helps with some of the concerns about release predictability raised above. It would take some changes to the patch (to move the code to a separate module) as well as a bit of organizational work to make the plan issue for an experimental module's roadmap. What do you think?
Comment #325
gambryBetween the two options - having this work (+ bugfixes and improvements) in core 8.3.x as-it-is OR adding it to a separated experimental in core or contrib module - I would choose the first one only because that means I can potentially start using the patch from tomorrow untill 8.3 release.
As separated module we'll need to re-re-factor @mpdonadio merge work, which means more work now and resulting in two separated points of failure for datetime related bugs.
Said that I'm happy with whatever the community decides.
Thanks @xjm for the time spent on explaing the whole decision process.
Comment #326
DuneBLI am completely lost...
The only question (on my side) is: Can I use this component in a production environment (I agree to patch it on a regular basis) or should I avoid this component and start using using 2 regular date field to mimic the start/end date behavior?
Is there anybody with enough knowledge to answer this simple question... I am sorry if the answer is obvious, but I can not answer it myself after reading all the posts...
Comment #327
jhedstromI've been thinking this over since yesterday evening when it was announced this would not make 8.2. I'd like to make the case that it should still be eligible.
As others have noted, this is imperative because 8.3 is 9 months out, 1.5 years after 8.0.0 was released. When the date module was put into core, that sent out a signal to the world of Drupal users that core would support the functionality that had previously resided in contrib. Indeed, any 8.x efforts on the Date module halted around that time (October 20012 to be exact).
Here's a brief history of the patch since it was first marked RTBC:
My proposal for inclusion in 8.2:
Other reasoning:
I'm leaving this at needs review, but if others think this is a way forward, please mark RTBC. I am bumping it back to 8.2 for now.
Comment #328
colanAgreed. Other options would create more work needlessly, and yes, it's a new field that won't interfere with anything.
Comment #329
yoroy CreditAttribution: yoroy at Roy Scholten commentedWithout this tag issues won't get on the ux team radar :-(
Comment #330
webchick@xjm and I discussed this at length at MWDS2016. I can see the difficulty with the fact that this issue sat at RTBC for a couple of weeks during beta1 without review (during which time there were people ready and willing to work on it), and when those RTBC reviews were provided a couple of days before beta2, they found problems for which there was then not sufficient time to fix (despite an additional ~72 hours being granted on the beta2 deadline).
I think we're in a tough spot because if this goes in as-is, it's a patch against a stable core feature, and that means we need to resolve the accessibility and cosmetic issues ahead of time. If we refactor it into an experimental module, this gives us the breathing room we need to make those issues follow-ups, because they can be further improved during patch releases. But beta2 has already shipped at this point. Additionally, it seems the UX team hasn't been looped in here at all yet, per #329.
I think at this juncture, the most logical next step is to use #294 to create an experimental module in 8.3.x (not #253, which doesn't have the required refactoring). Then it's in core, at least. Part of this is a roadmap that includes the issues unaddressed by this patch, such as the accessibility and any usability issues. That way, those things can be handled as follow-ups as @jhedstrom suggests.
And then, we can discuss what's best solution for 8.2 site builders that doesn't create a horrific upgrade path situation.
Comment #331
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedThis is really sad news. People who have been following this issue know how hard folks have been working for getting this feature in 8.2.
In my opinion, if this cant get into 8.2, i would be comfortable applying this as a patch for 8.1 as well as another patch for 8.2 if need be , instead of burdening the maintainers to create a contrib module or follow another route. That would be too much maintenance work for the folks.
regards,
Sukanya
Comment #332
swentel CreditAttribution: swentel commentedSo yes, this is kind of sad. I anticipated a bit that it would be pushed back, so I took a beer and chips, and went crazy on the code .. and tadaaaa .. a patch with an experimental module.
If you look at the code, it's really really simple. I had to add a trait for the buildDate() function that was introduced, which was the only part that was a bit hard to move out. I also added a hook_help, which is almost 90% the same of the datetime module.
Now. This is a really honest opinion: I think (I didn't want to say "I am certain" because I don't have scientific proof) it would be a bad idea to commit this on 8.3.x only. As an experimental in 8.2.x this would make a ton of people happy, and immediately allow contrib (especially calendar) to regain momentum to get multidays events working.
I know it's a extremely long shot, but one can always try, but imagine a release for 8.2.0 including end dates ? :)
Comment #334
mpdonadioIf an experimental module means this getting into 8.2.x, then I am for it. It would means that we have two datetime modules in the long run, but I can live with that. However, we already have a contrib module called "datetime_extras" for stuff we want for Drupal 8 but don't necessarily want in core. So, there is a potential namespace problem there. It may also complicate some of the planned views and timezone work, as well as some in progress patches (haven't thought about the last part fully).
However, if 8.2.x doesn't work out, then I don't see the point of adding this to 8.3.x as an experimental module (essentially at the beginning of the 8.3.x cycle). In that case, I am strongly suggesting that we commit #294 to 8.3.x, so we can free up that planned work, reroll some patches that fix some other bugs, and move on. One, a framework manager was already OK with that patch having followups, and two, the root cause of the pushback in #306 is due an existing bug (or bugs) in DateTime DefaultWidget and/or the Datetime element.
Comment #335
gambryI totally agree with #334, and the sooner this go to 8.3.x the closer the codebase will be with a potential 8.2.0 + patch #294, as 8.3.x branch has been open recently and shouldn't be *a lot* different than current 8.2.x.
Then the way I see it is: site builders can use the 8.2 stable release + adding #294 patch, we promise schema and config won't change but only UI and small optimisations and when 8.3 is out there won't be any pain for them, but worse scenario just an upgrade task.
Comment #336
swentel CreditAttribution: swentel commentedGood point about the namespace, renamed to datetime_range. hook_help failure should be ok now too.
Comment #338
swentel CreditAttribution: swentel commentedUgh
Comment #339
swentel CreditAttribution: swentel commentedSummary of changes compared to the patch - was a bit hard to create an interdiff which would be very noisy. Important also, nothing changes any more in the original datetime.module, in general, it simply moved the all new files there to this new module. I did attach an interdiff after my local rename from datetime_extras to datetime_range. The only real new piece in the patch then is the DateTimeRangeTrait class. Hope that helps.
- Added experimental module called 'datetime_range': provides a field type, widget and formatters for storing start and end dates
- Added hook_help: 90% equal to datetime
- Added entry in core/composer.json
- Added entry in Maintainers.txt
- Added DateTimeRangeTrait which contains two methods for building a render array for dates. Originally there was only one method called 'buildDate' in the original patch which was implemented by the formatters. This cleaned up some code and made them a bit easier to read. To get this patch forward fast I opted for a trait because otherwise I had to duplicate the method in all formatters.
Comment #340
borisson_Mostly nitpicks, overall this is look great.
I love that these methods are friendly :)
We can just return a new array here instead?
/s/iso/ISO/
I don't see the need to add this as a seperate line, we can do this while creating the array.
/s/with fully configurable a date/with a fully configurable date/
This hardcodes the spaces surrounding the separator, should we move those into the seperator variable instead?
/s/tht/that/, this also means that
types
will end up on the next line.I think this can lead to a negative value for $start, the min value should be 237 instead of 0?
This adds a new Simpletest, since we're phasing those out, should we change this test to BTB?
Needs reformatting to be within 80 cols.
I think this needs an
@var array
?Comment #341
mpdonadioQuick comments on #340.
#6 was functionality that has test coverage from the RTBC patch. I am very hesitant to introduce changes like this at this point. I also wonder about the UX of having to enter the spaces. If someone doesn't need the space, then they can do their own alter.
#8 REQUEST_TIME is current north of 1.4 billion, so I don't think there is any danger here. It is also just fake, sample data.
#9 See #2780063: Convert web tests to browser tests for datetime and datetime_range modules. BTB is missing some key methods right now for a quick conversion (but, I want to do this as soon as we can).
#10 We frequently let this slip. Search for `public static $modules` in core and see all of the examples, even with new tests.
Comment #342
borisson_#341
.6: Yeah - I wasn't sure about that either. The alter is fine.
.8: Oh, yeah, I didn't consider that. Sorry.
.9: Sure.
.10: There's a phpcs rule for that and I'm assuming that will get in at one point. This will be changed for those tests as well in the future. Let's not knowingly introduce things that break the code style rules, even if core doesn't do it right in other places.
Comment #343
YesCT CreditAttribution: YesCT commented@borisson_ Thanks for taking the time to look over this code. Questions are great feedback too.
I can make a new patch and interdiff to address some of those points, now.
Please keep in mind that the requirements for an experimental module are different than for going into core directly, and coding standards (that dont have automatic checks) are not required. https://www.drupal.org/core/experimental#requirements
Personally, I often notice things like that too, and will mention them, but making the changes is nice to do at the same time for nits.
Comment #344
alexpottPutting this back to 8.3.x-dev because both @xjm and @webchick decided in #306 and #330 to do it and as the release manager and product manager for Drupal 8 it's their call. It's also their call as to whether going the experimental module route makes this eligible for 8.2.x-dev again since adding will necessitate another beta release and more work. I know this issue is on their radar, they've spent part of their weekend looking at the issue, so I'm sure that they will be make a decision if the experimental module can be part of 8.2.0 soon.
Comment #345
xjmAgreed, thanks @alexpott. We can discuss what to do for 8.2.x once this is committed to 8.3.x, as @webchick said.
Many thanks to @swentel for getting the experimental module direction moving. I think this is a direction that will help not only for this feature but also for other core datetime work (to @jhedstrom's points about the outstanding overall roadmap for datetime).
I can help with the plan issue for the experimental module tomorrow. We can include parts of @borrison_'s review as issues on it, as well as @YesCT's feedback about string context, the accessibility issues, etc.
Both @effulgentsia and I think the code overall is in great shape.
Comment #346
YesCT CreditAttribution: YesCT commented344a addresses #340 3. 5. 7. 11.
This leaves questions in 2. and 4. to be answered. I might try and do that later.
I found some other nits as I was reading.
a. module name
could not find the standard for module naming (looked in 1354 and https://www.drupal.org/coding-standards#naming)
but grepping in core
ag --file-search-regex "info.yml" "name: " | grep -v test | grep "name: [A-Za-z]\+ "
All the multiword (non test) modules upper case each word in the name.
Changes only in info file, and .module
b. in the move from datetime extras to datetime range, the schema file name (and first line comment) didn't get renamed.
c. And other nits, see the interdiff a-b.
I tried not to do any changes that might break tests.
I'm gonna try some other changes to do with type hinting, next. and maybe take a look at 2. 4.
Comment #347
mpdonadioBefore we do any real heavy lifting on this, can we pause for a moment?
The current plan is to add this as an experimental to 8.3.x and then make a decision about whether we can get it into 8.2.x? And it may not make it into 8.2.x anyway? And this is because of a bug that is in 8.2.x that just happens to be more evident in this patch, and we are currently spinning in circles on that issue? That issue also skirts the true problem, which I am going to post an issue about tonight.
If that is the plan, then I am not for it.
I would like one of two things to happen.
- The experimental gets into 8.2.x first
- or we commit 294 to 8.3.x, start fixing things as followups, and move on with everything in one module. People can use the patch against 8.2.x if they want. I don't see the data model changing at this point.
I thought the idea of an experimental module was so that we could get this into 8.2.x, so people could use it while we iron out the widget problem. I don't see what an experimental in 8.3.x buys us in the long run, especially since we are at the beginning of that development cycle. Long term, we either end up with two date-related modules, or having to do a nasty update path to get everything back into one module.
Comment #348
YesCT CreditAttribution: YesCT commented@borisson_ I looked at #340 2. and 4.
And it looks like there are a bunch of examples in core where $build is made, and then altered with context in a separate statement after... maybe cause it takes less lines of code? But there are also examples where it is set all at once.
I did make it all in one assignment statement, (and got help in irc), and checked first that the array structure is exactly the same by doing a var_dump on both ways.
I also grep'd in core to see about making a $build and then (with no code in between) returning it. And this seems to be *very* common, and I think makes it easier to read.
So not returning just an array.
This is the end of changes from me for now, and addresses all of #340,
Next I'm gonna make a plan issue and work on documenting the experimental requirements as remaining tasks.
---
note 1 (just to document the info, no change), tim mentioned that usually methods like
are static ... like
---
note 2 (in case it is useful for future reviewers)
in the trait, phpstorm says $this->getFieldSetting() setTimeZone() formatDate() method not found
This is because DrupalDateTime extends DateTimePlus and DateTimePlus passes method not known to it with function __call()
Comment #349
YesCT CreditAttribution: YesCT commentedCreated #2788253: Plan for DateTime Range experimental module
[edit: doh, copy and pasted the id of this issue. fixed]
Comment #350
dkre CreditAttribution: dkre commentedI really need to concur with mpdonadio #347. We really need a final decision made on this.
While all of the decisions have made complete sense and there has been an incredible the amount of work put in this is a really painful issue for any site builder requiring this functionality.
I don't understand how D8 shipped with 'date in core' without clear documentation, or a huge disclaimer stating the limited implementation.
Comment #351
ndf CreditAttribution: ndf commented@YesCT: great spirit, you share an opportunity to validate the experimental module!
@mpdonadio: +1 #347
Is an upgrade path impossible at this point? Is it only the widget issue?
@alexpott: +1 #344
The current patch is rejected for 8.x-2.x
Is should be in in 8.x-3.x
One alternative is an experimental module in 8.x-2.x
Or in 8.x-3.x
Comment #352
gambry+1 for every single word in #347.
Please let's have a decision first and coding later if needed.
Comment #353
xjmThanks everyone for your ongoing interest in this issue.
For everyone asking for a decision, the decision was already made. To reiterate: This will not be considered for backport to 8.2.x unless it is an experimental module, and it will not be considered for backport until it is committed to 8.3.x. Three separate core committers have commented to this effect.
It's not helpful to continue commenting about how the issue is painful for site builders or how it's a thorny contrib blocker. We are very aware. The past 100 comments on this issue exist because we are very aware. Please limit your comments on this issue to reviewing and testing the current patch (aside from the subsystem maintainers who do need to give feedback about what works for them).
I've already signed off on the current patch being committable as an experimental module once it is reviewed, etc. (no more heavy lifting needed; swentel already did that). It just needs to be reviewed and the datetime maintainers needs to sign off on the approach. Then it's ready to go and people can plan for it.
It is not currently committable as a non-experimental module and it will take an unknown amount of time to get it to get it to a point that it is. That's bad for contrib, bad for core, bad for site builders.
Comment #354
xjmAh. I think this got missed somewhere which might have led to the confusion: At MWDS @webchick and I discussed whether it was to have both datetime and datetime_range as separate modules for the duration of 8.x, and we agreed that it was unless the datetime maintainers disagree. So @swentel's refactor would be the basis for a stable core module, and no additional upgrade path or anything would be required to support contrib or sites. @mpdonadio, does that make sense?
I commented to this effect on #2788253: Plan for DateTime Range experimental module.
Comment #355
mpdonadioIf getting this into core the fastest means starting out in 8.3.x as an experimental, and there is a glimmer of hope of getting it into 8.2.x this way, then let's do it. If we run into problems down the road, we'll deal with them then.
Do you agree @jhedstrom ?
Going to spend an hour with this patch tonight to see if there are any problems. In the meantime, someone else needs to review this. A lot of hands have been in this, so the number of people who can RTBC it are dwindling. I think we may want a
so at least there is basic Views functionality for the field with the standard plugins from the get go (tbh, though, I forget if these will work without the empty hook, hence some manual testing).
Comment #356
mpdonadioOk, verified we don't need a hook_field_views_data(); basic Views functionality will be there with the default plugins.
The problem I see with the patch in #348 is that a fair amount of the refactoring from #293 is gone.
#293 touched some of the DateTime classes to make the refactoring possible (eg, the buildDate method, createDefaultValue, and some more), but didn't touch any existing tests. So, we have duplicate code again between DateTime and DateRange, which was one of the reasons this got push back initially (the new trait partially addresses this w/in DateRange itself). Attached are the missing hunks, as far as I can tell.
Not sure what we want or can do about this with an experimental module. Can we touch the DateTime classes, or does that need to be another followup (or a precursor to this getting in)? Or since part of the experimental plan is "verify removing the module without can be done without lasting disruption" can we not do this?
@swentel, which patch did you start with?
@xjm / @webchick / @alexpott, what is possible or not possible with the changes to the DateTime classes if this is going in as an experimental?
Comment #357
xjmI discussed this a bit with @mpdonadio in IRC and also posted on #2788253: Plan for DateTime Range experimental module. I think the refactoring in #356 can go into stable datetime for 8.3.x after this lands; it's part of the roadmap to make the field stable, but enough of the code duplication is resolved still in this issue that it should be fine for the initial experimental module commit with those outstanding.
Comment #358
mpdonadioI read the diff between DateRangeFieldTest in #293 and #348. The changes are trivial. That makes me confident that we have a good patch to commit. Nothing turned up in my manual testing, either.
#348 has my blessing.
Comment #359
swentel CreditAttribution: swentel commented@mpdonadio I started from #294 - it was basically applying the patch, moving all the new files to the experimental module and then resetting the changes in datetime module itself. The trait is the only real new thing (and the info and module file too of course)
Thanks @all for considering and getting this forward :)
Comment #360
effulgentsia CreditAttribution: effulgentsia at Acquia commented#348 looks great to me as well. A couple small points that could potentially be follow-ups, but maybe are straightforward enough to just reroll for?
This trait is specific to formatters, so perhaps rename to
DateTimeRangeFormatterTrait
? And maybe also move into theDrupal\datetime_range\Plugin\Field\FieldFormatter
namespace?That last line should be
buildDateWithIsoAttribute
, notbuildDate
. Which points to a couple things. 1) We probably need a test added for this. 2) What's the point of a buildDateWithIsoAttribute() method on the trait? Why not just haveDateRangeDefaultFormatter
override the trait'sbuildDate()
method?defaultSettings()
,settingsForm()
, andsettingsSummary()
to the trait, since those are identical between the 3 formatters? EvenviewElements()
could almost be moved there, so long asDateRangeDefaultFormatter
could extend the trait's method and additionally handle the$item->_attributes
logic.Comment #361
swentel CreditAttribution: swentel commented1. Makes sense
2. Hmm, I copy pasted that from the original patch. I guess @mpdonadio knows more about the different between those two. There are tests that effectively test that Iso attribute, but I have to be honest, I'm a not 100% sure what goes on here :)
3. Makes sense too.
I'm starting to wonder whether we shouldn't create a baseFormatter here with all those methods in, then we can even drop the trait ?
Assigning to myself, should be able to roll quickly.
Comment #362
mpdonadio[I think I lost a post]
#360-1,3: When talking to @xjm last night, we decided to make a followup to redo some of the refactoring that fell out of this. Part of this was some rework to the DateTime classes to support better reuse in the DateRange classes. I would be hesitant to improve the DateRange classes/traits until we hash this out, as the end goal is to get closer to what #293 looks like, which would undo the suggestions above.
#360-2, yup. Looked at patch and we are missing formatter coverage when start==end. Per ^^, think buildDate() is a better name there.
#361-2, The default formatter outputs a HTML5 time element, which has an attribute with an ISO date; for DateRange is is
<time> - <time>
for the pair of them. The other two formatters just output text.Comment #363
swentel CreditAttribution: swentel commentedOk, here's a patch with everything besides viewElements moved out. Ran out of time, so couldn't do evertyhing.
Comment #364
xjmFor me #360.1 and .3 as well as #361 would also be worth exploring as followup issues if needed.
Comment #365
gambryI've done some manual tests and patch looks good to me.
I just want to flag this field is affected by #2778083: Default value for Date-Only fields is broken when UTC date is different than user current date too.
Not sure if we want this to fix it with the DateTime follow-ups mentioned at #362, with a proper DateRange follow-up or now before this patched is committed.
Comment #366
mpdonadio#365, a lot of the inprogress patches are going to affect this. Half tempted to postpone all of our in-progress work for a few days until this is in.
Right now, I am going to take #348 and fix #360-2 before I begin start paying work. Then I suggest
- commit new patch 8.3.x
- address the refactoring issues to get this closer to (which would possible make #360-1,3 and #361 not needed), and get this closer to #294
- touchup our bugfixes to account for this (though, if we do the refactoring properly and we are closer to #294, this may just be test coverage)
- move on with other work
On a side track, the committers can decide the fate of new patch for 8.2.x.
Once I post new patch, I still want @jhedstrom's blessing on everything.
Comment #367
mpdonadio#360-2 fixed based on #348; passed locally. Gotta clock in, but will be on IRC today.
Comment #368
jhedstromThank you so much to everybody that has been working on this!
This looks good. I've reviewed the patch in #367 quite extensively and have not found anything outstanding. A manual test was also successful.
I'm still not entirely happy with the separate module approach, but @xjm has addressed most of my concerns in #2788253-25: Plan for DateTime Range experimental module.
I hope to see this in 8.2.0, and also hope to see the same level of enthusiasm in the follow-up issues as we move to make this an official module that is enabled by default in the standard profile.
Leaving at NR for now, but I consider this RTBC and will mark it as such this evening (somebody else, feel free to do so as well) if there are no further issues with the current code that isn't already addressed by follow-ups.
Comment #369
effulgentsia CreditAttribution: effulgentsia at Acquia commentedLooks great to me too, so RTBC.
This issue still is tagged "Needs release manager review", so assigning to @xjm.
Comment #370
xjmI'm in the process of doing a final review on the patch. Some collected notes in case d.o melts:
This should probably just reference the main datetime help and handbook page (rather than an nonexistent
datetime_range
handbook page). That's docs gate, so I've added it to the summary of #2788253: Plan for DateTime Range experimental module.I had the same thought when reading that docblock. ;) There are a few docblocks that could be better, but since this is an experimental module, I'm going to take advantage of the loosened core gates and not post a bunch of minor docs feedback.
&emdash;
or whatever some typography stickler would prefer over a hyphen.) This might also impact translatability. Adding that to the "should" as well. On the upshot, though, at least it's not vulnerable to XSS:(Thanks, Twig!)
Comment #371
xjmI would've thought the separator should be translatable, but I could not figure out how to translate it if so. It's possible I missed something since the config translation UI is a touch arcane, but added it to the plan issue in case I didn't.
Comment #372
xjm(Just reuploading the file attachment since it went somewhere strange.)
Comment #373
xjmAnother thing I've noticed reading through the updated patch is that there is still some code duplication we could refactor away besides what ended up in the formatter trait. Many methods on the various plugins override the parent method to more or less repeat what it did, but on both the start and end dates. We could refactor away a lot of this by creating protected helper methods for things that are used internally in the parent and then reused in the overridden child method. However, some of this is probably also affected by the ongoing work to refactor and improve timezone handling, so I'll leave it to the subsystem maintainers to decide how to best fit it on their roadmap. That also would have been potentially commit-blocking for me with non-experimental code, so I'll add it as a note on the plan summary as well.
Comment #374
xjmComment #375
xjmSaving additional reviewer credit (which BTW is really annoying when the issue is two pages). :P
Comment #377
xjmAlright. I have:
I did not know that was a place!
@effulgentsia has already signed off on this as well. Based on that and my own review, I'm comfortable committing this as an experimental module.
@catch, @effulgentsia, @alexpott, @Cottser, and I also discussed today whether everyone was comfortable backporting #367 at least in principle. (Only @effulgentsia and I have actually reviewed the patch itself.) We decided to backport it, for the following reasons:
It's important to remember that this backport is also entirely a one-off. None of those reasons mean that we will break our release cycle schedule or policy in the future for a different patch. I should also note for posterity that the following things are not factors in deciding to backport the issue:
If you are following this issue and completely lost as to what gets into core when and why, you can read the Release cycle overview and Allowed changes during the Drupal 8 release cycle. Policies are never perfect and never cover every contingency, but those are the result of lots of thought and contribution across the community. The committers are also discussing some small improvements to clarify expectations for the next beta when it comes next year.
TLDR, disclaimers etc., blah blah blah. With the backport, barring some critical issue we haven't discovered, this feature will be available in 8.2.0, its release candidate in a couple weeks, and a beta that I will probably release tomorrow.
The work that has gone into this feature is awesome, and I've found the maintainers' responsiveness very helpful in navigating a difficult series of decisions. Thank you to everyone who contributed thoughtfully to this feature. I look forward to us completing the work outlined in #2788253: Plan for DateTime Range experimental module so that we can eventually enable this feature in standard.
Comment #379
xjmComment #380
heddnThank you everyone for all your effort here. I know a lot of pain, blood, sweet and tears went into this. Too many folks to thank and I'm sure I'd miss someone, so thank you Drupal community. This is why we are the best open source community on the planet.
Comment #381
ryantollefson CreditAttribution: ryantollefson commentedThanks for getting this in dev as experimental!
Where should we track issues at this point?
Comment #382
Gábor Hojtsy@ryantollefson: Let's use the module's own component.
Comment #383
xjmThis module was supposed to be treated as part of the datetime component. Not sure who created the
datetime_range
component.Comment #384
xjmMerging the issue back to the datetime component after checking with the subsystem maintainers in #2788253: Plan for DateTime Range experimental module. So please file issues with this module (or any datetime functionality) against the datetime component. Thanks!
Comment #385
xjmComment #387
Tsjippy CreditAttribution: Tsjippy commentedSo to make it clear:
I have installed drupal 8.2.4.
Which patch do I need to apply to make an enddate available in the date field?
Comment #388
dpi@Tsjippy no patch, install "Datetime Range" module. Its a new field.
Comment #389
Tsjippy CreditAttribution: Tsjippy commentedThanks, totally overlooked this!
It works
Comment #390
Tsjippy CreditAttribution: Tsjippy commentedHmm, I cannot get it to work in a view, for instance the from the Calendar module nor the Fullcalendar module.
It also lacks the recurring option.
ANy ideas how to use it?
Comment #391
MrPeanut CreditAttribution: MrPeanut commented@Tsjippy — Those are out of the scope of this issue. Here are some issues for recurring/repeating events:
#2305073: D8: Date repeat feature requests
#2775237: Repeating dates
#2775249: Provide a field for repeating / recuring dates
Or maybe check out Date Recur Field (repeating dates).
Comment #392
dankegel CreditAttribution: dankegel commentedIn case anyone didn't notice the many references to https://www.drupal.org/node/2788253 above, it's a happening place at the moment :-)