Validate the date format letters and reorganise _scheduler_strptime() [#2187361]

In working on a good solution for #2184517: Fatal error: Call to undefined function strptime() I discovered that we were not validating the letters used in the scheduler date format. When using date_popup the extra letters were working fine and the dates were returned. But switching back to text entry suddenly corrupted the scheduled nodes, ie when editing the node the format being used was not accepted in _scheduler_strptime() and the date had to be re-entered. I also noticed that the date letters being used in scheduler_admin_submit() were not the same as the letters shown in the help description.

To remedy this I have created two new constants

// The full set of date and time letters allowed in the scheduler date format.
define('SCHEDULER_DATE_LETTERS', 'djmnyY');
define('SCHEDULER_TIME_LETTERS', 'hHgGisaA');

These are now used in the help text and for validation in scheduler_admin_validate(). I have also reorganised _scheduler_strptime() so that it is easier to see what its doing. In a later patch I will add new letters (the ones that were accepted by date-popup) but this patch simply makes all the letter lists consistent and checks that we do not allow any additional lettters which are not catered for in _scheduler_strptime()

Comment	File	Size	Author
#5	2187361_interdiff.txt	2.37 KB	jonathan1055
#3	2187361_3.validate_formatting_letters.patch	7.07 KB	jonathan1055
#3
#1	2187361_1.validate_formatting_letters.patch	6.91 KB	jonathan1055
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jonathan1055 CreditAttribution: jonathan1055 commented 2 February 2014 at 20:55

Issue summary:	View changes
Status:	Active	» Needs review

File	Size
2187361_1.validate_formatting_letters.patch	6.91 KB

Patch for testing and comment.

Comment #2

pfrenssen

Sofia

CreditAttribution: pfrenssen commented 23 February 2014 at 14:29

Status:

Needs review

» Needs work

This looks good, the comments add more detail, and the code is much cleaner and easier to read. I only found a couple of coding standards violations.

+++ b/scheduler.module
@@ -264,9 +272,20 @@ function scheduler_admin_validate($form, &$form_state) {
+  $no_punctuation = preg_replace("/[^\w+]/", "", $form_state['values']['scheduler_date_format']);
+  if (preg_match_all("/[^" . SCHEDULER_DATE_LETTERS . SCHEDULER_TIME_LETTERS . "]/", $no_punctuation, $extra)) {

Use single quotes instead of double quotes.

+++ b/scheduler.module
@@ -931,21 +948,43 @@ function _scheduler_strtotime($str) {
+    'd' => '(\d{2})',      /* day of the month with leading zero */
+    'H' => '(\d{2})',      /* hours in 24-hour format with leading zero */
+    'h' => '(\d{2})',      /* hours in 12-hour format with leading zero */
+    'm' => '(\d{2})',      /* month number with leading zero */
+    'i' => '(\d{2})',      /* minutes */
+    'a' => '([ap]m)',      /* lower case meridian */
+    'A' => '([AP]M)',      /* upper case meridian */
+    's' => '(\d{2})',      /* seconds */
+    'y' => '(\d{2})',      /* two-digit year */
+    'Y' => '(\d{4})',      /* four-digit year */
+    'n' => '(\d{1,2})',    /* month number without leading zero */
+    'j' => '(\d{1,2})',    /* day of the month without leading zero */
+    'g' => '(\d{1,2})',    /* hours in 12-hour format without leading zero */
+    'G' => '(\d{1,2})',    /* hours in 24-hour format without leading zero */
+  );

Start these comments with a capital letter and end them in a period. I would personally prefer using // instead of /* */ but that is not strictly against coding standards, so if you prefer to keep it this way then that's fine.

Comment #3

jonathan1055 CreditAttribution: jonathan1055 commented 23 February 2014 at 18:02

File	Size
2187361_3.validate_formatting_letters.patch	7.07 KB

1 file was hidden/shown/deleted

File	Size
2187361_1.validate_formatting_letters.patch	6.91 KB

Well spotted with the double quotes. Yes I prefer the // but somehow I forgot that they could be used within an array (probably due to my programming in a different language from php in my day job). New patch attached. I also changed another double quotes to single quotes in that function.

When this patch is ok, could you mark as RTBC but not commit, then I can practice at making a commit to Scheduler. Thanks

Comment #4

jonathan1055 CreditAttribution: jonathan1055 commented 23 February 2014 at 18:03

Status:

Needs work

» Needs review

Forgot to set status

Comment #5

jonathan1055 CreditAttribution: jonathan1055 commented 30 May 2014 at 06:55

File	Size
2187361_interdiff.txt	2.37 KB

Here's the interdiff to show clearly the previous minor changes. The patch in #3 still applies cleanly.
Jonathan

Comment #6

pfrenssen

Sofia

CreditAttribution: pfrenssen commented 30 May 2014 at 10:25

Status:

Needs review

» Reviewed & tested by the community

Looks good! I have not retested the latest patch, but it contains only comment changes and a double quote to single quote conversion, so it will work equally well as the last one.

Comment #7

1 June 2014 at 13:55

Commit afacacc on 7.x-1.x by jonathan1055:

Issue #2187361 by jonathan1055: Add constants to hold date and time...

Comment #8

jonathan1055 CreditAttribution: jonathan1055 commented 1 June 2014 at 14:02

Status:

Reviewed & tested by the community

» Fixed

Thanks Pieter. Committed.
I made the array $date_entities_and_replacements even clearer by moving the lines so that they are grouped into date elements and time elements, in the same order as in the two new constants.