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.
Comment | File | Size | Author |
---|---|---|---|
#32 | 1336170-32-without-textgroups.patch | 992 bytes | Gábor Hojtsy |
#32 | 1336170-32-with-textgroups.patch | 150.96 KB | Gábor Hojtsy |
#31 | db_diff.txt | 530 bytes | Gábor Hojtsy |
#27 | locale-upgrade-1336170-27.patch | 150.33 KB | rvilar |
#27 | db_diff.txt | 169 bytes | rvilar |
Comments
Comment #1
catchAdding tag.
Comment #2
Gábor HojtsyComment #3
Gábor HojtsyI think adding to the current filled database dump a locale db with a couple of languages configured is good.
Comment #4
rvilarI start working on this right now!
Comment #5
rvilarDone! I attach a file that updates drupal-7.filled.database.php.gz with languages dump information
Comment #7
Gábor Hojtsy@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?
Comment #8
Gábor HojtsyUps, 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).
Comment #9
rvilarI attach a new pathc.
It's only the binnary patch, because the uncompressed version is very huge.
Comment #10
Gábor HojtsyWell, 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.
Comment #11
rvilarI attach the patch and a diff file to see what I do.
Comment #12
Gábor HojtsyIn 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!
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedcoming 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?
Comment #14
Gábor Hojtsy@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).
Comment #15
Gábor Hojtsy@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!
Comment #16
rvilarI think this patch is the correct. I've revised all d7 db structure and checked all is in D7 form
Comment #17
rvilarChanging status to execute the drupal test bot
Comment #19
Gábor HojtsyOk, looks like the filled test case fails on creating the languages table, from the logs linked above:
This seems to stem from the "length" property mistyped in
Comment #20
rvilarups! I'll reroll and upload the patch this afternoon
Comment #21
rvilarAdded a new patch that solves this issue.
Comment #22
Gábor HojtsyThis 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.
Comment #23
catchThis 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!
Comment #24
c960657 CreditAttribution: c960657 commentedIt 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.
Comment #25
Gábor HojtsyUhm, right. @rvilar: can you fix this? (I think this is still critical, but now its a bug).
Comment #26
rvilarOk, I check this right now!
Comment #27
rvilarHere is a patch that resolves this little but critical issue
Comment #28
rvilarSorry! Changing the state
Comment #29
Gábor Hojtsy@rvilar: thanks! @c960657: did you see any other missing pieces?
Comment #30
c960657 CreditAttribution: c960657 commentedI 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.
Comment #31
Gábor HojtsyAll 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.
Comment #32
Gábor HojtsyAs 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).
Comment #33
Gábor HojtsyAll 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.
Comment #34
catchLooks good, committed/pushed to 8.x.
Comment #35
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #37
Gábor HojtsyAdding UI language translation tag and base language tag.