Use a consistent/clean pattern for using $reset or drupal_static() [#581626]

Offshoot from #300993: User roles and permissions API, #574002: Remove $reset argument from user_access(), and #574796: "Article" content-type permissions not granted to admin role after installation.

The $reset argument has been removed from a few functions to leverage the new drupal_static() we have in D7 now.

In the case of the first two issues, it was basically required to remove the $reset argument, because the API functions exposing that $reset argument did not only clear their caches when passing $reset = TRUE, but they also auto-regenerated their data when being invoked that way. Multiple functions need to flush/reset the static cache after performing an action, from which some are kind of helper functions that may be called by another function. Invoking one of the helper functions directly as well as invoking one of the other functions must lead to a cache flush, so the (now removed) $reset parameter would have had to be passed multiple times, which would have lead to multiple re-generations of the statically cached data.

By replacing that $reset argument with a drupal_static() and just invoking drupal_static_reset() in all functions that perform actions on that data, we resolved that auto-rebuilding of statically cached data.

Of course, the same could have been achieved by completely altering the logic of the $reset argument. So $reset would just reset, but not automatically rebuild any data. But then again, invoking for example user_access(NULL, NULL, TRUE) doesn't really look very clean either. The $reset argument mainly made sense for the rare case where you want to retrieve new data for something, but ensure you get new data, e.g. as in user_access('access content', $account, TRUE).

Of course, the same could be achieved by introducing helper functions instead of using $reset arguments, e.g. user_access_reset(). However, given the amount of statics we have, this would most likely lead to a bloat of *_reset() helper functions.

I can see and understand the point that it is wonky to have one function to kill the static cache of another function. That can lead to some unexpected and very bad usage in contributed modules, especially when core "teaches" this pattern and development practice.

Let's discuss. :)

Comment	File	Size	Author
#13	reset_listeners.php_.v2.txt	1.65 KB	donquixote

Comments

Comment #1

moshe weitzman commented 19 September 2009 at 01:54

I think user_access_reset() is the best solution. I'm not to worried about lots of reset functions. It is a pattern that we can all get used to.

Comment #2

dries commented 19 September 2009 at 10:51

I agree with Moshe that we should have explicit APIs for resetting internal variables.

(1) As a rule, drupal_static_reset() can only be called from the test framework or from the function that defines the static variable.

If we want to relax that rule we could say:

(2) As a rule, drupal_static_reset() can only be called from the test framework or from the API group that defines the static variable.

I argued about this at length before -- I can look up and link to my old arguments if desired.

Comment #3

sun

German

Karlsruhe

commented 19 September 2009 at 17:12

I think that the relaxed, second variant makes more sense - modules can always do whatever they want to their own data and caches.

Both variants require us to expose a *_reset() function - at minimum for the most common API functions of modules. (That said, what is the rule for API functions not exposing a reset function? And if we forget to add a reset function somewhere?)

In the case of user_access(), the reset function name is fairly trivial and unique: user_access_reset()

However, in the case of node_type_get_types(), we have a drupal_static() in _node_types_build(), and a couple of other functions rely on this statically cached data. The last mentioned patch above removed the reset function node_type_clear(). To introduce a common pattern, we would probably just re-introduce a new node_type_reset() function.

So, a common pattern may be [module]_[object-list/feature]_reset()

Re-thinking the "what if we forget" question and the two variants: It seems like we could probably avoid missing any reset function if we ensure that all drupal_static_reset() invocations in tests can be replaced with reset function invocations.

Potential candidates:

node_type_reset()
node_revision_access_reset()
system_admin_menu_reset()
system_admin_tasks_reset()
system_theme_reset()
system_region_reset()
user_access_reset() (also clears 'user_role_permissions' static)
...

Though the more interesting apparently live in includes:

drupal_retrieve_form_reset() (or just form_forms_reset()?)
form_clean_id_reset()
batch_get_reset()
menu_get_item_reset() or menu_item_reset()
menu_tree_reset()
menu_tree_all_data_reset()
menu_get_names_reset() or menu_names_reset()
menu_local_tasks_reset()
...

Of course, not all of those perhaps need a reset function. I think it outlines that we can easily introduce a naming pattern for reset functions for statics in modules, but finding a sane pattern for those in include files won't be easy.

A logical naming pattern is important for good DX. We certainly can (and should) add @see statements to all related functions of a reset function (and vice versa), but ideally, those function names should be recallable just like the other API functions.

Comment #4

Crell commented 20 September 2009 at 17:04

So what would the logic of all of these resets look like? Would they all be a variant on:

function foo_thing_reset() {
  drupal_static('foo_thing', NULL, TRUE);
}

That seems kinda icky to me, even if it is standardized.

Comment #5

yched commented 23 September 2009 at 19:19

Comment #6

sun

German

Karlsruhe

commented 24 September 2009 at 18:45

Those would look like:

/**
 * Resets the static cache of all text formats.
 *
 * @see filter_formats()
 */
function filter_formats_reset() {
  drupal_static_reset('filter_list_format');
  drupal_static_reset('filter_formats');
}

Comment #7

sun

German

Karlsruhe

commented 24 September 2009 at 18:50

well, additionally clearing own database caches, if existent.

When also considering db caches, I wonder whether "_reset" is a valid/self-describing suffix though.

Comment #8

moshe weitzman commented 25 September 2009 at 19:41

Brain storming. How about we skip all those reset functions and instead expand the usage of the $reset param. It would not expect a boolean but rather NULL, or 2 constants:


// For quick purge of variable(s) from drupal_static
define('RESET', 1)

// Purge cache and then run the rest of function
define('RESET_AND_PROCEED', 2)

Comment #9

catch

he/him

English

commented 7 October 2009 at 02:15

I've been leaning towards the module_object_reset() approach but I'm not tied to it. The current situation in core is a mix of both and looks messy though, so this really needs to be cleaned up before release.

Comment #10

David_Rothstein commented 11 November 2009 at 14:35

Agreed this is kind of a mess as is. Moshe's idea seems nice - and in many ways makes drupal_static() completely unnecessary (assuming the modules actually follow correct patterns and clear their caches when they need to).

I think either way, a module_object_reset() wrapper function provides the best code readability, although really perhaps it should be _module_object_reset() since nobody outside the API group is supposed to need to call it.... I guess we are limited in what we want to change at this point in the development cycle, though.

Also, there is a somewhat related issue in which there are number of locations throughout core that are (deliberately) using regular PHP statics rather than drupal_static(). Should the pattern be that those instances should always be documented? Do we need to clearly define for contrib modules when you need to use drupal_static() and when you shouldn't bother?

Comment #11

sun

German

Karlsruhe

commented 11 November 2009 at 19:01

re: @moshe #8:

We need to cater for two possible "reset" operations that are different:

1) Just resetting the static, without any directly following rebuilding.

2) Resetting the static and rebuilding.

I'm not sure which one it was, but I at least had to introduce a new foo_bar_reset() function to allow multiple functions (a recursive stack of save operations) to reset a static without directly rebuilding the information, because the cached information should just be flushed, and the new information would only be needed when another function after the save process wants to access it.

If the cached information was directly rebuilt, then it would have been rebuilt several times within the same request, and on top of that, without even knowing whether something else has any use for it.

Comment #12

effulgentsia commented 12 November 2009 at 01:36

subscribing.

re: #10: PHPDoc for drupal_static() is included in #619666: Make performance-critical usage of drupal_static() grokkable, patch 34. This issue, however, may affect what we want to say there.

Also, cross-linking to #629452: Roll back drupal_static(), in case this issue results in drupal_static() going out of favor entirely.

Comment #13

donquixote commented 13 November 2009 at 23:20

Status	File	Size
new	reset_listeners.php_.v2.txt	1.65 KB

I would like to propose a listener system, where functions can subscribe their static variables to global events.

Benefits:
- You can request a reset without knowing about the listeners.
- Overhead only on first run and after a reset. No overhead in the average repeated function call. Except for the string compare to 'first run', which can hopefully be done in a different way.

Example code is attached. It should run out of the box, if you rename it and copy it into a web-accessible location.

I proposed the same thing in the other thread, #629452: Roll back drupal_static(), but I think here is the better place. And the new code actually works :)