Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the FormCache code hardcodes a magic value to govern how long a form can cache an object.
Issue priority Minor because no functionality has been changed. The "fix" here provides programmers a better way to override the magic value being set. This improves DX by not requiring that developers re-implement large sections of code and instead just override the object's constant.
Unfrozen changes Unfrozen because it only changes the storage location of the magic value being used to govern how long a form can be cached.
Prioritized changes This is not a prioritized change for the beta phase.
Disruption This is not a disruptive change. Also, because we're changing the internal logic of an object, and not an external API, this change could be made during the 8.1.x developer time period. It's just a troublesome reality to live with from a DX perspective.

Original description

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 .

CommentFileSizeAuthor
#106 interdiff-1286154-102-106.txt769 bytesmcdruid
#106 1286154-configurable-cache-form-expiration-106.patch2.83 KBmcdruid
#104 1286154-configurable-cache-form-expiration-104-test-only.patch1.18 KBmcdruid
#102 interdiff-1286154-99-102.txt750 bytesmcdruid
#102 1286154-configurable-cache-form-expiration-102.patch2.82 KBmcdruid
#99 interdiff-1286154-93-99.txt1.12 KBmcdruid
#99 1286154-configurable-cache-form-expiration-99.patch2.9 KBmcdruid
#93 interdiff-1286154-90-93.txt959 bytesmcdruid
#93 1286154-configurable-cache-form-expiration-93.patch1.64 KBmcdruid
#92 interdiff-1286154-90-92.txt844 bytesmcdruid
#92 1286154-configurable-cache-form-expiration-92.patch1.64 KBmcdruid
#89 1286154-configurable-cache-form-expiration-89.patch1.95 KBmcdruid
#86 1286154-configurable-cache-form-expiration-86.patch1.92 KBCameron Tod
#81 1286154-configurable-cache-form-expiration-81.patch1.05 KBmcdruid
#77 1286154-configurable-cache-form-expiration-77.patch7.03 KBmcdruid
#76 1286154-configurable-cache-form-expiration-75.patch6.45 KBmcdruid
#68 1286154-configurable-cache-form-expiration-68.patch6.67 KBCameron Tod
#68 interdiff.txt755 bytesCameron Tod
#66 1286154-configurable-FormCache-expiration-66.patch6.59 KBCameron Tod
#62 1286154_63.patch6.4 KBcosmicdreams
#59 1286154_59.patch4.28 KBcosmicdreams
#59 interdiff.txt2.46 KBcosmicdreams
#56 interdiff.txt442 bytescosmicdreams
#56 1286154_56.patch2.03 KBcosmicdreams
#53 1286154_52.patch2.04 KBcosmicdreams
#51 1286154_51.patch1.56 KBcosmicdreams
#49 1286154_48.patch1 KBcosmicdreams
#40 1286154_41.patch1.03 KBcosmicdreams
#39 1286154_39.patch1.06 KBcosmicdreams
#38 1286154_37.patch1.08 KBcosmicdreams
#33 1286154_33.patch927 bytescosmicdreams
#23 drupal-allow-custom-form-cache-expiration-1286154-23.patch6.05 KBheddn
#17 drupal-allow-custom-form-cache-expiration-1286154-17.patch5.72 KBdas-peter
#16 drupal-allow-custom-form-cache-expiration-1286154-16.patch5.72 KBdas-peter
#13 drupal-allow-custom-form-cache-expiration-1286154-13.patch5.25 KBdas-peter
#11 drupal-allow-custom-form-cache-expiration-1286154-11-d8.patch5.25 KBdas-peter
#9 drupal-allow-custom-form-cache-expiration-1286154-9.patch5.23 KBdas-peter
#6 drupal-allow-custom-form-cache-expiration-1286154-6.patch5.23 KBdas-peter
#3 drupal-allow-custom-form-cache-expiration-1286154-3.patch4.52 KBdas-peter
drupal-allow-custom-form-cache-expiration.patch4 KBdas-peter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

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

dawehner’s picture

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.

das-peter’s picture

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

das-peter’s picture

Status: Needs work » Needs review

Testbot: your turn.

xjm’s picture

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:

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

das-peter’s picture

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.
das-peter’s picture

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.

das-peter’s picture

Status: Needs work » Needs review
FileSize
5.23 KB

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

xjm’s picture

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?

das-peter’s picture

Status: Needs work » Needs review
FileSize
5.25 KB

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

xjm’s picture

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.

das-peter’s picture

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

xjm’s picture

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

xjm’s picture

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

das-peter’s picture

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

das-peter’s picture

Fixed typo in the code.

regilero’s picture

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.

geerlingguy’s picture

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

andrewbelcher’s picture

+1 to this as it could be a nice solution to #774876: Form cache entries can expire before their host page's cache entry, resulting in all AJAX failing 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!

iamEAP’s picture

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.

heddn’s picture

Issue summary: View changes
Status: Closed (fixed) » Needs review
FileSize
6.05 KB

I actually need this functionality in D7. Reroll of #1286154-9: Allow custom default form cache expiration/lifetime

Status: Needs review » Needs work

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

heddn’s picture

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

I should really set this to the correct version.

heddn’s picture

cosmicdreams’s picture

Also, D8 still uses a magic value to govern form cache lifetime. That's a code smell.

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

Exactly what needs to be done.

cosmicdreams’s picture

I understand that this issue doesn't have much visibility because for many, they think this issue can be resolved by overriding and implementing the FormCache that has hard-coded value ($expire, locally to the setCache function; With the hilarious comment "6 hours cache life time for forms should be plenty.") A developer needs to re-implement either one or all methods in the class in order to fix that one configuration value.

That makes no sense.

Why can't we just make the magic number a class constant or property and let developers decide what cache expiry life time should be plenty.

http://stackoverflow.com/questions/13613594/overriding-class-constants-v...

heddn’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Reviewed & tested by the community » Needs work

This needs to get fixed on D8 first per the backport policy. Reset the project version.

tstoeckler’s picture

In D8 you can swap the FormCache service. It would be nice if that had a $cacheLifetime property that you could simply override, but not sure if that should be done in this issue as the code in D7 and D8 is so vastly different.

cosmicdreams’s picture

Yes, you can swap out the service. Yes that would solve the issue. But come on, we're storing a magic value for the form cache lifetime (with a comment that is equivalent to 6hrs should be enough for everyone). Well its not.

All that is needed is to make that magic value a constant or a property so you can extend the FormCache object and override the value.

cosmicdreams’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
927 bytes

Here's a patch that creates this constant. Any preference on the value being stored in a constant or a property?

cosmicdreams’s picture

Status: Reviewed & tested by the community » Needs review

whoops, meant to put it in needs review mode.

tstoeckler’s picture

I think a constant makes sense in this case. We often put constant like this on the respective interface i.e. FormCacheInterface here. Since the interface itself is related to caching an expiration time seems like it might be not specific to the specific implementation but in fact have a value on the interface. I'm not sure, though. What do you think?

Some notes on the patch, regardless:

  1. +++ b/core/lib/Drupal/Core/Form/FormCache.php
    @@ -24,6 +24,7 @@
    +  const FORM_CACHE_DEFAULT_LIFETIME = 21600;
    

    This should be documented in some way.

    Also, since we're already in the FormCache class, I think DEFAULT_LIFETIME should be fine. Also, since we don't allow changing it in any way, why default? Seems like const LIFETIME is sufficiently explicit, no?

  2. +++ b/core/lib/Drupal/Core/Form/FormCache.php
    @@ -189,7 +190,7 @@ protected function loadCachedFormState($form_build_id, FormStateInterface $form_
         // 6 hours cache life time for forms should be plenty.
    -    $expire = 21600;
    +    $expire = static::FORM_CACHE_DEFAULT_LIFETIME;
    

    The comment can be removed now and (maybe?) moved to the constant documenation, also "should be plenty" is not really valuable information in any sense of the word, so we can maybe just drop it.

Also, now that we're targeting D8, this will need a beta evaluation template. There's really no disruption so we should definitely be able to get this in, but we have to write it up, I'm afraid.

cosmicdreams’s picture

Issue summary: View changes
tstoeckler’s picture

Issue summary: View changes

Uncommenting the beta evaluation. :-)

cosmicdreams’s picture

Issue summary: View changes
FileSize
1.08 KB

Fixing description and adding a beta evaluation header.

@1: I like the name. It's appropriately verbose as I don't think there's any confusion as to what it's for. Plus, that's what the D7 patch used.

@2: removed the hilarious comment in setCache, and added a descriptive comment to where the constant is being defined.

cosmicdreams’s picture

FileSize
1.06 KB

revised patch that reduces the verbosity of the constant.

cosmicdreams’s picture

FileSize
1.03 KB

Improved code comment and code cleanup after review.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests, -Needs issue summary update

Thanks a lot!

David_Rothstein’s picture

Issue tags: +Needs backport to D7

I agree this is needed for Drupal 8 as much as Drupal 7 (it was mentioned above that you can work around it in Drupal 8 by overriding class methods, but you can also do that in Drupal 7 via the swappable cache API).

The patch looks like an improvement, but why a class constant rather than a config setting? It seems a LOT less flexible to me. To override this I'd still need to write code that swaps out the service and replaces it with my own, all just to override one single number... And by doing that I'd also conflict with any other code that wanted to swap out the form builder service for a more legitimate reason, i.e. to make actual meaningful changes to it.

A config setting seems both simpler and more flexible - you could then change this value without writing any code, by using Drush (for example) to change the config. It would also be in line with how to do this for Drupal 7 (there we'd just add a variable_get()).

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review

Note that you don't need to swap the entire FormBuilder but just the FormCache service, for which there is less justification.

Let's get some more opinions on this, though.

heddn’s picture

I think I would vote for this being a config change as well. That's how the patch is written for D7 in #23. It is, after all, configuration.

cosmicdreams’s picture

The only argument against a config setting is that this setting would be the only reason the object has to activate the config system. And the audience I'm coding it for, the coworker that sits next me, is fine with modern PHP techniques like overriding a class and setting a constant.

@#44: If we make this variable a constant a developers has to do FAR less work in order to change the value of a simple variable. Plus, hard-coding magic variables is a code smell that we should endeavor to fix if there is the intent of overriding the object.

@#43: Yes, we could store this value in the config system. But what kind of config? https://www.drupal.org/node/2120523 points to State and the standard Config systems has been good candidates. This value seems as if it would be infrequently modified. To me it sounds like this is State config.

And remember, this value previously had a code comment that said 6hrs should be enough for everyone and to fix this one value we have override and reimplement whole methods within the FormCache object. So having the value be a State config is a much better situation.

cosmicdreams’s picture

Actually, I take some of that back. It looks like the FormCache object is already pulling config and configFactory is instantiated in the object. So we will just need to pull this specific config.

However, the question of State vs regular config should be considered.

David_Rothstein’s picture

Note that you don't need to swap the entire FormBuilder but just the FormCache service, for which there is less justification.

Oops, you are right - I was misreading which class it was. I agree there is less justification to swap that out than the whole form builder service, but still seems like something a contrib module might have a reason to do.

@#43: Yes, we could store this value in the config system. But what kind of config? https://www.drupal.org/node/2120523 points to State and the standard Config systems has been good candidates. This value seems as if it would be infrequently modified. To me it sounds like this is State config.

I would definitely say Config over State. This is a change to how your site behaves (that you might want to test out on a development server before pushing live) and as far as I understand that is what Config is for. I agree it will be infrequently modified but I don't think that matters for deciding whether something is config or not.

cosmicdreams’s picture

FileSize
1 KB

Here's a patch that uses config to see if I'm doing it right.

Status: Needs review » Needs work

The last submitted patch, 49: 1286154_48.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
1.56 KB

Ah, didn't include the new config into the system.schema.yml

Status: Needs review » Needs work

The last submitted patch, 51: 1286154_51.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
2.04 KB

Here's a more defensive way of coding this. Apparently it wasn't getting the value of my new config file. I'll need to look into that.

Status: Needs review » Needs work

The last submitted patch, 53: 1286154_52.patch, failed testing.

cosmicdreams’s picture

Hi my name is Chris Weber and I'm looking at this issue at the extended sprint for Drupalcon LA.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
2.03 KB
442 bytes

Every other config listed in the system.schema.yml uses config_object as it's type. So maybe that's the only fix I need to do.

heddn’s picture

Issue tags: +Needs tests

This is looking good now. Just need tests.

Status: Needs review » Needs work

The last submitted patch, 56: 1286154_56.patch, failed testing.

cosmicdreams’s picture

FileSize
2.46 KB
4.28 KB

Looks like that the value is being used. Even thought the test bot failed, many tests actually ran and they seem to be reporting that I'm trying to use the configFactory before it's instantiated and they're right.

So, I modified the service definition of FormCache to include it and let's see what happens.

cosmicdreams’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 59: 1286154_59.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
6.4 KB

I expected that the test would fail, because I hadn't actually fixed the test. This patch does.

Status: Needs review » Needs work

The last submitted patch, 62: 1286154_63.patch, failed testing.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Form/FormCache.php
@@ -184,8 +194,13 @@ protected function loadCachedFormState($form_build_id, FormStateInterface $form_
+    $system_form = $this->configFactory->get('system.form');
+    if (empty($system_form)) {
+      $expire = static::CACHE_LIFETIME;
+    }
+    else {
+      $expire = $system_form->get('cache_lifetime');
+    }

If we choose to do this, I'm pretty sure it could be
$expire = $this->configFactory->get('system.form')->get('cache_lifetime', static::CACHE_LIFETIME);

David_Rothstein’s picture

I marked #2091511: Make cache_form expiration configurable, to mitigate runaway cache_form tables as a duplicate - note that it has some patches for Drupal 7 and 8 that are newer (but that take a bit of a different approach than the latest patch here).

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
6.59 KB

I don't think get can accept a default as a second argument as described in #64. Rerolling.

Status: Needs review » Needs work

The last submitted patch, 66: 1286154-configurable-FormCache-expiration-66.patch, failed testing.

Cameron Tod’s picture

This should make the tests go green.

Status: Needs review » Needs work

The last submitted patch, 68: 1286154-configurable-cache-form-expiration-68.patch, failed testing.

Cameron Tod’s picture

Gah, all tests pass locally. Time to dig :(

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mcdruid’s picture

Confirmed that patch from #68 still applies to 8.3.x HEAD, and queued a new test against that branch.

Also noting that I re-opened #2091511: Make cache_form expiration configurable, to mitigate runaway cache_form tables where we can work on the D7 backport (but the change should be made in D8 first).

The last submitted patch, 68: 1286154-configurable-cache-form-expiration-68.patch, failed testing.

xjm’s picture

mcdruid’s picture

Been having a look at this, and I'm not yet sure why the cache_lifetime value is apparently not being picked up when the test runs:

  public function setCache($form_build_id, $form, FormStateInterface $form_state) {
    $args = func_get_args();
    $system_form = $this->configFactory->get('system.form');
    if (empty($system_form)) {
      $expire = static::CACHE_LIFETIME;
    }   
    else {
      $expire = $system_form->get('cache_lifetime');
    } 

AFAICS $system_form is an ImmutableConfig object, but $expire is not being set to the (default) value. Not sure why yet.

Also, is this supposed to be part of the patch or did it perhaps slip in by mistake?

+++ b/core/modules/system/config/schema/system.schema.yml
@@ -123,6 +123,25 @@ system.diff:
           type: integer
           label: 'Number of trailing lines in a diff'
 
+system.filter:
+  type: config_object
+  label: 'Filter settings'
+  mapping:
+    protocols:
+      type: sequence
+      label: 'Allowed protocols'
+      sequence:
+        type: string
+        label: 'Protocol'

AFAICS the system.filter configuration has been moved to a container parameter so even if it's there intentionally (which I doubt) this doesn't look like the correct way to do it any more.

Updated patch with those system.filter lines removed from system.schema.yml - but otherwise the same, so tests are likely to fail.

mcdruid’s picture

I think / hope it was as simple as this:

@@ -40,6 +40,7 @@ class FormCacheTest extends KernelTestBase {
   protected function setUp() {
     parent::setUp();
     $this->installSchema('system', array('key_value_expire'));
+    $this->installConfig('system');

...because KernelTests "can access files and the database, but the entire environment is initially empty" and that seems to include the default config not being installed.

Tests which were failing locally ran okay after this change, so fingers crossed...

mcdruid’s picture

Status: Needs work » Needs review

The last submitted patch, 76: 1286154-configurable-cache-form-expiration-75.patch, failed testing.

cosmicdreams’s picture

+1 from me. I've been lurking here for a long time hoping that this would get done. I'm happy to see it's passing it's tests now.

mcdruid’s picture

Based on a comment from catch over in #2091511-70: Make cache_form expiration configurable, to mitigate runaway cache_form tables could this actually be simplified by using Settings::get instead of a new config?

Attached patch is certainly smaller / simpler. Existing tests pass locally, but this patch doesn't include any new tests for the setting.

catch’s picture

Yes that's exactly what I meant, thanks!

cosmicdreams’s picture

the patch has been reduced to a super simple / trivial patch. I feel confident that it can be marked RTBC now.

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community
tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

I spot-checked a bunch of other Settings::get() calls, and all of the strings are documented in settings.php with commented out declarations.
I think we should do the same in this case.

Sorry to unRTBC! It is an elegant solution.

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
1.92 KB

I've added a slightly tweaked comment from mcdruid's patch to #2091511: Make cache_form expiration configurable, to mitigate runaway cache_form tables.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

David_Rothstein’s picture

This could use the same (minor) changes I just made to the Drupal 7 patch at #2091511-92: Make cache_form expiration configurable, to mitigate runaway cache_form tables ("expiry" => "expiration" and "AJAX" => "Ajax").

mcdruid’s picture

Made the suggested changes (somewhat reluctantly, as a Brit ;)

s/expiry/expiration
s/AJAX/Ajax

Could someone who knows the D8 cache / form APIs well confirm that the comments in settings.php (which came from the D7 patch) still make sense?

anavarre’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs review » Needs work
+++ b/sites/default/default.settings.php
@@ -440,6 +440,20 @@
+# $conf['form_cache_expiration'] = 21600;

This should be $settings

+++ b/sites/default/default.settings.php
@@ -440,6 +440,20 @@
+ * Busy sites can encounter problems with the cache_form table becoming very

In-progress forms are now stored in key_value. Overall, the help text should probably be refactored with the guidance of a Form API maintainer.

catch’s picture

I don't think this is relevant any more in 8.x, we've massively reduced the number of cache_form/state entries, in issues like #2263569: Bypass form caching by default for forms using #ajax..

There's a question as to whether we should still make the 6 hours configurable though so not closing just yet, but at least the comment would need to be updated and there shouldn't be a scaling issue here any more.

mcdruid’s picture

Oops, this shouldn't be $conf in D8; new patch (interdiff still against 89).

[edit] double oops; noticed I've been naming the interdiffs using 90 instead of 89, not going to fix that.

anavarre’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies successfully. With the settings.php override commented out, I'm able to manually pass an arbitrary value:

>>> \Drupal\Core\Site\Settings::get('form_cache_expiration');
=> null
>>> \Drupal\Core\Site\Settings::get('form_cache_expiration', 3600);
=> 3600

When it's commented in, I get

>>> \Drupal\Core\Site\Settings::get('form_cache_expiration');
=> 21600
>>> \Drupal\Core\Site\Settings::get('form_cache_expiration', 3600);
=> 21600

And then, upon changing the value in settings.php I get:

>>> \Drupal\Core\Site\Settings::get('form_cache_expiration');
=> 3600

Now, to actually test in-progress forms I performed the following:

Changed the override to 1mn in settings.php

$settings['form_cache_expiration'] = 60;

To start fresh, made sure key_value_expire is empty.

mysql> TRUNCATE key_value_expire;
Query OK, 0 rows affected (0.04 sec)
mysql> SELECT * FROM key_value_expire;
Empty set (0.00 sec)

Then rebuild caches:

$ drush cr

Next I gave anonymous users the permission to comment on any node, created a node, then visited /node/1, and, in an incognito window, entered 'Drupal' in the body and hit 'Preview'.

Querying the key_value_expire table now returns:

mysql> SELECT collection,name,expire FROM key_value_expire;
+------------+--------------------------------------------------+------------+
| collection | name                                             | expire     |
+------------+--------------------------------------------------+------------+
| form       | form-NZzCKpXvJ_fqki5kAS6QP3x2TGd3wGpAsvIKw8RKeD0 | 1497612091 |
| form_state | form-NZzCKpXvJ_fqki5kAS6QP3x2TGd3wGpAsvIKw8RKeD0 | 1497612091 |
+------------+--------------------------------------------------+------------+
2 rows in set (0.00 sec)

mysql> SELECT COUNT(*) FROM key_value_expire WHERE value LIKE '%Drupal%';
+----------+
| COUNT(*) |
+----------+
|        2 |
+----------+
1 row in set (0.00 sec)

Waited a few minutes, then ran cron to get the garbage collection to kick in.

Querying the table now returns nothing:

mysql> SELECT COUNT(*) FROM key_value_expire WHERE name LIKE "form-%" AND value LIKE '%Drupal%';
+----------+
| COUNT(*) |
+----------+
|        0 |
+----------+
1 row in set (0.00 sec)

I'm going to RTBC this because it works as anticipated in my testing.

naveenvalecha’s picture

RTBC +1
I have read the patch and thread. Also added the change record for this issue as this is the new setting getting introduced. https://www.drupal.org/node/2886836

//Naveen

anavarre’s picture

Ah yes, good idea. Tried to clarify the CR as well.

catch’s picture

Status: Reviewed & tested by the community » Needs work

This should have some test coverage that actually changing the setting affects the expiry timestamp on the cache item.

mcdruid’s picture

Added a test which stores a form in cache, but with an overridden form_cache_expiration which means the entry is already expired.

This is a similar approach to \Drupal\KernelTests\Core\KeyValueStore\DatabaseStorageExpirableTest::testExpiration

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
+++ b/core/tests/Drupal/KernelTests/Core/Form/FormCacheTest.php
@@ -101,4 +102,18 @@ public function testNoCacheToken() {
+    new Settings(['form_cache_expiration' => -1 * (24*60*60), 'hash_salt' => $this->randomMachineName()]);

I love it :)

Thanks!

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/tests/Drupal/KernelTests/Core/Form/FormCacheTest.php
@@ -101,4 +102,18 @@ public function testNoCacheToken() {
+   */
+  public function testCacheCustomExpiration() {
+    \Drupal::currentUser()->setAccount(new UserSession(['uid' => 1]));
+

Why does this need uid = 1 as the current user?

mcdruid’s picture

Thanks, you're right - it doesn't need that (blame copypasta).

catch’s picture

Thanks, looks good to me - it'd be great to have a test-only patch to see the assertion fail, otherwise RTBC I think.

mcdruid’s picture

Good idea; here's the test only, which should fail.

Status: Needs review » Needs work

The last submitted patch, 104: 1286154-configurable-cache-form-expiration-104-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mcdruid’s picture

Test failed as expected.

That also highlighted a codesniffer whitespace nitpick in the test, which I've fixed:

-    new Settings(['form_cache_expiration' => -1 * (24*60*60), 'hash_salt' => $this->randomMachineName()]);
+    new Settings(['form_cache_expiration' => -1 * (24 * 60 * 60), 'hash_salt' => $this->randomMachineName()]);

No other changes.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Back to RTBC.

cilefen’s picture

Title: Allow custom form cache expiration/lifetime » Allow custom default form cache expiration/lifetime

I am retitling this for accuracy and updating credit.

  • cilefen committed 4a629e3 on 8.4.x
    Issue #1286154 by mcdruid, cosmicdreams, das-peter, Cameron Tod, heddn,...
cilefen’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4a629e3 and pushed to 8.4.x and published the change record. Thanks everyone!

cilefen’s picture

Status: Fixed » Patch (to be ported)
cilefen’s picture

Issue tags: +8.4.0 release notes
naveenvalecha’s picture

Status: Patch (to be ported) » Fixed
Issue tags: -Needs backport to D7

Marking it as fixed as we have a separate issue for Drupal 7 #2091511: Make cache_form expiration configurable, to mitigate runaway cache_form tables

Status: Fixed » Closed (fixed)

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