Beta phase evaluation
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
.
Comment | File | Size | Author |
---|---|---|---|
#106 | interdiff-1286154-102-106.txt | 769 bytes | mcdruid |
#106 | 1286154-configurable-cache-form-expiration-106.patch | 2.83 KB | mcdruid |
#104 | 1286154-configurable-cache-form-expiration-104-test-only.patch | 1.18 KB | mcdruid |
#102 | interdiff-1286154-99-102.txt | 750 bytes | mcdruid |
#102 | 1286154-configurable-cache-form-expiration-102.patch | 2.82 KB | mcdruid |
Comments
Comment #2
dawehnerThis part could be moved into the condition so it's not loaded all the time.
Some comment should explain the use case
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.
Comment #3
das-peter CreditAttribution: das-peter commentedThank you very much for the review.
Done
Done - however I think someone with better English knowledge should check if this is understandable.
Indeed, I've just added it since I thought it wouldn't make the current ui more complicated anyway ;)
Comment #4
das-peter CreditAttribution: das-peter commentedTestbot: your turn.
Comment #5
xjmThis 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:
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. :)
Let's refactor this a little to handle bad values. Let's define a constant, something like FORM_CACHE_DEFAULT_LIFETIME. Then:
This text is a little unclear. Simpler is better. Here's a first try at rewording it:
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.
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: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.
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.
Comment #6
das-peter CreditAttribution: das-peter commentedThank you very much for the review.
Comment #7
das-peter CreditAttribution: das-peter commented*grml* forgot to change the status
Comment #9
das-peter CreditAttribution: das-peter commentedLooks like I've somehow messed up the patch - next try...
Comment #10
xjmAh--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?
Comment #11
das-peter CreditAttribution: das-peter commentedHere we go - my first patch against D8. I hope that works...
Comment #12
xjmAh, 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.Comment #13
das-peter CreditAttribution: das-peter commentedAh, I thought with a suffix it's version aware...
Next try then.
Comment #14
xjmA minor note here -- we're missing a space in front of each asterisk.
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.
Comment #15
xjmOh, one more thing (sorry!) -- Let's define the units here, and maybe save people arithmetic:
26 days to next Drupal core point release.
Comment #16
das-peter CreditAttribution: das-peter commentedAll mentioned thing solved.
@xjm: Thank you for your supportive way of reviewing - I very much appreciate that!
Comment #17
das-peter CreditAttribution: das-peter commentedFixed typo in the code.
Comment #18
regilero CreditAttribution: regilero commentedAbility 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.
Comment #19
geerlingguy CreditAttribution: geerlingguy commentedThis 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).
Comment #20
andrewbelcher CreditAttribution: andrewbelcher commented+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!
Comment #21
iamEAP CreditAttribution: iamEAP commentedSo 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.
Comment #23
heddnI actually need this functionality in D7. Reroll of #1286154-9: Allow custom default form cache expiration/lifetime
Comment #25
heddnI should really set this to the correct version.
Comment #26
heddn23: drupal-allow-custom-form-cache-expiration-1286154-23.patch queued for re-testing.
Comment #27
cosmicdreams CreditAttribution: cosmicdreams commentedAlso, D8 still uses a magic value to govern form cache lifetime. That's a code smell.
Comment #28
cosmicdreams CreditAttribution: cosmicdreams commentedExactly what needs to be done.
Comment #29
cosmicdreams CreditAttribution: cosmicdreams commentedI 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...
Comment #30
heddnThis needs to get fixed on D8 first per the backport policy. Reset the project version.
Comment #31
tstoecklerIn 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.
Comment #32
cosmicdreams CreditAttribution: cosmicdreams commentedYes, 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.
Comment #33
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a patch that creates this constant. Any preference on the value being stored in a constant or a property?
Comment #34
cosmicdreams CreditAttribution: cosmicdreams commentedwhoops, meant to put it in needs review mode.
Comment #35
tstoecklerI 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:
This should be documented in some way.
Also, since we're already in the
FormCache
class, I thinkDEFAULT_LIFETIME
should be fine. Also, since we don't allow changing it in any way, why default? Seems likeconst LIFETIME
is sufficiently explicit, no?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.
Comment #36
cosmicdreams CreditAttribution: cosmicdreams commentedComment #37
tstoecklerUncommenting the beta evaluation. :-)
Comment #38
cosmicdreams CreditAttribution: cosmicdreams commentedFixing 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.
Comment #39
cosmicdreams CreditAttribution: cosmicdreams commentedrevised patch that reduces the verbosity of the constant.
Comment #40
cosmicdreams CreditAttribution: cosmicdreams commentedImproved code comment and code cleanup after review.
Comment #41
tstoecklerThanks a lot!
Comment #42
cosmicdreams CreditAttribution: cosmicdreams commentedpruning file list.
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commentedI 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()).
Comment #44
tstoecklerNote 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.
Comment #45
heddnI 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.
Comment #46
cosmicdreams CreditAttribution: cosmicdreams commentedThe 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.
Comment #47
cosmicdreams CreditAttribution: cosmicdreams commentedActually, 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.
Comment #48
David_Rothstein CreditAttribution: David_Rothstein commentedOops, 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.
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.
Comment #49
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a patch that uses config to see if I'm doing it right.
Comment #51
cosmicdreams CreditAttribution: cosmicdreams commentedAh, didn't include the new config into the system.schema.yml
Comment #53
cosmicdreams CreditAttribution: cosmicdreams commentedHere'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.
Comment #55
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedHi my name is Chris Weber and I'm looking at this issue at the extended sprint for Drupalcon LA.
Comment #56
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedEvery 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.
Comment #57
heddnThis is looking good now. Just need tests.
Comment #59
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedLooks 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.
Comment #60
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedComment #62
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedI expected that the test would fail, because I hadn't actually fixed the test. This patch does.
Comment #64
tim.plunkettIf we choose to do this, I'm pretty sure it could be
$expire = $this->configFactory->get('system.form')->get('cache_lifetime', static::CACHE_LIFETIME);
Comment #65
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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).
Comment #66
Cameron Tod CreditAttribution: Cameron Tod as a volunteer and at Acquia commentedI don't think get can accept a default as a second argument as described in #64. Rerolling.
Comment #68
Cameron Tod CreditAttribution: Cameron Tod as a volunteer and at Acquia commentedThis should make the tests go green.
Comment #70
Cameron Tod CreditAttribution: Cameron Tod as a volunteer and at Acquia commentedGah, all tests pass locally. Time to dig :(
Comment #73
mcdruidConfirmed 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).
Comment #75
xjmComment #76
mcdruidBeen 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:
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?
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 fromsystem.schema.yml
- but otherwise the same, so tests are likely to fail.Comment #77
mcdruidI think / hope it was as simple as this:
...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...
Comment #78
mcdruidComment #80
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commented+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.
Comment #81
mcdruidBased 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.
Comment #82
catchYes that's exactly what I meant, thanks!
Comment #83
cosmicdreams CreditAttribution: cosmicdreams at ICF commentedthe patch has been reduced to a super simple / trivial patch. I feel confident that it can be marked RTBC now.
Comment #84
cosmicdreams CreditAttribution: cosmicdreams at ICF commentedComment #85
tim.plunkettI 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.
Comment #86
Cameron Tod CreditAttribution: Cameron Tod as a volunteer and at Acquia commentedI've added a slightly tweaked comment from mcdruid's patch to #2091511: Make cache_form expiration configurable, to mitigate runaway cache_form tables.
Comment #88
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis 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").
Comment #89
mcdruidMade 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?
Comment #90
anavarreThis should be
$settings
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.Comment #91
catchI 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.
Comment #92
mcdruidSimplified the comment in default.settings.php to remove references to the cache_form table entirely, scaling issues and Ajax forms.
Comment #93
mcdruidOops, 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.
Comment #95
anavarrePatch applies successfully. With the
settings.php
override commented out, I'm able to manually pass an arbitrary value:When it's commented in, I get
And then, upon changing the value in
settings.php
I get: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.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:Waited a few minutes, then ran cron to get the garbage collection to kick in.
Querying the table now returns nothing:
I'm going to RTBC this because it works as anticipated in my testing.
Comment #96
naveenvalechaRTBC +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
Comment #97
anavarreAh yes, good idea. Tried to clarify the CR as well.
Comment #98
catchThis should have some test coverage that actually changing the setting affects the expiry timestamp on the cache item.
Comment #99
mcdruidAdded 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
Comment #100
naveenvalechaI love it :)
Thanks!
Comment #101
catchWhy does this need uid = 1 as the current user?
Comment #102
mcdruidThanks, you're right - it doesn't need that (blame copypasta).
Comment #103
catchThanks, looks good to me - it'd be great to have a test-only patch to see the assertion fail, otherwise RTBC I think.
Comment #104
mcdruidGood idea; here's the test only, which should fail.
Comment #106
mcdruidTest failed as expected.
That also highlighted a codesniffer whitespace nitpick in the test, which I've fixed:
No other changes.
Comment #107
catchThanks! Back to RTBC.
Comment #108
cilefen CreditAttribution: cilefen commentedI am retitling this for accuracy and updating credit.
Comment #110
cilefen CreditAttribution: cilefen commentedCommitted 4a629e3 and pushed to 8.4.x and published the change record. Thanks everyone!
Comment #111
cilefen CreditAttribution: cilefen commentedComment #112
cilefen CreditAttribution: cilefen commentedComment #113
naveenvalechaMarking it as fixed as we have a separate issue for Drupal 7 #2091511: Make cache_form expiration configurable, to mitigate runaway cache_form tables