Rip out textgroup support from locale module

This seems like a reasonable step, but does this relate to the "context" parameter of t() or that's a separate thing?

Yes, this seems like what's needed. Gabor's arguments are convincing. My experience is that anyone who's worked deeply with the textgroup system in locale has had to spend more time working against the system than with it. While valiant, the effort to solve these problems in i18n in Drupal 6 couldn't address the heart of the problem and produced a mishmash of custom fixes and overrides.

Generally we don't strip a big system out before having a replacement for it, but in this case the system is used only in contrib, and continuing to use the system in contrib is the wrong focus for new work. Overall, this looks like a long overdue rollback that will provide a cleaner slate for starting afresh.

But I'm speaking as someone who worked in the past on internationalization but has moved on. We need to hear from those in the community most active in this area, including Jose Reyero, active contributors to http://drupal.org/project/i18n, sun, plach.

As I was writing elsewhere, I think one of our major targets for D8 should be to design an architecture in which all the different objects will be treated in a unique, consistent way. Textgroups are a first attempt in this direction, but they failed since they put far too much burden on the shoulders of developers willing to support them. #245424: Make Webform multilingual (i18n) aware through contributed modules is a clear example of this. IMO in D8 we should try hard to make i18n integration "come for free" whenever possible, so I fully support this approach. And I agree that removing textgroups from core in D8 should not be problematic at all, provided that we switch to a different implementation in the D7 version of i18n.

I tested the patch and it behaves as advertised. Most of the modified queries and above all the new ones could use some DBTNG-fying, but aside from this it looks RTBC. Obviously a feedback from Jose and Daniel would be welcome.

Minor thing:

+++ b/modules/locale/locale.install
@@ -262,3 +255,16 @@ function locale_schema() {
+  db_query("DELETE FROM {locales_target} WHERE lid IN (SELECT lid FROM {locales_source} WHERE textgroup != 'default')");
+  db_query("DELETE FROM {locales_source} WHERE textgroup != 'default'");
+  db_drop_field('locales_source', 'textgroup');

Is not dropping the textgroup column enough?

Edit: forget this, it was too late ;)

Powered by Dreditor.

Didn't mean to change the status, Dreditor did ;)

Yeah, I got that this morning: 3.00 am is not the best time to review a patch ;)

The idea looks good to me. However I would keep (hidden) 'textgroup' support as a method to keep track of groups of strings, I think we still have some use for it.

When you create a site with standard (contrib) modules and custom modules, you'll need a way to tell apart which strings are from which group. Also, for contributing strings back (l10n_client) we should have a way to differentiate these strings. And this is textgroups.

In summary, textgroup could become now the way to know which l10n server or which group of modules each string comes from.

So I would do the following:
- Make the t() function unaware of textgroups, searching on the whole locale source table. Anyway we can keep it as an optional parameter in the $options array.
- Leave textgroups only for export/import functions and l10n_client (only strings in default textgroup will be posted to central l10n server).

@Gabor,

Yeah, that makes sense. In any case we'll need some field to keep track of where strings come from, where to send/download translations for them.

So maybe yes, instead of repurposing old features/fields, we build something cleaner from scratch. Let's get rid of textgroup. And I think location too.

(Anyway we need better string tracking, aka string ids, to know which strings belongs to where so let's build it from scratch).

+++ b/modules/locale/locale.admin.inc
@@ -747,32 +747,26 @@ function locale_language_providers_session_form($form, &$form_state) {
+  if (!empty($num_strings)) {

This test looks new (or maybe it's restoring what was there before textgroups were introduced?). When would it be the case that there are no records in locales_source? Do we need a fallback message if $num_strings is empty, e.g., t('No strings are yet available for translation.')?

Yes, I'd missed the initialized value, and the rest of the patch is straightforward. Looks good to go.

Is not DBTNG-fying new queries mandatory for a patch to be committed?

Ok, this was my unique concern about the patch. Let's get it in.

The update here should use db_delete(), not db_query('DELETE FROM..').

I don't see any other queries that need dbtng-ifying, not sure which ones plach meant. Didn't fully review the rest of the patch but enough other people marked it RTBC.

Restoring status but this should not actually get committed until #1182290: Add boilerplate upgrade path tests is fixed, however all this is doing is cleaning out cruft so I don't think the update function needs its own tests at all.

I'm marking this postponed on #1182290: Add boilerplate upgrade path tests so we don't end up with a zero-test-coverage upgrade path again.

@Gabor

+ * Drop textgroup support.
+ *
+ * Update assumes i18n migrated this data before the update happened. Core
+ * never used textgroups for anything, so it is not our job to find place
+ * for the data elsewhere.
+ */
+function locale_update_8000() {
+  $subquery = db_select('locales_source', 'ls')
+    ->fields('ls', array('lid'))
+    ->condition('ls.textgroup', 'default', '<>');
+  db_delete('locales_target', 'lt')
+    ->condition('lt.lid', $subquery, 'IN');
+  db_delete('locales_source')
+    ->condition('textgroup', 'default', '<>');
+  db_drop_field('locales_source', 'textgroup');
+}

In the current state of Drupal 8, due to #1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path) which wholesale removed tests rather than updating them, but was committed anyway, this function will have zero code coverage.

When #1182290: Add boilerplate upgrade path tests is committed, it will be covered by the tests there (assuming the D7 database dump has locale module enabled), so could move straight back to RTBC.

I do not think this this upgrade function needs specific tests, nor should it be the responsibility of the first patch to be committed to D8 with an update function to re-add all the baseline tests for the entire upgrade path that were prematurely removed. Hence that critical task rather than adding the 'Needs tests' tag here.

But we should not knowingly add code to Drupal 8 with zero test coverage, especially for something as critical, and as horribly broken in Drupal 7 (still) as the upgrade path, which has been broken for more than 2 years and still has critical bug reports outstanding.

This has nothing whatsoever to do with providing an upgrade path for textgroup functionality, note I marked the patch RTBC in #25, but after discussion with webchick in irc today we thought it was better to mark these patches (that add hook_update_N()) postponed rather than RTBC so it is clear what the situation is.

If Dries wants to commit patches to Drupal 8 with no test coverage for the upgrade path, he's welcome to un-postpone this and commit it. But if that happens, don't expect me to help with any upgrade path issues in Drupal 8 or 7 at any point in the future, I imagine others who have spent dozens if not hundreds of hours painstainkingly trying to put the D7 upgrade path back together again from a broken mess on the floor over the past two years would likely feel the same way.

It will receive test coverage in the most basic sense that the actual code in the update function will be executed.

Right now I could upload a patch that used the non-existing function db_drop_column() and that would pass tests for Drupal 8.

We could add meaningful tests for this patch (that the schema in a site upgraded from Drupal 7 to Drupal 8 matches the schema definition in Drupal 8), but that is best left to #1119094: Add a test to verify the schema matches after a database update.

Tagging issues not yet using summary template.

I reviewed this patch and it's very straight-forward. I trust the judgement of the participants in this issue when they say it's better to get rid of textgroup support.

Because of the API and UI changes introduced in this patch, I don't think we can backport this to Drupal 7. Hence, I'm marking this issue 'fixed'. Feel free to re-open this issue if you feel otherwise and we can discuss it.

Thanks Gabor and team!