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.

CommentFileSizeAuthor
#166 drupal-501428-changelog-166.patch439 bytesdawehner
#160 501428-160.patch125.11 KBswentel
#160 interdiff.txt944 bytesswentel
#150 Date_With_Source.png156.38 KBmgifford
#148 date-501428-148.patch124.95 KBeffulgentsia
#148 interdiff.txt695 byteseffulgentsia
#147 date-501428-147.patch124.12 KBeffulgentsia
#142 Screen Shot 2013-02-09 at 17.21.11.png6.14 KBWim Leers
#142 Screen Shot 2013-02-09 at 17.21.19.png14.78 KBWim Leers
#142 Screen Shot 2013-02-09 at 17.21.33.png14.62 KBWim Leers
#141 SelectListsAndFieldsets.png245.47 KBmgifford
#139 date-501428-139.patch124.1 KBswentel
#139 interdiff.txt835 bytesswentel
#138 date-501428-138.patch123.28 KBswentel
#138 interdiff.txt3.84 KBswentel
#133 LabelPresentForDate.png140.07 KBmgifford
#132 interdiff.txt1.62 KBtim.plunkett
#132 date-501428-132.patch122.53 KBtim.plunkett
#131 date-501428-131.patch122.7 KBswentel
#130 date-501428-130.patch122.67 KBswentel
#128 date-501428-128.patch121.57 KBtim.plunkett
#127 Screen Shot 2013-01-04 at 10.09.15 AM.png133.27 KBmgifford
#127 Screen Shot 2013-01-04 at 10.10.04 AM.png54.97 KBmgifford
#124 date-501428-124.patch121.62 KBwebchick
#122 date-inline-form.png20.18 KBwebchick
#122 date-sad-panda.png9.01 KBwebchick
#120 date-501428-120.patch121.66 KBwebchick
#118 date-501428-118.patch121.66 KBtim.plunkett
#118 interdiff.txt2.88 KBtim.plunkett
#115 date-501428-115.patch121.65 KBtim.plunkett
#113 invalid-time-chrome.png35.66 KBwebchick
501428-112-date-field.patch93.57 KBwebchick
#104 Date-Fieldsets.png49.59 KBmgifford
#103 501428-103-date-field.patch121.61 KBLars Toomre
#103 interdiff-501428-100-103.txt48.9 KBLars Toomre
#100 501428-date-field-100.patch118.03 KBKarenS
#96 501428-date-field-96.patch124.16 KBKarenS
#93 501428-date-field-93.patch77.24 KBscor
#91 501428-date-field-91.patch77.39 KBKarenS
#78 501428-date-field-78.patch73.37 KBKarenS
#67 core-date-field-type-501428-67.patch59.52 KBnod_
#50 501428-date-field-50.patch63.41 KBKarenS
#46 501428-date-field-46.patch122.79 KBKarenS
#42 501428-date-field-42.patch122.56 KBKarenS
#40 501428-date-field-40.patch119.6 KBKarenS
#37 501428-date-field-37.patch112.89 KBKarenS
#35 501428-date-field-35.patch112.76 KBKarenS
#34 501428-date-field-34.patch153.35 KBKarenS
#23 501428-23.patch153.73 KBswentel
#23 interdiff.txt351 bytesswentel
#21 501428-date-field-in-core.patch153.54 KBKarenS
#21 501428-to-1811912-interdiff.txt46.04 KBKarenS
#19 501428-date-field-in-core.patch148.55 KBKarenS
#19 501428-to-1811912-interdiff.txt68.4 KBKarenS
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bjaspan’s picture

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

rickvug’s picture

I 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

yched’s picture

Status: Active » Closed (won't fix)

I think we can now say it won't happen in D7.

mpg’s picture

I 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

yched’s picture

Sylvain Lecoy’s picture

Title: date field type in core » date module in core
Version: 7.x-dev » 8.x-dev
Status: Closed (won't fix) » Active

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

Damien Tournoud’s picture

Title: date module in core » Field type for date and time

Retitling. I fully support this.

RobLoach’s picture

klonos’s picture

Title: Field type for date and time » Date and time field type in core

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

webkenny’s picture

I would like to help with this if I can. I am defecting from #51043: Add a "datetime" Form API element as well. :)

mioan’s picture

I 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

sun’s picture

Category: task » feature
Priority: Normal » Major
Issue tags: +Profile in core

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

sun’s picture

Issue tags: +Platform Initiative
sun’s picture

Issue summary: View changes

Updated issue summary.

KarenS’s picture

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

mgifford’s picture

Issue tags: +Accessibility

The 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

webchick’s picture

Adding this to the "KIller End-User Features" tag. :) Go, Karen, Go! :D

andypost’s picture

Probably we should close this as duplicate of #1802278: Add a Date component to core
Also related #51043: Add a "datetime" Form API element

KarenS’s picture

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

KarenS’s picture

Status: Active » Needs review
FileSize
68.4 KB
148.55 KB

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

Status: Needs review » Needs work

The last submitted patch, 501428-date-field-in-core.patch, failed testing.

KarenS’s picture

Status: Needs work » Needs review
FileSize
46.04 KB
153.54 KB

Let's try a re-roll, some things were goofed up in the separation between the patches.

Status: Needs review » Needs work

The last submitted patch, 501428-date-field-in-core.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
351 bytes
153.73 KB

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

Status: Needs review » Needs work

The last submitted patch, 501428-23.patch, failed testing.

KarenS’s picture

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

Crell’s picture

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

KarenS’s picture

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

Crell’s picture

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

fago’s picture

+++ b/core/modules/field/modules/timestamp/lib/Drupal/timestamp/Type/TimestampItem.php
@@ -0,0 +1,39 @@
+      self::$propertyDefinitions['value'] = array(
+        'type' => 'integer',
+        'label' => t('Timestamp value'),
+      );

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

+name = Timestamp
+description = A simple date/time field stored as a timestamp.

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.

Robin Millette’s picture

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

mgifford’s picture

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

mgifford’s picture

Issue summary: View changes

Updated issue summary.

KarenS’s picture

Assigned: Unassigned » KarenS

Two other patches build a foundation for this and must be committed first. Help getting them committed would be much appreciated:

  • #1802278: Add a Date component to core
  • #1811912: Add pluggable calendar backend to core and centralize date translation
  • In the meantime, I'm working on another re-roll to catch up with changes that have happened on those issues.

    KarenS’s picture

    Issue tags: +Datein8

    Tagging with Datein8

    KarenS’s picture

    FileSize
    153.35 KB

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

    KarenS’s picture

    FileSize
    112.76 KB

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

    KarenS’s picture

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

    KarenS’s picture

    FileSize
    112.89 KB

    OK, here's the right patch.

    jherencia’s picture

    Status: Needs work » Needs review

    Status: Needs review » Needs work

    The last submitted patch, 501428-date-field-37.patch, failed testing.

    KarenS’s picture

    Status: Needs work » Needs review
    FileSize
    119.6 KB

    Let's see if this fixes the broken tests. Still need to add new tests for the new functionality.

    Status: Needs review » Needs work

    The last submitted patch, 501428-date-field-40.patch, failed testing.

    KarenS’s picture

    Status: Needs work » Needs review
    FileSize
    122.56 KB

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

    KarenS’s picture

    Issue summary: View changes

    Add links to the other issues that need to be committed first.

    KarenS’s picture

    I updated the issue summary to describe the current state of the patch and describe what is in it.

    Status: Needs review » Needs work

    The last submitted patch, 501428-date-field-42.patch, failed testing.

    swentel’s picture

    +++ b/core/includes/form.incundefined
    @@ -2922,18 +2923,49 @@ function password_confirm_validate($element, &$element_state) {
    + * Returns HTML for a HTML5-compatible #datatime form element.
    

    Typo: #datatime should be #datetime

    +++ b/core/includes/form.incundefined
    @@ -2948,91 +2980,452 @@ function theme_date($variables) {
    +  $new_id = datepicker_settings_id($element['#id'], $settings, $date);
    

    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.

    +++ b/core/includes/form.incundefined
    @@ -2948,91 +2980,452 @@ function theme_date($variables) {
    + * The format is mportant because these elements will not
    

    Typo: important

    +++ b/core/modules/field/modules/datetime/datetime.infoundefined
    @@ -0,0 +1,6 @@
    +core = 8.x
    +dependencies[] = field
    \ No newline at end of file
    

    Needs a newline (or don't we do that for info files?)

    +++ b/core/modules/field/modules/datetime/datetime.installundefined
    @@ -0,0 +1,31 @@
    +function datetime_install() {
    +
    +}
    

    Needs a newline (but then again, that will be covered I guess once there's actual upgrade code)

    +++ b/core/modules/field/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,171 @@
    +  // Load the stored date values and create date objects for caching.
    

    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.

    KarenS’s picture

    Status: Needs work » Needs review
    FileSize
    122.79 KB

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

    Status: Needs review » Needs work

    The last submitted patch, 501428-date-field-46.patch, failed testing.

    KarenS’s picture

    LOL, the date component patch made it in, now I have to remove all that code from this patch :)

    swentel’s picture

    Yes, that's great, makes it even easier to review this one - it had the other one open to make sure I could skip parts :)

    KarenS’s picture

    Status: Needs work » Needs review
    FileSize
    63.41 KB

    OK, updated patch, without the cruft of the other one. This one is just the date element and field.

    swentel’s picture

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

    attiks’s picture

    I did a quick dreditor review

    +++ b/core/misc/datepicker.jsundefined
    @@ -0,0 +1,29 @@
    +/**
    + * Attaches the datepicker behavior to all required fields
    + */
    

    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?

    +++ b/core/modules/field/modules/datetime/datetime.views.incundefined
    @@ -0,0 +1,31 @@
    +  return $data;
    +}
    \ No newline at end of file
    

    new line missing

    arlinsandbulte’s picture

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

    Lars Toomre’s picture

    If this gets re-rolled, both docblock comments and in-line code comments should be examined to better wrap to 80 character hard limit.

    Lars Toomre’s picture

    Status: Needs review » Needs work

    Here are some additional thoughts from reading through the patch:

    +++ b/core/includes/common.incundefined
    @@ -2024,6 +2024,65 @@ function date_iso8601($date) {
     
     /**
    + * Handle a string like -3:+3 or 2001:2010 to describe a dynamic range
    + * of minimum and maximum years to use in a date selector.
    

    This docblock needs a one-line summary.

    +++ b/core/includes/form.incundefined
    @@ -2948,91 +2980,425 @@ function theme_date($variables) {
    + *     Options are:
    + *     - datetime: Use the HTML5 datetime element type
    + *     - datetime-local: Use the HTML5 datetime-local element type
    + *     - date: Use the HTML5 date element type
    + *     - text: No HTML5 element, use a normal text field
    + *     - none: Do not display a date element
    + *   - #date_date_callbacks: Array of optional callbacks for the date element.
    + *     Can be used to add a jQuery datepicker, for instance. Drupal provides the
    + *     'datetime_jquery_datepicker_callback' as one possible choice.
    + *   - #date_time_element: The time element.
    + *     Options are:
    + *     - time: Use a HTML5 time element type
    + *     - text: No HTML5 element, use a normal text field
    

    A bunch of missing periods '.' here.

    +++ b/core/modules/field/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,170 @@
    +
    +const DATETIME_STORAGE_TIMEZONE       = 'UTC';
    +const DATETIME_DEFAULT_STORAGE_FORMAT = 'Y-m-d\TH:i:s';
    +const DATETIME_DATE_STORAGE_FORMAT    = 'Y-m-d';
    

    These constants each need a docblock and need to be moved after 'use' statement.

    +++ b/core/modules/field/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,170 @@
    + * validator and transformed to an date object. We just need to
    + * convert the date back to a the right timezone and format for storage.
    + */
    +function datetime_widget_validate(&$element, &$form_state) {
    

    'to the right timezone'?

    +++ b/core/modules/field/modules/datetime/datetime.views.incundefined
    @@ -0,0 +1,31 @@
    +}
    \ No newline at end of file
    diff --git a/core/modules/field/modules/datetime/lib/Drupal/datetime/Plugin/field/formatter/DatetimeDefaultFormatter.php b/core/modules/field/modules/datetime/lib/Drupal/datetime/Plugin/field/formatter/DatetimeDefaultFormatter.php
    

    Add a new line at end of file.

    +++ b/core/modules/field/modules/datetime/lib/Drupal/datetime/Plugin/field/formatter/DatetimeDefaultFormatter.phpundefined
    @@ -0,0 +1,113 @@
    + * @file
    + * Definition of Drupal\datetime\Plugin\field\formatter\DateTimeDefaultFormatter.
    

    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.

    +++ b/core/modules/field/modules/datetime/lib/Drupal/datetime/Plugin/field/formatter/DatetimeDefaultFormatter.phpundefined
    @@ -0,0 +1,113 @@
    +   * @return string
    +   *   Returns a date formatted with the formatter's chosen format.
    

    Perhaps 'A string with the date formatted in the chosen format'?

    +++ b/core/modules/field/modules/datetime/lib/Drupal/datetime/Plugin/field/widget/DatetimeDatepicker.phpundefined
    @@ -0,0 +1,130 @@
    +   * @param Drupal\Component\Plugin\Discovery\DiscoveryInterface $discovery
    +   *   The Discovery class that holds access to the widget implementation
    +   *   definition.
    

    Very small nit... According to [#1353118] and [#1354], @param objects are supposed to start with a fully qualified path, i.e. '\'.

    KarenS’s picture

    Status: Needs work » Needs review

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

    KarenS’s picture

    Status: Needs review » Needs work

    Crosspost, I wasn't trying to change the status :)

    KarenS’s picture

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

    andypost’s picture

    @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

    KarenS’s picture

    It 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?

    andypost’s picture

    Status: Needs work » Needs review
    Issue tags: +D8MI, +Entity Field API

    @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

    falcon03’s picture

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

    sun’s picture

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

    nod_’s picture

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

    KarenS’s picture

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

    KarenS’s picture

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

    nod_’s picture

    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.

    KarenS’s picture

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

    arlinsandbulte’s picture

    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.

    I 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

    Crell’s picture

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

    arlinsandbulte’s picture

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

    KarenS’s picture

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

    KarenS’s picture

    Issue summary: View changes

    Update the issue summary to describe the current state of the patch.

    KarenS’s picture

    Issue summary: View changes

    Update issue summary with follow up issues.

    KarenS’s picture

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

    falcon03’s picture

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

    mgifford’s picture

    Is it possible to see a Demo somewhere with the related Date patches applied? I'd like to review the UI.

    falcon03’s picture

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

    KarenS’s picture

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

    KarenS’s picture

    FileSize
    73.37 KB

    This 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 seen @xjm recently state this type of declaration should be changed to 'Contains' instead of 'Definition of'. Not sure is that our standard now.

    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.

    Very small nit... According to [#1353118] and [#1354], @param objects are supposed to start with a fully qualified path, i.e. '\'.

    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.

    Lars Toomre’s picture

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

    swentel’s picture

    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

    That makes sense to me, so plus one from me. Moving to core/modules looks sane to me too.

    swentel’s picture

    I still need to add tests for the date element and the upgrade path

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

    KarenS’s picture

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

    KarenS’s picture

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

    yched’s picture

    Text 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 :-/

    sun’s picture

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

    attiks’s picture

    FYI: I created #1845546: Implement validation for the TypedData API to try to solve validation on all levels, should work for Data/Time as well.

    hass’s picture

    No upgrade path? With cck we also had an upgrade path as I know...

    bojanz’s picture

    @hass
    Not in core we didn't.

    dpi’s picture

    6 to 7 field data migration is handled by CCK 7.x

    KarenS’s picture

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

    KarenS’s picture

    FileSize
    77.39 KB

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

    Status: Needs review » Needs work

    The last submitted patch, 501428-date-field-91.patch, failed testing.

    scor’s picture

    FileSize
    77.24 KB

    The 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:

    diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
    old mode 100644
    new mode 100755
    
    KarenS’s picture

    Status: Needs work » Needs review

    Let's see what the bot thinks.

    Status: Needs review » Needs work

    The last submitted patch, 501428-date-field-93.patch, failed testing.

    KarenS’s picture

    Status: Needs work » Needs review
    FileSize
    124.16 KB

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

    KarenS’s picture

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

    swentel’s picture

    Quick review of the code/comments - some small nitpicks, not going to set to 'Needs work' for that, let's get more reviews first.

    +++ b/core/modules/datetime/datetime.installundefined
    @@ -0,0 +1,35 @@
    + * Renames all datetime fields that may have existed prior to D8 for later upgrade by contrib Date module.
    

    Comment shouldn't exceed 80 characters.

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1003 @@
    + * Expands a date element into an array of individual elements (i.e. year, month, day).
    

    Same here

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1003 @@
    + *     '#date_part_order' => array('month', 'day', 'year', 'hour', 'minute', 'ampm'),
    

    Here as well (although I don't know about example code tbh)

    +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/widget/DatetimeDatelistWidget.phpundefined
    @@ -0,0 +1,197 @@
    +      '#type' => 'select', ¶
    

    A space after the comma.

    +++ b/core/modules/datetime/lib/Drupal/datetime/Tests/DateTimeFieldTest.phpundefined
    @@ -0,0 +1,348 @@
    +            ¶
    

    Spaces

    KarenS’s picture

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

    KarenS’s picture

    FileSize
    118.03 KB

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

    Lars Toomre’s picture

    Here are some observations from reading through the patch in #100. The missing docblocks and @param/@return directives are the most important to address.

    +++ b/core/includes/common.incundefined
    @@ -1949,6 +1949,26 @@ function _format_date_callback(array $matches = NULL, $new_langcode = NULL) {
    + * Sometimes needed when the format type needs to be determined before a
    + * date has been created.
    + */
    

    Missing a @return directive.

    +++ b/core/includes/form.incundefined
    @@ -2962,118 +2963,29 @@ function password_confirm_validate($element, &$element_state) {
      */
     function theme_date($variables) {
       $element = $variables['element'];
    

    Can this be changed to 'array $variables'?

    +++ b/core/modules/datetime/datetime.installundefined
    @@ -0,0 +1,35 @@
    +/**
    + * Renames all datetime fields that may have existed prior to D8 for later upgrade by contrib Date module.
    + */
    +function datetime_install() {
    +  db_update('field_config')
    

    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?

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1005 @@
    +/**
    + * Validation callback for the datetime widget element.
    + *
    + * The date has already been validated by the datetime form type
    + * validator and transformed to an date object. We just need to
    + * convert the date back to a the storage timezone and format.
    + */
    +function datetime_datetime_widget_validate(&$element, &$form_state) {
    +  if (!form_get_errors()) {
    +    $input_exists = FALSE;
    +    $input = drupal_array_get_nested_value($form_state['values'], $element['#parents'], $input_exists);
    +    if ($input_exists) {
    +      // The date should have been returned to a date object at this
    +      // point by datetime_validate(), which runs before this.
    +      if (!empty($input['value'])) {
    +        $date = $input['value'];
    +        if ($date instanceOf DrupalDateTime && !$date->hasErrors()) {
    +
    +          // If this is a date-only field, set it to the default time so
    +          // the timezone conversion can be reversed.
    +          if ($element['value']['#date_time_element'] == 'none') {
    +            datetime_date_default_time($date);
    +          }
    +          // Adjust the date for storage.
    +          $date->setTimezone(new \DateTimezone(DATETIME_STORAGE_TIMEZONE));
    +          $value = $date->format($element['value']['#date_storage_format']);
    +          form_set_value($element['value'], $value, $form_state);
    +        }
    +      }
    +    }
    +  }
    +}
    +
    +/**
    + * Validation callback for the datetime widget element.
    + *
    + * The date has already been validated by the datetime form type
    + * validator and transformed to an date object. We just need to
    + * convert the date back to a the storage timezone and format.
    + */
    +function datetime_datelist_widget_validate(&$element, &$form_state) {
    +  if (!form_get_errors()) {
    

    Looks like a copy and paste duplicate of the function here.

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1005 @@
    +/**
    + * Sets a default value for an empty date field.
    + *
    + * Callback for $instance['default_value_function'], as implemented
    + * by Drupal\datetime\Plugin\field\widget\DateTimeDatepicker.
    + */
    +function datetime_default_value($entity_type, $entity, $field, $instance, $langcode) {
    

    I think this docblock needs both @param and @return directives.

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1005 @@
    + */
    +function datetime_date_default_time($date) {
    +  $date->setTime(12, 0, 0);
    

    Needs a @param directive.

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1005 @@
    + */
    +function theme_datetime_form($variables) {
    

    Again can this be 'array $variables'? Same with other theme functions in this patch.

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1005 @@
    + * @endcode
    + */
    +function datetime_datetime_form_process($element, &$form_state) {
    +
    +  // The value callback has populated the #value array.
    

    I think this doclock needs both @param and @return directives.

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1005 @@
    + * @param array $input
    + *   The incoming input to populate the form element. If this is FALSE, the
    + *   element's default value should be returned.
    

    Description needs to start with '(optional)'. Also should end with 'Defaults to FALSE.'

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1005 @@
    +/**
    + * Creates an example for a date format.
    + *
    + * This is centralized for a consistent method of creating these examples.
    + */
    +function datetime_format_example($format) {
    +  $date = &drupal_static(__FUNCTION__);
    

    Needs @param and @return directives.

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1005 @@
    +/**
    + * Element value callback for datelist element.
    + *
    + * Validates the date type to adjust 12 hour time and prevent invalid dates.
    + * If the date is valid, the date is set in the form.
    + */
    +function form_type_datelist_value($element, $input = FALSE, &$form_state = array()) {
    

    Think this docblock also requires @param and @return directives.

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1005 @@
    +/**
    + * Helper function to round minutes and seconds to nearest requested value.
    + */
    +function date_increment_round(&$date, $increment) {
    +  // Round minutes and seconds, if necessary.
    

    Needs @param and @return directives.

    +++ b/core/modules/datetime/lib/Drupal/datetime/DateHelper.phpundefined
    @@ -0,0 +1,522 @@
    + * Lots of helpful functions for use in massaging dates,
    + * specific to the the calendar system in use. The values include
    + * both translated and untranslated values.
    + *
    + * Untranslated values are useful as array keys and as css
    + * identifiers, and should be listed in English.
    + *
    + * Translated values are useful for display to the user. All
    + * values that need translation should be hard-coded and wrapped in
    + * t() so the translation system will be able to process them.
    

    These paragraphs should be rewrapped to better use 80 character line limits. Also could use a blank line before and after this #file docblock.

    +++ b/core/modules/datetime/lib/Drupal/datetime/DateHelper.phpundefined
    @@ -0,0 +1,522 @@
    +   * @param bool $required
    +   *   (optional) If FALSE, the returned array will include a blank value.
    +   *   Defaults to FALSE.
    +   * @param int $month
    +   *   (optional) The month in which to find the number of days.
    +   * @param int $year
    +   *   (optional) The year in which to find the number of days.
    

    Should explicitly state what the default values are.

    +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/formatter/DatetimeDefaultFormatter.phpundefined
    @@ -0,0 +1,133 @@
    +   * Output a formatted date value.
    

    Perhaps 'Creates a formatted date value as a string.'?

    +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/widget/DatetimeDatelistWidget.phpundefined
    @@ -0,0 +1,197 @@
    +  /**
    +   * Return the callback used to set a date default value.
    +   */
    +  public function defaultValueFunction() {
    +    return 'datetime_default_value';
    

    Needs a @return directive.

    +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/widget/DatetimeDatelistWidget.phpundefined
    @@ -0,0 +1,197 @@
    +    // We're nesting some sub-elements inside the parent, so we
    +    // need a wrapper. We also need to add another #title attribute
    +    // at the top level for ease in identifying this item in error
    +    // messages. We don't want to display this title because the
    +    // actual title display is handled at a higher level by the Field
    +    // module.
    

    This code comment should be rewrapped to better use 80 character limit.

    +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/widget/DatetimeDatelistWidget.phpundefined
    @@ -0,0 +1,197 @@
    +
    +  function settingsForm(array $form, array &$form_state) {
    +    $element = parent::settingsForm($form, $form_state);
    

    Missing docblock.

    +++ b/core/modules/datetime/lib/Drupal/datetime/Tests/DateTimeFieldTest.phpundefined
    @@ -0,0 +1,414 @@
    +   * @param string $view_mode
    +   *   (optional) The view mode to use for rendering.
    +   * @param bool $reset
    

    Needs to explicitly state what the default value is.

    tim.plunkett’s picture

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

    Lars Toomre’s picture

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

    mgifford’s picture

    FileSize
    49.59 KB

    Back up to #15 there are issues with complex forms & the need for fieldsets.

    KarenS’s picture

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

    KarenS’s picture

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

    mgifford’s picture

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

    Lars Toomre’s picture

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

    moshe weitzman’s picture

    Great work. Quick run through found two problems on the widget side

    1. Got an AJAX error when clicking on formatter settings on Manage Display.
      An AJAX HTTP error occurred.
      HTTP Result Code: 200
      Debugging information follows.
      Path: /d8x/system/ajax
      StatusText: OK
      ResponseText: 
      ( ! ) Fatal error: Call to undefined function Drupal\datetime\Plugin\field\formatter\system_get_date_types() in /Users/mweitzman/htd/d8x/core/modules/datetime/lib/Drupal/datetime/Plugin/field/formatter/DatetimeDefaultFormatter.php on line 104
      
    2. When entering in a date on node form, I am always getting an invalid value error, despite the date+time being valid. When I switch to Date-only, I get no error but I also don't see the javascript date picker which is sad.
    KarenS’s picture

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

    cosmicdreams’s picture

    Karen, 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?

    webchick’s picture

    FileSize
    35.66 KB

    I'm also having the problem Moshe is having in Chrome:

    invalid-time-chrome.png

    webchick’s picture

    When switching the widget type from "Date and time" to "Select list" I get the following notices:

    Notice: Undefined index: widget_type in field_ui_widget_type_form_submit() (line 752 of core/modules/field_ui/field_ui.admin.inc).
    Notice: Undefined index: module in field_ui_widget_type_form_submit() (line 753 of core/modules/field_ui/field_ui.admin.inc).
    Notice: Undefined index: widget_type in field_ui_widget_type_form_submit() (line 755 of core/modules/field_ui/field_ui.admin.inc).
    Notice: Undefined index: module in _field_write_instance() (line 615 of core/modules/field/field.crud.inc).
    Notice: Undefined index: in Drupal\field_ui\FieldOverview->form() (line 143 of core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php).
    
    tim.plunkett’s picture

    FileSize
    121.65 KB

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

    webchick’s picture

    Thanks, 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):

    Fatal error: Call to undefined function drupal_array_get_nested_value() in /Users/abyron/Sites/8.x/core/modules/datetime/datetime.module on line 621
    Call Stack
    #	Time	Memory	Function	Location
    1	0.0001	662736	{main}( )	../index.php:0
    2	0.0188	3280480	Symfony\Component\HttpKernel\Kernel->handle( )	../index.php:40
    3	0.0192	3372176	Drupal\Core\HttpKernel->handle( )	../Kernel.php:195
    4	0.0192	3373352	Symfony\Component\HttpKernel\HttpKernel->handle( )	../HttpKernel.php:51
    5	0.0192	3373352	Symfony\Component\HttpKernel\HttpKernel->handleRaw( )	../HttpKernel.php:73
    6	0.0292	3843968	call_user_func_array ( )	../HttpKernel.php:129
    7	0.0293	3844216	Drupal\Core\EventSubscriber\{closure}( )	../HttpKernel.php:0
    8	0.0293	3844264	call_user_func_array ( )	../LegacyControllerSubscriber.php:52
    9	0.0293	3844600	node_add( )	../LegacyControllerSubscriber.php:0
    10	0.0327	4188792	entity_get_form( )	../node.pages.inc:111
    11	0.0329	4213824	drupal_build_form( )	../entity.inc:446
    12	0.0341	4450520	drupal_process_form( )	../form.inc:381
    13	0.0547	5241480	drupal_validate_form( )	../form.inc:865
    14	0.0548	5242136	_form_validate( )	../form.inc:1154
    15	0.0554	5248672	_form_validate( )	../form.inc:1316
    16	0.0555	5249656	_form_validate( )	../form.inc:1316
    17	0.0556	5251000	call_user_func_array ( )	../form.inc:1425
    18	0.0556	5251040	datetime_datetime_validate( )	../form.inc:0
    
    webchick’s picture

    Looks like that got changed to NestedArray business: http://drupal.org/node/1870678

    tim.plunkett’s picture

    FileSize
    2.88 KB
    121.66 KB

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

    webchick’s picture

    AWESOME! 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):

    An AJAX HTTP error occurred.
    HTTP Result Code: 200
    Debugging information follows.
    Path: /8.x/system/ajax
    StatusText: OK
    ResponseText: 
    ( ! ) Fatal error: Call to undefined function Drupal\datetime\Plugin\field\formatter\system_get_date_types() in /Users/abyron/Sites/8.x/core/modules/datetime/lib/Drupal/datetime/Plugin/field/formatter/DatetimeDefaultFormatter.php on line 103
    Call Stack
    #TimeMemoryFunctionLocation
    10.0077691536{main}(  )../index.php:0
    20.140325900688Symfony\Component\HttpKernel\Kernel->handle(  )../index.php:40
    30.188726179496Drupal\Core\HttpKernel->handle(  )../Kernel.php:195
    40.188726180672Symfony\Component\HttpKernel\HttpKernel->handle(  )../HttpKernel.php:51
    50.188726180672Symfony\Component\HttpKernel\HttpKernel->handleRaw(  )../HttpKernel.php:73
    60.233928009736call_user_func_array
    (  )../HttpKernel.php:129
    70.233928009984Drupal\Core\EventSubscriber\{closure}(  )../HttpKernel.php:0
    80.233928010032call_user_func_array
    (  )../LegacyControllerSubscriber.php:52
    90.233928010280ajax_form_callback(  )../LegacyControllerSubscriber.php:0
    100.238428785648drupal_process_form(  )../ajax.inc:368
    110.253029407536drupal_rebuild_form(  )../form.inc:950
    120.253029407536drupal_retrieve_form(  )../form.inc:459
    130.253129409920call_user_func_array
    (  )../form.inc:817
    140.253129410632Drupal\field_ui\DisplayOverview->form(  )../form.inc:0
    150.269330905504Drupal\datetime\Plugin\field\formatter\DateTimeDefaultFormatter->settingsForm(  )../DisplayOverview.php:205
    
    webchick’s picture

    FileSize
    121.66 KB

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

    swentel’s picture

    Doesn't work with Edit module, and not sure why that is.

    I think that's because the formatters do not expose this. Looking at the annotations of the default text formatter of the text module:

     *   edit = {
     *     "editor" = "direct"
     *   }
    
    webchick’s picture

    FileSize
    9.01 KB
    20.18 KB

    That'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:

    date-inline-form.png

    But then when you save it, it blanks out the value. :(

    date-sad-panda.png

    Status: Needs review » Needs work

    The last submitted patch, date-501428-120.patch, failed testing.

    webchick’s picture

    Status: Needs work » Needs review
    FileSize
    121.62 KB

    My, that Drop moves quickly these days. :)

    This is just a re-roll and a couple of whitespace fixes to shut up git apply.

    Wim Leers’s picture

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

    fago’s picture

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

    mgifford’s picture

    FileSize
    54.97 KB
    133.27 KB

    Great 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:

    <div class="form-item">
    <label for="edit-field-date-und-0">Date </label>
    <div class="form-item form-type-datetime form-item-field-date-und-0-value">
    <label class="element-invisible" for="edit-field-date-und-0-value">Date </label>
    <div id="edit-field-date-und-0-value" class="container-inline">
    <div class="form-item form-type-date form-item-field-date-und-0-value-date">
    <label class="element-invisible" for="edit-field-date-und-0-value-date">Date </label>
    <input id="edit-field-date-und-0-value-date" class="form-date" type="date" size="12" value="2013-01-04" name="field_date[und][0][value][date]" max="2050-12-31" min="1900-01-01" title="Date (i.e. 2013-01-04)">
    </div>
    <div class="form-item form-type-date form-item-field-date-und-0-value-time">
    <label class="element-invisible" for="edit-field-date-und-0-value-time">Time </label>
    <input id="edit-field-date-und-0-value-time" class="form-date" type="time" size="12" value="10:03:31" name="field_date[und][0][value][time]" step="1" title="Time (i.e. 10:11:17)">
    </div>
    </div>
    </div>
    </div>

    Which contains a couple redundant labels.

    <div class="form-item">
    <h4 class="label">Date </h4>
    <div class="form-item form-type-datetime form-item-field-date-und-0-value">
    <div id="edit-field-date-und-0-value" class="container-inline">
    <div class="form-item form-type-date form-item-field-date-und-0-value-date">
    <label class="element-invisible" for="edit-field-date-und-0-value-date">Date </label>
    <input id="edit-field-date-und-0-value-date" class="form-date" type="date" size="12" value="2013-01-04" name="field_date[und][0][value][date]" max="2050-12-31" min="1900-01-01" title="Date (i.e. 2013-01-04)">
    </div>
    <div class="form-item form-type-date form-item-field-date-und-0-value-time">
    <label class="element-invisible" for="edit-field-date-und-0-value-time">Time </label>
    <input id="edit-field-date-und-0-value-time" class="form-date" type="time" size="12" value="10:03:31" name="field_date[und][0][value][time]" step="1" title="Time (i.e. 10:11:17)">
    </div>
    </div>
    </div>
    </div>

    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 using strtotime().

    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.

    tim.plunkett’s picture

    FileSize
    121.57 KB

    Due to #1778178: Convert comments to the new Entity Field API there were conflicts in

    core/modules/comment/lib/Drupal/comment/CommentFormController.php 
    core/modules/comment/lib/Drupal/comment/Tests/CommentPreviewTest.php 
    core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.php 
    core/profiles/standard/standard.info 
    

    I *think* I resolved them correctly, but I wasn't really involved with this patch.

    I didn't address #127.

    Status: Needs review » Needs work

    The last submitted patch, date-501428-128.patch, failed testing.

    swentel’s picture

    swentel’s picture

    FileSize
    122.7 KB

    Another re-roll.

    tim.plunkett’s picture

    FileSize
    122.53 KB
    1.62 KB

    Okay, this addresses most of #127. Mostly it removes the label pointed out as "redundant". The <h4 class="label"> happens automatically when it's multivalue.

    mgifford’s picture

    FileSize
    140.07 KB

    Thanks @tim, but I still see the label. Patch applied, cache cleared... I'm not sure what I'm missing.

    tim.plunkett’s picture

    Status: Needs review » Needs work

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

    swentel’s picture

    +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/widget/DatetimeDatelistWidget.phpundefined
    @@ -0,0 +1,213 @@
    +  public function formElement(array $items, $delta, array $element, $langcode, array &$form, array &$form_state) {
    +
    +    $field = $this->field;
    +    $instance = $this->instance;
    +
    +    $date_order = $this->getSetting('date_order');
    +    $time_type = $this->getSetting('time_type');
    +    $increment = $this->getSetting('increment');
    +
    +    // We're nesting some sub-elements inside the parent, so we
    +    // need a wrapper. We also need to add another #title attribute
    +    // at the top level for ease in identifying this item in error
    +    // messages. We don't want to display this title because the
    +    // actual title display is handled at a higher level by the Field
    +    // module.
    +
    +    $element['#theme_wrappers'][] = 'form_element';
    

    It's this one, but I couldn't fine any decent fix for it. It's also in the other widget file.

    webchick’s picture

    I 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!

    mgifford’s picture

    What are the outstanding barriers to RTBC on this one?

    I've got one at #133, but are there others that haven't been addressed?

    swentel’s picture

    Status: Needs work » Needs review
    FileSize
    3.84 KB
    123.28 KB

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

    swentel’s picture

    FileSize
    835 bytes
    124.1 KB

    Fixes the integration with the edit module. Date uses hook_field_load which needs a new explicit call so the date field is populated.

    Wim Leers’s picture

    So it sounds like #139 fixes #122 then :)

    mgifford’s picture

    FileSize
    245.47 KB

    That 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:

    function theme_datetime_wrapper($variables) {
      $element = $variables['element'];
      $output = '';
    
      // If the element is required, a required marker is appended to the label.
      $required = !empty($element['#required']) ? theme('form_required_marker', array('element' => $element)) : '';
    
      if (!empty($element['#title'])) {
        $output .= '<h4 class="label">' . t('!title!required', array('!title' => $element['#title'], '!required' => $required)) . '</h4>';
      }
      $output .= $element['#children'];
    
      return $output;
    }

    I would propose just doing:

    function theme_datetime_wrapper($variables) {
      $element = $variables['element'];
      $output = '';
    
      // If the element is required, a required marker is appended to the label.
      $required = !empty($element['#required']) ? theme('form_required_marker', array('element' => $element)) : '';
    
      $close_fieldset = '';
      if (!empty($element['#title'])) {
       if (isset($variables['element']['value']['#value']['date']) && isset($variables['element']['value']['#value']['time']) && $variables['element']['value']['#value']['time'] == '') {
        $output .= '<h4 class="label">' . t('!title!required', array('!title' => $element['#title'], '!required' => $required)) . '</h4>';
        } 
        else {
          $output .= '<fieldset><legend>' . t('!title!required', array('!title' => $element['#title'], '!required' => $required)) . '</legend>';
        $close_fieldset = '</fieldset>';
        }
      }
      $output .= $element['#children'];
    
      return $output . $close_fieldset;
    }

    The CSS for the fieldsets needs to change, but it's much better HTML for accessibility.

    Wim Leers’s picture

    UX feedback:

    • "Datetime" may make sense from a programmer POV, but does it really also make sense to end users? Shouldn't it be called "Date & time" or "Date and time". Note that only the module name & description seems to use "Datetime", the field name is already "Date and time". In any case, "datetime" is not a word in the dictionary. I think it might be a reference to PHP's DateTime, which makes sense, but we should make it more human-friendly.
    • On a fresh D8, I applied the patch, installed the module, added a "Date and time" field with the "Date and time" widget and tried to use it.
      1. Step 1: I fill it out, using the nice date picker widget. I don't enter seconds, because I don't care about them, which implies they should just be set to "00".
        Screen Shot 2013-02-09 at 17.21.11.png
      2. Step 2: oops. I guess I had to enter zeros for the seconds after all. Ah well, now the system already prefilled the zeroes for me. So let's just hit save again, it should work this time.
        Screen Shot 2013-02-09 at 17.21.19.png
      3. Step 3: WTF? Note that I did not manually enter the date, so it can't be a "swapped DD & MM" thing. Also note how the error message says the year should be first, whereas the widget has it last; the error message uses dashes, the widget uses slashes. This is a minor annoyance, but considering that I'm apparently unable to enter a valid date using the date picker widget itself, I'm now completely stuck…
        Screen Shot 2013-02-09 at 17.21.33.png
    • No such problems with the "Select list" widget.
    • There are two date types "Date and time" and "Date only". Why not also "Time only"?

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

    +++ b/core/includes/common.incundefined
    @@ -1955,6 +1955,30 @@ function _format_date_callback(array $matches = NULL, $new_langcode = NULL) {
    + *   A string as defined in \DrupalComponent\Datetime\DateTimePlus.php: either
    

    Nitpick: drop the ".php", since it's already a fully qualified namespace+class?

    +++ b/core/includes/theme.incundefined
    @@ -1564,6 +1564,12 @@ function theme_disable($theme_list) {
    + *   - attributes['timestamp']:
    + *   - timestamp:
    

    Nitpicky: missing docs. Also in a few other locations, there are @params $someParam docblock entries, without actual docs.

    +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.phpundefined
    @@ -212,8 +211,8 @@ public function validate(array $form, array &$form_state) {
    +      if ($date instanceOf DrupalDateTime && $date->hasErrors()) {
    

    I'm pretty sure Drupal core has standardized on instanceof, not instanceOf?

    +++ b/core/modules/datetime/datetime.installundefined
    @@ -0,0 +1,41 @@
    +/**
    + * Install the new to D8 Datetime module.
    + *
    + * As part of adding this new module to Drupal 8, the Datetime namespace is now
    + * reserved for this module.  This is a possible conflict with a popular contrib
    + * field DateTime that existed in D7.  Hence, any Datetime fields that may have
    + * existed prior to D8 need to renamed for later upgrade by contrib modules like
    

    Install -> Installs.

    The "D8" and "D7" references need to be cleaned up?

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1118 @@
    + * Validation callback for the datetime widget element.
    

    "Validation callback" — I've never seen this before in docs. Other #element_validate callbacks are doc'd differently.

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1118 @@
    +function datetime_default_value($entity, $field, $instance, $langcode) {
    +
    +  $value = '';
    

    Nitpick: extraneous newline. This happens in *many* locations, between the function signature and the first line of the function body.

    +++ b/core/modules/datetime/datetime.moduleundefined
    @@ -0,0 +1,1118 @@
    + * The default time for a date without time can be anything, so long as it is
    + * consistently applied. If we use noon, dates in most timezones will have the
    + * same value for in both the local timezone and UTC.
    

    Hard to parse :) Especially the last part, "will have the same value for in both".

    +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/formatter/DatetimeDefaultFormatter.phpundefined
    @@ -0,0 +1,133 @@
    + *     "format_type" = "medium",
    

    Nitpick: trailing comma.

    +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/widget/DatetimeDatelistWidget.phpundefined
    @@ -0,0 +1,213 @@
    +  /**
    +   *
    +   *
    +   * @param array $form
    +   *   The form definition as an array.
    +   * @param array $form_state
    +   *   The current state of the form as an array.
    +   *
    +   * @return array
    +   *
    +   */
    +  function settingsForm(array $form, array &$form_state) {
    

    Nitpicky: single line description missing :)

    +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/widget/DatetimeDatelistWidget.phpundefined
    @@ -0,0 +1,213 @@
    +        1 => t('1 minute'),
    +        5 => t('5 minute'),
    +        10 => t('10 minute'),
    +        15 => t('15 minute'),
    +        30 => t('30 minute')),
    

    Nitpick: s/minute/minutes/ ?

    +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/widget/DatetimeDefaultWidget.phpundefined
    @@ -0,0 +1,139 @@
    + * Plugin implementation of the 'datetime_default' widget.
    + *
    + * @Plugin(
    

    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?

    swentel’s picture

    Well, 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 :)

    cosmicdreams’s picture

    so if #142 can sufficiently be addressed can this patch be cleared for landing?

    nevergone’s picture

    The last submitted patch, date-501428-139.patch, failed testing.

    effulgentsia’s picture

    Status: Needs work » Needs review
    FileSize
    124.12 KB

    Just a no-op reroll so that it applies to HEAD.

    effulgentsia’s picture

    FileSize
    695 bytes
    124.95 KB

    This fixes the test failures, but doesn't address #142.

    effulgentsia’s picture

    So, what comes back on the entity from entity_form_submit_build_entity() isn't necessarily ready for displaying nor editing.

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

    mgifford’s picture

    FileSize
    156.38 KB

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

    arlinsandbulte’s picture

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

    mgifford’s picture

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

    falcon03’s picture

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

    renat’s picture

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

    renat’s picture

    Sorry, restore tags, somehow unassigned in previous message.

    arlinsandbulte’s picture

    After 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

    swentel’s picture

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

    mgifford’s picture

    Yes, 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?

    effulgentsia’s picture

    Status: Needs review » Needs work

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

    swentel’s picture

    Status: Needs work » Needs review
    FileSize
    944 bytes
    125.11 KB

    Good point. This should fix it, as of Chrome 24, seconds are omitted in case the input is empty.

    tim.plunkett’s picture

    Status: Needs review » Reviewed & tested by the community

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

    Wim Leers’s picture

    Yay! Awesome work, and glad to hear #142.2 was an easy fix :)

    Dries’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed to 8.x. Thanks everyone for all the hard work!

    jibran’s picture

    Title: Date and time field type in core » Change notice: Date and time field type in core
    Assigned: KarenS » Unassigned
    Category: feature » task
    Priority: Major » Critical
    Status: Fixed » Active
    Issue tags: +Needs change record

    I think it also needs CHANGELOG.txt entry

    sun’s picture

    Component: field system » datetime.module
    dawehner’s picture

    Status: Active » Needs review
    FileSize
    439 bytes

    Here is patch for the changelog entry.

    dww’s picture

    Congratulations 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

    effulgentsia’s picture

    Status: Needs review » Reviewed & tested by the community

    #166 looks good.

    swentel’s picture

    Added to the field types change record as well http://drupal.org/node/1796546

    jibran’s picture

    Title: Change notice: Date and time field type in core » Date and time field type in core
    Priority: Critical » Normal
    Issue tags: -Needs change record

    Added date module link to change record.

    Dries’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed to 8.x. Thanks!

    jstoller’s picture

    I just posted #1920778: Field description doesn't render on datetime field, which appears to come out of this issue.

    Status: Fixed » Closed (fixed)

    Automatically closed -- issue fixed for 2 weeks with no activity.

    yched’s picture

    Sorry 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 ? ? :-)

    Kiphaas7’s picture

    Also shamelessly pinging people for the following UI problem:
    #2050103: interaction with date field is confusing

    Kiphaas7’s picture

    Issue summary: View changes

    Adding follow up issues for timepicker and views

    MustangGB’s picture

    What happened to 'Time only'?

    klonos’s picture