Missing update for language_default in language langcode update

#1301040: Move language listing functionality from locale.module to a new language.module landed, so opening this one up.

Here is a quick initial patch. Will probably come with massive fails but its a start. Its pretty delicate to avoid changing user, node and comment related code while we do this to constrain the scope of this patch

Fixing several other ->language instances. Will no doubt turn up more through the tests.

Even more places converted.

Above patch was for #1357912: Convert path language code schema to langcode, please disregard. Here is the right one.

Found some code to be affected in user.module, filter.module, etc.

Even more places where langcode was missed. This is turning to be a huge megapatch. Hope to have it landed soon since managing it will be a nightmare :/ The language negotiation related fixes now included should fix many of the notices around not finding $langcode on objects not properly returned before by the negotiation system.

Drupal web test case had language initialization which in fact broke most of the tests. Changing that leads to much better results.

Comment token and node.module still had lingering language key based code.

Found two places where I was going over converting node related ->language properties which should be left alone for the scope of this patch. That resulted in many node language related fails. (This actually reduces the patch size a bit).

Tagging up so we can find the langcode API changes later.

Thanks for the review!

- Great find on that debug hunk, removed.
- No, an update function is not needed since languages are needed in the early bootstrap for update, so the patch includes changes in the update.inc code to do the update in this hunk:

diff --git a/core/includes/update.inc b/core/includes/update.inc
index a2cfd0c..1de14f0 100644
--- a/core/includes/update.inc
+++ b/core/includes/update.inc
@@ -141,6 +141,17 @@ function update_prepare_d8_language() {
     db_drop_field('languages', 'domain');
     db_drop_field('languages', 'native');
 
+    // Rename language column to langcode and set it again as the primary key.
+    db_drop_primary_key('languages');
+    $langcode_spec = array(
+      'type' => 'varchar',
+      'length' => 12,
+      'not null' => TRUE,
+      'default' => '',
+      'description' => "Language code, e.g. 'de' or 'en-US'.",
+    );
+    db_change_field('languages', 'language', 'langcode', $langcode_spec, array('primary key' => array('langcode')));
+
     // Rename the languages table to language.
     db_rename_table('languages', 'language');

- Finally, added this sentence to the issue summary: "Other objects like nodes, fields and path aliases are converted in related issues. See a full issue list for this conversion at http://drupal.org/project/issues/search/drupal?issue_tags=langcode"

Add issue list link for related issues.

#21: language-to-langcode-21.patch queued for re-testing.

Rerolled patch to fix the apply problems.

#24: language-to-langcode-24.patch queued for re-testing.

#24: language-to-langcode-24.patch queued for re-testing.

#24: language-to-langcode-24.patch queued for re-testing.

@catch: its pretty impossible to change all schemas to use langcode instead of language. There is at least one more patch queued up already at #1357912: Convert path language code schema to langcode and the langcode tag is used to track future issues that are going to change all those. (http://drupal.org/project/issues/search/drupal?issue_tags=langcode).

You are right though that the comments for the schema are not properly updated. I found four occurrences (comment.install and locale.install were also affected on top of your two finds above) and all are fixed in the attached patch. Otherwise unchanged.

http://drupal.org/node/1399806 added with both a developer example and a themer example (with possible D8 theme changes in the future in mind).

BTW next one up in this conversion is #1357912: Convert path language code schema to langcode which has a patch that was previously passing tests (but was made not possible to apply by this patch, so just updated there). Reviews welcome there! Thanks a lot!

The primary key change is definitely not going to work this way in the D7-> D8 upgrade path as far as I see since there is no language table in D7.

Looks like you are now not actually calling language_update_8000() vs. the previous patch, so it breaks differently now. You let the bootstrap through before the update with the wrong schema, so there you get those test fails AFAIS.

@sun: so here is the patch that reflects your intentions (AFAIS).

With that the problem is that the language update will run in itself and by that time the language column is no more. By adding appropriate conditions in the update function, we can avoid running parts of it as needed. Adding a db_field_exists('language', 'language') to the DB change and an isset($language_default->language) to the variable update. Should pass now.

I still don't agree that this is any more elegant compared to having it all in the bootstrap prepare code. And we could easily make it work for D8 -> D8 updates in the bootstrap prepare code too by using the current conditionals there. Anyway, I hope this should pass tests now and if this is deemed better, then so be it. The real point is fixing the remaining issue with not updating language_default in the original patch and it is done either way.

Also elevating. This is at least a major upgrade path bug.

Here is a suggested text expansion to add language_count and language_default to the filled database. English and Catalan were already present in the DB, so I decided to make Catalan the default here. I think this should cover the data requirement we need. I've also added test coverage to test language_default() and language_list() output on the D8 end, so that Catalan is returned to be the default and that is the only one returned to be the default.

Posting a diff including the above patch and without so that we can see if it properly fails / passes.

Ups, fixing PHP error in the test. Same patches otherwise.

Ok, well, the serialization of language_default was not proper either (due to UTF8 byte differences). Fixing that. Let's see if this correctly passes and fails now :)

All right, one more curly brace was missing in the serialized language_default. I did run this test now locally as well. It looks like the test system *itself* will produce notices until the update.php code actually runs that updates the language_default variable early in the first bootstrap. I reproduced those notices on my machine. Not sure how could we fix that.

All right, so as can be seen the "pass" patch actually passes with notices, the fail patch produced the two test failures expected. However, since all those notices are added, it is obviously not feasible to commit these to core. Now because the notices come from the test system running on a not yet updated D7 database, how could we make sure that the test system is not affected? We could extend the setUp() in UpgradePathTestCase to run update_prepare_d8_bootstrap() let's say, not sure how pristine the update testing would be at that point. Any good ideas? @sun? @catch?

Ok, well up on more testing it turns out that the bootstrap prepare code does run in time BUT the test system has outdated cached values in place. like $conf['language_default'], the internal cache of language_list() and the global $language values. If we clear / reinitialize those from the database / backend data, the test system becomes perfectly happy. Putting this right before it causes a problem. Now this needs serious feedback :)

All right, I must have had a test glitch where it actually passed on my machine. I cannot reproduce it passing anymore. Doh. My best patch is in #52 and I request that someone please take this over from me because I wasted way too much time on trying to work around this and apparently failed. Please help.

Marking needs focus.

Uploading 'pass' test with fixed PHP syntax. It will still not pass. Still needs serious help to figure out clearing the test environment properly to be able to pass through this. Please!

#1331370: [Upgrade path broken] Stored include paths need to be updated to /core before running the upgrade path is now committed so this will not apply anymore I guess. Also that does the bootstrap already, so this patch can be simpler. @swentel: can you help move this forward?

@swentel: thanks!

In general we are having issues with keeping the dump up to date and looks like it would be better to move to a componentized model for the upgrare test. Would you be interested in looking into that with this patch? That would allow us to move forward much quicker in other patches! I've looked into how D7 componentized the update tests, summarized my findings a bit in http://drupal.org/node/1410096#comment-5558268

Here is a rework of the above patch that would not make @chx cry (I'd imagine :). The updgrade test system is built with componentization in mind (at least where possible), and now that we start to add language asserts in the test, we ought to set up our own .test file for it. We can/should also introduce a db-addon file for this which contains the language data. We can take advantage of the system's support for non-gz dump files, so since the language add-on would be simple, we can keep it diffable and make people's work and patch rerolls in related issues *way* easier. Rerolling patches against the gz dumps is a huge pain.

So the attached patch takes the cues from the componentized D7 dumps/tests (http://drupalcode.org/project/drupal.git/tree/refs/heads/7.x:/modules/si...) not yet introduced in D8 yet. The language related dump data is moved out to a drupal-7.language.database.php (not locale, because we want to work with node, comment, field language, etc. type of data here too). It also introduces an upgrade.language.test file which builds on the core filled test file and adds on this dump. Then runs its own asserts.

So we leave the generic filled dump and test pristine and can work in our own addon dump/test to prove language updates are good. The filled dump got all the data removed that you can see added in the drupal-7.language.database.php, all others are left intact.

This passes on my local machine, hope it passes on the testbot.

@swentel, all, please review, because this blocks changes like #1410096: Convert comment language code schema to langcode and friends, basically any language related schema change, of which we are working on / planning a lot.

BTW wrote some upgrade test docs based on all this work at http://drupal.org/node/1429136. IMHO let's get this refactoring/fix in, then we can move on with a simpler workflow for writing the language update tests.

Reviews please!

@clemens: thanks for the review. Here is an updated patch with plenty more comments in the dump.

Keep the reviews coming!

Updated patch with only the missing bid fixedextra bid removed. I don't think we should document the test data in the detail you are looking for. That would require us to update the comments in the dataset every time we update the tests, which is likely going to fall unmaintained. Also, this dump already has plenty more comments compared to the dumps in Drupal 7.

On the blocks, well, we don't currently need them. We can remove them, but they were in the filled dump, so then we'd lose those blocks and would require extra work when we come to moving those blocks from locale module to language modul and I don't want to inflict that extra work on anybody.

@catch: thanks for the quick feedback! We don't do the componentized db dumps in D8 yet, it is only done in D7 (and D7 does not even gz the base filled database either). I think going this way will make writing upgrade path tests much easier for language stuff (of which we need to do a lot these days).

Looks good, I think this is still RTBC if it passes tests, which is more than likely :)

Fix tags :)

Yay, thanks. This just unblocked #540294: Move node language settings from Locale to Node module and #1410096: Convert comment language code schema to langcode which are going to extend the tests/dumps added here :)

Also remove sprint tag.

Ok, @Dries, you forgot to commit the new files from the patch. (http://drupalcode.org/project/drupal.git/commit/fdca0419a524917d0d8b9126...)

Off-sprint then :)