Convert comment language code schema to langcode

Changes based on test failures. locale.test referred still to $comment->language in its tests and language form altering also used 'language' in place of 'langcode'. Let's see this one.

Patch still needs update path and testing in the update, since it seems like it is certainly not tested given that it passes without the update path.

Thanks for working on this! Regarding the added update code:

- we should drop the index first and then change the column name to be fully compatible with all databases supported; some databases don't like if you drop a column that takes part in an index :)
- I think this would be fine in a comment_update_8000() in comment.install, unless there is any special reason to do it in the bootstrap early; I don't think comments need to be loaded at that stage (unlike languages and path aliases), so we are fine running this update later as part of the standard update procedure; the bootstrap prepare updates are really special cases that we need to do early to even start up Drupal to update itself

@fastangel: all right, the update function looks right. Update path testing is still missing unfortunately. Here are some tips to get going with that:

1. core/modules/simpletest/tests/upgrade/ has database dumps and test files to test the update
2. drupal-7.filled.database.php.gz is the dump for a filled Drupal 7 database to test with
3. If you ungzip the file, you'll notice it has PHP code to produce the Drupal 7 database setup, it has several nodes but it does not have comments
4. Edit the file and add a dump of a comment in the Drupal 7 database form (you can use core/scripts/dump-database-d7.sh to generate a dump of a comment table and copy-paste the relevant data into the dump)
5. Create a diff of the ungzipped original version and the new version (for posting your changes)
6. gzip the new version
7. Change the test in the upgrade_filled.test code to drupalGet() the node that you've attached the comment to, and check if the comment displayed. Also do a comment_load() on the comment to ensure that it loads and has the expected langcode value.
8. Enable the Testing module and run only the filled upgrade test (from under the Upgrade tests on Admin » Configuration » Testing), ensure it passes.
9. Do a git diff with git diff --binary so you get the diff of the gzip file as well to post.

This is about it to complete this patch. Can you try and look into making this addition? (Without update testing the patch is not committable because it does change the schema and ensuring it worked on an updated site would be important).

Tagging as current focus.

+    $this->drupalGet('node/38#comment-1');

#comment-1 is not needed here, the URL path is absolutely enough.

+    $id_comment = $this->xpath('//div[@id=:comment_id]/', array(':comment_id' => 'comment-1'));

You can just put comment-1 right there in place of :comment_id.

+    $this->assertTrue(!empty($id_comment), t('Comment id 1 not found.'));

The t() in the assertTrue() should have a message that applies to the *success*. So it should say "Comment 1 found after update." or something like that.

+    $comment = comment_load(2);
+    $this->assertTrue($this->commentExists($comment->langcode), t('Comment id 2 found.'));

The t() is almost good here. Since we are checking for the langcode property specifically, we can say t('Comment 2 has a language code.')
I also don't see why you'd need $this->commentExists()? You can just do !empty($comment->langcode).

I ran the tests. They seem to result in

Fatal error: Class 'CommentStorageController' not found in {.....}/core/modules/entity/entity.module on line 298

Which seems to be a stock case of the class registry not being refreshed on the D8 update as far as I see or at least the test is not getting refreshed data when it attempts to run the comment API function. If you don't call the comment API directly from the test (ie. you only do the /node/28 drupalGet() test, NOT the comment_load() test), it is not an issue, so I suspect a local registry cache is the culprit.

So let's work with drupalGet() for now instead of trying to work around the particularities of the test system.

Then the second problem seems to be that node/38 does not exist. If you run the tests, i.e unable Testin module locally, go to Admin > Config > Testing an pick the Filled upgrade test from under the Upgrade tests, you get this at the end https://skitch.com/gabor.hojtsy/gh9t2/test-result-d8x.localhost. This basically says the node/38 is a 404. Looking at the DB dump, node 38 seems to be testing a node without a body field which is not in fact updated. That sounds reasonable IMHO. So let's pick a node that actually appears after the update and test with that!

Let me know if you need more help!

@fastangel: do you intend to keep working on this or should I try and find someone else to help?

@fastangel: 1. To directlyy get a comment, you can invoke its edit page too, at least to check it works. 2. Lets pick a different node that has a body (and does not have comments turned off). Thanks for your work!

@fastangel: I think we discussed above that comment_load() would hardly work in this update environment without a lot further tweaking. Can you please re-read my suggestions in #28? Also, it would be superb if you could write a few notes on what have you changed instead of just stating you upload a new patch. That you did upload a new patch we can see anyway from the attachment! More communication can help push issues along a great deal. Thanks!

@fastangel: I think we also agreed above that you'd pick another existing node instead of changing the database dump for nodes. The nodes represented there are supposed to give a variety of update scenarios, and if you modify them at will, their test coverage will weaken. There must be a node among the 38 that can have comments and is published, right? Why mess around with what you have when you can pick a different one. I suggested this above in #28 and #32, now I'm suggesting it a third time.

On the comment_load() we figured out above that we are better off not trying to fix the whole testing environment but just read a different data source to verify it worked. I'd say the node test in the first place would pretty well ensure the langcode is there since the new code uses langcode all around and would spit out various notices if it would not work. If you want to be very specific to test if the right language code is used, you can write a direct DB query to avoid the update issues as well. That way the node test would test the whole cycle of attachment to nodes with language and then the db query would test for the langcode. Something like:

$comment = db_query('SELECT * FROM {comment} WHERE cid = :cid', array(':cid' => 2))->fetchObject();

(Instead of the comment_load()). And then check the langcode as with the existing assert() that is in your patch.

@fastangel: if all nodes are marked to not allow comments, it sounds fine to add a whole new node to test this to make sure we are not stepping on other toes.

I think this fails due to recent changes in the DB dump. I looked into how the DB dumps are managed in D7 and there seem to be great componentization there that we could leverage there, with the smaller dumps not even gz-ed, so we can patch them better. See http://drupalcode.org/project/drupal.git/tree/refs/heads/7.x:/modules/si...

To me it looks like we should migrate to that model sooner than later to avoid this continued need to reroll the update dumps (and post them as binary patches). We can either do it here or in #1357918: Missing update for language_default in language langcode update and then work with that setup for further language tests. Clemens, would you be interested to help out with that?

@clemens: the D7 dumps I linked mostly dont use gz and are easier to diff/patch.

@clemens: well, there are various non-gz files there, like http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/si.... If you look at how they are used (http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/si...), you'll see they are loaded on top of the filled database and then used for testing. In effect, the filled database is a common default for all kinds of update test, where the actual sub-feature tests are componentized into their own add-on database dumps. I'm working on that to introduce in #1357918: Missing update for language_default in language langcode update hopefully later today. Stay tuned :)

@clemens: I wrote this doc page based on your excellent questions: http://drupal.org/node/1429136. Also introduced easier to diff componentized language test files in #1357918: Missing update for language_default in language langcode update, so I think we should postpone this on that change. I hope it will be in quick, so we can move on with this.

Tagging for the content handling leg of D8MI.

Great, #1357918: Missing update for language_default in language langcode update landed, so now we can continue fixing the tests. @fastangel: can you continue work on this?

1. Good new is that you should not touch drupal-7.filled.database.php.gz anymore. Leave that alone.
2. All the db dumps you added to that gz before now should be added to the drupal-7.language.database.php file in the same directory. This is not gziped so we should be able to review your changes much easier, it should be easier to roll the patch itself and it will result in much smaller patches.
3. All other things we discussed above should still be fine.

Thanks for helping with this!

Needs tests as in test cleanups.

Working on fixing that update test, since @fastangel apparently dropped the ball.

With tests that actually pass. The test now uses the uncompressed dump, so we can have a full reviewable diff.

- added node 38 to the dump
- added comment 1 and comment 2 to the dump
- extended test to check for node 38 and both comments on the node page and checking the comment schema directly too

Passes upgrade tests on my machine. Please review!

Minor code comment update.

@fastangel: please use your available time to review the patch and if you think it properly tests the upgrade functionality! Let's move this forward finally!

@fastangel: We use these to check for the unique comment titles. What would comment/1 give us on top of that?

+++ b/core/modules/simpletest/tests/upgrade/upgrade.language.testundefined
@@ -40,5 +40,15 @@ class LanguageUpgradePathTestCase extends UpgradePathTestCase {
+    $this->assertText('First test comment', t('Comment 1 displayed after update.'));
+    $this->assertText('Reply to first test comment', t('Comment 2 displayed after update.'));

(Attached patch fixes only the missing newline at the end of the DB dump PHP file, no other change).

RTBC based on both feedback comments above.

Fixed one minor typo. Interdiff would be:

-    // Directly check the comment language property on the first omment.
+    // Directly check the comment language property on the first comment.

Should not change RTBC status.

Change notice added at http://drupal.org/node/1450144, removing outdated tags.

The D7 schema has the language column, so that error sounds like it is an issue in your system. Also the code does attempt to add a comment_nid_langcode index, not a comment_nid_language index.

Right! So that is why it was looking for the old index. Duh. Trivial.

Tag again for sprint.

Superb, thank you.

We are closing in on having everything standardized like this. Fields are being worked on at #1439692: Rename field language properties to langcode, help welcome there!