Convert field to the new static caching API

The only $reset parameters in Field API code were for _field_info_collate_types and _field_info_collate_fields. In fact, we had a few static caches that were not resettable (oops).

Patch attached.

Ooops, I forgot an & before a call to drupal_static.

Looks good, indeed. I wasn't sure why you set a default array() value for $allowed_values when the static call hadn't done so earlier, but it looks better this way.

Although apparently you can implicitly cast an uninitialized variable to array without a Notice by just assigning an array element (as long as it's with an explicit key).

looks ok to me, as long as there are no tests that need to be updated to match.

We should not call drupal_static_reset() from non-test code. Use proper $reset parameters instead in regular code. Only tests should call drupal_static_reset().

[ Setting back to RTBC so I know Dries will look at it. ]

Ummmm... what? You want me to put back the $reset parameters? The whole point (I thought) of the static cache API was to eliminate the need for that code pattern. My calls to drupal_static_reset() are not wiping the global cache, they are wiping only the caches that Field API maintains internally. If this is not how drupal_static_reset() is supposed to be used, then either I'm confused, or we need a new static API function, or the static API was a mistake (which I do not think it was).

I'll try to get some more feedback.

It is important that the test code can wipe the static variables -- it makes it possible to write code that is testable and that doesn't have weird side-effects. By registering static variables using drupal_static(), the test framework can now blow all static variables. That was the intent of the static variables registry patch; improving testability.

However, I don't think drupal_static_reset() is something that should be used in regular code because it is a Drupal-ism. I prefer to keep using $reset parameters in regular code because that is how people expect static variables to be cleared. Plus, it helps to "prevent" that foo.module does a drupal_static_reset() on a fields variable.

@Dries - I disagree 100% with you. Having a $reset param with unknown side effects is at least as much of A Drupalism and makes the Drupal API harder to use and less consistent. My intention with the statics API was always that we should remove all these $reset params wherever possible.

Per Barry's excellent DX series, we should also look at removing as many statics as possible to only retain those with real performance benefit.

I am with pwolanin 100%, So what if foo module does a static reset? It's a performance hit at most but surely modules wont throw static resets around just because they can. I do not see how calling the func with a reset param from foo module or calling drupal_static_reset instead makes debugabbility or readibility harder. The reset params are ugly anyways and it's a nuisance to cope with it in the functions that have it.

Having a $reset param with unknown side effects is at least as much of A Drupalism and makes the Drupal API harder to use and less consistent.

The goal of having an explicit $reset param is so we can document that a function uses an internal cache, and so we can explain when the cache should or should not be cleared. Furthermore, a single $reset parameter can reset any number of static variables, clean-up some database records, reset the internal cache of helper functions, etc.

I fail to see how an invisible reset parameter improves DX. Without an explicit API contract, I have to look at the source code to identify variable names that could be reset, I have to understand the function's implementation to understand if it is safe to reset or to figure out if I need to reset helper functions, etc.

We have debated this at length in this and other threads. There are pros and cons to each approach. There is no good or wrong. Let's stick with $reset parameters for now. Thanks.

Per Barry's excellent DX series, we should also look at removing as many statics as possible to only retain those with real performance benefit.

I agree.

We have debated this at length in this and other threads.

While we may have debated it, I think it is clear that we did not actually understand each other's positions. If I had known you were going to reject patches like the one here in favor of $reset arguments, I would not have supported the static API in its current form.

@dries:

Without an explicit API contract, I have to look at the source code to identify variable names that could be reset, I have to understand the function's implementation to understand if it is safe to reset or to figure out if I need to reset helper functions, etc.

I agree completely. The problem we had is that many of Drupal's APIs had internal caches whose existence leaked out through the public API via $reset parameters (e.g. node_load and node_save) or, worse, through Hysteresis that could be observed but not worked around. The whole point (I thought) of the static API was to make it easier for module authors to write code using static caches *without* keeping or re-introducing these problems.

The caches addressed by this patch are purely internal to the Field API. By design, they are intended to be invisible to all outside callers. The API contract is that they do not exist. The pre-patch implementation of the API includes magical $reset arguments to a couple functions that external callers are told not to use. The post-patch implementation use drupal_static_clear() to achieve the same goal, in a way that external callers can completely ignore.

What I really want here (in Java terms) is a static synchronized hashmap for class Field that all Field methods can use to communicate through. I thought that's what drupal_static() was for.

Excellent! Time for a Dries vs. webchick smackdown! We should sell tickets... :-)

Lol. I don't think a smackdown will happen.. =P

I made a few patches to move to the new static caching API. My understanding was that a primary motivation was to remove $reset params, but got shot down a few times. Thinking about it more, replacing $reset params with drupal_static_reset('some_function_name') isn't much improved for the developer as they still need to know enough to call this specific reset function with the correct function name.

A good compromised seems to have been established in #422362-20: convert form.inc to use new static caching API, where exposed drupal_static_reset('function') calls are wrapped in a function_reset() function. This allows the reset feature to be well-documented, incorporates resetting of multiple static vars, and provides a place to do other resetting-related stuff.

I've replicated that emerging pattern in #480424: Update locale module to use drupal_static() in hopes that this becomes the "proper" use of drupal_static_reset(), which removes the dependence on $reset params and still makes it easy for resets to be called and documented.

This follows the accepted pattern of using drupal_static_reset. The List module's static cache has already been converted.

Missed a few in the test files.

I have merged our two patches into one. The advantage of your patch is the static caching is faster, the advantage of this patch is the removal of the $reset parameters which I think is a huge win.

Still desireable?