eck_clean_up() logic wrapper

Quick review:

1. +++ b/eck.features.inc
   @@ -120,10 +120,9 @@ function eck_entity_type_features_rebuild($module) {
   -    variable_set('eck_clear_caches', TRUE);
   
   +++ b/eck.module
   @@ -908,24 +908,27 @@ function eck_init() {
   +  if (variable_get("eck_clear_caches", FALSE)) {
   +    variable_set("eck_clear_caches", FALSE);
   

   When is this now set to true?
   nit: single quotes

2. +++ b/eck.module
   @@ -908,24 +908,27 @@ function eck_init() {
   +  else {
   +    return FALSE;
   +  }
   

   Shouldn't need to return false because you aren't returning true or checking the output.

Thanks for the clean-up fixes.

+++ b/eck.module
@@ -908,24 +908,24 @@ function eck_init() {
 function eck_clean_up() {
...
+  if (variable_get('eck_clear_caches', FALSE)) {

Does this need to be a variable using variable_get() or could it just be variable within the function, a static if it needs to turn itself off between calls. Is there a chance this cache clear will be called on some separate request requiring this state being held in the variables table?

Reason I ask it feels like a moot function if it's off most of the time and maybe it shouldn't be called in the places where it needs to be false most of the time.

@joelpittet In features we are cleaning the caches directly, but when we create entity types or bundles through the UI, we set the variable to true in one request (ex. creating the entity type and a bundle) and we let the caches be cleared during hook_init in the next request. The reason for all this obscurity is trying to minimize the number of times these caches are cleared as they can be pretty expensive operations.. I am open to better ways of handling this if any come to mind.

I'd have to look at it more deeply to have any opinions on the matter.

So regarding this patch though, before the patch, that function was able to run without setting a global variable that gets stored in the variables table, but now it requires it. This seems like a not so good pattern, maybe to ensure this function can be useful at call time a $force parameter can be added? Even that feels a bit wrong but better than the API where you must call variable_set('eck_clear_caches', TRUE); before calling it.

+++ b/eck.features.inc
@@ -120,10 +120,9 @@ function eck_entity_type_features_rebuild($module) {
-    variable_set('eck_clear_caches', TRUE);
+    eck_clean_up();

This looks like a possible dead end if variable hasn't been set to true that function call will do bupkis.

My recommendation re #11 the obscurity how the cache clears in hook_init() are happening could be cleared if the calls to clear the cache only happen at the time they are needed instead of in the next request.

Like on the form submit callback and only when things change that would need them.

This may not be the best solution but maybe it will spark an idea or get this on a better track?

#16 to stop the weirdness?

@joelpittet A little history :). What you are suggestion was what we had initially, we would clear the caches when a relevant action was taken (delete entity type, create a new bundle, etc), but we found that in some instances this code was being called multiple times. For example when you add an entity type, a bundle is also created. So, the solution was, lets delay clearing the caches until the end of the request so we only call it once. That sounded great but I was not able to get the "clear at end of request" approach working. So, at that point the next best option was to clear it at the beginning of the next request, which works even though is quirky, and that is where we are now.

Thanks for the history. So why not look for a solution to the initial problem instead of trying to fix the work around?

If the bundle save is causing the cache to clear twice, why not statically save the state of the cleared cache in the function to avoid doubling up in the same request?

Or even, make the two (as In the patch in #16 does) cache clears separate and only clear the cache's needed for each.

I may be naive here but I'd really like to fix this strange pattern.

@joelpittet Regarding the solution with a static variable, That can only keep us from running the thing more than once, but really what we need is for the caches to be cleared by the last thing that needs them cleared. That is not an impossible thing to do, but it would be much more complex than what we are doing now. What we are doing now is a little quirky, but it is simple.

Making the cache cleaning more granular was my original intent, but after spending way too much time trying to reconcile the many caching layers that affect the entity system, I decided to go with the shotgun approach.

So, really the only feasible solution I came up with was to try and clear the caches at the end of the request, but the only appropriate hook for that is hook_exit, and that did not work (but I must admit that I did not spend much time trying to debug why clearing the caches that late in the request did nothing), so doing it in the next request as it is now seemed like the next best thing.

I enjoy thinking and talking about these things, but the most concerning thing in my opinion is that any refactoring that we do to what we have now (which seem to be stable) will require pretty extensive testing as any mishaps in clearing caches usually have some pretty extensive consequences when you are dealing with the entity system, and given that our behat tests are not as complete as they should be, this sounds like a ton of work and time that I rather spend backporting cool features from 3.x to 2.x :)

If you find this interesting and really want to have a go at it I will be happy to assist you as much as possible, but I think we should move the discussion to a new ticket, and focus this ticket on whether the previously reported issues are solved by the approach given in the patch here. #2444517: Features rebuild fails when same features export also adds fields #2289241: Warning: array_keys() expects parameter 1 to be array, null given in drupal_schema_fields_sql()

The patch in #17 applied cleanly and fixes error warnings when enabling some features.