Summary
Dates are an important data type for a lot of different websites. We might not need all the complexity that the date module can currently handle (repeating dates, etc), but we should at least be able to handle a simple "Date of birth" field out of the box. Dries expressed his interest in having date fields in Drupal 7 (http://buytaert.net/drupal-7-fields-in-core-status-update-and-next-steps).
The patch adds a new form element type called 'datetime'. This is an element that splits the field into a 'date' and a 'time'. The element handles its own validation and manages the explosion of a date into date and time parts, and the transformation of the user input back into a date, so adding it to a form is pretty simple. Each of those parts is constructed to be HTML5-compatible.
Since the support for HTML5 date and time is horrible (missing on most desktop browsers), the jQuery datepicker is provided as a fallback for the 'date' field by testing to see if the HTML5 date is supported and providing the datepicker if it is not. I've tried it out on both a browser that supports HTML5 date fields (Chrome) and a browser that doesn't (Firefox), and it seems to be working right, but I'm open for ideas on ways to do it better.
The original patch expected a date/time string in the ISO form as a default value for the form, but it has been altered so that you now pass a DrupalDateTime object as the default value instead of a date string. The advantage of this is that the previous form processing had multiple steps where the string had to be converted to a date object and then converted back into a string (the value callback, the #process function, the validation function, etc.) Passing in a date object as the default value makes it possible to keep passing that object around without re-creating it over and over. At the end of its processing it passes back the final date object, which the caller can then modify and/or format as necessary, without creating yet another date object from the result.
Since this is a regular form element type, it can be used by any form, not just as a Field module field. So I've switched the node and comment creation dates to use it. This makes the node and comment creation dates HTML5-compatible, and provides the jQuery datepicker as a fallback for browsers that don't support the HTML5 widgets.
Then I added a new field, a 'Datetime' field. It's as simple as I can make it and still provide something useful. It has just a couple settings -- an option to use either 'Date and time' or 'Date only', an option to set the default value to either the current date or empty, and a formatter setting to choose the format for the displayed date. It creates a single value (no 'Start' and 'End' date) that is stored in the database as an ISO string, and it has one widget that uses the datetime form element to create HTML5 date and time fields on the form. Since it wraps around the datetime element mentioned above, we avoid several more steps that might require transforming strings into date objects and then back into strings by creating a date object from the value stored in the database that is passed to the form element. The datetime field then transforms the date object that is returned into the right value to store in the database before saving it. This means that the code for the date field is actually pretty simple, most of the heavy lifting is handled by the form element in a way that works both in date fields and for the node and comment creation dates.
Comment | File | Size | Author |
---|---|---|---|
#166 | drupal-501428-changelog-166.patch | 439 bytes | dawehner |
#150 | Date_With_Source.png | 156.38 KB | mgifford |
#142 | Screen Shot 2013-02-09 at 17.21.11.png | 6.14 KB | Wim Leers |
#142 | Screen Shot 2013-02-09 at 17.21.19.png | 14.78 KB | Wim Leers |
#142 | Screen Shot 2013-02-09 at 17.21.33.png | 14.62 KB | Wim Leers |
Comments
Comment #1
bjaspan CreditAttribution: bjaspan commentedIMHO, core does not and should not contain everything a site might need, even "all of the basics." We have contrib for a reason.
I vote against a date field type. The full date field type is incredibly complex and needs regular tweaking, exactly what we do not want for core, and I think that separate date field modules in core and contrib would just be confusing. Furthermore, who are you going to find to develop and maintain a sub-standard date-for-core module?
However, I know that Dries and I disagree on the fundamental concept of what should be in core and why.
Comment #2
rickvug CreditAttribution: rickvug commentedI agree that the "move X to core" train has to stop at some point. However, I would disagree that field types such as Date, URL, Filefield or Imagefield do not belong in core. These field types and would be used on the majority of sites and are flexible building blocks. Shipping with core has a number of advantages:
- We can leverage the fields in core install profiles.
- Better initial experience for end users. They would show off the Fields UI (if that gets in).
- These fields are needed for a proper upgrade path for profile module.
- Parts of the field modules could be abstracted, placed into inc files and re-used. For example, the Date API or url parsing functions.
Note: cross posting at #501434: Move Link/URL field type into core since the conversation is the same thus far
Comment #3
yched CreditAttribution: yched commentedI think we can now say it won't happen in D7.
Comment #4
mpg CreditAttribution: mpg commentedI just installed D7 and i am shock to see that there is not a date field option when creating a form!!!! This was avaliable in 5........am i missing something?
How do I add a date field to a D7 form?
thanks
Comment #5
yched CreditAttribution: yched commentedhttp://drupal.org/project/date
Comment #6
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedThe module since 2.0 version has reduced lot of complexity, maybe we could investigate into some possibilities for D8 ?
Dates can be coded in so many way that a standardization would be a good starting base for Drupal.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedRetitling. I fully support this.
Comment #8
RobLoachRelated: #51043: Add a "datetime" Form API element... Subsubsub
Comment #9
klonosTitle change (yet again - sorry Damien) in order to be in tandem with the related/similar #501434: Move Link/URL field type into core.
...of course I am for date/time *basic* handling in core. Contrib can be left to handle the more complex features (think re-occurrence etc).
Comment #10
webkenny CreditAttribution: webkenny commentedI would like to help with this if I can. I am defecting from #51043: Add a "datetime" Form API element as well. :)
Comment #11
mioan CreditAttribution: mioan commentedI am testing D7 and am also shocked to find out that there is no date field in core.
I cannot agree to comments that a date field is difficult for core to handle.
I can understand that Drupa's programmers are among the best ones in the CMS development so they are persons with strong knowledge and opinion. There is always a "war" between what a programmer believe is the "correct" and what the customer wants as final functionality.
I think is is a case that the customers are asking for something reasonable.
Mike
Comment #12
sunAdding this to the Profile in core effort.
It looks like some progress on Date module in core has been started in a separate sandbox (which I'm currently unable to find).
However, for the other Profile in core issues, we concluded that it makes more sense to port and prepare the entire module for D8 inclusion in the actual, official contrib module project as a regular 8.x-X.x branch. That has a couple of benefits: Some improvements can be backported, the module can be tested more easily, and the existing project queue can be used for collaboration. Lastly, in case we do not manage to pull the code into core, then there is a fully working D8 branch at the time D8 is released.
The only "barrier" to that approach is granting interested contributors git write access to the contrib repository (and tell them to stay in the 8.x branch). However, that's just a matter of trust and be done very easily. We already started to do this with Link module. I'd highly recommend that approach.
Lastly, also note that there are some architectural discussions happening on g.d.o; e.g., http://groups.drupal.org/node/221254 — I think it would make sense to summarize the existing discussions and move them into separate/focused issues instead (because issues can be marked as resolved when an agreement has been reached).
Comment #13
sunComment #13.0
sunUpdated issue summary.
Comment #14
KarenS CreditAttribution: KarenS commentedThere is working going on at http://groups.drupal.org/date-api. I plan to get back into that effort in a couple of camps that are coming up, the NY Camp and the Midwest Developer Summit.
Comment #15
mgiffordThe most accessible way to add in a date/time widget is to add in a single field that allows users to plunk in their values and then has PHP or jQuery resolve that into a machine readable fashion.
Rather than using a dropdown box where I fill in a day, month, year using select boxes (say we're talking about 10, July, 2012 as separate columns), it's just easier for everyone to just be able to type in "tomorrow". That can be enhanced using javascript to have a date picker widget, but there should be an option to type in date in any way that php's strtotime() or jQuery datejs' parseExact.
Machines can do this easier than people can.
Couple related links:
http://groups.drupal.org/node/19118
#504962: Provide a compound form element with accessible labels
Comment #16
webchickAdding this to the "KIller End-User Features" tag. :) Go, Karen, Go! :D
Comment #17
andypostProbably we should close this as duplicate of #1802278: Add a Date component to core
Also related #51043: Add a "datetime" Form API element
Comment #18
KarenS CreditAttribution: KarenS commentedRe #15, PHP can handle this just fine for strings input in English. There is no way to do that in a multilingual setting that I know of. We would have to un-translate the values that were input and then pass them to the PHP parser, so that's a no-go as the only solution provided by core.
More working going on at #1802278: Add a Date component to core and #1811912: Add pluggable calendar backend to core and centralize date translation where I've posted two critical patches that will move this along. I'm also etting ready to add a summary page to http://drupal.org/community-initiatives/drupal-core with more explanation and I'm going to post a zip download that has all the proposed core patches applied to a recent version of core to make it easier for others to try it out. I hope to get this done by the end of the week.
There are lots of small issues that play into this that are not a part of the critical path of getting a date field into core, like the reworking of the date format system for CMI. My focus is on the critical path ATM.
Comment #19
KarenS CreditAttribution: KarenS commentedOK, I have a patch now. It builds on #1802278: Add a Date component to core and #1811912: Add pluggable calendar backend to core and centralize date translation and adds a date form element and a date field to core. I don't yet have tests for this patch, but I want to post it so people can take a look and try it out and provide feedback.
The patch adds a new form element called 'datetime'. This is an element that splits the field into a 'date' and a 'time'. Each of those parts is constructed to be HTML5-compatible. Since the support for HTML5 date and time is horrible (missing on most browsers), I've used the jQuery datepicker as a fallback for the 'date' field by testing before using the datepicker to see if the HTML5 date is supported. I've tried it out on both a browser that supports HTML5 date fields (Chrome) and a browser that doesn't (Firefox), and it seems to be working right, but I'm open for ideas on ways to do it better.
Since that is a regular form element type, it can be used by any form, not just as a Field module field. So I've switched the node creation date to use it. The element handles its own validation and manages the explosion of a string value into date and time parts, and the transformation of the user input back into a string date, so adding it to a form is pretty simple.
Then I added a new field. I ended up calling it a 'Timestamp' field. It's as simple as I can make it and still provide something useful. It has just a couple settings -- an option to use either 'Date and time' or 'Date only', an option to set the default value to either the current date or empty, and a formatter setting to choose the format for the displayed date. It creates a single value (no 'Start' and 'End' date) that is stored in the database as a timestamp, and it has one widget that uses the datetime form element with its datepicker.
I have a potential additional widget to create a drop-down selector of year, month, day, etc instead, but I pulled that out to keep this as simple as possible. That can go in contrib, or in core, whichever makes more sense.
I would kind of like to store this as an ISO date in a varchar field instead of a timestamp, for reasons I outlined at http://groups.drupal.org/node/221254, but I didn't want to get into a whole long discussion about performance implications, if any. This patch is still a work in progress, so we could still make that switch. It would actually only involve changing a few lines of code in the field.
We're running out of time before feature freeze, so I wanted to go ahead and get this on the table for discussion.
I'm posting a patch that includes everything in the earlier issues so I can see what's happening with tests. I'm also posting an interdiff that just shows what's changed between this patch and the other two for a patch that's easier to read.
If you want to try it out, apply the full patch, then try out the new field on the node creation date and test adding a new Timestamp field to a content type. See http://drupal.org/node/1819442 for more information about the thought process that has gone into this, and http://groups.drupal.org/date-api.
Comment #21
KarenS CreditAttribution: KarenS commentedLet's try a re-roll, some things were goofed up in the separation between the patches.
Comment #23
swentel CreditAttribution: swentel commentedjthorson told me apache error log was throwing 'Class 'DrupalDateTime' not found in common.inc on line 2048.', so let's see if this works. interdiff attached.
- edit - oh wait, they are already plugins, nevermind :)
Comment #25
KarenS CreditAttribution: KarenS commentedAh. Well FWIW, I was avoiding adding the use statement and instead trying to use the full path to the class so that code wasn't loaded on every page load. Maybe that's more cautious than I need to be, it would be easier to use the 'use' statement.
Comment #26
Crell CreditAttribution: Crell commented"use" statements do not load code. They're simply an alias for the compiler. They have zero runtime impact at all.
For non-namespaced classes, current coding standards are to not "use" them and simply say
new \DateTime();
, or whatever."Drupal" as as a prefix for a class name is directly contrary to current coding standards. It should either reuse the parent class name, in a new namespace, or else have some more descriptive prefix. Eg, "FancyDateTime", "ExtendeDateTime", etc.
Comment #27
KarenS CreditAttribution: KarenS commentedDrupalDateTime extends DateTimePlus which extends DateTime. I'm out of ideas for the name that make sense. DateTimePlus is a component that doesn't use Drupal functions, and DrupalDateTime extends that to use Drupal extensions. And all of that is in another patch anyway, #1802278: Add a Date component to core where we already discussed the names and got webchick's buy off on the idea.
Comment #28
Crell CreditAttribution: Crell commentedIt also looks like the method and property names in the DateTime component are all not following coding standards. They should be camelCased, not underscore_cased. (Looking at the patch in #23).
Comment #29
fagoIt's stored as timestamp, but it's a date. So it should be type 'date' and tell the system it's a date. This will give you the date object directly as value also, at least once we've converted field API to go with the new API...
That's very storage centric and I'm not sure all users know what timestamps are. Why not call it date field? Most users won't care how it is stored anyway - I could even see that being up to the storage layer.
Comment #30
Robin Millette CreditAttribution: Robin Millette commentedThe related API issue #1802278-78: Add a Date component to core was (un)tagged with needs accessibility review so I'm tagging this issue. I'm not 100% sure if this is the right thing to do.
Comment #31
mgifford@Robin - Seems reasonable to me. Thanks.
I'm still not sure how to test it.
I guess we're not quite at the stage where I'll see it between Boolean & Decimal here:
/admin/structure/types/manage/article/fields
Or are we?
Comment #31.0
mgiffordUpdated issue summary.
Comment #32
KarenS CreditAttribution: KarenS commentedTwo other patches build a foundation for this and must be committed first. Help getting them committed would be much appreciated:
In the meantime, I'm working on another re-roll to catch up with changes that have happened on those issues.
Comment #33
KarenS CreditAttribution: KarenS commentedTagging with Datein8
Comment #34
KarenS CreditAttribution: KarenS commentedLatest reroll. This includes the other two patches -- the date component and the pluggable calendar system, but I want to post this much.
After some thought, I re-named the new field module to 'Datetime'. That way it does not imply any special storage mechanism and it makes more sense. That means we have to add an install step that will rename any existing datetime fields, since the Date module used that field name. Then Date module will have to finish the update of those fields, but the rename will keep them from being broken when the new core module is enabled.
I went back to ISO storage, too. I'm going to see how that works. A simple field is working at this point. More work and tests are still needed, and we have to see if the UX is right.
Comment #35
KarenS CreditAttribution: KarenS commentedLatest status. Reworked the code so the #default_value is a date object instead of a string. This has the advantage of eliminating a lot of extra work where we keep creating new date objects in every processing step. Now we can create a date object once and just pass it around. I also added in the improvement tim.plunkett added to the D7 version where we create a date object in field_load so it can be cached.
The patch now has two ways to store dates -- as a full date/time or date only. If it's date-only, it's stored that way in the database rather than appending it with time. We still go through the exercise of timezone conversion so that everything in the database is in UTC by giving the date a time of midnight before we save it. I'm not 100% sure if this is the best solution or if we should bypass timezone conversion.
This feels like it's working pretty well. Next we need lots of tests. I'd love to have people try this out and help create tests and poke holes in things. This patch is against head and includes the date component patch that it depends on, so applying just this patch to a clean checkout should get things working.
Comment #36
KarenS CreditAttribution: KarenS commentedUgh. I lost some work in that patch when I pulled in the latest changes from head. Looks like that patch does not have all my latest work, I'll have to figure out what got lost and try again.
Comment #37
KarenS CreditAttribution: KarenS commentedOK, here's the right patch.
Comment #38
jherencia CreditAttribution: jherencia commentedComment #40
KarenS CreditAttribution: KarenS commentedLet's see if this fixes the broken tests. Still need to add new tests for the new functionality.
Comment #42
KarenS CreditAttribution: KarenS commentedThat didn't work so well. Changing the node and comment timestamps to use the new HTML5 date & time elements breaks a lot of tests. I think I fixed most of them now.
Comment #42.0
KarenS CreditAttribution: KarenS commentedAdd links to the other issues that need to be committed first.
Comment #43
KarenS CreditAttribution: KarenS commentedI updated the issue summary to describe the current state of the patch and describe what is in it.
Comment #45
swentel CreditAttribution: swentel commentedTypo: #datatime should be #datetime
Is there a special reason you're not using drupal_html_id() ? The function also passes on $settings and $date but they are not used in that function at all.
Typo: important
Needs a newline (or don't we do that for info files?)
Needs a newline (but then again, that will be covered I guess once there's actual upgrade code)
Not sure this inline comment needs to be here. The documentation in datetime_field_load() is already fantastic and should explain what will go on in the code imo.
This looks already fantastic in my opinion. I also tested the field type quickly and looks to be working nicely. I'm wondering whether we also need tests for the field type module ? Then again, there are already so many tests in #1802278: Add a Date component to core for the format of a date/time that I think we can ship without, unless someone else disagrees.
Comment #46
KarenS CreditAttribution: KarenS commentedI didn't know about drupal_html_id(), so I've added that in and fixed the other suggestions. I also should have the last broken test working again. I'm starting the process of adding in better Views integration by swapping in a date filter for the string filter that the Field module puts there. There seems to be a bug that makes it impossible to change the filters, no matter what I do I still get the string filter form, so I'm posting a report about that with link to this issue so they can help me figure out why that won't work.
I'm trying to decide what tests we need. As noted above, the API is already tested and there are already tests for a lot of the form processing. I think we need tests for the HTML5 elements, that they create the right output, and some tests around storing and saving full datetime and date-only values in the field.
Comment #48
KarenS CreditAttribution: KarenS commentedLOL, the date component patch made it in, now I have to remove all that code from this patch :)
Comment #49
swentel CreditAttribution: swentel commentedYes, that's great, makes it even easier to review this one - it had the other one open to make sure I could skip parts :)
Comment #50
KarenS CreditAttribution: KarenS commentedOK, updated patch, without the cruft of the other one. This one is just the date element and field.
Comment #51
swentel CreditAttribution: swentel commentedOh, also, as per #1823042: Add maintainers and d.o components for all field type modules - we're trying to list individual maintainers per field type. I guess this might be one for you Karen. So your patch can incorporate a change to maintainers.txt, unless you don't feel like it of course.
Comment #52
attiks CreditAttribution: attiks commentedI did a quick dreditor review
Question: this means that jquery.js, datepicker.js will always be loaded, even if the browser supports html5.
Can this be marked somewhere in the code so that if/when we solve the feature detection we can alter this.
Why says the comment: to all 'required' fields?
new line missing
Comment #53
arlinsandbulte CreditAttribution: arlinsandbulte commentedDid a VERY quick UI review and works great. Below are my comments/questions:
(Note only tested with Date & Time field in FF & Chrome)
No timepicker. Date picker works well. Is this as designed? Adding a timepicker could/should be a followup if it not supposed to be in here. I think this NEEDS to get into core for a UX perspective, and I think HTML5 complicates this due to lagging desktop app support. You know I am a big fan of the wega-type time picker.
Do we not need to limit the datepicker year range anymore? Previously, if it was not limited, bots could bring down a site by continuously clicking through to higher (or lower) dates.
Bunch of other features missing too (End date, granularity, time zone conversion options), but they can either be follow-up issues or maybe handled in contrib. I am not exactly sure what the roadmap is... is there an up dated one somewhere?
Otherwise, great work! Applied cleanly and worked as expected.
Comment #54
Lars Toomre CreditAttribution: Lars Toomre commentedIf this gets re-rolled, both docblock comments and in-line code comments should be examined to better wrap to 80 character hard limit.
Comment #55
Lars Toomre CreditAttribution: Lars Toomre commentedHere are some additional thoughts from reading through the patch:
This docblock needs a one-line summary.
A bunch of missing periods '.' here.
These constants each need a docblock and need to be moved after 'use' statement.
'to the right timezone'?
Add a new line at end of file.
I have seen @xjm recently state this type of declaration should be changed to 'Contains' instead of 'Definition of'. Not sure is that our standard now.
Perhaps 'A string with the date formatted in the chosen format'?
Very small nit... According to [#1353118] and [#1354], @param objects are supposed to start with a fully qualified path, i.e. '\'.
Comment #56
KarenS CreditAttribution: KarenS commentedI don't think we can supply a timepicker in core because core has no available timepicker. And there has been endless discussion about which timepicker is superior, so it would be hard to get consensus even if we tried. The code supports adding a timepicker which can be done by any contrib module that provides a callback. either by form_altering the callback in or by providing an alternate widget that has one.
I need help on the best way to provide the datepicker js only when HTML5 is not available. I fixed it the only way I could. If there's a better way, please provide a patch.
Comment #57
KarenS CreditAttribution: KarenS commentedCrosspost, I wasn't trying to change the status :)
Comment #58
KarenS CreditAttribution: KarenS commentedAlso there is no intention to add end date, granularity, timezone options, etc to this field. This is intended to be a simple field that provides a basic date or date and time and no more. The D8 version of the contrib Date module will provide a more complex field that is comparable to what is in Date now. My plan is to call the new contrib Date field the 'Date Range' field, to indicate it supports a start and end date and the other complexity. The upgrade path will be that all existing date fields will be migrated to the new contrib Date module Date Range field, not to this simple core field which won't support all the options they have now. The new Date module field can still take advantage of the new core date component and datetime element, which should provide a basis for everything we need to do.
Comment #59
andypost@KarenS I think there's enough time before feature freeze to add time support and convert some entity fields with a help of EntityNG to use DateTime field before code freeze
Comment #60
KarenS CreditAttribution: KarenS commentedIt has "time support" already, and it supports the HTML5 time picker, it just doesn't have a jquery timepicker. Not sure what you mean by adding time support. Are you suggesting adding a jquery timepicker to core?
Comment #61
andypost@KarenS suppose timePicker widget could be polished before code-freeze.
I think we can ask community for better js DateTime picker, let's get more reviews
Comment #62
falcon03 CreditAttribution: falcon03 commentedIMO the only feature I miss very much with this approach is date validation.
Would it be very difficult to add the ability to select a range which valid dates should belong to?
Also, what about collecting only time attributes (e.g. hours and minutes) without a date? Or what about collecting only a date attribute (e.g. only the year)??
The Date module is amazing for its enormous flexibility; but I think that we are loosing too much of it moving this field to core (this doesn't mean that I do not want to see a date field in core, but it means that I would like to have a more flexible date field in core).
Comment #63
sunRegarding a front-end timepicker, that should just simply be a polyfill JS library, which only triggers itself when the user-agent does not support a native timepicker. Definitely a separate issue, so I recommend to remove any special support code from this patch (if there is any, didn't review it yet).
Comment #64
nod_Agreed with sun, I'd defer all the JS stuff for later. We can only do something relevant if we get Modernizr in core and that's not yet sure we'll have it.
We should forget about supporting retarded browsers for now, what's important is the field type and all the validation around it, not really the JS widgets (that contrib won't have any issue adding).
Comment #65
KarenS CreditAttribution: KarenS commentedThe only support in this patch for a timepicker is that the element accepts an array of callbacks for the date and time elements so that a contrib module can add them. The date element does use the jQuery timepicker that is already in core as a fallback for the date component because without that this will be a plain text field for virtually everyone using a desktop browser, since there is hardly any support for HTML5 date elements in desktop browsers.
I totally agree that the whole question of adding a timepicker should be a separate issue because core doesn't have any possible solution right now. I think we have to address the question of how to use the jQuery datepicker as a fallback. Without that much of a fallback this field will be is virtually useless to most sites (nobody wants a plain textfield for dates). I proposed a solution here which does work. AFAIK the potential problem with my solution is that it loads the javascript even if it isn't used, it just doesn't use it.
Comment #66
KarenS CreditAttribution: KarenS commentedAnd I just want to emphasize that it's not 'retarded browsers' that don't support the HTML5 date elements -- none of the desktop browsers support it except Chrome and Opera, and they only support the date element, not time, not datetime, not datetime-local.
Comment #67
nod_Ok sorry the "retarded" was uncalled for. I'm an Opera user and it has support for all date things since a while ago.
The problem is that we don't have a strategy to deal with how to do polyfills yet and we can't just commit this and introduce a precedent. This is just the date input, we have color, we have range, we have a lot of other things that needs to be polyfilled eventually and it needs to be part of a global strategy.
Took out the JS part of things and opened a follow up #1835016: Polyfill date input type. Today the Modernizr issue has something to move on #1252178: Add Modernizr to core.
Comment #68
KarenS CreditAttribution: KarenS commentedOK, my assessment of the above comments:
- We won't try to have a fallback jQuery datepicker, that goes into the follow up issue in #67
- We need a follow up issue for adding a jQuery timepicker as a fallback, but it won't go into this patch
- I think the feature set here is about right, at least for the initial patch. If people want more complexity in the core date field that can come in follow up issues.
- I still need to add tests
- I still need to add better Views support, but that will probably also be a follow up issue. Out of the box we get nothing but a string filter and argument, nothing to do with date selection, no recognition of timezone adjustment.
- I need to do some doc cleanup.
- I need to decide how to handle date-only fields, whether to set them to midnight and do timezone adjustment (what the current code does) or if timezone adjustments should be omitted altogether in that case.
Comment #69
arlinsandbulte CreditAttribution: arlinsandbulte commentedI have a suggestion: Set date-only fields to 12:00:00 UTC (e.g. noon UTC) and do time zone adjustment.
This way, time zone adjustment WILL NOT alter the actual date value.
Time zone adjustment could be applied universally to all date values... no special case needed making the code simpler.
This suggestion also applies for 'all day' dates, but that is another issue.
Note: This trick might not always work properly in the following time zone areas:
UCT-12:00
UTC+12:00
UTC+13:00
UTC+13:45
UTC+14:00
Comment #70
Crell CreditAttribution: Crell commentedIMO, we should just make "date only" its own field type that's backed by a date-only field in the DB/storage engine. Mixing date, date-time, and time into one field has bitten me many times before, on timezones as well as other things, and I'd be a lot more comfortable with them as separate fields. (Even if date-time still has lots of options to use in abbreviated form.)
Comment #71
arlinsandbulte CreditAttribution: arlinsandbulte commentedI'm not sure, but I don't think the date component that was just introduced into core (#1802278: Add a Date component to core) includes date without time support...
So, I assumed that all date info would include both date and time, even if one or both or part might be irrelevant.
Part of the reason I assume this is that by keeping all parts, we can better address issues like fuzzy granularity later, even if that is handled by contrib.
Comment #72
KarenS CreditAttribution: KarenS commentedThe date component works fine with either date or date and time, that's not an issue.
We never mix date, date-time, and time into one field and we never have. We have one field *type* that can handle more than one of those, but each individual field is one or the other, based on the field settings. The only way you would ever have date and datetime mixed together in a single field would be if you created a field one way, created some content, ignored the field setting message about not changing field settings once you have content and changed it anyway.
We can decide to have two field types -- one that is just date and one that is both date and time. It adds more options to the field selector. The only extra protection it provides really is that it is not even possible to change the field from one to the other. It simplifies the field just a bit because you can remove all logic that is checking that field setting. My only concern is that if we start down the path of providing a different field for every variation we will have a lot of fields, and people have already been complaining that date module has to many types of fields. Which is why I was trying to simplify it.
Comment #72.0
KarenS CreditAttribution: KarenS commentedUpdate the issue summary to describe the current state of the patch.
Comment #72.1
KarenS CreditAttribution: KarenS commentedUpdate issue summary with follow up issues.
Comment #73
KarenS CreditAttribution: KarenS commentedI added follow up issues to the issue summary, so further discussion about those topics should go in those issues. I don't think they should block getting this initial patch in.
Comment #74
falcon03 CreditAttribution: falcon03 commented@KarenS: what about my questions in #69? Will they be discussed in separate issues?
Also: could we fix these follow-up issues after Feature Freeze?
Comment #75
mgiffordIs it possible to see a Demo somewhere with the related Date patches applied? I'd like to review the UI.
Comment #76
falcon03 CreditAttribution: falcon03 commented@mgifford: patch in #67 applied nicely for me.
I simply applied the patch and added a "date" field" to a content type and it worked nicely, except for the issues I reported in this issue in my previous comments...
Comment #77
KarenS CreditAttribution: KarenS commentedre #62, that's a list of additional functionality you want to add, and I said in #68 "I think the feature set here is about right, at least for the initial patch. If people want more complexity in the core date field that can come in follow up issues." Every additional feature we add requires more complexity in the UI and in the processing. Plus most of the things in your list aren't even possible in contrib at the moment. I don't think we want to add untested features to core, we need to add them to contrib first and figure out the best way to implement them.
I'm going to work on a re-roll in the next day or two and see how much I can incorporate from these comments. Then hopefully we can get some agreement on what should go in this first patch and get it in.
Comment #78
KarenS CreditAttribution: KarenS commentedThis is a re-roll to catch up to head, remove a little bit of remaining code related to the jquery datepicker removed by _Nod, and addressing most of the doc clean up comments. I also have added tests for the date field.
I have use the same doc block comments used by the Text field. I think we need to be consistent in our core field examples. If they need to be changed, they both need to be changed together. And that definitely should be handled in a follow up unless it is changed for the Text field before this gets in.
This is another copy of the doc blocks from the Text field. As above, both fields should use the same language, so I haven't made any changes here.
Next steps:
I still need to add tests for the date element and the upgrade path. I'm thinking about moving the Datetime module from inside field/modules to its own spot in core/modules, similar to the File module. This is just a very simple implementation of date and it already involves a lot of code. The other modules inside field/modules are very simple fields that probably won't grow into more.
I'm also wondering about moving all the code to create the date element into the datetime module as well and out of common.inc and form.inc, to consolidate all the date-related code in a single place. Again this would be similar to what has been done with File module. The only problem with that is that I'm using the date element for node and comment creation dates, which would make them dependent on the datetime module. Or I could change the code that swaps the datetime element into node and comment creation dates to only happen if the datetime module is enabled.
I'd like feedback on the idea of moving this code that way, and I'll add in the other tests. I'm not 100% opposed to adding more functionality to this field, but only things that are already tried and tested in contrib.
Comment #79
Lars Toomre CreditAttribution: Lars Toomre commentedWell, the text field appears to be out of date with regard to our documentation standards. Both the @file and @param type hinting in the patch in #78 and in the text field need to be corrected.
Because an existing example in core is incorrect, I am a bit lost about why it is OK to repeat those same items in this patch.
Also we need to open an issue to correct the documentation for the text field.
Comment #80
swentel CreditAttribution: swentel commentedThat makes sense to me, so plus one from me. Moving to core/modules looks sane to me too.
Comment #81
swentel CreditAttribution: swentel commentedWhat exactly do we need for upgrade path ? Is that for the profile module, if so maybe just leave that and move it into #1261576: Profile module data is not upgraded which already tracks this issue.
Comment #82
KarenS CreditAttribution: KarenS commentedAh, we need profile upgrade path too I suppose, but the one I'm thinking about is for existing date fields using the datetime type, since I'm reclaiming that type name for core. I just need a step to rename the existing fields to something like 'datetime_old' so that Date module can finish the upgrade by merging those into the right contrib date fields.
Comment #83
KarenS CreditAttribution: KarenS commentedAs to the incorrect documentation for the field doc blocks, first of all, I don't see a clear decision about how they should be described. "I have seen @xjm recently state this type of declaration should be changed to 'Contains' instead of 'Definition of'. Not sure is that our standard now." is not a clear call to change it. If someone opens an issue for Text module and makes the decision what the doc blocks should contain, I'll follow that. I see no reason to arbitrarily use different documentation on the only two examples in core of how field modules should work, especially since there is no clear decision about what they should say.
Comment #84
yched CreditAttribution: yched commentedText module classes got in before that was settled, but all recent patches got in with the "Contains \Full\Namespaced\ClassName." thing.
Kind of tedious, I know, but that seems to be the current agreement :-/
Comment #85
sunThis patch is on my (rather huge) list of things-to-review-ASAP. For now I only want to mention:
In #1668332: Add an E-mail field type into core we've collectively learned that Drupal core does not care for the upgrade path from contributed modules. The identical upgrade path problem exists for E-mail module's 'email' field type already, which existed identically in that form in D7, but is taken over by Drupal core now. Thus far, Drupal core denied to provide an upgrade path (not my decision, and I disagree). But anyway, the same lead should be followed here; the upgrade path to D8 needs to be handled by a contributed migration module.
Comment #86
attiks CreditAttribution: attiks commentedFYI: I created #1845546: Implement validation for the TypedData API to try to solve validation on all levels, should work for Data/Time as well.
Comment #87
hass CreditAttribution: hass commentedNo upgrade path? With cck we also had an upgrade path as I know...
Comment #88
bojanz CreditAttribution: bojanz commented@hass
Not in core we didn't.
Comment #89
dpi6 to 7 field data migration is handled by CCK 7.x
Comment #90
KarenS CreditAttribution: KarenS commentedWe have to do some renaming in core or anyone with date data using that field type would have their data destroyed by the upgrade. Contrib updates run after core, there is no way contrib can fix it alone. There is precedent for that. For the D5-D6 upgrade core took over table names that had been used by CCK so core renamed the old tables. For the D6-D7 upgrade core took over date format tables that had been used by Date module, so core renamed the old tables. Core also took over timezone names and the core upgrade took into account the fact that anyone using Date module already had timezone names in the user table. I don't propose that core do the entire upgrade, just that they rename values that were previously used so Date can fix them when the Date upgrade runs.
Comment #91
KarenS CreditAttribution: KarenS commentedOK, here is the latest re-roll. I've made the following changes:
- Moved the datetime hook_element into the Datetime module instead of common.inc, along with its theme, and made Datetime a dependency for Node and Comment. We're already discussing trying to actually make the Node creation date a regular field, so it will become a dependency any way.
- Fixed the debated doc comments for the field doc blocks to match the new preferred language.
- Updated the formatter to use the datetime theme as a render array instead of simple markup.
- Added tests for the date field.
- Added an upgrade step to rename existing datetime fields.
When I tried to use the datetime theme in the formatter (which was added last year in #1183250: Add a theme_datetime() function to consistently theme dates and datetimes, I found that you can't actually pass attributes to it and have them work correctly in the twig template. I could correct that by adding a line to the datetime preprocesser to output the attributes in the preprocessor. I could find no way that they could be printed in the Twig template from an array of values. I don't know if that's the right fix or not, but it's the only one that would work. I'll add a follow up issue to examine that change and see if it's the right one. Hopefully we don't have to let that hold up this issue. Another related follow up issue would be to decide how best to incorporate RDFa attributes into the formatted date.
Tests pass locally, let's see what the bot thinks. If tests pass, hopefully we can get this in and start working on all the follow up issues.
Comment #93
scor CreditAttribution: scor commentedThe RDFa layer will need to be refactored as whole in D8, but for now the markup you have there is pretty decent. The content attribute is no longer needed in RDFa/HTML5 when using the time element, the datetime attribute will be used instead, so I've removed @content from the patch. I've also removed the last hunk from the patch:
Comment #94
KarenS CreditAttribution: KarenS commentedLet's see what the bot thinks.
Comment #96
KarenS CreditAttribution: KarenS commentedI added a second form element to the module, the option to have a drop-down select list of date parts. This is a really commonly requested form element for a date, and it's the better option for things like historical dates that won't work right as HTML5 elements. It's also potentially useful for non-Gregorian calendar system dates, if we get calendar system support in, because HTML5 only works for Gregorian dates. I temporarily added a DateHelper class to define the values for month, day, year, etc, but I hope we can get a calendar plugin system into core to replace the hard-coded assumption that those things can only have the Gregorian values.
I also did more code cleanup and I think I found the source of the tests that were failing.
Comment #97
KarenS CreditAttribution: KarenS commentedForgot to mention that the latest patch also includes updates to use the new config date format values added by the regional settings changes that just landed.
Comment #98
swentel CreditAttribution: swentel commentedQuick review of the code/comments - some small nitpicks, not going to set to 'Needs work' for that, let's get more reviews first.
Comment shouldn't exceed 80 characters.
Same here
Here as well (although I don't know about example code tbh)
A space after the comma.
Spaces
Comment #99
KarenS CreditAttribution: KarenS commentedI appreciate the code review, and I know there are a few things left to fix there, but I really hope people will actually try this out and see if we need to make it more or less configurable, fix the descriptions presented to the user, and otherwise see if the UX is right. It also needs more tests of the basic datetime and datelist form elements and the new datelist widget, and I'd love to have other people add some tests to this patch.
Comment #100
KarenS CreditAttribution: KarenS commentedReroll to latest head. Did some doc cleanup. Removed the attempt at Views integration for now, because it can't work until a couple other patches get in and we already have a follow up issue for that. Added tests for the datelist field and fixed a couple small problems with the select list uncovered when I added the tests.
Comment #101
Lars Toomre CreditAttribution: Lars Toomre commentedHere are some observations from reading through the patch in #100. The missing docblocks and @param/@return directives are the most important to address.
Missing a @return directive.
Can this be changed to 'array $variables'?
This needs to be a one line summary of no more than 80 characters. Perhaps this line should be a second paragraph after a blank line?
Looks like a copy and paste duplicate of the function here.
I think this docblock needs both @param and @return directives.
Needs a @param directive.
Again can this be 'array $variables'? Same with other theme functions in this patch.
I think this doclock needs both @param and @return directives.
Description needs to start with '(optional)'. Also should end with 'Defaults to FALSE.'
Needs @param and @return directives.
Think this docblock also requires @param and @return directives.
Needs @param and @return directives.
These paragraphs should be rewrapped to better use 80 character line limits. Also could use a blank line before and after this #file docblock.
Should explicitly state what the default values are.
Perhaps 'Creates a formatted date value as a string.'?
Needs a @return directive.
This code comment should be rewrapped to better use 80 character limit.
Missing docblock.
Needs to explicitly state what the default value is.
Comment #102
tim.plunkett@Lars Toomre, why can't you just make those changes yourself? It's unreasonable to leave such a review.
Especially when some parts are wrong. We don't typehint $variables in theme functions. At least not yet.
Comment #103
Lars Toomre CreditAttribution: Lars Toomre commentedThanks for your feedback @tim.plunkett. Now that I have had a chance to move to a computer that has git installed on it, here is a patch and interdiff that addresses the various notes from #101.
While creating this patch, I also noticed that there were numerous instances in the #100 patch that used string substitution in creating the text in assertion messages. Each of those cases have now been converted to use format_string() as well. I also converted strings within double quotes to strings enclosed within single quotes as noted as a Drupal best practice.
Unlike other patches where I have had a question and denoted it with a string '???', this time I have simply left that line and/or type hint blank.
Comment #104
mgiffordBack up to #15 there are issues with complex forms & the need for fieldsets.
Comment #105
KarenS CreditAttribution: KarenS commentedThe latest patch actually goes too far:
- It includes changes to documentation for theme_datetime, which was an existing function not added or altered by this patch. If you want to propose clean up for existing documentation that should be another issue.
- We don't provide @param or @return on form functions, including the validation callbacks.
- We don't provide @param or @return on hook implementations. The default_value callbacks are implementations handled by the Field module.
- There are lots of extra lines added. AFAIK there is no documentation standard for adding all those lines. If there is, please point it out. I see no reason to make this patch bigger than it already is and IMO it is readable as-is.
- There is no documentation standard that documentation be exactly 80 characters, only that it be no longer than that, so squeezing in a few extra characters into each line is not required by documentation standards. I won't object to making that change, but it was not required.
I missed format_string(), that's a good change.
Can you re-roll without all the extra stuff? Otherwise I have to go through and try to pick it out.
Comment #106
KarenS CreditAttribution: KarenS commented@mgifford, it would be more helpful if you could provide a patch or at least an example of the html output you think is required. Otherwise I have to read through that whole thread and try to figure out what is needed.
Comment #107
mgiffordI provided some links back in #15 to related issues.
Some html examples can be found here:
http://webaim.org/techniques/forms/controls
http://www.nomensa.com/blog/2010/three-rules-for-creating-accessible-forms/
http://www.w3.org/TR/2011/WD-html5-author-20110809/spec.html#the-fieldse...
It's just basically grouping the date/time elements in a fieldset. That and ensuring that every label is explicitly tied to a fieldset.
The WAVE toolbar from WebAIM provides a good summary and it's a free Mozilla plugin.
Comment #108
Lars Toomre CreditAttribution: Lars Toomre commented@KarenS - I will not have a chance to address #105 comments until later this week at the earliest.
Sorry if I documented more docblocks than were necessary. Please remove what docblocks are incorrect. The wrapping of strings to best use 80 characters comes from repeated comments witnessed while working on API docs clean-up sub-issues.
Regarding blank lines after start of class declarations and before closing class braces, there is some confusion. @tim.plunkett, @xjm and @sun all have said it is a standard. Hence, I have followed their lead.
Comment #109
moshe weitzman CreditAttribution: moshe weitzman commentedGreat work. Quick run through found two problems on the widget side
Comment #110
KarenS CreditAttribution: KarenS commentedYou won't see the javascript date picker, you will only see a HTML5 date widget if your browser supports it (most don't). I originally had a fallback to the jquery datepicker but adding the fall-back javascript date picker got pulled into a follow up issue.
The fatal error is related to the regional configuration patch that just landed which removed that function. I have a new patch that fixes that which I'll upload soon. I'm not sure why you're getting an invalid date error, I need to puzzle over that.
Comment #111
cosmicdreams CreditAttribution: cosmicdreams commentedKaren, I can try to pick up this issue tonight. That function as removed the regional CMI issue. Lars do you think you can review tomorrow?
Comment #113
webchickI'm also having the problem Moshe is having in Chrome:
Comment #114
webchickWhen switching the widget type from "Date and time" to "Select list" I get the following notices:
Comment #115
tim.plunkettThat's #1872786: Can't change widget types
I too get the Chrome error.
Here's a full reroll, the above patch only included new files, not changed ones.
Comment #116
webchickThanks, Tim. :) I unpublished my comment so I don't get commit credit for screwing up the patch. :P
I tried to work around the Chrome issue by not giving the field a default value, but then I get this on submission (only appears for a sec):
Comment #117
webchickLooks like that got changed to NestedArray business: http://drupal.org/node/1870678
Comment #118
tim.plunkettThe #date_increment attribute was being multiplied by 60, which was needed by D7 Date, but isn't with the core input types. It used to be seconds, now it is unitless.
Also, I fixed the NestedArray bugs.
Comment #119
webchickAWESOME! It's working now. :D Doesn't work with Edit module, and not sure why that is.
Here's the full backtrace of that fatal error when you click "Configure" next to the field on the manage display screen (sorry about the formatting):
Comment #120
webchickSwitching that from system_get_date_types() to system_get_date_formats() seems to do the trick.
Let's see if I can not screw this up this time.
Comment #121
swentel CreditAttribution: swentel commentedI think that's because the formatters do not expose this. Looking at the annotations of the default text formatter of the text module:
Comment #122
webchickThat's a really good thought! However, it seems it defaults to form if unspecified, so even by putting those lines in DatetimePlainFormatter.php and DatetimeDefaultFormatter.php it's still not working. :(
Or rather, it's kinda working. You click on the field and get the form:
But then when you save it, it blanks out the value. :(
Comment #124
webchickMy, that Drop moves quickly these days. :)
This is just a re-roll and a couple of whitespace fixes to shut up git apply.
Comment #125
Wim Leers#122: if you reload the page, does the date render properly then? It's very odd that only the field *label* gets re-rendered, but not the field value.
Comment #126
fagoGreat to see this evolving. Are there already plans on integrating this with the new Entity Field API? Generally we need to solve #1867888: Improve TypedData date handling, then integration should be straight-forward. For now it could be integrated just as 'string_field'.
Comment #127
mgiffordGreat to see progress here. There are still problems with the use of Labels, similar to:
#882666: Core form descriptions shouldn't use a label when not associated with a form
Right now it gives:
Which contains a couple redundant labels.
I've changed the first one to:
<h4 class="label">Date </h4>
Deleted one which was redundant:
<label class="element-invisible" for="edit-field-date-und-0-value">Date </label>
Furthermore, I was surprised that the form wasn't using
trim()
on the user input to remove spaces. I also had really hoped that the form would be smart enough to take in any given human readable text (July 1, 2014) and convert that into the matching machine readable format usingstrtotime()
.Mostly though it's the accessibility issues with the input form that need to be addressed. I do think we can get away without adding a fieldset here.
Comment #128
tim.plunkettDue to #1778178: Convert comments to the new Entity Field API there were conflicts in
I *think* I resolved them correctly, but I wasn't really involved with this patch.
I didn't address #127.
Comment #130
swentel CreditAttribution: swentel commentedSome more changes due to #1253820: It's impossible to submit no value for a field that has a default value and #1852966: Rework entity display settings around EntityDisplay config entity
Comment #131
swentel CreditAttribution: swentel commentedAnother re-roll.
Comment #132
tim.plunkettOkay, this addresses most of #127. Mostly it removes the label pointed out as "redundant". The
<h4 class="label">
happens automatically when it's multivalue.Comment #133
mgiffordThanks @tim, but I still see the label. Patch applied, cache cleared... I'm not sure what I'm missing.
Comment #134
tim.plunkett@swentel found out it was caused when form_element was a #theme_wrapper, but he didn't say which one of the 5 was the offending one.
Comment #135
swentel CreditAttribution: swentel commentedIt's this one, but I couldn't fine any decent fix for it. It's also in the other widget file.
Comment #136
webchickI just tried applying this on the off-chance it might work, but sadly no.
Any way of cranking on this in the next week? Really, really, REALLY want to see this land!
Comment #137
mgiffordWhat are the outstanding barriers to RTBC on this one?
I've got one at #133, but are there others that haven't been addressed?
Comment #138
swentel CreditAttribution: swentel commentedThis should fix the label issue from #133 but not sure if this is the way to go ?
I'm still looking why submitting using the Edit module doesn't render the date back, but maybe Wim knows this in 2 seconds ..
Comment #139
swentel CreditAttribution: swentel commentedFixes the integration with the edit module. Date uses hook_field_load which needs a new explicit call so the date field is populated.
Comment #140
Wim LeersSo it sounds like #139 fixes #122 then :)
Comment #141
mgiffordThat removes the Label & puts in a heading instead, which is great for the Date field. It's alright for the Date/Time field. But it's really not appropriate for the select list.
So rather than:
I would propose just doing:
The CSS for the fieldsets needs to change, but it's much better HTML for accessibility.
Comment #142
Wim LeersUX feedback:
DateTime
, which makes sense, but we should make it more human-friendly.I thought swentel asked me to do a review of the entire patch, but shortly after I'd begun he told me he just wanted to feedback on #139's interdiff… :) I figured I'd continue my review, hopefully there's some useful things in there.
Overall, this is clearly a monumental piece of work. Deeply, properly reviewing this would take at least one full day of work, if not more. It's also been reviewed many times times already, so all of it is nitpicks or nitpicky/ish things.
Nitpick: drop the ".php", since it's already a fully qualified namespace+class?
Nitpicky: missing docs. Also in a few other locations, there are
@params $someParam
docblock entries, without actual docs.I'm pretty sure Drupal core has standardized on
instanceof
, notinstanceOf
?Install -> Installs.
The "D8" and "D7" references need to be cleaned up?
"Validation callback" — I've never seen this before in docs. Other #element_validate callbacks are doc'd differently.
Nitpick: extraneous newline. This happens in *many* locations, between the function signature and the first line of the function body.
Hard to parse :) Especially the last part, "will have the same value for in both".
Nitpick: trailing comma.
Nitpicky: single line description missing :)
Nitpick: s/minute/minutes/ ?
Nitpicky: "Plugin implementation" is a really bizarre thing to read here.
As for the changes in #139: no, they're not acceptable, at least not at first sight. I had the same in there before, but @effulgentsia was particularly against it because a
->save()
call should've been sufficient. Are you saying that there is a special class of fields that needs some special pre-render initialization?Comment #143
swentel CreditAttribution: swentel commentedWell, date implements hook_field_load which is invoked from field_attach_load which itself is called only in DatabaseStorageController.php . So, what comes back on the entity from entity_form_submit_build_entity() isn't necessarily ready for displaying nor editing. So yeah, that change is needed and otherwise, I can predict it will come back one day from contrib :)
Comment #144
cosmicdreams CreditAttribution: cosmicdreams commentedso if #142 can sufficiently be addressed can this patch be cleared for landing?
Comment #145
nevergone CreditAttribution: nevergone commented#139: date-501428-139.patch queued for re-testing.
Re-roll and go-go-go in Drupal 8! :)
Comment #147
effulgentsia CreditAttribution: effulgentsia commentedJust a no-op reroll so that it applies to HEAD.
Comment #148
effulgentsia CreditAttribution: effulgentsia commentedThis fixes the test failures, but doesn't address #142.
Comment #149
effulgentsia CreditAttribution: effulgentsia commentedThat's a bug, but not one that should hold up this issue. I opened a spin-off for it: #1916858: EntityFormController::submit() returns an unrenderable $entity depending on hook_field_load() implementations.
Comment #150
mgiffordI just tested out the patch in http://simplytest.me
It applies nicely, and huge job with this important patch.
I noticed that there are no fieldsets for the date/time select lists. It's better than than it was, but fieldsets provide semantic meaning for related input forms. I don't think it should be considered as a nice ad-on.
Perhaps this can be spun off into a related issue, but it's been one I've been trying to raise awareness for about dates for a long while.
Also, the formatting is a bit weird. I've attached screenshot where the positioning is just too close to the next element.
Comment #151
arlinsandbulte CreditAttribution: arlinsandbulte commentedDate fieldsets have been an ongoing debate for a while.
See here for more information: #1467712: Make it possible to disable fieldset for date field
I really don't know where to stand on that.
Comment #152
mgiffordI've tried to ask for more clarity in that list. I really don't think that there should be any visual changes. We can add CSS so that it doesn't provide any UI changes and yet provide more semantic meaning to assistive technology.
If it's important to be able to disable this or change the markup through the UI that can be added to the patch, but I do think it's important that we are grouping related fields semantically in order to meet WCAG 2.0 AA goals established in D7.
Comment #153
falcon03 CreditAttribution: falcon03 commented+1 for grouping the date field form components in a fieldset. As mgifford has already said, though, it is important to avoid any visual change through CSS.
I absolutely don't like the idea of adding the ability to disable the fieldset, since this way site builders/developers could easily degrade drupal' accessibility.
Comment #154
renat CreditAttribution: renat commentedPlease-please-please, do not limit Drupal capabilities just because someone can make bad things with it. We have an excellent example in D7 - dreadful shortcuts limit, and it was introduced with a very similar reasoning to #153. Now we have to use ugly hack to mitigate that limit's influence - despite huge efforts, looks like it will not be undone in D7.
Comment #155
renat CreditAttribution: renat commentedSorry, restore tags, somehow unassigned in previous message.
Comment #156
arlinsandbulte CreditAttribution: arlinsandbulte commentedAfter reviewing a little more, I think the fieldset issue is ONLY relevant when a date & time input form is a SINGLE text input field.
When the it is only a SINGLE text field, there is no need to group it under a field set.
But, when the Field is split into multi-select widget, for instance, the fieldset would definitely be needed.
Come to think of it, I don't remember or know all the details, but what is the state of the HTML5 date & time element? Does this patch default to HTML5 and then revert for browsers without HTML5 support. Note that HTML5 date & time element support is severely lacking in nearly ALL current desktop browsers.
Anyway, the HTML5 implementation probably has a significant effect on the date element & fieldset questions.
See here for more about HTML5: http://groups.drupal.org/node/252878
Comment #157
swentel CreditAttribution: swentel commented@all - do you think we could simply RTBC this one and create follow ups, we're close to feb 18 and this patch is in good shape already imho. Thoughts ?
Comment #158
mgiffordYes, I think that should be possible.
I do think that the addition of fieldsets (although important) is really such a minor part of this much bigger patch. I've added a follow-up issue about fieldsets here #1918994: Improve Datetime and Daterange Widget accessibility
There's also the question of the HTML5 implementation. What are the other outstanding issues?
Comment #159
effulgentsia CreditAttribution: effulgentsia commentedAt a minimum, I think #142.2 needs to be fixed. On a default install, a node can't be saved successfully on Chrome, because Chrome seems to populates the HTML5 widget differently than datetime.module expects to receive that data.
Comment #160
swentel CreditAttribution: swentel commentedGood point. This should fix it, as of Chrome 24, seconds are omitted in case the input is empty.
Comment #161
tim.plunkettOh, that was a pretty straightforward fix, and I agree that it was a commit blocker. Thanks @mgifford for opening the fieldset follow-up.
I manually tested this more, and had no problems at all.
Comment #162
Wim LeersYay! Awesome work, and glad to hear #142.2 was an easy fix :)
Comment #163
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks everyone for all the hard work!
Comment #164
jibranI think it also needs CHANGELOG.txt entry
Comment #165
sunGreat to see this in! :)
#1919420: Remove datetime upgrade path check from its hook_install()
Comment #166
dawehnerHere is patch for the changelog entry.
Comment #167
dwwCongratulations and thanks everyone! So glad to see this in. This makes me think I'm ready to start building www.derekwrightmusic.com as a D8 site (since dated events will be a major component of the content there). :) I wish I could use Portfolio for it. See #1549130-5: WANTED: Project manager for Portfolio ;)
Cheers and appreciation,
-Derek
Comment #168
effulgentsia CreditAttribution: effulgentsia commented#166 looks good.
Comment #169
swentel CreditAttribution: swentel commentedAdded to the field types change record as well http://drupal.org/node/1796546
Comment #170
jibranAdded date module link to change record.
Comment #171
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #172
jstollerI just posted #1920778: Field description doesn't render on datetime field, which appears to come out of this issue.
Comment #174
yched CreditAttribution: yched commentedSorry for pinging 80 people on a long closed issue, but if anyone could shed some light on #1989468: Weird messing with 'default_value_function' in date widgets ? ? :-)
Comment #175
Kiphaas7 CreditAttribution: Kiphaas7 commentedAlso shamelessly pinging people for the following UI problem:
#2050103: interaction with date field is confusing
Comment #175.0
Kiphaas7 CreditAttribution: Kiphaas7 commentedAdding follow up issues for timepicker and views
Comment #176
MustangGB CreditAttribution: MustangGB commentedWhat happened to 'Time only'?
Comment #177
klonos...follow-up issues moved to metadata (as child issues).