Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Clearing the cache_form table results in invalid forms. This can happen by using memcache on cache_form, by manually clearing the table, or with other approaches to caching. Using memcache we've run into this problem, most recently: #406050: CCK Add Another Item causes fields to disappear
A cache table could be defined as something which can safely be truncated, and cache_form doesn't meet that description. I'm thinking we should rename the table and pull it out of the regular cache system. Thoughts?
Comment | File | Size | Author |
---|---|---|---|
#173 | form-cache-512026-173.patch | 16.89 KB | Berdir |
#173 | form-cache-512026-173-interdiff.txt | 1.01 KB | Berdir |
#169 | form-cache-512026-169.patch | 17.19 KB | Berdir |
#169 | form-cache-512026-169-interdiff.txt | 1.35 KB | Berdir |
#167 | form-cache-512026-167.patch | 16.02 KB | Berdir |
Comments
Comment #1
Ashford CreditAttribution: Ashford commentedAs an intermediate Drupal user, I would like the data table renamed.
Whenever our site crashes or goes to the white screen of death, one of my first troubleshooting techniques is to use phpMyAdmin and truncate the cache tables. I do go by the names. If it's labeled as cache_anything, I think it is safe to truncate.
Ashford
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedouch, i'd forgotten about this one, bump. if we can't get this in to D7, got to make sure it gets in for D8.
Comment #3
sun+1
We can safely rename this table. No module and no functionality other than Form API itself should access it. Hence, still in the loop for D7.
However, I'm trying to think of a name. We should not use {form}, because the table contains no forms in terms of forms as entities. {form_cache} may be it. Oh, perhaps to the point?
{form_state}
That's it. :)
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedpatch attached does the simple rename.
discussed this with sun in #drupal, he suggested keeping this as simple as possible.
i'd be interested in feedback on whether its too late to not use cache_set/cache_get to store/read this data. while we continue to do this, rather than using the database directly, we still run the risk of an apache/memcached restart killing in-process forms. webchick or dries - too late to fix that for D7?
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedmissed one.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #10
thommyboy CreditAttribution: thommyboy commentedhi there, as my cache_form grew > 350MB i was wondering wether it's save to truncate it like other
posts suggest? but here it's said that it's NOT save to do that?!
Comment #11
Eric_A CreditAttribution: Eric_A commented@thommyboy:
It's not safe to truncate this table because it contains form states and your users would lose data while going through form steps. I suppose your users are requesting a lot of cacheable forms in little time. (This cache expires in 6 hours.) You could try to delete just the entries that do not start with storage_ or form_state_.
@sun and justinrandell:
Isn't renaming to form_state misleading when cache_form contains both (part of) form_states and form builds? Is using two tables cache_form and form_state doable?
Comment #12
sunNo, this table contains only cached states of forms that "in progress" currently. The cached data is only used for the same user.
Comment #13
sun#6: rename_cache_form.patch queued for re-testing.
Comment #14
thommyboy CreditAttribution: thommyboy commentedactually it seems that most of the 300mb are an mysql "ueberhang" (no idea what the english word is, it's the space getting allocated for data and when data is deleted it stays allocated) which can be optimized by OPTIMIZE TABLE. I was just wondering wether doing a (backup-) dump of my database should include the cache-tables or not- only some editors often use forms on my site, most of the traffic is anonymous without any forms...
Comment #15
Eric_A CreditAttribution: Eric_A commented@#12: I must be missing something big time..
Comment #16
sun@Eric_A: I'm not sure what is unclear for you. Form states are cached by $form_build_id, which is unique hash for a single user, for a single form, at a certain time. The cached form state is only ever loaded from cache, if a subsequent request contains a form_build_id parameter. You never ever fiddle with those cached rows on your own.
Comment #17
Eric_A CreditAttribution: Eric_A commentedI agree with you on how form_state is cached. My point is that this table stores the acompanying forms too (Again, by build_id.)
This is storage/form_state:
388 cache_set('form_state_' . $form_build_id, $data, 'cache_form', REQUEST_TIME + $expire);)
This is the fully built form:
383 cache_set('form_' . $form_build_id, $form, 'cache_form', REQUEST_TIME + $expire);
It is this last cache entry that allows drupal_build_form() to not build a form everytime.
This is my understanding of the system, but I might be wrong. I remember one time when this cache was causing me problems: my form alter did not run after the form was submitted. I then decided to do certain stuff in the callback functions, and not during form definition time.
Comment #19
Eric_A CreditAttribution: Eric_A commented#12 describes the contents of the table as forms that are in_progress currently.
+1 for "forms_in_progress" or "form", -1 for the misleading "form_state".
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedHow about "form_build"? Because what it contains is $form and (most of) $form_state for a particular build id.
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedAnother thought: leave cache_form for caching $form, which ought to be re-constructable if the table is truncated. Add a "form_state" table for storing just (most of) $form_state, which has nothing to do with caching, and should be read/written with functions other than cache_(get|set).
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedThe cache system allows one to use a pluggable cache handler object, and since caching is purely for performance, it should be possible to use a cache handler that always returns FALSE from its get() method with no loss of functionality (only loss of performance). So, it should be possible to write a test that uses such a cache handler, and fails to submit a form correctly. That test should then be made to pass.
Comment #23
sun$form_state cannot be retrieved from non-volatile cache without $form. See http://api.drupal.org/api/function/form_get_cache/7
Comment #24
Eric_A CreditAttribution: Eric_A commentedThis issue started out with the problem of people accidentally killing form_states when they truncated cache_form. But effulgentsia makes an even stronger point.
If a cache handler returns FALSE ("Sorry, storage backend is down. No cache today.") , the system will not be be able to retrieve form_state and all multi-step forms are broken.
Yes, form_states should expire, but only when the complete form has been submitted.
Comment #25
Damien Tournoud CreditAttribution: Damien Tournoud commentedHm. I'm not confortable with losing the pluggability of the form state cache. Because we now have a proper per-bin pluggable cache architecture, I tend to consider that this issue is moot.
I suggest we simply:
Comment #26
Damien Tournoud CreditAttribution: Damien Tournoud commentedActually, #16 basically does what I described in #25. Free reroll.
Comment #27
sunThanks! I really like the additional idea of
Comment #28
Damien Tournoud CreditAttribution: Damien Tournoud commentedA first shot at adding a default bin implementation in default.settings.php.
Comment #29
Eric_A CreditAttribution: Eric_A commentedI'm not confortable with losing the pluggability of the form state cache.
Is your concern about pluggability of storage backend or do you really need form state cached...?
I understand the first case, if your sites is hit all the time by multi step form requests.
I don't really understand the need for caching simple form state data. And it is even more strange that form_state lives in cache only.
Comment #31
sunFixed the syntax in default.settings.php, which should make the installer pass again.
Comment #33
rfaysubscribing
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commented@Eric_A - nice work, i missed the $form part.
i think #21 is by far the cleanest approach. it matches a natural language description of the problem - keep cache form for, well, cached forms, and keep form state, which cannot be treated like cached data, in a non-volatile db table.
don't understand why we're persisting with a non-obvious implementation for vague performance concerns.
Comment #35
sunSee #23.
Comment #36
Eric_A CreditAttribution: Eric_A commentedSee #23.
@sun: any special reason why the implementations of form_get_cache() and form_set_cache() could not or should not be changed in line with the suggestions done by justinrandell/ effulgentsia?
Comment #37
sunWe can fix (rename) the cache table name for D7 to clarify that it contains non-volatile cache data and prevent users from mistakenly flushing that cache. Anything on top of that is D8 material.
Comment #38
effulgentsia CreditAttribution: effulgentsia commentedDespite the following text in http://en.wikipedia.org/wiki/Cache
I agree that doing the "right thing" and not using the word 'cache' anywhere in relation to persisting $form_state would be too destabilizing to D7. Hopefully we can find the right word and API for the concept in D8. In the meantime, I think Damien and sun are on the right track with what CAN be improved in D7 still. Here's a re-roll with some additional cleanup.
Comment #39
effulgentsia CreditAttribution: effulgentsia commented.
Comment #40
effulgentsia CreditAttribution: effulgentsia commentedI don't believe this track requires new tests.
Comment #42
rfayComment #43
effulgentsia CreditAttribution: effulgentsia commentedNeed to use DrupalFakeCache during install. Hopefully this fixes the test failure.
Comment #44
Wim LeersSubscribing.
Excellent initiative.
Comment #45
sunLet's get this in.
Comment #46
marcingy CreditAttribution: marcingy commented#43: drupal.form-state-cache.43.patch queued for re-testing.
Comment #48
marcingy CreditAttribution: marcingy commentedJust a reroll upto head.
Comment #50
marcingy CreditAttribution: marcingy commentedMissed renumbering the update function.
Comment #51
effulgentsia CreditAttribution: effulgentsia commentedThanks. Confirmed that #50 is a straight re-roll plus the renumbering of the update function.
Comment #52
marcingy CreditAttribution: marcingy commentedReroll back up to head.
Comment #53
rfayIt's important to get this in. (and there's no reason not to do so)
I found out in a training this week that confusion about this issue caused a major hiccup in performance work being done by a significant Drupal firm... it was assumed cache_form be treated like a cache table.
Comment #54
Eric_A CreditAttribution: Eric_A commentedApplying this patch is now affected by #791168: Does form_state cache never get cleaned?, apart from the update function number thingy.
The cid change is very trivial and has no effect whatsoever on any "logic" here.
(Cannot reroll myself right now...)
Comment #55
Eric_A CreditAttribution: Eric_A commentedUpdating patch from #52 as per #54.
Comment #56
effulgentsia CreditAttribution: effulgentsia commentedWhy did we lose this hunk from #50 in #52/#55?
40 critical left. Go review some!
Comment #57
sunRe-added that lost hunk and shortened the contained inline comment about the non-volatile form_state bin.
Comment #58
q0rban CreditAttribution: q0rban commentedsubscribe
Comment #59
rfayThis is a key issue to go into D7. IMO it should go in as soon as possible, before a beta release. Because it's an API change at its core (changing a table name certainly changes an API right?)
Could we have an issue summary and a statement of the impact this will have on people please?
Comment #60
sun#57: drupal.form-state-cache.57.patch queued for re-testing.
Comment #62
sunRe-rolled against HEAD.
Comment #63
moshe weitzman CreditAttribution: moshe weitzman commented$conf['cache_class_form_state'] = 'DrupalDatabaseCache';
This line and its long comment are too much for settings.php in my opinion. Only one in a million people will really needs to take action on this. But anyway, others seem to want it. So RTBC.
I removed API change tag since form.inc is free to change its table at any time. It does not offer an API for others to mess with that table. BUt if this bothers you, put it back. No biggie.
Comment #64
Dries CreditAttribution: Dries commentedGoing to move this one to Drupal 8.
Comment #65
jbrauer CreditAttribution: jbrauer commentedChanging this to a bug and would really like to change the title to reflect that it's a bug -- and a bug that causes data loss for site users. The key here is making sure we store form state information in a reliable way. The changing of naming is something we should do to be consistent but this is a pretty serious, if only hit occasionally on some sites, bug dating back several versions of Drupal. We really need to consider how this gets back to those versions with this bug as well.
Comment #66
pillarsdotnet CreditAttribution: pillarsdotnet commentedTrivial, but "git apply" complained because the patch in #62 adds a blank line at the end of default.settings.php
Comment #67
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-roll against fresh 8.x checkout as #62 no longer applies.
Comment #68
pillarsdotnet CreditAttribution: pillarsdotnet commentedRTBC as before.
Comment #69
pillarsdotnet CreditAttribution: pillarsdotnet commented#67: rename-cache_form-to-form_state-512026-67.patch queued for re-testing.
Comment #70
catchBy taking this out of system_cron(), this is never going to have garbage collection run on it. I don't see any code anywhere in the patch that adds back the garbage collection, so the table could grow in size indefinitely unless that cache is cleared.
Both form cache and form_state are set to expire in six hours with this patch, nothing here changes that so if it's going to do that, then something needs to clear out those entries when the six hours has expired.
I checked form.inc for cache_clear_all() and it only has this line:
so the only clearing of this that ever gets done in form.inc itself only happens if page caching is off.
#22 looks better to me - add a {form_state} table, write to that directly (not via the cache API), actual caching of forms can stay in cache_form, form_state can have some very conservative garbage collection on it just for sanity. That would be a less intrusive change for Drupal 7 too, it would also avoid the settings.php change.
Damien wanted to keep pluggable storage for form_state though. One other thing that occurs to me, it's a bit ugly but not necessarily a lot worse than what we have now - what if we stuck $form_state into $_SESSION somewhere. If you lose your session, you've lost your form data anyway, session backends are pluggable, cache_form could still be used for caching actual forms.
Adding backport tag since something needs to happen in D7 for this.
Comment #71
sunVeto: Forms can be cached for anonymous users and anonymous users may not necessarily have or need a session.
Comment #72
catchForms can be, but $form_state? this would only happen when you submit a form, and we'd have to have cleanup on the request processing the form either way.
I think a {form_state} table (in addition to, not replacing, cache_form) is better - I don't really care about that being pluggable even on high traffic sites, but if others are really, really tied to pluggable storage, short of adding a generic pluggable storage layer to D7 I don't see how to do that cleanly (and using cache functions isn't an option IMO).
Comment #73
sunRenaming is resolving a common confusion, but this is not a bug.
Comment #74
pillarsdotnet CreditAttribution: pillarsdotnet commentedTagging per #22
Comment #75
johnny-j-hernandez CreditAttribution: johnny-j-hernandez commentedsubscribing
Comment #76
catchWe should be able to move form_state into the key/value store and then move actual cached forms back to a cache table?
Comment #77
effulgentsia CreditAttribution: effulgentsia commentedAgreed. But we'll need expiration and possibly "owner", so getting #1642062: Add TempStore for persistent, limited-term storage of non-cache data in first would make sense. I'm not sure if this issue should still have the "needs backport to D7" tag, but leaving it for now.
Comment #78
Berdir#1547008: Replace Update's cache system with the (expirable) key value store is doing the same for cache_update. I'm wondering if it would be worth to have a meta issue for "key-value conversion" issues to actually look through core and check what can be converted to this system and have an overview of the progress and different issues.
Comment #79
catchThat's a good idea, I opened #1801962: [meta] Convert custom non-volatile cache-alike things to k/v store and added these two and the unique queue runner thingy.
Comment #80
BerdirOk, after #1642062: Add TempStore for persistent, limited-term storage of non-cache data and #1547008: Replace Update's cache system with the (expirable) key value store will be in, I don't think there's more to do here than what the attached patch does. cache_form doesn't change and is now just a normal cache back without special meaning.
Yes, I guess/hope that adding wrapper functions for those services will happen, chaining 3x get() is crazy. Writing it on a single line explicitly to do easier search & replace later on.
Adding a combined patch to see if works. After that, this can probably sleep until the other issues are dealt with, which should hopefully happen soonish.
Comment #82
BerdirSame problem as in cache_update, CoreBundle isn't loaded during upgrades, so there's no keyvaluestore.expirable service :(
Comment #83
BerdirOk, resurrecting this. This is now based off #1809206: KeyValueFactory hard-codes DatabaseStorage, which is RTBC and will hopefully get in soon (tm).
One patch against that and one against 8.x to verify that this is now fixed. Can someone confirm that the current approach is correct (still using cache() for the form structure and keyvalue.expirable for the form_state)? I'm a bit unsure right now.
Comment #85
BerdirOuch. I know that exception from the database issue, this happens when we try to execute a query after the connection was destroyed. And in this case, it happens when trying to compile the container.
So, this is basically what happens:
- setUp()
- install_begin_request()
- enable modules
- update Kernel
- compile kernel
- __destruct()
- connection already destroyed.
- kaboom.
I think we should not do the compilation during installation, sounds like this is a serious slowdown even when it works?
Any pointers on how to do that?
Comment #86
effulgentsia CreditAttribution: effulgentsia commentedLooking into #85.
Comment #87
effulgentsia CreditAttribution: effulgentsia commentedThe problem seems to be some stray objects getting destroyed semi-randomly (i.e., when PHP feels like garbage collecting). Curious if this patch manages to be green. It's just a band-aid though.
Comment #88
katbailey CreditAttribution: katbailey commentedAny compelling reason why we can't just check whether the table exists before trying to delete from it in the destructor? It's not the connection being closed that's causing the Exception to be thrown, it's the fact that the table doesn't exist.
I can't help but notice, looking at the errors from the test failure in #83, that there's always a graph involved, whether it's the ServiceReferenceGraph.php from Symfony or the Graph component in core - is it just that these are hideously memory intensive data structures that cause PHP do go doing GC? I'm not familiar enough with PHP's memory management :-/
Comment #90
BerdirWe can try that.
I still think this isn't a problem with the missing table but the connection being destroyed (not __destruct()-ed, destroy()-ed, which changes the statement class back to the default, then this error happens). But that might be something that we can maybe check for too.
Comment #92
BerdirBetter, but...
Segmentation fault is not good.
Edit: Checking if this is reproducable, the following tests failed:
Comment #93
Berdir#90: form-state-keyvalue-512026-90.patch queued for re-testing.
Comment #95
BerdirSlightly different but similar fails. Are these maybe related to the fact that we seem to have broken the register bundle on install functionality (see first test fail) and not actually this issue?
What's obvious here is that #1708692: Bundle services aren't available in the request that enables the module introduced a *huge* performance decrease, this patch was 10 minutes faster than HEAD (the current one even took 49 minutes, but I think it usually took around 42-44 minutes yesterday).
So I'm wondering if we should re-open that issue or open a separate (critical?) follow-up to improve this.
Comment #96
effulgentsia CreditAttribution: effulgentsia commented#1809206: KeyValueFactory hard-codes DatabaseStorage landed, so reuploading just the small patch from #83. I also left more info in #1708692-55: Bundle services aren't available in the request that enables the module.
Comment #98
BerdirAnd here is the patch with the table exists check. Let's see how this behaves without the Kernel changes.
Comment #100
BerdirSame segfaults, just slower and obviously without the bundle enable test failures.
I tried locally and I can reproduce the segfault, happens in gc_collect_cycles() apparently. Disabling the GC using zend_enable_gc = Off makes the test pass. But I have a feeling that's not really an option :)
Since there's no reason a php script should cause a segfault, this might even indicate a PHP bug?
Need to figure out what's different in these two tests that they're causing this.
Comment #101
BerdirOk, here's the backtrace reported by gdb:
This is the offending line:
https://github.com/php/php-src/blob/PHP-5.3/Zend/zend_gc.c#L372
https://github.com/php/php-src/blob/PHP-5.4/Zend/zend_gc.c#L425
Don't understand enough C to know what to make of this. I'll try if I can reproduce this using a more recent PHP version. Doesn't seem to have changed much, just moved around.
Edit:
Still segfaults with "PHP 5.4.8-1~precise+1 (cli) (built: Oct 29 2012 15:34:15) " from https://launchpad.net/~ondrej/+archive/php5
Comment #102
Berdir#1825450: Use lock service in lock() might be related to this, also getting segmentation faults there, but mostly on page requests.
Comment #103
Berdir#98: form-state-keyvalue-512026-98.patch queued for re-testing.
Comment #105
BerdirOk, so disabling the calls within __destruct() seems to allow this to pass. Same behavior as #1825450: Use lock service in lock(). Weird.
Let's see if the testbot can confirm this.
Comment #107
catchIf __destruct() happens during object destruction at the end of the request (as opposed to the object being destroyed before that), then there's a chance if gc is buggy, that objects within it might have been destroyed first. This shouldn't happen but who knows.
While I've not produced a segfault with this, I did get an issue with a missing class trying to backport DrupalCacheArray to D6 with the $database connection global. By explicitly assigning a reference I was able to work around that, might be worth trying here just to see if it changes the behaviour, i.e.:
in http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21D...
Might be ammunition for the PHP bug report if nothing else.
Otherwise, we might have to remove the 'operations in __destruct()' pattern (which I know I introduced...) and use register_shutdown_function() instead.
The class could register itself, and call a destroy() method which would make the __destruct() call later on a no-op, then have __destruct() call that destroy() method for if the class goes out of scope earlier. shutdown functions run before gc so there shouldn't be any chance of a class going missing.
Of course it could also be something else entirely.
Comment #108
sunPHP segfaults are odd, but I think I started to work on this problem for some other issue (can't recall which).
Since #1810990: Teared down test environment leaks into parent site/test runner, we've taken care of
__destruct()
methods of instantiated objects stored indrupal_static()
that are invoked upondrupal_static_reset()
.I believe what we're missing is the equivalent of that for
drupal_container()
.This patch here adds essentially that: A service that gets destructed upon invocation of
drupal_container($this->originalContainer)
. However, that is currently happening after the test environment has been fully destructed, so, among other things, the database connection no longer exists, but yet,__destruct()
tries to operate on it.The solution could potentially be the exact equivalent of the
drupal_static()
solution: Simply force-rebuild the container viadrupal_container(NULL, TRUE)
before the test environment is destroyed, and keep the existing, second replacement back to the original container of the original test runner environment later on intearDown()
.Comment #109
BerdirYes, I guess the two possible approaches are...
a) Try to make __destruct() invocations as predictable as possible, make dependencies explicit. That might be quite hard, because e.g. the lock issue is also having segmentation fails on page requests. And it won't get easier if the database connections are also in the DIC, something that currently causes huge issues with open connections as we're re-creating so many containers.
b) Accept that __destruct() is not the correct place to run such clean up tasks. Maybe we can loop through the actually instantiated services in the container (is that possible?) in a kernel terminate event, check if they implement a certain interface (something like RequiresTermination*) and if so, call a method on them (e.g. terminate())*). What we need to make sure here is that we always invoke that method when we're supposed to. Not so relevant for the garbage collection here but quite important for the releaseAll() in the lock backend...
* Naming it a bit less dramatic and not calling for Arnie to show up is ok too :)
Comment #110
katbailey CreditAttribution: katbailey commentedHere's a PoC patch based on a discussion Berdir and I had in IRC today and on his option b) above. The idea is that there could be several services that need a terminate() method called on them explicitly. This uses a compiler pass to register them to a ServiceTerminator, which implements EventSubscriberInterface and listens to the kernel's terminate event.
Not really sure how to test it for the form_state stuff though.
Comment #111
BerdirI think this looks great, much more explicit than relying on __destruct(), thanks for working on this! And the patch is green :)
Not sure how to test it correctly for this either, but I'll try to create a patch for the lock service, We do have tests there that releaseAll() is actually called, so that should allow us to test if this pattern really works.
Comment #112
BerdirOk, so the lock stuff is more complicated, so let's add some basic tests to this (which prove that this is working nicely, @katbailey++).
Reviews, anyone? :)
Comment #113
Lars Toomre CreditAttribution: Lars Toomre commentedHere are some coding and doc standard thoughts from reading through patch:
There is inconsistent use of blank lines after class declaration and before class closing brace in this patch. Some say that is a standard and others there is no standard at all. We simply should be consistent at least in the same patch.
Should be 'Contains' instead of 'Definition of'. Elsewhere in patch as well.
Missing class docblock here. Also missing method docblock.
I think the docs standard calls for a leading '\' here. Elsewhere in patch as well.
'ids' should be 'IDs'.
Needs one line description.
Missing docblocks.
Comment #114
BerdirAdded/Fixed the comments except "I think the docs standard calls for a leading '\' here. Elsewhere in patch as well.", we currently never add a leading \ to the Implements/Overrides lines and in most cases leave out the namespace as well.
Not going back and forth on that anymore until we actually have an agreement in the namespace in comments issue and it's marked as fixed. It has already changed multiple times, and we will have to look over and fix it everywhere at some point anyway.
Comment #116
katbailey CreditAttribution: katbailey commentedActually, I think the service terminator belongs with the rest of the event subscribers - this patch moves it in there and renames it to KernelTerminateSubscriber. I don't think there's any need to rename the test class.
Comment #117
Crell CreditAttribution: Crell commentedI talked to Berdir about this issue for a while in IRC. Short version, I'm not sure if this is a terrible idea or such a great idea it belongs in Symfony proper. :-) For the time being therefore I think we can/should proceed.
That said, I see two issues.
1) We should probably split the "do I need to do any termination?" logic out to a separate method for tidiness. Making services integrate that into a single method seems wrong.
2) It means that any service that needs termination becomes Drupal-dependent. Services should be as Drupal independent as possible. Is it too wonky to have a "helper service" do the shutdown routine, so that a shutdown-needing service can stay clean? Or is that not really feasible for the sorts of services we're talking about?
Comment #118
Berdir1) sounds fine to me, I'm not sure if I get 2) yet.
#1792536-41: Remove the install backend and stop catching exceptions in the default database cache backend is another nice example why __destruct() is problematic. We have no control over when and in which environment it runs, as in the example shown there, it is initialized within the test environment but __destruct() is only invoked after tearDown() has been executed and the test tables where dropped and then it fails. Which means that we should convert the __destruct() there to terminate() as well with this patch. Probably easier to wait until that other issue is in, as we can prove that this actually works and remove the try/catch added there.
Using the service termination pattern, we can decide if we want to either run it *before* we tear down the test environment or not run it at all.
There is one problem with the current pattern, that is, if a termination needs to happen even if (or actually, *especially* if) we have an unclean shutdown, like a php fatal error. This is important for the lock system as it wants to release all locks in case we have a fatal error during lock execution.
The only way to make that work for service terminator would be to enforce that a Kernel::terminate() call always happens. If not, then we still need a separate solution for the lock service.
Comment #119
Crell CreditAttribution: Crell commentedPerhaps lock should keep its destructor as a fallback? That at least sometimes works. :-)
Comment #120
catchLock should probably register itself in register_shutdown_function() - then it's guaranteed to run before garbage collection (which I don't entirely trust to destroy objects in the right order), but regardless of whether there's a clean exit or not.
For testing I opened #1188316: DrupalUnitTestCase incompatible with any classes with destructors that access the database a while back which was very similar.
Comment #121
BerdirOk, re-rolled and improved documentation a bit, injected state, moved the termination to a separate method (though I don't quite understand how that's nicer ;)) and used it for ViewsDataCache.
Comment #122
katbailey CreditAttribution: katbailey commentedI know there are already at least two patches in the works that do this*, but let's add one more and whichever one gets in first fixes this for the others :-) i.e. we need a 'state' service so we don't have to inject the factory when all we need is the state collection.
* #1331486: Move module_invoke_*() and friends to an Extensions class and #1786490: Add caching to the state system
Comment #123
BerdirOk, defined like that, also updated the AliasManager to receive the state service directly.
Moved the ServiceTerminatorTest into DrupalKernel, already updated the test group and I think this makes sense given that the implementation now also is a KernelSomething.
Comment #125
BerdirThis should fix the tests.
And the patch keeps getting bigger and bigger :( Time to get this in?
Comment #126
dawehnerThis looks really good so far. It seems to be that we have tests for the terminator already, and for sure as well for the form_state.
Do we need anything extra?
Do we have any general idea how to handle the weight between different services? What would for example happen if the database connection would be closed in a terminate event.
Nitpick: \Drupal\Core ... or just relative.
We maybe should document how the table could not exist.
I'm wondering whether we should create multiple storages for the same collection?
The variable is a KeyValueStoreInterface, so $this->state documentation should be better updated as well (is marked as databaseStorage, so maybe out of scope).
Comment #127
BerdirI am not 100% we are talking about the same thing, because what I understood has nothing to do with the weight of the event. However, this is a valid point. Imagine both database and keyvalue.expirable.database have a needs_termination tag. Then we must not terminate database before keyvalue.expirable.database.
The update implements the most basic implementation for that, reverse the order of services so that they are terminated in the opposite order in which they are defined. So that means if you define X and Y and Y depends on X (gets it passed as a reference), then you need to define X first. Which seems like something you automatically do. It does get interesting when you start altering definitions, though...
Is that enough or do we need something more complicated?
- Fixed nitpicks
- Removed the table exists condition. I think that is a left-over from older patches. It should no longer be necessary.
- It is not necessary to avoid creating duplicates in KeyValueDatabaseExpirableFactory, KeyValueExpirableFactory takes care of that. We just store them there so that we can pass the terminate call through.
Comment #128
effulgentsia CreditAttribution: effulgentsia commentedYes. There's no requirement by ContainerBuilder for dependencies to be defined before their dependents and lots of cases where such a requirement would be annoying, such as module X depending on module Y, but having a larger weight or being later in the alphabet. Since this is running in a compiler pass, we have access to all of ContainerBuilder, so detecting references and ordering accordingly shouldn't be too hard, but I'd be ok with that punted to a follow up.
Comment #129
BerdirYeah, I feared as much :)
I'm happy to try and implement that, but I would *really* prefer to do that in a follow-up.
This doesn't look like it but apart from being a major task, it is also blocking a rather long list of issues (alias cache improvements, moving cache_update into key/value, improving CacheArray and moving it into the container, cache bin unification). And I'd love to get those into D8 :)
So, what can I do to get this to RTBC? Would an updated issue summary help, will also create a follow-up task for #128 if people are fine with that.
Comment #130
effulgentsia CreditAttribution: effulgentsia commentedRerolled for HEAD changes.
Comment #131
effulgentsia CreditAttribution: effulgentsia commentedSince this is blocking other issues, per #129, I think this is ready enough, so RTBC.
This issue still has the "needs tests" tag, set in #74 for the test I suggested in #22, so after committing, this should be set back to "needs work" for that.
Comment #132
webchickHm. Unless there's an incredibly compelling reason, I'm not sure I like the idea of committing something this far-reaching without test coverage. #129 alludes to a bunch of stuff, but nothing I'm aware of in that list that is "stuff is so broken, it's worth potentially introducing more brokenness" unless I'm wrong?
Comment #133
effulgentsia CreditAttribution: effulgentsia commentedThe patch has good test coverage of the service termination API that's the bulk of the patch. And there's existing test coverage in HEAD for multistep forms (and therefore, $form_state persistence) in general.
The only coverage lacking is one that demonstrates that multistep forms fail in HEAD with the Drupal\Core\Cache\NullBackend, but succeed with this issue's patch, which come to think of it, is probably not actually working yet, given that form_get_cache() doesn't get $form_state out of the key-value store if $form itself wasn't retrieved from cache.
So, @Berdir: if what you're mostly wanting to land quickly is the service termination stuff, how about splitting that off into a separate issue. But, if you're wanting the $form_state stuff, then that test still needs to be written and made to pass.
Comment #134
sunI think it would make sense to split those service termination changes into a separate issue. They make up a pretty big part of this patch, but are actually rather irrelevant to the actual topic/change of this issue.
Also, any reason why we're not using Symfony\Component\HttpKernel\TerminableInterface? That's used by the Kernel for bundles that need termination already. Not sure whether it would be legit, but we could possibly fake/mock a Request and Response, in case there is none (e.g., in tests).
Comment #135
BerdirOpened #1891980: Add Service termination API to reliable terminate/shut down services in the container. Will work on tests according to #22 later.
Comment #136
BerdirWell, @effulgentsia was of course right. Started with those tests and it obviously doesn't pass.
And the only way I can think of making it work is to move everything into the keyvalue store. Which in turn makes writing a test that uses the NullBackend pointless ;)
Or is there a way to recover from a missing cache entry? Can we rebuild the form or something like that? I wouldn't know how...
Comment #137
BerdirRe-roll, changed both "caches" to keyvalue.expirable, as discussed with @catch. Completely untested.
There is no need to add an additional test anymore I think, as we are no longer using the cache system at all.
Comment #139
BerdirDoes that fix the installation?
Comment #141
BerdirOk, but this should. We need a null implementation during installation.
Comment #143
BerdirThis should fix the views test fails. We could alternatively also use the null backend there.
Comment #144
alexpottThis looks terrific!
Just wondering why...
I get why form state is moving to a keyvalue.expirable collection but why the regular form cache?
Comment #145
BerdirFrom what I understand, there isn't really a difference between the two.
If you look at form_get_cache(), it first loads the form cache and only if that succeeds and has a cache token, then it also loads the form_state. So it's pointless to just add $form_state to keyvalue as it won't even try to load the form_state if it fails to load the form.
Comment #146
alexpottRerolled patch to use Drupal::service() instead of drupal_container()... also this code is weird (not just the changed code - the entire condition)...
This code can never ever be called... $form_state['values']['form_build_id'] is never set... and what this seems to be trying to cover is the case where you are not using drupal's standard database cache and the cache replacement might not support time based expiry (eg. memcache) and thereby delete unecessary form state information once the form has been processed.
I think that now we are using the KeyValueExpirable interface it is upto the implementation to work out what is going to happen on the destruct event and we should not be making any assumptions about the implementation here. Hence I've removed the code.
Comment #147
Berdir#146: 512026.form-state-key-value.146.patch queued for re-testing.
Comment #149
effulgentsia CreditAttribution: effulgentsia commentedIdeally, we should fix that, but we can do that in another issue after this one lands. Given that that is the behavior of HEAD, I think it's fine for this issue's scope to move both of them.
Related to above, there should be. All state information should be in $form_state. $form should always be regeneratable. However, given that Drupal has always cached/stored the two together, it's very likely that some forms in core and many forms in contrib put state information in $form. If/when we do the other issue of splitting $form and $form_state storage, it'll be easier to find and fix those forms.
Comment #150
BerdirWow, git actually managed to rebase that after the cache_update removal went in which overlapped with this issue.
Changed Drupal::service() to the new Drupal::keyValueExpirable() that I added over there.
Comment #152
Berdir#150: 512026.form-state-key-value.150.patch queued for re-testing.
Comment #154
BerdirOh, forgot to implement the new deleteAll() method.
Comment #155
chx CreditAttribution: chx commentedI think all questions are addressed by now. I have no idea why this is marked with a backport though.
Comment #156
msonnabaum CreditAttribution: msonnabaum commentedThis looks good to me for the most part. The only part I really don't like is having to define a service and factory for the NULL backend, but that's beyond the scope of this issue so I started #1948694: KeyValue $conf overrides should not be services to address it.
Did find one small doc mistake:
Comment #157
BerdirFixed that. Not sure about the backport tag either.
Comment #158
xjm@catch added the backport tag going on two years ago in #70, presumably because of #65:
I thought our strategy now was to keep tables around and drop them in D9? Maybe not applicable to a table with temporary data, I guess?
Shouldn't we be adding test coverage for this? It looks like the tag might have been removed by accident?
Comment #159
BerdirHm, we'll have to see if we can go back to the original plan here of renaming cache_form to form_state or something like that or simply stop using the cache API for it similar to update.module. Not really a backport :)
Yes, there's no need to keep that table, there is no data in there that you still need after upgraded to 8.x.
This is covered by tests, various ajax-related things fail if this doesn't work correctly. The suggestion in #133 to test it when using a null cache backend would make sense if we were still partially using the cache API but we aren't so that is pointless with the current implementation. If we want to look into using the normal cache again for $form in a follow-up issue then we could add something like that.
Comment #160
xjmPersonally I'd prefer to see explicit test coverage, even something simple that just confirms that values are set correctly in the cache.
From way back in #37:
Let's open a followup to do that for D7?
Comment #161
BerdirAs discussed IRC:
- Two patches attaches as examples that we do have test coverage for this, "Multistep Form using form storage" for example fails with IMHO pretty explicit test failures and the point of that test is basically testing this functionality.
- We are using an API, and IMHO, we don't need to test if that API works correctly (e.g. checking database records), expirable key value store has (good!) test coverage already.
- So the only thing that we could additionaly do is check form_set_cache() and form_get_cache() with direct API calls. If necessary, I can work on that....
Comment #163
BerdirPatches failed as expected, setting back to needs review. I'll see what I can do about API tests.
Comment #164
BerdirOk, here we go, added some tests. Explicit enough? :)
Noticed two fragile things about DrupalUnitTestBase:
- The global user is the parent site user. That means the default environment for the UI test runner and run-tests.sh is quite different one is uid 1 and the other is uid 0. Added some code to make sure it's the same and restore the previous value, this should be moved into the base class I think.
- Changing the session breaks the parent batch in the UI, had to change the private key to simulate a changed session.
Also, what doesn't seem to be quite in line with the token behavior is that a cache token is only generated if a user is logged in, should be possible to do the same if we have a session? But that's not relevant for this issue.
Comment #165
BerdirNoticed a small bug in the tests.
Comment #166
xjmThose tests look complete. Thanks! However, the patch no longer applies due to #1764474: Make Cache interface and backends use the DIC (and might need some changes from that?). Meanwhile, here's an interdiff to clean up some small things.
Comment #167
BerdirRe-roll and removed two @todo's added by the cache DIC issue.
Comment #169
BerdirAlso need to remove the cache bin from the browser, obviously...
Comment #170
xjmOkay, I think this is ready now. :) Let's file that followup for the unit test user ID bug thing and link it here?
Comment #171
BerdirSure: #1950684: Mock and protect $GLOBALS['user'] in DUTB tests
Comment #172
xjm#169: form-cache-512026-169.patch queued for re-testing.
Comment #173
BerdirSmall update based on the learnings from #1950684: Mock and protect $GLOBALS['user'] in DUTB tests, TestBase already takes care of restoring the original user object at the end, we don't have to do that ourself. The only thing is that that global $user is not always an anonymous user, we could remove the ->uid = 0 line in that issue once this is on but we could also keep it for clarity.
Comment #174
xjmWe're not using
state()
; it's a separate kvstore implementation.Comment #175
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #176
xjmQuestions raised while reviewing this that we should either document here or address in followups:
NULLNull (coding standards fingers!) storage would have security implications for the installer, but Alex suggested that is not a concern -- the CSRF token generation is separate from the form session handling.state()
. It's a separate, expirable implementation.So let's open a separate issue to address that and
fixmitigate the D7 bug.Comment #177
BerdirAs a quick response to 1.
The focus of the user temp store is the user. The goal is to give back the same thing from the store based on the user context. For this, the user is irrelevant*, the same form build id gets back the same object, but a different form build id doesn't, does not matter if it's the same user, a different or anonymous. You could say this is a "form temp store". user and form temp store already have a common denominator: they expirable key value store. The features that the user temp stores provides on top of that (locking) are not interesting for us, it's not possible that two different sessions end up with the same form build id, so we don't need to lock it.
I hope that clarifies this :)
* The only thing that's relevant is that for authenticated users, we maintain a cache token based on the session id.
Comment #180
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis issue, via the interdiff in #146, removed the deletion of $form_state from cache/kv when the form is submitted. That comment presented the following rationales for doing so:
This surprises me. I wonder what made it true at the time of that comment. It is set in 7.x, and in current 8.0.x HEAD.
No. What this also covered is preventing the user from submitting the form and then getting the browser back to an earlier form state (e.g., via a back button) and resubmitting it with a now outdated $form_state (e.g., serialized entities, or whatever, in there that have since been outdated by the prior submission). Sorry I didn't catch that at the time, but #2473759: Form caches should be deleted after submission is now proposing to restore those deletions.
Comment #181
xjmRemoving the tag; it was re-added by System Message crossposting with @Berdir back in the day.