follow-up to: #254491: Standardize static caching

see last patch examples: http://drupal.org/node/254491#comment-1430180 and also: http://drupal.org/node/224333#static_variable_api

Apply this conversion pattern to includes/form.inc to convert all static variables there. Pay close attention to any place a reset parameter is provided and add a call to drupal_static_reset() where appropriate (e.g. in any calling function that uses the reset parameter)

CommentFileSizeAuthor
#23 jamesan_422362-23.patch6.21 KBJamesAn
#19 jamesan_422362-19.patch5.92 KBJamesAn
#17 jamesan_422362-17.patch4.49 KBJamesAn
#13 jamesan_422362-13.patch5.87 KBJamesAn
#6 jamesan_422362-6.patch7.03 KBJamesAn
#1 jamesan_422362.patch6.92 KBJamesAn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JamesAn’s picture

JamesAn CreditAttribution: JamesAn commented
Status: Active » Needs review
FileSize
jamesan_422362.patch6.92 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

JamesAn CreditAttribution: JamesAn commented

Oops. I missed something. I'll get to it tomorrow.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Per discussion w/ catch - if you have a variable name more complex than just __FUNCTION__, use a ':' to separate the suffix (this will avoid colliding with any other valid function name). e.g. :

&drupal_static(__FUNCTION__ . ':second_var');
JamesAn’s picture

JamesAn CreditAttribution: JamesAn commented
Status: Needs work » Needs review
FileSize
jamesan_422362-6.patch7.03 KB

Try, try, try again!

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

This patch seems to add a couple resets that were not present in the existing code, such as:

   $t = get_t();
@@ -786,6 +786,7 @@ function _form_validate($elements, &$for
 
       if (isset($elements['#options']) && isset($elements['#value'])) {
         if ($elements['#type'] == 'select') {
+          drupal_static_reset('form_options_flatten');
           $options = form_options_flatten($elements['#options']);
         }

Can you explain the reasoning behind this?

JamesAn’s picture

JamesAn CreditAttribution: JamesAn commented

Sorry for the lack of explanation. Those resets were supposed to be there in the first place.

form_options_flatten() resets on every call by default, unlike some of the other reset features. That wasn't taken into account in the first patch, but it's reflected here with additional reset calls.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

If all current calls to it reset, maybe that logic should be preserved, rather than requiring an extra function call each time - probably need input from eaton, chx, or some other FAPI guru.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Needs work

explanation from chx:

the function recurses, and while recursing keeps the value, when coming from outside it resets itself.
i would say, it's totally pointless to convert, just add a comment.

chx suggests we open a separate issue to clean up function form_options_flatten(), but leave it alone in this patch.

Dries’s picture

Dries CreditAttribution: Dries commented

I think it is bad form for function A to blow out the static variable from function B. Regular static variables can't do that. Only tests should blow away a static variable from underneath another function. If function A blows out a static variable from function B, we might as well start using goto's. Bring back the $reset parameter please -- it makes for more readable and predictable code.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@Dries - part of the point was to eliminate the reset parameters that make for odd and unpredictable function signatures. We absolutely want to be able to externally clear the static cache - see the taxonomy module now. For example, we clear cached data about vocabularies whenever a vocabulary is saved or deleted. This is exactly what we want to be able to do.

These static variables are just a temporary cache - the only effect of wiping them is the cost of rebuilding the data.

JamesAn’s picture

JamesAn CreditAttribution: JamesAn commented
FileSize
jamesan_422362-13.patch5.87 KB

Ok. The patch ignores the reset parameter in form_options_flatten() and adds a comment referencing this discussion.

JamesAn’s picture

JamesAn CreditAttribution: JamesAn commented
Status: Needs work » Needs review

%*&@#$^!!

I'm sure I flipped the status switch here. Methinks sometimes the status is not switched if I select a change and then upload the file. =\ Or maybe I'm just forgetful.. -.-

JamesAn’s picture

JamesAn CreditAttribution: JamesAn commented

I've opened an issue for form_options_flatten() at #437018: convert form_options_flatten() in form.inc to use new static caching API. I've proposed a solution there as well.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Needs work

Minor improvement needed - this does not conform to our normal code style:

+// $reset has been ignored here as per http://drupal.org/node/422362#comment-1438376 discussion.
 function form_options_flatten($array, $reset = TRUE) {

Code comments before the function should be 1-line doxygen describing the function.

You could include a show comment within the function body summarizing why the conversion was not done, but I would not reference the issue directly. Generally those references in code are only in rare cases where we are working around a bug expected ot be fixed later.

JamesAn’s picture

JamesAn CreditAttribution: JamesAn commented
Status: Needs work » Needs review
FileSize
jamesan_422362-17.patch4.49 KB

Sorry for the delay.. here's the fixed patch.

The comment is moved into the function and says:

+  // $reset has been ignored here as the function recurses, retaining
+  // its value while recursing and resetting itself its called.

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

JamesAn CreditAttribution: JamesAn commented
Status: Needs work » Needs review
FileSize
jamesan_422362-19.patch5.92 KB

Hrm. That patch I uploaded didn't even include all the changes.

Here's the correct patch.

Dries’s picture

Dries CreditAttribution: Dries commented

drupal_static_reset('form_set_error') a developer can't discover because it doesn't have API documentation, form_set_error(NULL, '', TRUE) is a bit ugly. Better than calling drupal_static_reset('form_set_error') or form_set_error(NULL, '', TRUE) is calling something like form_clear_error() -- this can be documented and would be nice, clean and intuitive.

eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
CreditAttribution: eojthebrave commented

This comment,

+  // $reset has been ignored here as the function recurses, retaining
+  // its value while recursing and resetting itself its called.

Should probably be something like

+  // $reset has been ignored here as the function recurses, retaining
+  // its value while recursing and resetting itself when called.
eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
CreditAttribution: eojthebrave commented
Status: Needs review » Needs work

Oops. Changing status to needs work.

JamesAn’s picture

JamesAn CreditAttribution: JamesAn commented
FileSize
jamesan_422362-23.patch6.21 KB

@Dries, form_clear_error() sounds like a decent compromise to me.
@eojthebrave, thanks for catching that typo! I do that occasionally.. ^^"

Try, try, try again...

JamesAn’s picture

JamesAn CreditAttribution: JamesAn commented
Status: Needs work » Needs review

testbot?

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Reviewed & tested by the community

Looks like a nice clean-up to me. Will commit later unless someone objects.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Looks fine to me, and form_clear_error() seems like a decent pattern we can use elsewhere (I think we're already doing that elsewhere when there's more than one static which needs to be cleared).

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

I agree that this is an acceptable pattern ... Committed to CVS HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -Novice

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

Issue tags: +Novice

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.