Currently the form cache has a lifetime of 6 hours by default.
I'd like to see the possibility to define a cache lifetime by using relative date formats like in php's strtotime.
Further it would be nice to be able to set the form cache expiration per form - e.g. with $form_state['cache_expire'] (Another solution could be to store this information straight into $form_state['cache'])

The goal I've in mind is to be able to get the lifetime of the page cache and the form cache in sync (The page cache is a whole different story ;) )
The scenario I think of is a website where all the pages, including the forms on it, are cacheable until tomorrow (midnight). But if the form cache is cleared before the page cache things break e.g. the ajax magic.

The attached patch contains the changes needed to introduce a custom form lifetime. The setting can be found in admin/config/development/performance .

Files: 
CommentFileSizeAuthor
#17 drupal-allow-custom-form-cache-expiration-1286154-17.patch5.72 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 33,970 pass(es).
[ View ]
#16 drupal-allow-custom-form-cache-expiration-1286154-16.patch5.72 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 33,798 pass(es).
[ View ]
#13 drupal-allow-custom-form-cache-expiration-1286154-13.patch5.25 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 33,811 pass(es).
[ View ]
#11 drupal-allow-custom-form-cache-expiration-1286154-11-d8.patch5.25 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-allow-custom-form-cache-expiration-1286154-11-d8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 drupal-allow-custom-form-cache-expiration-1286154-9.patch5.23 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 36,880 pass(es).
[ View ]
#6 drupal-allow-custom-form-cache-expiration-1286154-6.patch5.23 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-allow-custom-form-cache-expiration-1286154-6_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#3 drupal-allow-custom-form-cache-expiration-1286154-3.patch4.52 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 36,481 pass(es).
[ View ]
drupal-allow-custom-form-cache-expiration.patch4 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to drop checkout database.
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, drupal-allow-custom-form-cache-expiration.patch, failed testing.

Issue tags:+Needs tests

+++ b/includes/form.incundefined
@@ -503,20 +503,29 @@ function form_get_cache($form_build_id, &$form_state) {
+  $expire = variable_get('form_cache_expire', 21600);

This part could be moved into the condition so it's not loaded all the time.

+++ b/includes/form.incundefined
@@ -503,20 +503,29 @@ function form_get_cache($form_build_id, &$form_state) {
+  if (!is_numeric($expire)) {
+    $expire = strtotime($expire);

Some comment should explain the use case

+++ b/modules/system/system.admin.incundefined
@@ -1673,6 +1673,33 @@ function system_performance_settings() {
+  $form_cache_expire = variable_get('form_cache_expire', 21600);
+  $form['caching']['form_cache_expire'] = array(
+    '#type' => 'select',
+    '#title' => t('Expiration of cached forms'),

In general it should be discussed whether there should be an ui or not.

From my perspective this is a pretty advanced feature like session_inc, which would confuse users and let them input bad stuff, for example an incredible high value.

It seems to be that there is no documentation for session_inc on drupal itself, but i guess it can be documented in form.inc

-4 days to next Drupal core point release.

Add a tag as well.

StatusFileSize
new4.52 KB
PASSED: [[SimpleTest]]: [MySQL] 36,481 pass(es).
[ View ]

Thank you very much for the review.

This part could be moved into the condition so it's not loaded all the time.

Done

Some comment should explain the use case

Done - however I think someone with better English knowledge should check if this is understandable.

In general it should be discussed whether there should be an ui or not.

Indeed, I've just added it since I thought it wouldn't make the current ui more complicated anyway ;)

Status:Needs work» Needs review

Testbot: your turn.

Status:Needs review» Needs work
  1. +++ b/includes/form.incundefined
    @@ -501,22 +501,44 @@ function form_get_cache($form_build_id, &$form_state) {
    + * Besides a numeric value the expiration option accepts relative date formats
    + * like for php's strtotime, too.
    + * Using relative date formats enables you to set the cache expiration to a time
    + * independent from the time the cache is build.
    + * E.g. if you use 'tomorrow' as expiration time, the cache will expire next
    + * midnight independent if the cache was built 10am or 11:55pm.

    This wraps early at the end of each sentence. It would be better to continue with whatever text fits until the end of the line.

    I think it could also be clarified a little. How about:

    The form cache expiration (set in $form_state['cache_expire']) may be either a numeric value (in seconds) or a relative date format (for use with the PHP strtotime() function). Using a relative date format allows the cache expiration to be set independently of the cache build time. For example, if the expiration time is set to 'tomorrow', the cache will expire at midnight, regardless of whether the the cache was built at 10:00 a.m. or 11:55 p.m.

  2. +++ b/includes/form.incundefined
    @@ -501,22 +501,44 @@ function form_get_cache($form_build_id, &$form_state) {
    function form_set_cache($form_build_id, $form, $form_state) {

    Documentation of these parameters is missing from the docblock. However, that's a pre-existing bug, and will be fixed in #1317620: Clean up API docs for includes directory, files starting with D-G so I wouldn't worry about it for this patch. :)

  3. +++ b/includes/form.incundefined
    @@ -501,22 +501,44 @@ function form_get_cache($form_build_id, &$form_state) {
    +  // Evaluate the cache lifetime.
    +  if (!empty($form_state['cache_expire'])) {
    +    $expire = $form_state['cache_expire'];
    +  }
    +  else {
    +    $expire = variable_get('form_cache_expire', 21600);
    +  }
    +  // Allow relative date formats.
    +  if (!is_numeric($expire)) {
    +    $expire = strtotime($expire);
    +  }
    +  else {
    +    $expire = REQUEST_TIME + $expire;
    +  }

    Let's refactor this a little to handle bad values. Let's define a constant, something like FORM_CACHE_DEFAULT_LIFETIME. Then:

    <?php
     
    // If this form has a custom cache expiration set, use that.
     
    if (!empty($form_state['cache_expire'])) {
       
    $expire = $form_state['cache_expire'];
      }
     
    // Otherwise, retrieve the configured cache lifetime.
     
    else {
       
    $expire = variable_get('form_cache_expire', FORM_CACHE_DEFAULT_LIFETIME);
      }
     
    // If the expiration is non-numeric, assume it is a relative datetime.
     
    if (!is_numeric($expire) && ($strtotime = strtotime($exipre))) {
       
    $expire = $strtotime;
      }
     
    // A numeric expiration time is given as seconds from now.
     
    elseif (is_numeric($expire)) {
       
    $expire = REQUEST_TIME + $expire;
      }
     
    // Fall back to the default.
     
    else {
       
    $expire = REQUEST_TIME + FORM_CACHE_DEFAULT_LIFETIME;
      }
    ?>
  4. +++ b/modules/system/system.admin.incundefined
    @@ -1673,6 +1673,33 @@ function system_performance_settings() {
    +    '#description' => t('Define a <a href="http://www.php.net/manual/en/datetime.formats.relative.php">relative date format</a> like for php\'s <a herf="http://php.net/manual/en/function.strtotime.php">strtotime</a>.'),

    This text is a little unclear. Simpler is better. Here's a first try at rewording it:

    Enter a custom expiration time using a relative datetime format.

    Also, there's a typo in the link attribute (herf).

    Finally, instead of putting the link paths inside the t() string, add them to t() as !arguments. See the format_string() documentation for more information.

  5. +++ b/modules/system/system.admin.incundefined
    @@ -1718,6 +1745,29 @@ function system_performance_settings() {
    + * Validates the custom form expiration and stores it back into the used
    + * variable name

    The summary should be one line less than 80 chars, and should have a period. We probably just need to reword it a little. How about:

    Validates and stores custom form expiration.

    Edit: Just realized this is a form validation handler. We have doxygen standards for validation handlers, but only for the whole form: http://drupal.org/node/1354#forms

    I'll look into this and get back to you regarding it.

  6. +++ b/modules/system/system.admin.incundefined
    @@ -1718,6 +1745,29 @@ function system_performance_settings() {
    +      form_error($element, t('The value of the custom form expiration evaluates to a time in the past. Please check the value.'));

    I'd shorten this text string to, "The form's custom expiration is set to a time in the past."

    Also, we might want to include the field label and user-submitted value in the error message.

StatusFileSize
new5.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-allow-custom-form-cache-expiration-1286154-6_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Thank you very much for the review.

  1. Changed.
  2. Skipped.
  3. Good idea (!) - changed.
  4. Sentence changed, but link path kept in place. The link is language specific and thus I think it should be translated too.
  5. Changed. I used the approach described in the doxygen standards - hope this fits.
  6. Changed.

Status:Needs work» Needs review

*grml* forgot to change the status

Status:Needs review» Needs work

The last submitted patch, drupal-allow-custom-form-cache-expiration-1286154-6.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.23 KB
PASSED: [[SimpleTest]]: [MySQL] 36,880 pass(es).
[ View ]

Looks like I've somehow messed up the patch - next try...

Version:7.x-dev» 8.x-dev
Status:Needs review» Needs work

Ah--good point about the link language. You're right that translators can use the equivalent page on the appropriate php.net subsite .

I also didn't notice before this was filed against 7.x. Can you reroll the patch against 8.x?

Status:Needs work» Needs review
StatusFileSize
new5.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-allow-custom-form-cache-expiration-1286154-11-d8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here we go - my first patch against D8. I hope that works...

Ah, you'll want to rename it so that it can run through testbot. When a patch has a suffix -dN (N a number) then the patches are skipped.

StatusFileSize
new5.25 KB
PASSED: [[SimpleTest]]: [MySQL] 33,811 pass(es).
[ View ]

Ah, I thought with a suffix it's version aware...
Next try then.

+++ b/core/includes/form.incundefined
@@ -90,6 +90,11 @@
/**
+* The default cache lifetime for forms.
+*/

A minor note here -- we're missing a space in front of each asterisk.

+++ b/core/modules/system/system.admin.incundefined
@@ -1719,6 +1746,36 @@ function system_performance_settings() {
+        t('The form\'s custom expiration is set to a time in the past.')

Another minor note here: It would be better to use double quotes so that the apostrophe doesn't need to be escaped. (That makes it a little easier on translators.)

Other than those two small things, the patch is looking nice to me. It would be good to get a couple people testing the functionality, if possible.

+++ b/core/includes/form.incundefined
@@ -90,6 +90,11 @@
+* The default cache lifetime for forms.

Oh, one more thing (sorry!) -- Let's define the units here, and maybe save people arithmetic:

/**
* The default cache lifetime for forms, in seconds.
*
* 21600 seconds is six hours.
*/

26 days to next Drupal core point release.

StatusFileSize
new5.72 KB
PASSED: [[SimpleTest]]: [MySQL] 33,798 pass(es).
[ View ]

All mentioned thing solved.
@xjm: Thank you for your supportive way of reviewing - I very much appreciate that!

StatusFileSize
new5.72 KB
PASSED: [[SimpleTest]]: [MySQL] 33,970 pass(es).
[ View ]

Fixed typo in the code.

Ability to define exceptions on cache duration for each form is nice.

But why not providing also a variable for default cache duration. 6 hours is quite very long. I,ve seen a cache-form with more than 100 000 entries, most of theses entries were comment forms for forums (and this was a very big data set), so effectively in this case targeting these special form could be done, but reducing the default to one hour and adding a bigger time to specific forms (the one used on anonymous cached pages for example) would be simplier.

Status:Needs review» Needs work

This patch will need to be reworked for CMI. And I second the call for a configurable default form cache time; in my case, I have the opposite use case that regilero has in #18 above; on a few smaller sites, where there are only a few users, people often leave tabs open with forms they're working on over the course of a few days, and the form is always expired by the time they click submit. It'd be nice, in those cases, if I could set the sitewide default to something longer in settings.php (or via config in general in D8).

+1 to this as it could be a nice solution to #774876: Form cache entries can expire before their host page's cache entry to be fixed which essentially stops anonymous AJAX forms being viable for cached sites...

Am happy to try and re-work this for CMI if you could provide a pointer for where to read up on how to go about it!

Status:Needs work» Fixed

So this is actually possible now, thanks to #2112807: Move the form builder functions in form.inc to a form service

Steps to do so:

I don't see a needs backport tag, so I'm going to mark this fixed.

Status:Fixed» Closed (fixed)

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