Locale uninstall hook broken

Easy fix.

Could we add a test for the uninstall feature? Several core modules have explicit uninstall code... we should test those!

The issue was a little more complex as the uninstall hook did not clear the variables; this caused a couple of further problems:

• if uninstalling with a UI language different from English the procedure failed;
• in any case after launching the procedure the site became unfunctional as the drupal_init_language function attempted to load the language data from the languages table because of the language_count variable dirty value:
  

  Fatal error: Uncaught exception 'PDOException' with message 'SELECT * FROM {languages} ORDER BY weight ASC, name ASC - 
  Array
  (
  )
  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal_7x_dev.languages' doesn't exist' in ...\includes\database\database.inc:510
  Stack trace:
  #0 ...\includes\database\database.inc(1617): DatabaseConnection->query('SELECT * FROM {...', Array, Array)
  #1 ...\includes\bootstrap.inc(1239): db_query('SELECT * FROM {...')
  #2 ...\includes\language.inc(18): language_list('enabled')
  #3 ...\includes\bootstrap.inc(1218): language_initialize()
  #4 ...\includes\bootstrap.inc(1164): drupal_init_language()
  #5 ...\includes\bootstrap.inc(1066): _drupal_bootstrap(7)
  #6 ...\index.php(21): drupal_bootstrap(9)
  #7 {main}
    thrown in ...\includes\database\database.inc on line 510
  

Moreover the function used to delete the javascript translation files was file_delete instead of file_unmanaged_delete, which caused the notices (the patch fixes this also in locale.module).

@Damien Tournoud: [Changing status to CNR] I agree on the uninstall test, but that should be a separate task IMHO.

the patch...

patch fixed

Here is the patch and its test.

There are two test scenarios: Locale is uninstalled either with English as default UI language or with French.
Obviously the patch should pass both, before the patch there were two different kinds of failures.

This is starting to look good. The code comments in the test need a bit of work to confirm with the coding standards: // Start comments with a capital letter and end with point.

Sorry, my bad, comments fixed.

This all looks great, thanks plach.

+  protected $langcode;
+  
+  public function __construct($test_id = NULL) {
+    parent::__construct($test_id);
+    $this->langcode = 'en';
+  }

Please:
- document the member variable $langcode
- use setUp() rather than __construct() to modify its value. __construct is not a well-defined interface, while setUp() is.

I tried to make the change you suggested but this way I don't have the $langcode available in LocaleUninstallTest::getInfo. I could set the $langcode value directly there but I don't like this solution very much.
Any advice?

@plach: you can implement getInfo() explicitly. Our coding standard for the tests (http://drupal.org/node/325974) mandates this anyway.

Sorry, did not mean to change the status.

I moved the initialization of $langcode in setUp and implemented getInfo without using its value.

Some further code polishing.

After reading http://drupal.org/node/325974#comment-1116106 I decided to clean-up the whole locale.test file.
I have a couple of doubts:

• the first one is about the coding standards (see http://drupal.org/node/325974#comment-1196841)
• the second is about the patch encoding: do we have a standard for it? I'm asking this because I found that my Eclipse IDE is unable to apply the patch correctly if it is encoded in UTF-8 (the "Français" word would be messed up), so I had to convert it to ISO-8859-1

Rerolled patch

Rerolled again.

The patch won't pass all the tests because the ones introducted in #361683: Field API initial patch are failing.

The patch and the tests looks very good. Thanks for the clean-up plach.

The patch looks good, minus one little point. From locale_uninstall:

+ // Switch back to English to avoid problems with t().

I'd like to see us document this a bit better. What exactly is the problem with t()? I assume we need to reset the global $language object. I spent about 8 minutes trying to figure out where we'd run into a problem but could not find it. It would be good if we were a tiny bit more explicit in the code comments. (Update: I now realize it is somewhat explained in comment #3.)

In the documentation, we seem to mix 'javascript' and 'JavaScript'.

Not resetting the language may end in locale() to be called during the uninstall process, which may lead to queries on just dropped tables.

Hope this is now more clear
(also set "JavaScript" in comments where needed)

I have just realized that the JS translation files were not actually deleted, fixed locale_uninstall and added a test for this.
Morever I missed some locale variables, also this should be fixed.

This one should pass the tests.

Summary of current patch:

- Errors caused while deleting translated javascript files are fixed. Now using file_unmanaged_delete() rather than file_delete().
- The same fix was applied to _locale_rebuild_js() in locale.inc.
- Cleanup/delete locale variables on uninstall
- During uninstall, use drupal_init_language() to force language back to 'en', since successive calls to t() might cause lookups on the removed tables.
- Added tests for uninstalling the locale module.
- Cleaned up the whole locale.test file according to http://drupal.org/node/325974#comment-1116106.

Reviewed and looks good to me.

I tested this and it works great, so rtbc from me too.

Talked with webchick. She's basically happy with the patch but would like to see some more documentation about the unusual last test.

class LocaleUninstallFrenchFunctionalTest extends LocaleUninstallFunctionalTest {

This test has a setUp() method but - unusually - no tests of its own.

This is true because it inherits the test in the class it extends, LocaleUninstallFunctionalTest. In setUp() it sets a new UI language ('fr') and reruns the test.

Draft additional documentation:

/**
* Because this class extends LocaleUninstallFunctionalTest, it doesn't require a new
* test of its own. Rather, it switches the default UI language in setUp and then
* runs the testUninstallProcess (which it inherits from LocaleUninstallFunctionalTest)
* to test with this new language.
*/

And here it is.

Looks good, I think that takes care of the issue.

Great work on this! Thanks for the clarifying comment; I really think that'll help the next person who comes across this to understand what's happening.

Committed to HEAD. Awesome job! :)

@webchick: thank you :)