Posted by catch on November 9, 2011 at 4:22am
8 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | locale.module |
| Category: | task |
| Priority: | critical |
| Assigned: | rvilar |
| Status: | closed (fixed) |
| Issue tags: | D8 upgrade path, D8MI, language-base, language-ui |
Issue Summary
The current 'filled' upgrade test database does not include locale module, however locale module has an update already, and #1323176: Decouple UI translation language information from language list schema is about to add another one.
So let's fix that. This means either adding to that database dump with locale enabled and a couple of language configured. Or a partial db dump with only locale table stuff in it and a new test.
Comments
#1
Adding tag.
#2
#3
I think adding to the current filled database dump a locale db with a couple of languages configured is good.
#4
I start working on this right now!
#5
Done! I attach a file that updates drupal-7.filled.database.php.gz with languages dump information
#6
The last submitted patch, locale-db-dump-133170-5.patch, failed testing.
#7
@rvilar: well, its a binary file, so it cannot really be posted this way. Can you post a diff of the non-binary versions and attach the binary (gz) version?
#8
Ups, I'm obviously not up to date on how binary patches are in fact possible to be rolled :) http://drupal.org/node/1107552#comment-4329566 so let's try a binary patch (but please include a patch with the uncompressed gz if not entirely too huge).
#9
I attach a new pathc.
It's only the binnary patch, because the uncompressed version is very huge.
#10
Well, I looked into the diff between the two PHP files, and this is what I got (attached). Its great that it has the languages table and English and Catalan. It does not (a) have the module enabled (b) say this in the comment at the top (c) have any of the other tables of the module (which I think we should include without actual data for now). I think if those are done, we have the dump there and the update tests will perform an update with this config.
#11
I attach the patch and a diff file to see what I do.
#12
In terms of what is included in the dump, that looks pretty darn nice. In terms of how it was generated, we do need this to represent a Drupal 7 database to run Drupal 8 updates on, and the database dump seems to reflect the current state of D8 not D7. The 'languages' schema does not have the 'native' column, the version for locale is 8001, etc. So the data itself looks great, we need to have this from a Drupal 7 site though. That should basically be it I think :)
Thanks again for working on this!
#13
coming here from #1331370: [Upgrade path broken] Stored include paths need to be updated to /core before running the upgrade path - so we're trying to get a db dump of a D7 site with enough locale and language data to test the upgrade path?
#14
@beejeebus: yes, that is our goal here. So far we had update functions for the languages table schema, so we concentrated on that and to have locale enabled and include the rest of the locales_* tables. No negotiation settings were yet include in the dump. Also, as you can see above, the dump is not yet D7, its D8. Looks like we should include negotiation settings too to have test coverage for that issue. We can either add it here or there (both are critical).
#15
@rvilar, @beejeebus: it would be pretty important to try and get this wrapped up sooner then later. Lots of other patches are waiting on this one to happen. @rvilar: do you need help or someone else to take over? @beejeebus: can you potentially help with this? Thanks all!
#16
I think this patch is the correct. I've revised all d7 db structure and checked all is in D7 form
#17
Changing status to execute the drupal test bot
#18
The last submitted patch, locale-upgrade-1336170-16.patch, failed testing.
#19
Ok, looks like the filled test case fails on creating the languages table, from the logs linked above:
exception 'PDOException' with message 'SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'NOT NULL DEFAULT '',PRIMARY KEY (`language`),
INDEX `list` (`weight`, `name`)' at line 12' in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/database/database.inc:2132
This seems to stem from the "length" property mistyped in
< 'javascript' => array(< 'type' => 'varchar',
< 'lenght' => 64,
< 'not null' => TRUE,
< 'default' => '',
< ),
#20
ups! I'll reroll and upload the patch this afternoon
#21
Added a new patch that solves this issue.
#22
This looks great to me, and very reassuring that it passes tests. With this it should mean that even if we start from a D7 site with locale enabled and some basic languages set up, it works with the tests. We'll need some negotiation settings added for #1301460: Decouple domain/path language negotiation storage from language config storage but we can/should do it there, since we don't yet have any update code for that, so the current D8 code uses the D7 backend intact. Once we change that in #1301460: Decouple domain/path language negotiation storage from language config storage, we do need to have settings added to the dump.
#23
This looks like a good start, like Gabor says we can improve the actual coverage in other patches now there's something in here.
Committed/pushed to 8.x, thanks!
#24
It still doesn't look quite right. The column {locales_source}.textgroup is missing. This was removed in #1188430: Rip out textgroup support from locale module for D8, but it is still there in D7 - but it is missing from the dump in drupal-7.filled.database.php.gz.
db_create_table('locales_source', array('fields' => array(
'lid' => array(
'type' => 'serial',
'not null' => TRUE,
),
'location' => array(
'type' => 'text',
'not null' => FALSE,
'size' => 'big',
),
'source' => array(
'type' => 'text',
'mysql_type' => 'blob',
'not null' => TRUE,
),
'context' => array(
'type' => 'varchar',
'length' => 255,
'not null' => TRUE,
'default' => '',
),
'version' => array(
'type' => 'varchar',
'length' => 20,
'not null' => TRUE,
'default' => 'none',
),
),
'primary key' => array(
'lid',
),
'indexes' => array(
'source_context' => array(
array(
'source',
30,
),
'context',
),
),
'module' => 'locale',
'name' => 'locales_source',
));
#25
Uhm, right. @rvilar: can you fix this? (I think this is still critical, but now its a bug).
#26
Ok, I check this right now!
#27
Here is a patch that resolves this little but critical issue
#28
Sorry! Changing the state
#29
@rvilar: thanks! @c960657: did you see any other missing pieces?
#30
I didn't see any other missing pieces, but I did not do a complete check. I only saw this specific problem while trying to add some upgrade hooks to the locale module.
#31
All right, I did a line by line review of the schema and all its properties, and indeed the textgroup was the only one missing. Then applied the patch and looked if it was added at the right place in the schema (which the non-unified diff posted above did not really prove). And it was added at the right place. Here is the diff in unified form.
#32
As pointed out #1223502: Update for ripping out textgroup support in Drupal locale module is a great test for whether this fix is good, so I'm re-posting that here for proof as well as that combined with our fix from above to prove it fixes the schema as proven by the update. If we only try to update the schema without it being in D7, we get a fail (first patch attached). If we do have the textgroup in the schema, then we have it run fine (second patch).
#33
All right I prove with updates, I double checked with the schema as I documented above, so this looks good to go. The RTBC patch is in #27, which would kick forward #1223502: Update for ripping out textgroup support in Drupal locale module to pass as demonstrated by the passing test in #32.
#34
Looks good, committed/pushed to 8.x.
#35
#36
Automatically closed -- issue fixed for 2 weeks with no activity.
#37
Adding UI language translation tag and base language tag.