I was deeply involved with adding textgroups into core (in #139970: The big locale cleanup (and adding text domain functionality)). They proved to be a very flawed concept over the years though. The goal of textgroups in locale module was to avoid coding a custom translation backend for all the configuration pieces in Drupal: menus, blocks, contact forms, etc. These all need translation for some of their properties but all our custom, so it looked like it makes sense to provide a central storage place for them.

Now, all that Drupal core does is this central storage place, and it exposes editing/translation for these on the regular locale UI. There are various issues with that as I've detailed in my post on why using t() is wrong for configuration items (read at http://groups.drupal.org/node/149984):

- the locale subsystem assumes the source strings are English, while for configuration that is not true; we assume the configuration strings are in the site default language, which can be wrong at times as well; this messes up the translation editing UI and process quite a bit; English to other language and foreign language to other foreign language items show up side by side

- the locale subsystem does not have any structure or relations between strings; if you have t('Home') and t('Next'), these are just standalone strings to translate; a menu item has a title and description, a contact form as a subject and an autoreply text; these should be edited related to each other, not in isolation; the locale system does not provide anything for that

- the locale subsystem does not have provisions for input widgets and validation; the translations input are checked for XSS, but that is it; if your input should be email addresses or a username, validation will not be available; also the widget will not have autocompletion, a format selector or anything like that

- the main reason we reused this was for .po file versions of the custom configuration strings to be exportable and importable for free; turns out it was impossible to use this feature on Drupal 6 for years(!) see #384794: Preceding white space inserted in locales_source, so it was not actually that much desired

- text stored with the locale subsystem obeys the generic "translate interface" permission, which can be too broad to let people edit allowed field values or reconfigure contact forms

The i18n module makes a LOT of effort to plug in some holes in this system. It tries to augment the permission handling for example with its own input format checker, so that you at least don't get text for translation that was in a source input format that the site did not allow for translation. It is a distinct setting though for i18n, not a permission, since it cannot be dynamic. i18n module also uses the context feature in Drupal 7 to store structure for these strings, but that makes the filtering and the overall locale interface very confusing.

All-in-all, I've proposed for i18n in Drupal 7 to move away from textgroups to a better system, and Drupal 8 should not have any reason whatsoever to keep this frankeinstein there. If we happen to not solve any of the configuration translation needs, textgroups is still a very bad idea to try and support some contributed module to do that.

It should not take a lot to remove this given we only support it on the schema level and minimally on the UI, there is no real API to get data in or out of textgroups (minus the .po import/export which is pretty standard).

Parent issue

#1260628: META: Better translation management API in locale module

Files: 
CommentFileSizeAuthor
#41 remove-textgroups-for-good-4.patch41.76 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 33,638 pass(es). View
#37 remove-textgroups-for-good_3.patch40.38 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 33,563 pass(es). View
#29 remove-textgroups-for-good_3.patch41.04 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 33,513 pass(es). View
#27 remove-textgroups-for-good_2.patch41.03 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-textgroups-for-good_2.patch. View
#24 remove-textgroups-for-good_0.patch41.03 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-textgroups-for-good_0_0.patch. View
#4 remove-textgroups-for-good.patch40.93 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 31,422 pass(es). View
#2 remove-textgroups-for-good.patch40.84 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 31,422 pass(es), 0 fail(s), and 27 exception(es). View

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review

Here is the patch to rip them out completely. The patch:

- removes documentation of hook_locale and all core uses of it
- updates the schema and removes all non-default textgroup strings (assuming it will be the job of i18n module to provide an upgrade path)
- makes the user interface of export/import and string translation more intuitive by removing the confusing textgroup selector
- make the code a LOT simpler at places and sets us up for #361597: CRUD API for locale source and locale target strings that I'd like to work on next

This is required to rework this part of the locale module to be a modern, well focused tool for interface translation. Textgroups were never well supported. There are many ongoing issues in core about their brokenness such as #611480: _locale_import_one_string_db() should look at location for non-default textgroups and #495930: Translated strings not marked as translated when default language is not English (additionally to the recently fixed and above mentioned #384794: Preceding white space inserted in locales_source which made it impossible to actually use the locale import/export functionality with anything but interface strings).

This is clearly a dead end that we should get rid of as soon as possible to make space for the rest of the work.

Gábor Hojtsy’s picture

FileSize
40.84 KB
FAILED: [[SimpleTest]]: [MySQL] 31,422 pass(es), 0 fail(s), and 27 exception(es). View

Status: Needs review » Needs work

The last submitted patch, remove-textgroups-for-good.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
40.93 KB
PASSED: [[SimpleTest]]: [MySQL] 31,422 pass(es). View

Fixed one occasion of $omit that I missed. (I tried but failed to run the tests on my own machine due to some semaphore table errors, which seemed unrelated and are not reproduced by the testbot anyway).

Gábor Hojtsy’s picture

i18n module issue to prepare for the demise of this feature: #1191662: Migrate away from textgroups to own storage in i18n.

pwolanin’s picture

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

Gábor Hojtsy’s picture

This has nothing to do with context on t(). That is untouched by this patch, and it is intended to be fully supported in Drupal 8 (in fact possibly even more so than in Drupal 7 depending on where the discussion in http://groups.drupal.org/node/154394 goes).

Textgroups were an idea to support translation of object strings with the locale UI/database. Think blocks, menu items, contact categories, etc. i18n module in Drupal 7 uses it extensively but it has lots of problems doing so and using its own system would both require less code, be more secure and make more sense for users. We have #1191662: Migrate away from textgroups to own storage in i18n open for that migration in i18n module with various other reasons listed. Drupal 7 is going to have this functionality, but once i18n migrates away from it, there are no other modules left which use it. It is a confusing, badly implemented approach to provide a database backend for user editable configuration translation. The underlying problem of translating configuration pieces those will be solved by (a) making more things entities, see http://groups.drupal.org/node/155634 and by (b) dressing up the config system with metadata and hooks about its data objects, see http://groups.drupal.org/node/154434

Yes, those two things are not in place, but i18n will migrate away from textgroups anyway and we need to get rid of them soon to be able to continue with the refactoring in other patches like #361597: CRUD API for locale source and locale target strings and #1189184: OOP & PSR-0-ify gettext .po file parsing and generation, so we don't need to be bothered with textgroups in those issues anymore.

nedjo’s picture

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.

plach’s picture

Status: Needs review » Needs work

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.

plach’s picture

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review

We need to drop all data that is not in the default textgroup, because otherwise they would linger around there. Given that i18n module should migrate that data already, there should not be anything there theoretically by this time, but better be safe than sorry. Thanks for the support :)

plach’s picture

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

Jose Reyero’s picture

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

Gábor Hojtsy’s picture

@Jose Reyero: The supplemental info you describe sounds like what location field would be useful for. In fact with the current l10n_server => l10n_update method, location is kept empty. That is already used to maintain which file it comes from and I think we should work on using that field better instead of keeping another field for it. What do you think?

Jose Reyero’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for all the support, let's get this in then! Jose, I've opened #1206122: Better/consistent location tracking for translatable UI strings for your topic.

nedjo’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.')?

Gábor Hojtsy’s picture

Status: Needs work » Needs review

No, it is not at all new. The previous code had number of strings per group in the $groupsums array per textgroups. $num_strings is just what $groupsums['default'] was before. !empty($num_strings) is then also a replacement to the line two lines up from there in the patch which starts off !empty($groupsums[$data->textgroup]). There is already code to handle if there are no strings in the initialization of $rows[$langcode]. If you look there, the row is initialized to '0/' . $num_strings . ' (0%)', so it will start off as 0/0 0% at the start. This is the same before/after patch, the previous code just used $groupsums[$group] as discussed. It did initialize 0/0 0% and then overwrote it to 0/0 0% in this case, so the new code is more efficient in that it avoids queries and does not overwrite the properly initialized value. Please verify and help get this in.

nedjo’s picture

Status: Needs review » Reviewed & tested by the community

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

plach’s picture

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

Gábor Hojtsy’s picture

It did not seem to have been a requirement when context support was added to those queries. Either way, I'd like to continue with #361597: CRUD API for locale source and locale target strings which will rework many of the queries involved here in new ways, so I'm not sure it is time well spent if we convert them to something just to rewrite them again later. Really trying to avoid scope-creep here and make progress.

plach’s picture

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

catch’s picture

Status: Reviewed & tested by the community » Needs work

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
41.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-textgroups-for-good_0_0.patch. View

Well, here you go.

catch’s picture

Status: Needs review » Reviewed & tested by the community

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, remove-textgroups-for-good_0.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
41.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-textgroups-for-good_2.patch. View

Wups, rolled another version, hope this applies and passes :)

Status: Needs review » Needs work

The last submitted patch, remove-textgroups-for-good_2.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
41.04 KB
PASSED: [[SimpleTest]]: [MySQL] 33,513 pass(es). View

Updated my git checkout and rolled the patch again. It did apply to my updated checkout, so surprising that it did not apply on the testing server. Hm.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC based on test pass and numerous reviews above.

Gábor Hojtsy’s picture

I've documented how this fits to overall Druapl 8 plans at http://groups.drupal.org/node/161589.

catch’s picture

Status: Reviewed & tested by the community » Postponed

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.

Gábor Hojtsy’s picture

@catch: how could core concern itself with an upgrade path for a feature that was never used in core? The data format or usage of the feature provided was never defined and therefore cannot be relied on. I18n itself uses it differently depending on version even. Also we'll only see what to migrate that data to once (a) this have been removed and (b) we could work on real features for configuration translation. That sounds like postpones this for a year or so, for which time we need to toss around this big chunk of code we know we don't want. Is this the best process we can follow, really?

catch’s picture

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

Gábor Hojtsy’s picture

@catch: the core locale tests have nothing to cover non-default textgroups I believe, which this patch/update is about. Non-default textgroups are not a functionality that Drupal core used, and their format and usage is not defined at all. Therefore I don't think adding back general local update test coverage would do anything to help with test coverage for this update. We are updating from an unknown data structure that was only ever used in contrib (and even there depends on other schema elements that are outside of Drupal core's control such as other tables with metadata). I don't think we can make a meaningful test case for that in core. Also, this update merely drops data (assuming it will not actually drop any data, since the contrib should migrate its data away from there by the time D8 hits). The update does not migrate data, because the source data format is unknown, and we don't even know yet where it would be able to migrate data anyway. The actual update code will be the responsibility of those modules that know the source data format, and that is not in core.

In short, we are dropping data, while assuming we don't actually drop data, and even with the update tests added back, they'll not contain data for these conditions (Drupal's locale module never stored data with these conditions), so it will not receive any test coverage.

Now what?

catch’s picture

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 schema matches after 8-8 upgrade.

Gábor Hojtsy’s picture

Status: Postponed » Reviewed & tested by the community
FileSize
40.38 KB
PASSED: [[SimpleTest]]: [MySQL] 33,563 pass(es). View

Here is the patch without the update function, adding the update function at #1223502: Update for ripping out textgroup support in Drupal locale module on its own issue, so we can commit this one. Back on RTBC due to above reviews and discussions.

xjm’s picture

Tagging issues not yet using summary template.

Gábor Hojtsy’s picture

Priority: Normal » Major

Marking with proper priority.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
41.76 KB
PASSED: [[SimpleTest]]: [MySQL] 33,638 pass(es). View

Rerolled with the exact same changes. Also adding file level comment to locale.pages.inc that the other patch missed.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Was just a reroll, still passes so moving back to RTBC based on above reviews.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

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!

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: +language-ui

Adding UI language translation tag.

Gábor Hojtsy’s picture

Issue tags: +language-config

Tagging for config leg of D8MI.

Gábor Hojtsy’s picture

Issue summary: View changes

Add parent