convert mail.inc, module.inc, and pager.inc to use new static caching API

Gah. Forgot to flip the switch.

The last submitted patch failed testing.

Let's try again. ^^"

Just double checking:

The static class member, $maxElement, on line 23 of pager.inc. Can that use the drupal_static function?

The last submitted patch failed testing.

@JamesAn

No, static class members/variables should imho not be converted.

I'm not sure how that would work - we would like some mechanism to reset them, but if it doesn't work for the first pass, then we can revisit it.

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

In the current patch, we're generating variables like 'module_liststatic_list' - this is a bit hidden by using the unnecessary concatenation between the two strings, but we should avoid underscores or munging together for these variables because there's a chance it'd conflict with another function name.

Discussed this with pwolanin in irc and we came up with
drupal_static(__FUNCTION__ . ':suffix');

So the drupal_static_reset here would be drupal_static_reset('module_list:sorted_list');

Just wondering, if a function has multiple static variables, we would need to call clear_cache() multiple times and know how they are called then? A way to clear all static vars of a specific function would be nice...

About the class members, it would technically be possible to convert it to a non-static and call drupal_static() in the constructor, but I think that's not a good idea, because it would need special handling for static methods and so on.

@Berdir - depends on how they are used.

If you look at what I did in taxonomy module - in the one case w/ multiple vars it's constructed such that cleaning the one that matches the function name clears all the rest too as a side effect.

Ok, I ignored the static class member.

Oops. Forgot to flip the status again. -.-"

This one shoudl probably not be converted:

As far as I can tell - the static is just there so that the array doesn't need to be repeatedly declared - it has a constant value, so a reset is meaningless.

Makes sense. I removed the if statement as unnecessary logic so that it looks:
static $supported_tags = array('a', 'em', 'i', 'strong', 'b', 'br', 'p', 'blockquote', 'ul', 'ol', 'li', 'dl', 'dt', 'dd', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'hr');

I'm not sure. I don't think function A should blow away function B's static variable, except when function A is a test. It's really messy for one function to mess with another function's innards. Let's keep the reset parameters and only allow tests to use drupal_static_reset()?

@Dries - no, that suggestion makes no sense to me at all. You are not "messing" with innards - we are just providing a uniform way to clear *any* static cache without actually calling the function. Again, this is going to be very useful - see above example in taxonomy. How can I get this point across?

No, it is better to call the original function with a $reset parameter instead of drupal_static_reset() because:

1. Static variables are a known design pattern instead of a Drupal-ism.
2. I can quickly jump to the function declaration using my IDE.
3. I don't have to look at the source code of the function to see that the function can be reset, or that the function has an internal cache.
4. The function has a designed API, proper API documentation and behaves according to an API contract. It doesn't render api.drupal.org unusable.
5. It doesn't break IDEs that report all callers of my function and makes debugging and refactoring easier.
6. I can have a richer API; with a single $reset parameters I can do more than one thing.
7. It is more robust when I rename my function. I'll get a 'function undefined' error instead of silently being ignored in case I make a mistake. At least SimpleTest will catch my error.
8. It forces good API design. It forces people to think twice when writing code.

This sounds like Computer Science 101 to me. As a rule, only SimpleTest should mess with a function's static variables and internal workings.

1. Yes, but most of them aren't real statics anymore.
2. Is not needed anymore. drupal_static_reset('function_name'); You don't need to know which parameter reset is (first, second, third, key of $options. Everything exists currently)
3. This is true. But there are for example functions (like drupal_add_js) which hide the reset flag in array $options. I doubt that many IDE's will display that. We could maybe add a hint to the apidoc, that the function uses a "static" cache. Either a simple text or even @static_cache.
5. This is correct, but grep should still work :). One could also say that it does not "use" the function :)
6. The function could do "stuff" when the static cache is not set. See also #11 and #12
7. True. But we could check with drupal_function_exists() in drupal_static_reset() and generate a warning if there is no such function.

@Dries - none of your points solve the problem of "how do I clear the static cache in node_load() from within node_save()". In addition to enabling better testing, this is the key functionality enabled by this API, as far as I am concerned. In Drupal 6 this is impossible.

In all these patches we need to double-check that the final code is sensible - there will be a few statics that should not be converted since they play some role other than as a cache for expensive calculations.

@pwolanin, I'm not suggesting that we get rid of the new drupal_static_reset(). We can use it in .test files to enable better testing -- I support that. That is the reason I committed the original drupal_static_reset() patch. What I'm saying is that drupal_static_reset() shouldn't be used in regular code, for the reasons outlined above. It should be a test-ism only.

It seems a little odd to have a drupal_static_reset() function and then tell people not to use it. If that's the case, it should be changed to a private function within the Simpletest module. But Simpletest is code just like anything else, so if it's bad form to use it in other code, it should be bad form to use it there too...

I think if you really want a "Computer Science 101" solution you would not do this whole drupal_static() thing at all. Instead, just lay down the following rules that module authors are expected to follow:

1. If you ever use a static variable that might possibly need to be cleared, your function must have a $reset parameter.
2. Your module must also implement a hook that resets all its static caches at once, for example:
   <?php
function node_reset_static_cache() {
  node_get_types('types', NULL, TRUE);
  node_load_multiple(array(), array(), TRUE);
  // Clear any other static caches in node.module here too...
}
?>

   In that case, Simpletest (or anyone else) could clear all the caches just by doing this:

   <?php
  module_invoke_all('reset_static_cache');
?>

That would certainly work (and maybe it's even what should be done). But my understanding is that the rationale behind this whole drupal_static() thing was that the reset parameters were perceived to be undesirable and that it was a pain for developers to keep having to deal with them.

@pwolanin: The node_load()/node_save() issue is a bug which is possible to fix in Drupal 6. In fact I just posted a patch :) See #221081: node_load cache out of sync with node_save

I don't think we can always guarantee that function(NULL, NULL, TRUE) is going to be possible - case in point taxonomy_get_vocabularies()

http://api.drupal.org/api/function/taxonomy_get_vocabularies/7

taxonomy_term_load_multiple() with all the arguments set to empty arrays returns /all/ the vocabularies - I don't want to have to do that just to reset the static.

Well, a reset parameter can be written to behave however you want - so the function could just have it so that if $reset is TRUE, it clears the static and bails out immediately without bothering to load or return anything.

But yes, there's a point at which a lot of special case code would be added just to deal with these various $reset cases... and with the other approach you do not have to worry about that at all.

That's true - but in terms of upgrade path we have a bunch of code which already calls the $reset. Which means instead of "remove this reset parameter, in most cases this'll be handled internally by the API but otherwise use drupal_static_reset()" - we'd have "call the function once with the reset, then call it again without". Which I guess is fine, but just seems crufty.

Also, certain functions such as the theme() function have static variables, yet it cannot have a reset flag as a function argument because it can have an arbitrary number of arguments. The only clean way to clear that static cache is to delegate it to a third party function like drupal_static(). The nice thing about drupal_static() is it's a centralized API - I don't have to look up the order of parameters that a function takes in order to figure out how to reset its static cache.

I made a few changes from #16:

• I forgot to remove the $reset param of _drupal_html_to_mail_urls().
• I moved the function call to initialize $regexp of _drupal_html_to_mail_urls() into an if statement so it's not always called.
• I made $string of pager_get_querystring() statically cached with drupal_static as per #6 of #422366: convert database drivers to use new static caching API.

The last submitted patch failed testing.

Oy vey.. embarassing PHP syntax errors. ^^" All fixed.

Please do not remove $reset/$refresh parameters and only call drupal_static_reset() from test functions.

@Dries - they *should* be removed wherever possible. One might argue, for example, that calling the reset param is pretty fundamental to module_list(), so it should be left, but some of the other ones might be reasonable to remove.

What is the policy on this? The API update guidelines only give one example for the static cache, and that is menu_rebuild() calling drupal_static_reset(). It doesn't mention cases where the $reset parameters are to be kept - in the relevant issue (#254491) $reset is even repeatedly called an anti-pattern to be universally removed. If there are exceptions, then could we reflect that in the docs?

--

I looked over the code but noticed no obvious problems.

Arancaytar, there's no policy. I think most of us arguing for the static API wanted to remove $reset parameters with it, but Dries apparently doesn't.

I left reset in node_load when I converted it to drupal_static() for exactly the encapsulation reasons mentioned by Dries. I think they are quite sane. Internally, the $reset can trigger a drupal_static_reset(__FUNCTION__)

So what's the consensus/decision on this?

There isn't much consensus. If I was you, I would heed the advice of a committer (i.e. Dries).

Hmm.. okies.

How about creating a separate function to make drupal_static_reset() calls as suggested in #422362-20: convert form.inc to use new static caching API? Seems like a nice compromise to me. We get to make rigorous use of drupal_static_reset(), which seemed to be the intent of it, and remove the use of $reset params, all the while maintaining readable code.

Instead of calling stuff like drupal_static_reset('form_set_error') and any other drupal_static_reset() calls if the function has additional static vars, we can call something like form_clear_errors, which will make whatever drupal_static_reset() is necessary.

Yes/no?

I've added two reset functions: module_list_reset() and _drupal_html_to_mail_urls_reset() to make internal calls to drupal_static_reset(). Where the $reset param was used, I've placed these reset functions.

Personally, I don't think wrapper function proliferation is the answer. I have no issue with the $reset params. We can call drupal_static_reset() for really ugly cases like taxonomy_get_vocabularies(). Dries already gave pretty firm direction in this issue. See #32, #37.

@moshe - latest advice from Dries on this was http://drupal.org/node/422362#comment-1645324 and onwards - which suggests wrappers.

The last submitted patch failed testing.

Rerolled.

The last submitted patch failed testing.

Rerolled again.

The last submitted patch failed testing.

The "Module API" test must've changed since I made this patch, as there's now one more module_list reset line that the patch needs to change.

The last submitted patch failed testing.

Hmm..

The last submitted patch failed testing.

subscribing