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 .

Files: 
CommentFileSizeAuthor
#53 1286154_52.patch2.04 KBcosmicdreams
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,871 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#51 1286154_51.patch1.56 KBcosmicdreams
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#49 1286154_48.patch1 KBcosmicdreams
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#40 1286154_41.patch1.03 KBcosmicdreams
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,727 pass(es).
[ View ]
#39 1286154_39.patch1.06 KBcosmicdreams
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,735 pass(es).
[ View ]
#23 drupal-allow-custom-form-cache-expiration-1286154-23.patch6.05 KBheddn
PASSED: [[SimpleTest]]: [MySQL] 41,122 pass(es).
[ View ]

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

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

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:

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

das-peter’s picture

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.
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
StatusFileSize
new5.23 KB
PASSED: [[SimpleTest]]: [MySQL] 36,880 pass(es).
[ View ]

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

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

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.

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

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!

das-peter’s picture

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

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 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
StatusFileSize
new6.05 KB
PASSED: [[SimpleTest]]: [MySQL] 41,122 pass(es).
[ View ]

I actually need this functionality in D7. Reroll of #1286154-9: Allow custom 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
StatusFileSize
new927 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,577 pass(es).
[ View ]

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
StatusFileSize
new1.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,729 pass(es).
[ View ]

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

StatusFileSize
new1.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,735 pass(es).
[ View ]

revised patch that reduces the verbosity of the constant.

cosmicdreams’s picture

StatusFileSize
new1.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,727 pass(es).
[ View ]

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!

cosmicdreams’s picture

pruning file list.

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

StatusFileSize
new1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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
StatusFileSize
new1.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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
StatusFileSize
new2.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,871 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

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.