Allow more popup formats

In scheduler_admin_validate() the $acceptable array can therefore be defined as:

    // The Date Popup function date_popup_time_formats() only returns the values
    // 'H:i:s' and 'h:i:sA' but Scheduler can accept more variations than just
    // these. Firstly, we add the lowercase 'a' alternative. Secondly timepicker
    // always requires hours and minutes, but seconds are optional.
    $acceptable = array('H:i:s', 'h:i:sA', 'h:i:sa', 'H:i', 'h:iA', 'h:ia');

Then in _scheduler_strtotime() instead of using the fixed constant SCHEDULER_DATE_FORMAT, we can use the following to derive the format in which date_popup will return the string:

      // Date Popup currently returns the value into the form using its default
      // format DATE_FORMAT_DATETIME but reduced to the same granularity as the
      // requested format. Until issue drupal.org/node/1855810 is
      // resolved we can use the date_popup functions date_format_order() and
      // date_limit_format() to derive the format of the returned string value.
      // This allows Scheduler to make use of the full variety of time formats
      // for Date Popup and not be limited to just 'H:i:s' and 'h:i:sA'
      $granularity = date_format_order($date_format);
      $date_format = date_limit_format(DATE_FORMAT_DATETIME, $granularity);

Patch against 1.1+35
Jonathan

Following from a bug discovered in #1069668: Default time with user override comment #68 when using date_popup with no time entry, we need one extra line in this patch. In addition to deriving $date_format as described above, we need to do the same for $date_only_format

      $date_only_format = date_limit_format(DATE_FORMAT_DATETIME, array('day', 'month', 'year'));

Patch rolled against 1.1+38

Here's the interdiff to show what has actually been changed.

Upping the priority, because this now fixes a bug in the committed code for #1069668: Default time with user override. If this patch gets done, we get two greens for the price of one.

Read https://drupal.org/node/1069668#comment-8170819 comments #68 and #69 (ignore #70 to #75 as that was someone hijacking the issue to report a different problem)

2: 2123103_2.more_popup_formats.patch queued for re-testing.

Thanks for the review.

The description alongside the "Date format" field needs to be updated. It still mentions that the options "a" and "A" are not allowed with the date picker. The sentence "If you are using Date Popup..." still uses the original list of supported time formats, but maybe it can be removed? The validation message is very complete, and the description seems a bit long.

We think along very similar lines, and I have already coded each of these changes in #2123223: Improve the configuration page text and add links

I looked into allowing periods as separator as this is also popular in some countries, but this is not supported by the Date Popup module.

With this enhancement they are now! The beauty of it is that we parse the text string returned from date_popup in exactly the way that date_popup wants, then internally Scheduler redisplays the date with our format. Dots are fine:

and you get

Thank you for taking care of the tests. I've learned a lot from your test work, so should try writing my own next time. Are you going to use the new format with the the separate .test file? That works well because the tests are split up.

Did you mean dots in the time format? They are not supported by date_popup if you use the jQuery timepicker. But they are ok if using popup for the date but plain text entry for the time. We could allow these in the validation but convert . to : behind the scenes at the point of entry if using the timepicker.

I will look at allowing . and also - in the time part. If it is a trivial fix then it would be good to add, but if it is compex I agree it is not a blocker and we can cope without it.

As for 'h' that would be complex to do, because that letter is used as the standard php replacement token for hours.

I looked at it but it is a bit messy to code right now. Yes I agree, lets leave the actual changes as they are in the patch (way back in #2). If/when #1855810: Define #return_value_format for date_popup to set the form value gets committed then a change to allow . and - will be trivial. For the record, this is what it would need in scheduler_admin_validate()

    $time_format_to_check = str_replace(array('.','-'), ':', $time_format);
    if ($time_format_to_check && !in_array($time_format_to_check, $acceptable)) {
      form_set_error('scheduler_date_format', t('When using the Date Popup module, the allowed time formats are: !formats. You can also use - or . as separators', array('!formats' => implode(', ', $acceptable))));
    }

So we are both happy with the changes to .module. What kind of tests were you thinking about?

Jonathan

OK. If all the tests are going to be done in other issues, is this one ready for committing? I presume it would be good to get this in, then the tests can be developed more easily. I can then help with the tests.

All looks good, and the tests are a great addition. One minor point, there are six time formats in the new $allowed array, but you have only tested five of them. For completeness, would it be good to add 'h:iA' in the group labelled 'Test a number of supported date formats'?

When you've done, it will be RTBC, as you have reviewed my .module changes in #6.

Jonathan

Hi vitalius2009,

This issue was closed nearly three years ago. Your idea is reasonable but it should be on a new issue.

After uploading the patch on the new issue, remember to set the status to 'needs review' so that automated testing will be triggered.

Jonathan