Cache fields info (and small API improvement)

I'll do, as soon as I can free some time to do it:)
The main problem here is I was not able to find any CCK hooks to tell modules that CCK fields have been modified. The last solution to do it is implementing hook_form_alter() on CCK field edit forms and add a custom submit callback. I think this is ugly, but it should work.

BTW, all the CCK fields handling could be in a submodule, it would make the main apachesolr API cleaner, what do you think?

That's a good point, I agree with the fact a submodule would be too much. Is it possible to make the CCK features working using main API hooks instead of embedded code? It would be a nice mixed up solution to get a lighter API - I mean it would avoid specific code in generic API and it'll centralize the CCK feature code to some hooks implementation. Using an include file to put these functions is a good idea.

Found the right hook, see attached patch.

I don't remember the nature of $cached, but if it's not an object, PHP would copy the variable instead of using the reference.

EDIT: and because my variable's goal only for convenience, I prefer to minimize the impact on performances.

RE-EDIT: and $data->cached is a big array with string as content, if we don't use the &, it will cause PHP to recopy the full array, this copy will be useless because there is no need to protect the $data->cached from changes.

I think you should have only one 'return' statement within the function, it's I think more readable.

Attached a patch:

• Added some '\n' for readability (more air in the code)
• Added static variable explicit initialization to 'NULL' constant (sometimes, PHP does affect random memory values to non initialized variables, I already had this bug)
• Replaced is_null() function call with '===' operator, which is exactly the same thing, without a function call (I don't really know if it makes a difference for PHP)
• Added a 'else' statement instead of an early return (non mandatory) to have only one return statement in the full method (more readable and if the signature changes in the future, there is only one place to change it), also there is no performance cost at all here

Notice: the fact I added an else statement changes the function's body indentation, so the patch is kinda heavy, but the whole function's business part remains unchanged.

EDIT: is seems our posts have crossed each other.

Fixed a typo error.

Also changed the return statement replacing is_null() for $field variable to empty() function call (maybe more secure, people could have passed FALSE or '' in method signature instead of NULL constant.

I prefer the empty() because the $field is user input. It could be provided by a HTTP request in case it could be just an empty string instead of the NULL constant. This extra precaution is I think really necessary because else where in Drupal core and modules NULL input is pretty much the same as '' or 0.

The NULL init is a coding style, in many languages it's always recommended to initialize all variables, but in PHP as you say there the 'isset' keyword so why not.

EDIT: Plus, I prefer the === operator which is a generic operator that says "Is the same instance as". I think using the is_null() is wrong because as it is not an operator even a keyword, but an API function, it makes it less clear that using a real operator.

A real operator is much clear than an API function call which is, a function (which implies a function call btw) not a keyword nor an operator.

The patch IS NOT missing the cache refresh, see at end of file.

Re-EDIT: stroked some text which has no point since I'm OK with your static non init and the isset() usage.

@claudiu.cristea
Reading what you said, I did another patch, I reverted the static $fields init / isset() usage stuff.
I let the empty() stuff in the return statement (with the isset, there is no '===' operator usage at all, so it resolves this point).

@claudiu.cristea
What about the:

    if ($cache = cache_get('cck_fields', 'cache_apachesolr')) {
      $fields = &$cache->data;
    }

I think we should keep the '&' to avoid the full array copy.

Oh sorry, I misread the refresh part.
Indeed there is no urge, but it would be a good thing to do it right now, while we are talking about this.

Same patch with refresh, keeping array recopy instead of using reference.

Even better, using () in there would be better:

if (! $reset && ($cache = cache_get('cck_fields', 'cache_apachesolr'))) {

Instead of:

if (! $reset && $cache = cache_get('cck_fields', 'cache_apachesolr')) {

Agree, thanks for your time.

D7 API probably has changed too much to set this issue for 7.x release. I probably think that, if the same problem is spotted on D7 then it should have its own issue. This particular bug was CCK specific when created.

My patch, a year ago for D6 took care of the cache clear. I can't tell you with D7 and Field API, but there hooks, thousands of hooks, millions of hooks in D7, there is one out there that must help you for this! Basically you have to clear your cache at cache clear all time and when fields are being modified.