Database driver names aren't translated

This is by design to an extent. st() is a Drupal-specific function and we're trying really really really hard to keep Drupal-specific dependencies out of the DB code. That creates a morass of hard circular dependencies that have a tendency to break in spectacular and horrific ways. :-)

I mean we really need to find an alternate way to handle translation since this is one of an increasing number of cases where "regex for t(" as a way to find translatable strings fails. Unfortunately that's not something that was figured out for the D7 cycle. :-(

Let's get some input here, since there are clearly competing architectural priorities and we can't have a real resolution until D8 at this rate.

In this case, I'd go with keeping the DB layer portable over translating one obscure string.

+1 to this patch.
It introduces a hard dependency of the DB system on install.inc. Why that is not THAT bad in this case:
1. it's a UI-facing name. It's not integral to making the DB system work. You can use DBTNG just fine without st(), you just can't display that name.
2. It just one string. It's not a new paradigm we're introducing here or a circular dependency or anything horrible. It's just one string.

Unless I'm mistaken, there are already some other minor things like this in DBTNG that make it non-portable in D7. Making it fully portable in D8 is a good goal. But I can't really see the logic in introducing this user-facing inconsistency to achieve an ideal architectural goal that isn't actually there in D7. Unless I'm missing something?

My only reply is that Drupal is a battlefield. If you have a shared goal in mind, you have to push toward it all the time. In general, I dislike the argument of "Its already a little imperfect, so why not degrade some more.". Or "We do it like that over there so we should/have_to be consistent here."

All that said, this is the most minor of issues and any resolution will be fine.

That's a fair comment.

I'm coming at it more from a "which is going to generate more support requests and be a bigger burden on the community? The fact that there's this blatantly English string staring every single multilingual site install in the face, or the fact that DBTNG can't be copy/pasted into a separate PHP application?" Since DBTNG already can't be copy/pasted into a separate PHP application, the latter is a no-op, whereas the first is a huge WTF.

Actually most of DBTNG *can* be copy/pasted into another PHP application. You need to comment out 2, maybe 3 lines (like drupal_alter() in SelectQuery) and it works (at least on MySQL; I don't know who else has tried). So we're surprisingly close as is. :-)

I know the menu system manages to pull off post-DB translations for page titles. How does it do that, and could we do the same thing here?

It does that by not translating the strings, and leaving it instead to i18n module/Content Translation. Neither of which are available during installation.

And "you need to comment out 2, maybe 3 lines" is not "you can copy/paste into another PHP application." It's "you can copy/paste into another PHP application *if*..."

So this just keeps the existing status quo (albeit by adding one more if), and doesn't need to introduce something weird and one-off for this one string in Drupal.

Incidentally, if you are not going to listen to committer feedback when you receive it, then please don't ask for it. :P

This is a patch that maybe makes Crell happy. It probably makes Gabor Hojtsy a bit unhappy, though. :)
Idea coming from David_Rothstein in #914788: Database driver names aren't translated (which I just marked duplicate of this).

Explanation:
This translates the strings on display in install.inc. Therefore there is no dependency of the DB-system on Drupal. Translating variable strings is not all that nice, but it's also not THAT bad. We would just have to put

st('MySQL, MariaDB or equivalent');
st('PostgreSQL');
st('SQLite');

somewhere in install.inc for the translation parser to pick the right strings up, but I don't know what the preferred way of doing that is. Contrib database types just have to put st('My DB'); somewhere in their code, if they want the name to be translatable.

Edit: Grammar.

My rationale for this is a simple one: $driver->name() is a generic method, and we can't predict all the ways people might use it (within Drupal itself, I mean - forgetting about other hypothetical PHP apps). For example, if someone wants to use it outside of the installer, or if they want to use it in a context where it needs to be translated into some other language besides the default one, then having st() already called on it is the wrong behavior.

I could be mistaken, but as far as I know there is no huge barrier to getting it translated with this approach. I think there would just need to be a followup issue at http://drupal.org/project/potx to add some code that looks for these. The strings here appear in a very specific place in the codebase (only in database drivers, only in a method called 'name'), so it should be a lot easier than most of the special-case code that module already has; for example, the code it has to parse the entire codebase looking for format_plural() calls and such.

The above patch looks like it was missing a couple other places where this needs to be translated, though. I think the attached gets all of them.

Great, thanks! I had only looked at where the driver selection form gets rendered, I hadn't actually grepped for "->name" or whatever.

I cannot speak for Gabor but I am OK with #13.

This works for me too, but I'd like Gábor to take a look. I left him a message in IRC, so hopefully he'll see it. If not, I'll get this in in the next couple of days.

As @tstoeckler in #12 explained, it is pointless to commit this patch if the strings themselves do not show up in some way for string extraction to be saved.

@Crell: re #3, "regex for t(" is not what is actually happening. Drupal core only recognizes strings that it encounters with t() invocations (which are stored as translatable in the DB once found). st() works from pretranslated strings. The potx module does tokenization of the PHP code and parsing of the tokenized structure. It cannot obviously run the source code to have the same effect as Drupal. While that might find some strings that people insist translating dynamic, it would miss a whole bunch of other strings, like error messages. Not even our simpletests run all possible code paths in all our files to touch all lines, and combinations of strings translated.

So we so far resorted to static code analysis in preparsing files. When the static code analyzer sees st($installer->name()), it will throw a warning, as it does not know what kind of values $installer->name() might return, and whether those can be collected some other way in the code (see #12 again).

The single focus of looking for translatable strings is that they should be identifiable with static code parsing. There is no other way to look at all lines of code. So if the DB drives are not going to return their names and translatable text already translated via st(), then we need to figure out a different way to mark translatable strings, which is statically parsable. Since Drupal core itself does not do any static code parsing, this is not really a Drupal core API question. it is not something we missed figuring out in Drupal 7, because Drupal core does not do any of this. We can always figure this out.

We can obviously introduce new no-op ways to mark strings, like: #st(....) but these are not easily tokenized, and we would in fact need to resort to regexing, which is not good. A possible no-op way currently is:

mymodule_translatable_strings_for_installer() {
  st(....);
}

This keeps the code as tokenizable PHP while introduces a function that nobody will ever invoke. There is no defined pattern for this function naming AFAIS, so I just made it up.

Finally, we could always make up ways to inject translatable strings via .info files, but database drivers, among other objects in Drupal lack .info files, right? They only have running code, no metafiles to use for this purpose.

This patch as-is has no use, since the database driver names will not be available for translation with the patch applied, unless translators put in the source strings manually to their .po files. We need to add a way for them to be found. Ideas are always welcome. I'm always happy to extend potx if you have better ideas. Once again, it is not something that is limited to the D7 cycle, we can introduce new patterns for Drupal 5 modules even :)

Quoting David_Rothstein in #13:

I could be mistaken, but as far as I know there is no huge barrier to getting it translated with this approach. I think there would just need to be a followup issue at http://drupal.org/project/potx to add some code that looks for these. The strings here appear in a very specific place in the codebase (only in database drivers, only in a method called 'name'), so it should be a lot easier than most of the special-case code that module already has; for example, the code it has to parse the entire codebase looking for format_plural() calls and such.

Yeah, I don't understand why this was set back to "needs work"? We already know we need a followup POTX patch for this. But the patch here, the one for core, shouldn't need to change.

To see a little more detail of what we would need to do to POTX, I took a quick look here:
http://drupalcode.org/viewvc/drupal/contributions/modules/potx/potx.inc?...

There are already functions in that file like _potx_find_watchdog_calls() and _potx_find_format_plural_calls() that go through the tokenized file and look for untranslated strings that appear in expected places. Don't we just need to write another function like one of those?

By the way, the reason I previously removed the "string freeze" tag from this issue is.... we aren't changing any strings :) The strings are already there.

Am I wrong (i.e., is there any reason this is on some kind of RC1 deadline)? As far as I can tell, this patch could be committed at any time.

Well, wouldn't it impact translators, by adding new strings on them at some point in the future?

Well, yes, and the sooner the better, I guess.... But the point is, the bug is already there. The strings are being displayed untranslated already.

In other words, I don't think we would bump this issue to D8 right after RC1, because what would be the point? If the patch goes in but the translators take a while to get around to translating these strings (or even never do), the situation is no worse than if the patch didn't get committed at all. So it doesn't seem like there's any downside to committing it whenever it's ready.

Sure, we always allow bug fixes of strings, even into stable releases. But "string freeze" is still an appropriate tag, since we can't call strings fully 'frozen' until this is fixed.

Well, in my understanding, special casing is much more error prone then just going with the standard API. Looking at the database drivers and the patch, we seem to require translation for the database driver name only so far. There is at least #346494: DB drivers need to be able to change the configure database form during install in the D7 critical queue, which adds a healthy dose of form API and st() calls to the sqlite driver. We can strive to avoid st()s in database drivers here, but it seems to happen anyway in other issues.

Let's assume we avoid st() for now. We should then define a pattern for identifying the name string. Note that for a static code parsing method to work, we need a well defined pattern. It looks like the pattern would be: A string literal in a method called name() in a class extending DatabaseTasks. Note that if you use different inheritance for example (just to use the OOP framework for what it excels at), then this pattern will break down. Once you start to inherit your class from customDatabaseTasks or somesuch, your name string will not be found. This is the achilles heel of these special casings (and the reason we try to avoid them).

So are we setting a rule that each database driver should inherit strictly from DatabaseTasks and define its name as a literal string in the name() method, which we are going to implement special casing for in potx to avoid using st() for a few more days in database drivers (until #346494: DB drivers need to be able to change the configure database form during install lands)?

Gábor, the database drivers already use st() in some places, and #346494: DB drivers need to be able to change the configure database form during install would add more, but in places where it makes sense to. What we're trying to avoid here is adding it in a place where it doesn't make sense - see #13.

Since the install code for these drivers always lives in includes/database/*/install.inc I would think there are a number of easy ways we can detect we are in the right method. I haven't tried to write it though.

Well, I understand there might be places where it makes sense to translate in other ways. get_t() already exists to support both runtime and install time translation and is used extensively in core. That covers supporting runtime translation. As for translating to other languages, most Drupal APIs do not support that. Form API is defined for example in that it defines translated strings. Of course we can go other ways here, there is already menu API which is an exception.

I understand there is no support for using the translation API here, and instead we will go to define more exceptions to the basic rules. Let's do that then. I've opened #980852: Add exceptions for D7 database driver names for the extraction, contributions are welcome there once this lands.

st('MySQL, MariaDB or equivalent');
st('PostgreSQL');
st('SQLite');

MySQL, MariaDB, PostgreSQL and SQLite are product names. They don't need to be translated. Thus, st('PostgreSQL') and st('SQLite') is rather pointless.

The only problem I see is 'or equivalent'. Maybe we can fix this bug by writing: "MySQL/MariaDB"?

Needs review for an answer to Dries's question.

But that actually sounds fine to me, other than if the "/" would be different in different languages.

Don't they still need to be translated in languages that use a different alphabet?

(Also, of course there could be a contrib driver that has the same problem - I don't think we should decide this based on the ones that happen to be core.)

I'm pretty sure the "MySQL/MariaDB" wording was discussed and rejected in #861192: Document that MariaDB works, too, because we want to make sure it's completely clear to people that they can use one or the other. We don't want anyone to mistakenly think that they need to go out and find some MySQL/MariaDB hybrid thing that they've never heard of in order to install Drupal :)

OK, well, blargh.

I just reviewed the code in the DB layer, and it looks like we are *already* using st() in the driver install classes. MySQL doesn't, but Postgres has additional checks that have their own error messages, which are already wrapped in st().

Since the installer is (somewhat) Drupal-centric anyway, it's unlikely that a 3rd party version would use them, at least in their current form. Plus, we are going to have that problem anyway at least for Postgres. It looks like this is a considerably more complicated problem than it appears, too.

Personally I would call that a critical flaw in the way we handle translation, as it means we cannot translate anything that is not hard-coded specifically for Drupal; that's a terribly bad idea, especially if we ever hope to better integrate and collaborate with the rest of the PHP-using world (which we absolutely have to do if we want to remain a leading platform). It also creates a circular dependency within the system. That's a problem we will have to solve, but we're not going to solve it right now. File that as a Drupal 8 critical. :-)

So for now I am going to rescind my objection here and green-light just throwing st() into the driver install classes. That said, the patch in #1 is incomplete as it only addresses MySQL. Here's a new patch that addresses all three driver classes.

Hm. Should that be st() and not t() on pgsql/mysql? or is sqlite wrong?

In any case, something here needs work. :)

Why yes, yes it should... *facepalm*

See #13.

Er, and #26, I think I meant to say :)

Actually, if Crell is basically fine with #32, I'd rather commit that. It's a much more straight-forward fix, and forcing a new requirement on the potx maintainers 3 days before string freeze seems like a very unpleasant thing to do.

So, committed #32 to HEAD.

I see Dries's point that we really ought not to be translating these product names in the first place, but this future-proofs us in case the a driver ever has to put something like "Oracle: The Evil Empire" or "DB2: You might as well shoot yourself right now" in its description. :P It's also more consistent with just about everywhere else we put user interface strings.

OK, count me confused...... Without at least a get_t() in there, won't this potentially cause white screens in calling code, based on the discussion above?

(In the other issue, Gabor and I already figured out how to deal with this in POTX with no pain whatsoever, by the way.)

Answered my own question :) Because the DatabaseTasks class lives in install.inc, that file will always be autoloaded when the class is used, so st() will always be available also. That's fragile, but it works.

The committed patch is still wrong, because we should always be using t() when available, but I don't personally care enough to reroll it for that... I'll RTBC if someone else does.