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?

CommentFileSizeAuthor
#173 form-cache-512026-173.patch16.89 KBBerdir
#173 form-cache-512026-173-interdiff.txt1.01 KBBerdir
#169 form-cache-512026-169.patch17.19 KBBerdir
#169 form-cache-512026-169-interdiff.txt1.35 KBBerdir
#167 form-cache-512026-167.patch16.02 KBBerdir
#167 form-cache-512026-167-interdiff.txt1.33 KBBerdir
#166 form-cache-interdiff.txt2.53 KBxjm
#165 form-cache-tests-512026-165.patch15.23 KBBerdir
#165 form-cache-tests-512026-165-interdiff.txt818 bytesBerdir
#164 form-cache-tests-512026-164.patch15.19 KBBerdir
#164 form-cache-tests-512026-164-interdiff.txt3.56 KBBerdir
#161 broken_form_state_cache-512026-161.patch11.65 KBBerdir
#161 broken_form_cache-512026-161.patch11.64 KBBerdir
#157 512026.form-state-key-value.157.patch11.63 KBBerdir
#157 512026.form-state-key-value.157-interdiff.txt867 bytesBerdir
#154 512026.form-state-key-value.154.patch11.63 KBBerdir
#154 512026.form-state-key-value.154-interdiff.txt676 bytesBerdir
#150 512026.form-state-key-value.150.patch11.5 KBBerdir
#150 512026.form-state-key-value.150-interdiff.txt1.65 KBBerdir
#146 512026.form-state-key-value.146.patch16.69 KBalexpott
#146 143-146-interdiff.txt2.74 KBalexpott
#143 form-state-key-value-512026-143.patch16.58 KBBerdir
#143 form-state-key-value-512026-143-interdiff.txt2.55 KBBerdir
#141 form-state-key-value-512026-141-interdiff.txt4.55 KBBerdir
#141 form-state-key-value-512026-141.patch14.04 KBBerdir
#139 form-state-key-value-512026-140.patch9.49 KBBerdir
#139 form-state-key-value-512026-140-interdiff.txt839 bytesBerdir
#137 form-state-keyvalue-137.patch9.49 KBBerdir
#130 form-state-service-termination-512026-130.patch23.42 KBeffulgentsia
#127 form-state-service-termination-512026-127.patch23.43 KBBerdir
#127 form-state-service-termination-512026-127-interdiff.txt2.93 KBBerdir
#125 form-state-service-termination-512026-125.patch23 KBBerdir
#125 form-state-service-termination-512026-125-interdiff.txt1.18 KBBerdir
#123 form-state-service-termination-512026-123.patch21.82 KBBerdir
#123 form-state-service-termination-512026-123-interdiff.txt4.95 KBBerdir
#121 form-state-service-termination-512026-121.patch19.87 KBBerdir
#121 form-state-service-termination-512026-121-interdiff.txt10.31 KBBerdir
#116 form-state-service-termination-512026-116.patch17.3 KBkatbailey
#116 interdiff.txt3.83 KBkatbailey
#114 form-state-service-termination-512026-114.patch17.11 KBBerdir
#114 form-state-service-termination-512026-114-interdiff.txt3.84 KBBerdir
#112 form-state-service-termination-512026-112.patch16.46 KBBerdir
#110 form-state-service-termination-512026-110.patch12.35 KBkatbailey
#105 form-state-no-destruct-512026-105.patch3.52 KBBerdir
#98 form-state-keyvalue-512026-98.patch3.42 KBBerdir
#96 form-state-keyvalue-512026-83.patch2.71 KBeffulgentsia
#90 form-state-keyvalue-512026-90.patch27.81 KBBerdir
#90 form-state-keyvalue-512026-90-interdiff.txt723 bytesBerdir
#87 form-state-keyvalue-512026-87.patch27.51 KBeffulgentsia
#87 interdiff.txt1.35 KBeffulgentsia
#83 form-state-keyvalue-512026-83.patch2.71 KBBerdir
#83 form-state-keyvalue-512026-83-with-factory.patch26.16 KBBerdir
#80 form-state-key-value-512026-80.patch1.89 KBBerdir
#80 form-state-key-value-512026-80-combined.patch73.73 KBBerdir
#67 rename-cache_form-to-form_state-512026-67.patch11.82 KBpillarsdotnet
#62 drupal.form-state-cache.62.patch11.35 KBsun
#57 drupal.form-state-cache.57.patch11.3 KBsun
#55 drupal.form-state-cache.55.patch9.98 KBEric_A
#52 drupal.form-state-cache.52.patch9.39 KBmarcingy
#50 drupal.form-state-cache.50.patch10.88 KBmarcingy
#48 drupal.form-state-cache.48.patch10.88 KBmarcingy
#43 drupal.form-state-cache.43.patch11.53 KBeffulgentsia
#38 drupal.form-state-cache.38.patch10.78 KBeffulgentsia
#31 drupal.form-state-cache.31.patch7.77 KBsun
#28 512026-form-cache-rename.patch7.35 KBDamien Tournoud
#26 512026-form-cache-rename.patch6.11 KBDamien Tournoud
#16 rename_cache_form_0.patch6.41 KBsun
#6 rename_cache_form.patch6.41 KBAnonymous (not verified)
#4 rename_cache_form.patch5.7 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Ashford’s picture

As 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

Anonymous’s picture

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

sun’s picture

Title: cache_form should not be a cache table? » cache_form should not be a cache table

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

Anonymous’s picture

Assigned: Unassigned »
Status: Active » Needs review
FileSize
5.7 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

FileSize
6.41 KB

missed one.

Anonymous’s picture

Status: Needs work » Needs review
Anonymous’s picture

Assigned: » Unassigned
thommyboy’s picture

hi 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?!

Eric_A’s picture

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

sun’s picture

No, this table contains only cached states of forms that "in progress" currently. The cached data is only used for the same user.

sun’s picture

#6: rename_cache_form.patch queued for re-testing.

thommyboy’s picture

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

Eric_A’s picture

@#12: I must be missing something big time..

628 	 $schema['cache_form']['description'] = 'Cache table for the form system to store recently built forms and their storage data, to be used in subsequent page requests.'; 
371 	/**
372 	* Store a form in the cache.
373 	*/
374 	function form_set_cache($form_build_id, $form, $form_state) {
375 	// 6 hours cache life time for forms should be plenty.
376 	$expire = 21600;
377 	
378 	// Cache form structure.
379 	if (isset($form)) {
380 	if ($GLOBALS['user']->uid) {
381 	$form['#cache_token'] = drupal_get_token();
382 	}
383 	cache_set('form_' . $form_build_id, $form, 'cache_form', REQUEST_TIME + $expire);
384 	}
385 	
386 	// Cache form state.
387 	if ($data = array_diff_key($form_state, array_flip(form_state_keys_no_cache()))) {
388 	cache_set('form_state_' . $form_build_id, $data, 'cache_form', REQUEST_TIME + $expire);
389 	} 
sun’s picture

FileSize
6.41 KB

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

Eric_A’s picture

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

178	    // If the incoming input contains a form_build_id, we'll check the
179	    // cache for a copy of the form in question. If it's there, we don't
180	    // have to rebuild the form to proceed. In addition, if there is stored
181	    // form_state data from a previous step, we'll retrieve it so it can
182	    // be passed on to the form processing code.
183	    if (isset($form_state['input']['form_id']) && $form_state['input']['form_id'] == $form_id && !empty($form_state['input']['form_build_id'])) {
184	      $form_build_id = $form_state['input']['form_build_id'];
185	      $form = form_get_cache($form_build_id, $form_state);
186	    }
187	
188	    // If the previous bit of code didn't result in a populated $form
189	    // object, we're hitting the form for the first time and we need
190	    // to build it from scratch.

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.

Status: Needs review » Needs work

The last submitted patch, rename_cache_form_0.patch, failed testing.

Eric_A’s picture

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

effulgentsia’s picture

How about "form_build"? Because what it contains is $form and (most of) $form_state for a particular build id.

effulgentsia’s picture

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

effulgentsia’s picture

Title: cache_form should not be a cache table » Persisting $form_state does not qualify as caching, and should not be done in a cache_* table or using the cache_set() function
Issue tags: +Needs tests

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

sun’s picture

$form_state cannot be retrieved from non-volatile cache without $form. See http://api.drupal.org/api/function/form_get_cache/7

Eric_A’s picture

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

Damien Tournoud’s picture

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

  • rename {cache_form} to {form_state} (which should discourage users to truncate this table),
  • add an explicit 'cache_class_form_state' variable that points to DrupalDatabaseCache, so that sites that uses memcache (for example) as their default cache will still store the forms in the database by default, but still can explicitely override it if they want to.
Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
6.11 KB

Actually, #16 basically does what I described in #25. Free reroll.

sun’s picture

Thanks! I really like the additional idea of an explicit 'cache_class_form_state' variable that points to DrupalDatabaseCache

Damien Tournoud’s picture

A first shot at adding a default bin implementation in default.settings.php.

Eric_A’s picture

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

Status: Needs review » Needs work

The last submitted patch, 512026-form-cache-rename.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
7.77 KB

Fixed the syntax in default.settings.php, which should make the installer pass again.

Status: Needs review » Needs work

The last submitted patch, drupal.form-state-cache.31.patch, failed testing.

rfay’s picture

subscribing

Anonymous’s picture

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

sun’s picture

See #23.

Eric_A’s picture

See #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?

sun’s picture

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

effulgentsia’s picture

Title: Persisting $form_state does not qualify as caching, and should not be done in a cache_* table or using the cache_set() function » Rename 'cache_form' to 'form_state' to help developers and administrators avoid wiping unrecoverable data
FileSize
10.78 KB

Despite the following text in http://en.wikipedia.org/wiki/Cache

As opposed to a buffer, which is managed explicitly by a client, a cache stores data transparently: This means that a client who is requesting data from a system is not aware that the cache exists, which is the origin of the name cache (from French "cacher", to conceal).

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.

effulgentsia’s picture

Status: Needs work » Needs review

.

effulgentsia’s picture

Category: bug » task
Issue tags: -Needs tests

I don't believe this track requires new tests.

Status: Needs review » Needs work

The last submitted patch, drupal.form-state-cache.38.patch, failed testing.

rfay’s picture

Title: Rename 'cache_form' to 'form_state' to help developers and administrators avoid wiping unrecoverable data » Rename 'cache_form' table to 'form_state' to help developers and administrators avoid wiping unrecoverable data
effulgentsia’s picture

Status: Needs work » Needs review
FileSize
11.53 KB

Need to use DrupalFakeCache during install. Hopefully this fixes the test failure.

Wim Leers’s picture

Subscribing.

Excellent initiative.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in.

marcingy’s picture

#43: drupal.form-state-cache.43.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal.form-state-cache.43.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
10.88 KB

Just a reroll upto head.

Status: Needs review » Needs work

The last submitted patch, drupal.form-state-cache.48.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
10.88 KB

Missed renumbering the update function.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. Confirmed that #50 is a straight re-roll plus the renumbering of the update function.

marcingy’s picture

Reroll back up to head.

rfay’s picture

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

Eric_A’s picture

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

Eric_A’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
9.98 KB

Updating patch from #52 as per #54.

effulgentsia’s picture

+++ sites/default/default.settings.php	30 May 2010 23:26:06 -0000
@@ -407,3 +406,22 @@
+/**
+ * Cache configuration.
+ *
+ * Persistent caches are split up into several cache bins. Each cache bin can
+ * use a different backend cache implementation. Modify the configuration
+ * below to use a different backend implementation. Core provides two default
+ * implementations:
+ * - DrupalDatabaseCache: The standard database-backed cache.
+ * - DrupalFakeCache: A black-hole cache implementation, which does nothing.
+ *
+ * Contributed modules can additionally provide custom implementations.
+ */
+$conf['cache_default_class'] = 'DrupalDatabaseCache';
+// Unlike other cache bins, data in the 'form_state' bin cannot be regenerated:
+// flushing entries before they expire means that some forms displayed to users
+// lose their state and the data already submitted to them. Use non-volatile
+// backend implementations only.
+$conf['cache_class_form_state'] = 'DrupalDatabaseCache';

Why did we lose this hunk from #50 in #52/#55?

40 critical left. Go review some!

sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
11.3 KB

Re-added that lost hunk and shortened the contained inline comment about the non-volatile form_state bin.

q0rban’s picture

subscribe

rfay’s picture

Issue tags: +API change

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

sun’s picture

Issue tags: -API change

#57: drupal.form-state-cache.57.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +API change

The last submitted patch, drupal.form-state-cache.57.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
11.35 KB

Re-rolled against HEAD.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -API change

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

Dries’s picture

Version: 7.x-dev » 8.x-dev

Going to move this one to Drupal 8.

jbrauer’s picture

Category: task » bug
Priority: Normal » Major

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

pillarsdotnet’s picture

Trivial, but "git apply" complained because the patch in #62 adds a blank line at the end of default.settings.php

pillarsdotnet’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.82 KB

Re-roll against fresh 8.x checkout as #62 no longer applies.

pillarsdotnet’s picture

Status: Needs review » Reviewed & tested by the community

RTBC as before.

pillarsdotnet’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

By 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:


 if (!variable_get('cache', 0) && !empty($form_state['values']['form_build_id'])) {
        cache_clear_all('form_' . $form_state['values']['form_build_id'], 'cache_form');
        cache_clear_all('form_state_' . $form_state['values']['form_build_id'], 'cache_form');
      }

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.

sun’s picture

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.

Veto: Forms can be cached for anonymous users and anonymous users may not necessarily have or need a session.

catch’s picture

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

sun’s picture

Category: bug » task

Renaming is resolving a common confusion, but this is not a bug.

pillarsdotnet’s picture

Title: Rename 'cache_form' table to 'form_state' to help developers and administrators avoid wiping unrecoverable data » Add 'form_state' table to hold nonvolatile data currently stored in 'cache_form' table.
Issue tags: +Needs tests

Tagging per #22

johnny-j-hernandez’s picture

subscribing

catch’s picture

We should be able to move form_state into the key/value store and then move actual cached forms back to a cache table?

effulgentsia’s picture

Title: Add 'form_state' table to hold nonvolatile data currently stored in 'cache_form' table. » Move $form_state storage from cache to new state/key-value system

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

Berdir’s picture

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

catch’s picture

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
73.73 KB
1.89 KB

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

Status: Needs review » Needs work

The last submitted patch, form-state-key-value-512026-80-combined.patch, failed testing.

Berdir’s picture

Same problem as in cache_update, CoreBundle isn't loaded during upgrades, so there's no keyvaluestore.expirable service :(

Berdir’s picture

Status: Needs work » Needs review
FileSize
26.16 KB
2.71 KB

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

Status: Needs review » Needs work

The last submitted patch, form-state-keyvalue-512026-83-with-factory.patch, failed testing.

Berdir’s picture

exception 'PDOException' with message 'SQLSTATE[HY000]: General error: user-supplied statement does not accept constructor arguments' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php:322
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php(322): PDO->prepare('DELETE FROM sim...')
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php(521): Drupal\Core\Database\Connection->prepareQuery('DELETE FROM {ke...')
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Query/Delete.php(141): Drupal\Core\Database\Connection->query('DELETE FROM {ke...', Array, Array)
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php(158): Drupal\Core\Database\Query\Delete->execute()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php(63): Drupal\Core\KeyValueStore\DatabaseStorageExpirable->garbageCollection()
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Compiler/ServiceReferenceGraph.php(115): Drupal\Core\KeyValueStore\DatabaseStorageExpirable->__destruct()
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Compiler/ServiceReferenceGraph.php(115): Symfony\Component\DependencyInjection\Compiler\ServiceReferenceGraph->createNode()
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Compiler/ServiceReferenceGraph.php(94): Symfony\Component\DependencyInjection\Compiler\ServiceReferenceGraph->createNode('lock', Object(Symfony\Component\DependencyInjection\Definition))
#8 /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Compiler/AnalyzeServiceReferencesPass.php(105): Symfony\Component\DependencyInjection\Compiler\ServiceReferenceGraph->connect('user.tempstore', Object(Symfony\Component\DependencyInjection\Definition), 'lock', Object(Symfony\Component\DependencyInjection\Definition), Object(Symfony\Component\DependencyInjection\Reference))
#9 /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Compiler/AnalyzeServiceReferencesPass.php(72): Symfony\Component\DependencyInjection\Compiler\AnalyzeServiceReferencesPass->processArguments(Array)
#10 /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Compiler/Compiler.php(119): Symfony\Component\DependencyInjection\Compiler\AnalyzeServiceReferencesPass->process(Object(Drupal\Core\DependencyInjection\ContainerBuilder))
#11 /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php(449): Symfony\Component\DependencyInjection\Compiler\Compiler->compile(Object(Drupal\Core\DependencyInjection\ContainerBuilder))
#12 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/DrupalKernel.php(186): Symfony\Component\DependencyInjection\ContainerBuilder->compile()
#13 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/DrupalKernel.php(154): Drupal\Core\DrupalKernel->buildContainer()
#14 /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Kernel.php(133): Drupal\Core\DrupalKernel->initializeContainer()
#15 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/DrupalKernel.php(124): Symfony\Component\HttpKernel\Kernel->boot()
#16 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(509): Drupal\Core\DrupalKernel->updateModules(Array)
#17 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(1716): module_enable(Array, false)
#18 [internal function]: _install_module_batch('node', 'Node', Array)
#19 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/batch.inc(245): call_user_func_array('_install_module...', Array)
#20 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/form.inc(4930): _batch_process()
#21 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(533): batch_process('core/install.ph...', 'http://drupalte...')
#22 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(429): install_run_task(Array, Array)
#23 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(85): install_run_tasks(Array)
#24 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php(713): install_drupal(Array)
#25 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorTestBase.php(25): Drupal\simpletest\WebTestBase->setUp()
#26 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/aggregator/lib/Drupal/aggregator/Tests/FeedParserTest.php(23): Drupal\aggregator\Tests\AggregatorTestBase->setUp()
#27 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(694): Drupal\aggregator\Tests\FeedParserTest->setUp()
#28 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(381): Drupal\simpletest\TestBase->run()
#29 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('9', 'Drupal\aggregat...')
#30 {main}

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

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

Looking into #85.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
27.51 KB

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

katbailey’s picture

Any 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 :-/

Status: Needs review » Needs work

The last submitted patch, form-state-keyvalue-512026-87.patch, failed testing.

Berdir’s picture

Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
FileSize
723 bytes
27.81 KB

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

Status: Needs review » Needs work

The last submitted patch, form-state-keyvalue-512026-90.patch, failed testing.

Berdir’s picture

Better, but...

Field: Views Data 15 passes, 0 fails, and 0 exceptions
Segmentation fault
FATAL Drupal\views\Tests\Handler\FilterNumericTest: test runner returned a non-zero error code (139).
Node: Node Type field 2 passes, 0 fails, and 0 exceptions
Field: Field handler 24 passes, 0 fails, and 0 exceptions
Node: User has revision Filter 4 passes, 0 fails, and 0 exceptions
Node: Revision integration 3 passes, 0 fails, and 0 exceptions
FATAL Drupal\views\Tests\Handler\FilterStringTest: test runner returned a non-zero error code (1).

Segmentation fault is not good.

Edit: Checking if this is reproducable, the following tests failed:

Drupal\system\Tests\Bundle\BundleTest
Drupal\views\Tests\Handler\FilterNumericTest
Drupal\views\Tests\Handler\FilterStringTest
Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs backport to D7

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs backport to D7

The last submitted patch, form-state-keyvalue-512026-90.patch, failed testing.

Berdir’s picture

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

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.71 KB

Status: Needs review » Needs work

The last submitted patch, form-state-keyvalue-512026-83.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.42 KB

And here is the patch with the table exists check. Let's see how this behaves without the Kernel changes.

Status: Needs review » Needs work

The last submitted patch, form-state-keyvalue-512026-98.patch, failed testing.

Berdir’s picture

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

Berdir’s picture

Ok, here's the backtrace reported by gdb:


Program received signal SIGSEGV, Segmentation fault.
zval_mark_grey (pz=0x2ff2d50) at /build/buildd/php5-5.3.10/Zend/zend_gc.c:372
372	/build/buildd/php5-5.3.10/Zend/zend_gc.c: No such file or directory.
(gdb) bt
#0  zval_mark_grey (pz=0x2ff2d50) at /build/buildd/php5-5.3.10/Zend/zend_gc.c:372
#1  0x00000000006b739c in zval_mark_grey (pz=0x2ff2d50) at /build/buildd/php5-5.3.10/Zend/zend_gc.c:379
#2  0x00000000006b739c in zval_mark_grey (pz=0x2ff2d50) at /build/buildd/php5-5.3.10/Zend/zend_gc.c:379
#3  0x00000000006b739c in zval_mark_grey (pz=0x2ff2d50) at /build/buildd/php5-5.3.10/Zend/zend_gc.c:379
#4  0x00000000006b7ef9 in gc_collect_cycles () at /build/buildd/php5-5.3.10/Zend/zend_gc.c:405
#5  0x00000000006b8164 in gc_zval_possible_root (zv=0x2ff2d50) at /build/buildd/php5-5.3.10/Zend/zend_gc.c:166
#6  0x00000000006a7e20 in zend_hash_destroy (ht=0x3824dc0) at /build/buildd/php5-5.3.10/Zend/zend_hash.c:729
#7  0x00000000006994cf in _zval_dtor_func (zvalue=0x2ea6c70) at /build/buildd/php5-5.3.10/Zend/zend_variables.c:46
#8  0x000000000070ab3f in ZEND_ASSIGN_SPEC_CV_VAR_HANDLER (execute_data=0x7ffff7ed33c8) at /build/buildd/php5-5.3.10/Zend/zend_variables.h:35
#9  0x00000000006c036b in execute (op_array=0x128f8b0) at /build/buildd/php5-5.3.10/Zend/zend_vm_execute.h:107
#10 0x000000000069b8d0 in zend_execute_scripts (type=1528496254, retval=0x87e363b32, file_count=3) at /build/buildd/php5-5.3.10/Zend/zend.c:1308
#11 0x0000000000647f43 in php_execute_script (primary_file=0x7ffff5f02cc0) at /build/buildd/php5-5.3.10/main/main.c:2323
#12 0x000000000042c797 in main (argc=32767, argv=0x7fffffffe4eb) at /build/buildd/php5-5.3.10/sapi/cli/php_cli.c:1188

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

Berdir’s picture

#1825450: Use lock service in lock() might be related to this, also getting segmentation faults there, but mostly on page requests.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs backport to D7

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs backport to D7

The last submitted patch, form-state-keyvalue-512026-98.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.52 KB

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

Status: Needs review » Needs work

The last submitted patch, form-state-no-destruct-512026-105.patch, failed testing.

catch’s picture

If __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.:

$this->connection = &$connection;

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.

sun’s picture

PHP 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 in drupal_static() that are invoked upon drupal_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 via drupal_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 in tearDown().

Berdir’s picture

Yes, 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 :)

katbailey’s picture

Status: Needs work » Needs review
FileSize
12.35 KB

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

Berdir’s picture

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

Berdir’s picture

Ok, 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? :)

Lars Toomre’s picture

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

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterServicesForTerminationPass.phpundefined
@@ -0,0 +1,36 @@
+ * @file
+ * Definition of Drupal\Core\DependencyInjection\Compiler\RegisterServicesForTerminationPass.

Should be 'Contains' instead of 'Definition of'. Elsewhere in patch as well.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterServicesForTerminationPass.phpundefined
@@ -0,0 +1,36 @@
+
+class RegisterServicesForTerminationPass implements CompilerPassInterface {
+  public function process(ContainerBuilder $container) {

Missing class docblock here. Also missing method docblock.

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.phpundefined
@@ -158,4 +150,13 @@ protected function garbageCollection() {
+  /**
+   * Implements Drupal\Core\TerminationInterface::terminate().

I think the docs standard calls for a leading '\' here. Elsewhere in patch as well.

+++ b/core/lib/Drupal/Core/ServiceTerminator.phpundefined
@@ -0,0 +1,58 @@
+  /**
+   * Holds an array of service ids that will require termination.

'ids' should be 'IDs'.

+++ b/core/lib/Drupal/Core/ServiceTerminator.phpundefined
@@ -0,0 +1,58 @@
+  /**
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container

Needs one line description.

+++ b/core/lib/Drupal/Core/ServiceTerminator.phpundefined
@@ -0,0 +1,58 @@
+
+  public function registerService($id) {
+    $this->services[] = $id;
+  }
+
+  public function onKernelTerminate(PostResponseEvent $event) {
+    foreach ($this->services as $id) {

Missing docblocks.

Berdir’s picture

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

Status: Needs review » Needs work

The last submitted patch, form-state-service-termination-512026-114.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
3.83 KB
17.3 KB

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

Crell’s picture

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

Berdir’s picture

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

Crell’s picture

Perhaps lock should keep its destructor as a fallback? That at least sometimes works. :-)

catch’s picture

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

Berdir’s picture

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

katbailey’s picture

+++ b/core/modules/system/tests/modules/bundle_test/lib/Drupal/bundle_test/TestClass.phpundefined
@@ -15,6 +16,17 @@
+  protected $state;
+
+  public function __construct(KeyValueFactory $keyvalueFactory) {
+    $this->state = $keyvalueFactory->get('state');
+  }

I 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

Berdir’s picture

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

Status: Needs review » Needs work

The last submitted patch, form-state-service-termination-512026-123.patch, failed testing.

Berdir’s picture

This should fix the tests.

And the patch keeps getting bigger and bigger :( Time to get this in?

dawehner’s picture

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

+++ b/core/lib/Drupal/Core/EventSubscriber/KernelTerminateSubscriber.phpundefined
@@ -0,0 +1,74 @@
+    $events[KernelEvents::TERMINATE][] = array('onKernelTerminate', 100);

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.

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.phpundefined
@@ -158,4 +150,13 @@ protected function garbageCollection() {
+   * Implements Drupal\Core\TerminationInterface::terminate().

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueDatabaseExpirableFactory.phpundefined
@@ -28,6 +36,17 @@ class KeyValueDatabaseExpirableFactory extends KeyValueDatabaseFactory {
+   * Implements Drupal\Core\TerminationInterface::terminate().

Nitpick: \Drupal\Core ... or just relative.

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.phpundefined
@@ -158,4 +150,13 @@ protected function garbageCollection() {
+    if ($this->needsGarbageCollection && $this->connection->schema()->tableExists($this->table)) {

We maybe should document how the table could not exist.

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueDatabaseExpirableFactory.phpundefined
@@ -28,6 +36,17 @@ class KeyValueDatabaseExpirableFactory extends KeyValueDatabaseFactory {
   public function get($collection) {
-    return new DatabaseStorageExpirable($collection, $this->connection);
+    $storage = new DatabaseStorageExpirable($collection, $this->connection);
+    $this->storages[] = $storage;
+    return $storage;
+  }

I'm wondering whether we should create multiple storages for the same collection?

+++ b/core/lib/Drupal/Core/Path/AliasManager.phpundefined
@@ -78,9 +78,9 @@ class AliasManager implements AliasManagerInterface {
+  public function __construct(Connection $connection, KeyValueStoreInterface $state) {
...
+    $this->state = $state;

The variable is a KeyValueStoreInterface, so $this->state documentation should be better updated as well (is marked as databaseStorage, so maybe out of scope).

Berdir’s picture

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.

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

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterServicesForTerminationPass.php
@@ -0,0 +1,46 @@
+    // Reverse the order of tagged services to terminate them in the opposite
+    // order of which they are defined.
+    // @todo: Does this need a better system to detect references?

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

Berdir’s picture

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

effulgentsia’s picture

Rerolled for HEAD changes.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Needs review

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

effulgentsia’s picture

Status: Needs review » Needs work

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

sun’s picture

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

Berdir’s picture

Berdir’s picture

Well, @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...

Berdir’s picture

Status: Needs work » Needs review
FileSize
9.49 KB

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

Status: Needs review » Needs work

The last submitted patch, form-state-keyvalue-137.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
839 bytes
9.49 KB

Does that fix the installation?

Status: Needs review » Needs work

The last submitted patch, form-state-key-value-512026-140.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
14.04 KB
4.55 KB

Ok, but this should. We need a null implementation during installation.

Status: Needs review » Needs work

The last submitted patch, form-state-key-value-512026-141.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
16.58 KB

This should fix the views test fails. We could alternatively also use the null backend there.

alexpott’s picture

This looks terrific!

Just wondering why...

Re-roll, changed both "caches" to keyvalue.expirable, as discussed with @catch. Completely untested

I get why form state is moving to a keyvalue.expirable collection but why the regular form cache?

Berdir’s picture

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

alexpott’s picture

Rerolled patch to use Drupal::service() instead of drupal_container()... also this code is weird (not just the changed code - the entire condition)...

@@ -921,8 +920,8 @@ function drupal_process_form($form_id, &$form, &$form_state) {
       // here, though.
       $config = config('system.performance');
       if (!$config->get('cache.page.use_internal') && !empty($form_state['values']['form_build_id'])) {
-        cache('form')->delete('form_' . $form_state['values']['form_build_id']);
-        cache('form')->delete('form_state_' . $form_state['values']['form_build_id']);
+        drupal_container()->get('keyvalue.expirable')->get('form')->delete($form_state['values']['form_build_id']);
+        drupal_container()->get('keyvalue.expirable')->get('form_state')->delete($form_state['values']['form_build_id']);
       }

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.

Berdir’s picture

Issue tags: -Needs backport to D7

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, 512026.form-state-key-value.146.patch, failed testing.

effulgentsia’s picture

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.

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

From what I understand, there isn't really a difference between the two.

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
11.5 KB

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

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, 512026.form-state-key-value.150.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, 512026.form-state-key-value.150.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
676 bytes
11.63 KB

Oh, forgot to implement the new deleteAll() method.

chx’s picture

Status: Needs review » Reviewed & tested by the community

I think all questions are addressed by now. I have no idea why this is marked with a backport though.

msonnabaum’s picture

This 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:


/**
 * Defines the key/value store factory for the database backend.
 */
class KeyValueNullExpirableFactory {
Berdir’s picture

Fixed that. Not sure about the backport tag either.

xjm’s picture

@catch added the backport tag going on two years ago in #70, presumably because of #65:

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

+++ b/core/modules/system/system.installundefined
@@ -2158,6 +2156,13 @@ function system_update_8051() {
 /**
+ * Remove {cache_form} table.
+ */
+function system_update_8052() {
+  db_drop_table('cache_form');
+}

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?

Berdir’s picture

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

xjm’s picture

Issue tags: -Needs backport to D7

This is covered by tests, various ajax-related things fail if this doesn't work correctly.

Personally 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:

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

Let's open a followup to do that for D7?

Berdir’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, broken_form_cache-512026-161.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests

Patches failed as expected, setting back to needs review. I'll see what I can do about API tests.

Berdir’s picture

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

Berdir’s picture

Noticed a small bug in the tests.

xjm’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
FileSize
2.53 KB

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

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
1.33 KB
16.02 KB

Re-roll and removed two @todo's added by the cache DIC issue.

Status: Needs review » Needs work

The last submitted patch, form-cache-512026-167.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
17.19 KB

Also need to remove the cache bin from the browser, obviously...

xjm’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Okay, I think this is ready now. :) Let's file that followup for the unit test user ID bug thing and link it here?

Berdir’s picture

xjm’s picture

#169: form-cache-512026-169.patch queued for re-testing.

Berdir’s picture

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

xjm’s picture

Title: Move $form_state storage from cache to new state/key-value system » Move $form_state storage from cache to new key-value system

We're not using state(); it's a separate kvstore implementation.

Dries’s picture

Title: Move $form_state storage from cache to new key-value system » Move $form_state storage from cache to new state/key-value system
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

xjm’s picture

Questions raised while reviewing this that we should either document here or address in followups:

  1. There is a lot of conceptual overlap between this and the TempStore (which also uses the kvstore, but adds its own factory to create individual collections per user or anonymous session). I've asked about this on IRC a couple of times and folks (@berdir?) have given me an explanation as to why, but that's not documented on this issue. So, we'd suggest opening a followup to discuss that further.
  2. Moshe raised a concern about whether using the NULL Null (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.
  3. Moshe was concerned about whether this would interfere with efforts to add state caching (on account of the earlier title), but this patch doesn't use state(). It's a separate, expirable implementation.
  4. This issue was originally filed as a bug report for D7 (since the form state can't be regenerated, and is therefore not a cache). Per #37, the most backportable solution is probably:

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

    So let's open a separate issue to address that and fix mitigate the D7 bug.

Berdir’s picture

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

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

Issue summary: View changes

This 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 code can never ever be called... $form_state['values']['form_build_id'] is never set

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.

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.

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.

xjm’s picture

Issue tags: -Needs backport to D7

Removing the tag; it was re-added by System Message crossposting with @Berdir back in the day.