The admin form and config settings for specifying a default time have been converted for 8.x. However, the actual values entered are not used anywhere as this functionality has not been converted yet. When attempting to enter just a date, and use the default time suggested in the description we get the validation error:

The Publish on date is invalid. Please enter a date in the format 2016-01-05 17:32:09.

The 7.x code which did this was in _scheduler_strtotime() http://cgit.drupalcode.org/scheduler/tree/scheduler.module?h=7.x-1.x#n259

CommentFileSizeAuthor
#11 2644690-11-interdiff-5-to-11.txt3.01 KBjonathan1055
#11 2644690-11.default-time-processing.patch3.47 KBjonathan1055
#5 2644690-5.default-time-processing.patch2.57 KBjonathan1055
#2 2644690-1.default-time-processing.patch4.3 KBjonathan1055

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review
StatusFileSize
new 2644690-1.default-time-processing.patch4.3 KB

It has taken a while to investigate but I have a working version of the default-time processing. The change is small, it required a #value_callback function on our customised datetime widget. I tried to do the manipulation in the existing massageFormValues function, but that is too late - the validation has already been done and if a time is omitted the error message is already created. The valueCallback method works on user input before validation, which is what we need.

In this patch I've placed the callback function as a normal helper function in .module but it would be nicer if it was contained within the TimestampDatetimeNoDefaultWidget class. However, when I tried this, I got errors about uninitialised 'date' offset in datetime.php, and the new callback did not seem to be executed. I also tried creating a function named valueCallback in the TimestampDatetimeNoDefaultWidget class, hoping that this would be called instead of the standard Datetime::valueCallback, but it was not being executed. Maybe someone with more OO experience will be able to help on this. I've left comments about this in the code.

Status: Needs review » Needs work

The last submitted patch, 2: 2644690-1.default-time-processing.patch, failed testing.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

Good. As I hoped, this now fixes the date tests entirely. 

2		Scheduler.Drupal\scheduler\Tests\SchedulerDateCombinedFunctionalTest
✓		- setUp
✓		- testDefaultTime

My manual testing was working correctly too, so functionally this is now operating as required. But it would be better to have the callback included in the widget class, so any help on how to get that working would be appreciated.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 2644690-5.default-time-processing.patch2.57 KB

I've now got the correct syntax for #value_callback to be a function within the same class:

    $element['value']['#value_callback'] = array($this, 'valueCallback');

Code is much neater, now all contained within TimestampDatetimeNoDefaultWidget

Status: Needs review » Needs work

The last submitted patch, 5: 2644690-5.default-time-processing.patch, failed testing.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

Same as before, this fixes SchedulerDateCombinedFunctionalTest.
Setting back to 'needs review' as it would be good to have someone else cast an eye over the code before I commit.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Assigned: Unassigned » pfrenssen
Issue tags: +SprintWeekend2016

Assigning for review as part of the global sprint in Sofia.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Assigned: pfrenssen » Unassigned
Status: Needs review » Needs work 
  1. +++ b/src/Plugin/Field/FieldWidget/TimestampDatetimeNoDefaultWidget.php
@@ -32,14 +32,41 @@ class TimestampDatetimeNoDefaultWidget extends TimestampDatetimeWidget {
+    // @todo Do not these lines. The #description is entirely replaced by custom
+    // text in scheduler_form_node_form_alter()
     $date_format = DateFormat::load('html_date')->getPattern();
     $time_format = DateFormat::load('html_time')->getPattern();

    Let's keep these, this is a generic reusable field widget after all, we might use it outside the node edit form in the future. 

  2. +++ b/src/Plugin/Field/FieldWidget/TimestampDatetimeNoDefaultWidget.php
@@ -32,14 +32,41 @@ class TimestampDatetimeNoDefaultWidget extends TimestampDatetimeWidget {
+    // Set the callback function to allow interception of the submitted user
+    // input and add the default time if needed. It is too late to try this in
+    // function massageFormValues as the validation has already been done.
+    $element['value']['#value_callback'] = array($this, 'valueCallback');

    Using the valueCallback() is a really elegant solution for this problem. Great work! 

  3. +++ b/src/Plugin/Field/FieldWidget/TimestampDatetimeNoDefaultWidget.php
@@ -32,14 +32,41 @@ class TimestampDatetimeNoDefaultWidget extends TimestampDatetimeWidget {
+  public function valueCallback(&$element, $input, FormStateInterface $form_state) {

    This has to be a public static function. 

  4. +++ b/src/Plugin/Field/FieldWidget/TimestampDatetimeNoDefaultWidget.php
@@ -32,14 +32,41 @@ class TimestampDatetimeNoDefaultWidget extends TimestampDatetimeWidget {
+      $date_input  = $element['#date_date_element'] != 'none' && !empty($input['date']) ? $input['date'] : '';
+      $time_input  = $element['#date_time_element'] != 'none' && !empty($input['time']) ? $input['time'] : '';

    Duplicate space before the '='.

jonathan1055’s picture

jonathan1055 commented

Thanks for the review, really helpful.

  1. Yes, but the point I was making is that these lines have been left in when this function was created based on the core version http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Datetime/Plu...
    The only use for $date_format and $time_format is in creating $element['value']['#description'] which we completely overwrite in scheduler_form_node_form_alter(). Maybe there should be a comment saying where this function was copied from? I don't quite see the point in leaving in unused variables and creating a #description which gets overwritten, as it gives the misleading impression that these lines are doing something. But happy to leave them in, provided a comment says that they are currently redundant.
  2. Good, glad you liked it. It took me several hours of research to find out how to do that, so I am pleased it was worth it.
  3. Thanks, I will change that. I am not up-to-speed with why things are static or not static. I will have to learn more about it.
  4. Good spot!. Ha ha I got this code from core http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Datetime/Ele... where four rows had been lined up. I did not notice that.

I will re-roll the patch when I understand what you want in (1). Will be good to get this committed as it fixes a whole test class.

Thanks again for the review. Hope your global sprint in Sofia went well.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review
StatusFileSize
new 2644690-11.default-time-processing.patch3.47 KB
new 2644690-11-interdiff-5-to-11.txt3.01 KB

Thinking about it more, I get your point about not leaving an incorrect #description in the widget, even if that description is not currently being used. I have added a comment to explain this, and addressed the other issues. Also slightly altered the description to say 'for no date' instead of 'to disable'.

Here's the patch and interdiff.

Status: Needs review » Needs work

The last submitted patch, 11: 2644690-11.default-time-processing.patch, failed testing.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

Switching back to 'needs review' as I have finished the changes. Unless anyone has further questions/suggestions/review observations I think this is ready to commit.

  • jonathan1055 committed b11b100 on 8.x-1.x 
    Issue #2644690 by jonathan1055: Include default time processing in...
jonathan1055’s picture

jonathan1055 commented
Title: Convert Default Time processing to 8.x » Include default time processing in datetime widget for 8.x
Status: Needs review » Fixed

It's been a month with no objections, so decided to commit the patch from #11. I will make adjustments if required, but everything in the review in #9 was addressed.

Nice to get another test class fully passing.

pfrenssen’s picture

pfrenssen
Location Sofia
commented

Just did a quick review. Looking good!

Status: Fixed » Closed (fixed)

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