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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Issue tags: +D8 upgrade path

Adding tag.

Gábor Hojtsy’s picture

Issue tags: +D8MI
Gábor Hojtsy’s picture

I think adding to the current filled database dump a locale db with a couple of languages configured is good.

rvilar’s picture

Assigned: Unassigned » rvilar

I start working on this right now!

rvilar’s picture

Status: Active » Needs review
FileSize
353 bytes

Done! I attach a file that updates drupal-7.filled.database.php.gz with languages dump information

Status: Needs review » Needs work

The last submitted patch, locale-db-dump-133170-5.patch, failed testing.

Gábor Hojtsy’s picture

@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?

Gábor Hojtsy’s picture

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).

rvilar’s picture

Status: Needs work » Needs review
FileSize
191.42 KB

I attach a new pathc.

It's only the binnary patch, because the uncompressed version is very huge.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
FileSize
1.86 KB

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.

rvilar’s picture

Status: Needs work » Needs review
FileSize
191.83 KB
4.48 KB

I attach the patch and a diff file to see what I do.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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!

Anonymous’s picture

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?

Gábor Hojtsy’s picture

@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).

Gábor Hojtsy’s picture

@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!

rvilar’s picture

FileSize
5.26 KB
329.67 KB

I think this patch is the correct. I've revised all d7 db structure and checked all is in D7 form

rvilar’s picture

Status: Needs work » Needs review

Changing status to execute the drupal test bot

Status: Needs review » Needs work

The last submitted patch, locale-upgrade-1336170-16.patch, failed testing.

Gábor Hojtsy’s picture

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' => '',
<     ),
rvilar’s picture

ups! I'll reroll and upload the patch this afternoon

rvilar’s picture

Status: Needs work » Needs review
FileSize
5.26 KB
480.5 KB

Added a new patch that solves this issue.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

Status: Reviewed & tested by the community » Fixed

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!

c960657’s picture

Status: Fixed » Needs work

I think this patch is the correct. I've revised all d7 db structure and checked all is in D7 form

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',
));
Gábor Hojtsy’s picture

Title: Add locale module to upgrade tests » Incorrect schema in locale module update tests
Category: task » bug

Uhm, right. @rvilar: can you fix this? (I think this is still critical, but now its a bug).

rvilar’s picture

Ok, I check this right now!

rvilar’s picture

FileSize
169 bytes
150.33 KB

Here is a patch that resolves this little but critical issue

rvilar’s picture

Status: Needs work » Needs review

Sorry! Changing the state

Gábor Hojtsy’s picture

@rvilar: thanks! @c960657: did you see any other missing pieces?

c960657’s picture

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.

Gábor Hojtsy’s picture

FileSize
530 bytes

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.

Gábor Hojtsy’s picture

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).

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks good, committed/pushed to 8.x.

Tor Arne Thune’s picture

Title: Incorrect schema in locale module update tests » Add locale module to upgrade tests
Category: bug » task

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Gábor Hojtsy’s picture

Issue tags: +language-base, +language-ui

Adding UI language translation tag and base language tag.