_features_set_export_language is clearing drupal static cache too aggressively breaking search and possible many other modules

In this we can replace drupal_static with plain static.
Thanks to Heine for this patch.

Indeed I forgot the change the path.
Also I added this patch to the wrong issue so ignore this.

I'm totally backing this issue, after hours of debugging I managed to find this bug breaking our Core search indexer. The Search module's function search_dirty() uses drupal_static to mark dirty words (in other words: words that have been marked new or changed). Before Search is able to fill up it's search_total table the static gets cleared so nothing gets indexed.

Let's fix this A.S.A.P.

@Ambidex, I spent hours debugging this as well yesterday and today, I actually reported #2628888: Strangest bug with search_update_totals not updating indexed words where I got some pointers that helped me today to reach the root of the issue which it was features.

Attached is a patch that adds search_dirty to the static data to keep, at least not to break core. But I do agree that this is VERY bad behaviour. I am not in the position to properly assess a better patch, but the code documentation says:

    // Ensure that static caches are cleared, as they might contain language
    // specific information. But keep some important ones. The call below
    // accesses a non existing key and requests to reset it. In such cases the
    // whole caching data array is returned.

So, instead of clearing all and keep, better to clear the ones that are known to have language information, at least it's better to keep track of the ones that needs cleaning than the ones that should be kept, which I expect the latter to be affect most cases.

However, I strongly suggest applying this patch for the time being and releasing a new version with it. Breaking core should be avoided.

I would think is is a fairly critical issue.

I agree to this issue being lifted to critical status. And indeed, like I see it, it's very bad practice to approach this kind of problem through a pattern of "exclude all unknown, include some known" instead of "include all unknown, exclude some known".

And agreed; above patch seems to be a valid workaround for fixing core search, though this piece of destructive code should be cleared ASAP and the patch should not close this issue.

Since above patch is a nice workaround, I'm switching this back to Needs work, since this should probably not be applied as a valid stable patch.

On the contrary, I would push for this to be committed as ASAP, this is currently on a stable features release, and as it is, it's breaking a core functionality. I agree with you it should be fixed for good, but that could take a more time than reviewing a one line patch, committing it and then set this to Needs work until it's finally fixed.

Also spent an hour or two debugging this, and came here to see if someone posted a similar patch. I 100% agree that we should get this quick fix in. It is a critical issue, breaking core functionality, in a stable release.

Tested the patch (as a matter of fact it was identical to the one I was planning to upload - maybe no surprise :p) and it fixes the problem with search.

Also worth noting that if you turn off "rebuild features on cache clear" it will fix your problem. But then again, it is on by default so I think we should get this in.

That being said, though. I find it somewhat unreliable in the core search to rely on that not one implementer of hook_cron could drupal_static_reset. Seems likely that there might be other modules doing something like that.

Also included: Very detailed steps to reproduce on clean d7 (updated the issue summary with the same):

- Enable locale and features modules
- Add a new language and set it as default.
- Add some content.
- Run cron and expect search index to be built.
- Search returns no results, because "search_total" table never gets populated.

This is a very critical bug. At first, I just saw the Drupal UI broke sometimes after upgrading features but realize now that it also cause the core search to break as described above. It also causes other reported bugs: #2610862: Features and navbar modules problem in other languages and who knows what else.

Although the export function has been around in 6 months the problems seems to be introduced with the latest release 7.x-2.7 where _features_set_export_language is called in features_get_component_states function in features.export.inc line 895 as a patch for #1988252: Use the same language consistently in generated comments and strings (patch #43). A quick temporary solution could be to delete this line (patch attached). Another issue #2608176: _features_set_export_language failes to restore statics that should be kept. also seems to be related but the patch does not work for me.

Hope something is done asap. Unfortunately i am not skilled to handle the underlying problem with the static cache.

Morten

This issue is not resolved. It is resolved for the search module, but not for every other module.

The patch from #14 does even remove the call to _features_set_export_language. It is in there for a reason.

Agreed resetting the static cache to try to make sure the export is in English, and restoring an arbitrary list of variables is a bad approach, but there needs to be another way to solve this. So there's still work needed.

PS: In my case the variable which was broken by _features_set_export_language (menu_get_custom_theme) was not being restored properly, so the solution for that is actually provided in #2608176: _features_set_export_language failes to restore statics that should be kept., while this issue addresses the fact that features can not know each and every variable that needs to be restored.

_features_set_export_language() causes our cronjobs to be completely broken as well. system_cron() calls hook_flush_caches() which in turn causes features to force language to english for the rest of the cron run. This can be wrong depending on the language set on the corresponding site.
I attached a patch that seems to fix our issue for the moment. Nevertheless, the root cause is still _features_set_export_language() doing really nasty things.

Can we check if _features_set_export_language() running during system_cron() and skip the static cache handling? Because as far as I remember the _features_set_export_language() bit is only really required in case the features are exported.

Doing that to the static cache is dangerous outside of cron as well.

Applied #16 and it fixed search indexing for us. Hope to see it committed even though it only fixes part of the problem.

#16 doesn't work for me, instead the #6 does the job.
It fixed the search indexing. Tx

Sorry I've been out of the issue queue for a while on this. I updated the original #1988252: Use the same language consistently in generated comments and strings issue that started this mess. I'm very tempted to revert that issue and its subsequent patches.

I agree that the way it's clearing the cache and adding "exceptions" is very bad practice.

The patch in #6 here isn't good enough...that only fixes search.

The patch in #16 doesn't help because after reseting the language back to the original you'd need to clear static caches *again*.

I'm no expert on multi-lingual, but we know D7 is not ideal. Features worked fine for many years without the patch in #1988252: Use the same language consistently in generated comments and strings so I'm asking for advice on what should really be done here. So far I haven't seen any workaround that I'm happy with.

@das-peter, #18

Because as far as I remember the _features_set_export_language() bit is only really required in case the features are exported.

No, with the change from the patch in #1988252-43: Use the same language consistently in generated comments and strings, _features_set_export_language() is also called when checking the state of a feature. This was done to fix the issue where a feature would be marked as overridden. This happened if the site default language was not English.

OK, Here is the patch I am proposing to fix this.

1) It removes the cache clearing from the _features_set_export_language() function. If some modules have translatable config in static caches, oh well, tough luck. It isn't Features problem to fix that. And we don't want kludgy static cache manipulation here.

2) The _features_set_export_language() function is renamed _features_export_language(). Instead of always just setting the language to english, it now takes an argument to set the language, and also returns the previously set language. This allows us to do #3:

3) Similar to #16, Wherever we were calling _features_set_export_language() we now do this:

  // Set language to English but return current language setting
  $language = _features_export_language()
  ...do some export stuff...
  // Restore language setting
  _features_export_language($language);

That sounds like a good solution. I think we all agree that the patch in #1988252: Use the same language consistently in generated comments and strings created more issues than it fixed.

+1, #24 looks good on first read. We will try it out.

Thanks for the patch. It works for me and resolves both search and UI bug.

Morten

@mpotter I just tried this. I have a few comments, I am not sure I know exactly what's happening but at least let me say what I saw:

I tried 2.7 with this #24 in a multilingual Spanish/English site I have.

In either language the features were displayed as "Default" which is good.

I, because of previous experience, normally set the admin UI to English, so regenerating the features changed nothing.

Then I switched the UI to Spanish, went again to the features admin page, also all showed as "Default"

I then regenerated all of them again in Spanish and what I saw, on the views_default include file, a bunch of comments now in Spanish, such as:

-  /* Relationship: Entity Reference: Referencing entity */
+  /* Relación: Referencia a entidades: Referenciando entidad */

Which I don't really care, however one thing I did saw as well was something like this:

     t('‹ anterior'),
     t('siguiente ›'),
     t('última »'),
-    t('Content entity referenced from field_negociacion'),
-    t('Content entity referenced from field_comunicacion_externa'),
-    t('Content entity referenced from field_comunicacion_interna'),
-    t('Content'),
-    t('- Choose an operation -'),
-    t('Type'),
-    t('Title'),
+    t('Contenido entidad referenciada de field_negociacion'),
+    t('Contenido entidad referenciada de field_comunicacion_externa'),
+    t('Contenido entidad referenciada de field_comunicacion_interna'),
+    t('Contenido'),
+    t('- Elegir una operación -'),
+    t('Tipo'),
+    t('Título'),
     t('Page'),
-    t('more'),
   );

That comes from

$translatables['huerfanos'] = array(

So i am not sure whether that's an issue or not. It's not to me, but I'd definitely expect anything between t('') to be en english and I understood that was the purpose or goal of your patch. Odd thing is that I do have other t('') that doesn't seem to change when changing Admin UI default language.

I haven't tested any breaking of core, but seems you removed the harmful code I assume that's considered fixed for granted.

Re #28: The views export is being done by views itself, not Features. And I think this is one of those areas where it's using a static cache, which we are no longer clearing. I believe this is one of those cases that caused the original issue to play games with the static cache. Once I commit #24 somebody is going to need to open a new issue to propose how to deal with this, and it might be something that needs to be addressed in ctools or views directly in their export code.

Re #29: I cannot find any API for setting the global language. Drupal itself does this in drupal_language_initialize() in bootstrap.inc and directly sets the $GLOBAL array value there. If somebody finds a cleaner way to set the language, they can open a new issue with a proposed patch.

OK, at this point I think I've gotten the feedback I need. I'm going to roll a new Features release later today with #24 to at least fix the critical regression caused by the original language patch.

It's possible that this puts us back where we were originally and that languages are not being handled in the export (as hinted in #28). But at least we won't be playing games with the static cache that cause all sorts of unknown side-effects.

Committed to c120135.

I'm attempting to use Features 7.x-2.10 to update a feature via drush. This command fails, with this output:

PHP Fatal error:  Call to undefined function _features_export_language() in /var/aegir/.drush/features/features.drush.inc on line 627
Fatal error: Call to undefined function _features_export_language() in /var/aegir/.drush/features/features.drush.inc on line 627
Drush command terminated abnormally due to an unrecoverable error. [error]
Error: Call to undefined function _features_export_language() in /var/aegir/.drush/features/features.drush.inc, line 627

This seems directly related to the fix implemented out of this issue? I downgraded to 7.x-2.7 (that's what we use on our production sites) and I was able to update the feature using drush with no errors.

Are you sure you haven't installed two versions side by side? The error message claims _features_export_language() is missing, but that function was added to features.module in 7.x-2.8 and has been there since as far as I can tell.

Please open a new issue if you are still having a problem. As mentioned in #35, the _features_export_language() is in the features.module file so something must have gone wrong during your update process.

@fuzzy76 & @mpotter, you are both correct. I was using Features 7.x-2.7 for my sites and had installed Features 7.x-2.10 for drush. It didn't occur to me that they were interdependent and the drush functions would rely on the module code installed for my sites. So I didn't install two versions side-by-side per se, but I did have two installed versions that were apparently not cooperating.