Transliteration component must not contain drupal_alter()

(Drupal 8 Multilingual Initiative) is the tag used by the multilingual initiative to mark core issues (and some contributed module issues). For versions other than Drupal 8, use the i18n (Internationalization) tag on issues which involve or affect multilingual / multinational support. That is preferred over Translation.

Apparently we are not allowed to have Drupal-isms such as hooks and alter hooks in \Component classes. The new Transliteration classes do have alter hooks and they are currently in \Component.

So we should either:

a) Move this class out of \Component so it is allowed to have alter hooks.
b) Take the Drupal-specific parts out of this class, and extend it with a new class somewhere else that does have the Drupal-specific bits.

This issue is to figure out whether (a) or (b) makes more sense, and then to do that option.

Comments

But I don't see the advantage of having one class with no Drupal-specific stuff and another one with the drupal_alter()... Why exactly would we want to have the non-Drupal-specific class at all? Why would it be useful to anyone?

If we don't see the value in a non-Drupal specific class we can just move the whole thing over to the Drupal namespace, and just ignore Component. The thinking behind putting things in component is to have non-Drupal specific code that others might want to reuse. Any project might look at our libs and want to reuse our stuff theoretically.

In which case my question is whether the class is any use at all, and why we want to have one Component class that we suggest no one ever uses, and a second "we actually want to use this one" class? Why not just have one class that has a chance of actually being useful?

Documentation adjustments needed (and I don't have time to make a new patch, sorry!)

a) The Component class still has docs saying:

* This class is the registered transliteration class returned from
* drupal_container()->get('transliteration') by default.

which is no longer accurate.

b) The Component class's readLanguageOverrides() method docs still mention the hook, which is no longer invoked.

c) I think the Core class's documentation should be expanded a bit to mention the specific alter hook, and the one-line doc that says "Enhances PHPTransliteration with..." is ambiguous, since its own class name is PHPTransliteration too.

Regarding the code:

d) I think you have also changed the alter hook. The previous definition of the hook said that you passed in the full overrides array and let hooks alter it. This definition has it only getting the part of the array for this language. That is OK, but the test class would need to be changed, and also the hook documentation if you want to make that change.

Those test failures are strange... It looks like for some reason the character Ä is being transliterated to a W by default, and the override in the language test class is changing that to an A (it should be a Z in that test class).. I am not sure why but there is some kind of an error there...

So, let's see. A few things I can see with that last patch:

a) The base \component class is now missing the @ingroup transliteration that it had before, I think that should be put back in.

b) I think that the new \core class needs that method you took out of the base class, doesn't it?

c) The test probably needs to instantiate the new Core class instead of the old Component class.

d) #13 (c) still applies [docs update]

e) Oh, I see the problem. The test class needs to not have this part any more:

else {
- $overrides['zz'][0xC4] = 'W';
+ $overrides[0xC4] = 'W';
}

That just needs to be removed entirely. That was only working due to the previous functionality of how the hook was being done -- if you invoked the hook on language 'qq' and only changed 'zz', that change was being ignored by the previous code, since after invoking the alter hook it only kept the part relevant to the language it asked for. Now it is being done differently because the hook is different. So just take that out and the tests will pass. I think. :)

So... that issue is still open... and once again I am wondering if we should postpone this issue until we decide on that other one, because I think if we use YAML files, then we don't need this separation of Component/Core (I think if we use our YAML system we lose the Component, right?). I am not conversant enough with how this would work to be sure, but let's discuss over on the other issue for the moment.

I do not think that the file format of the data files changes anything with regard to this issue. Additional transliteration data provided by modules can use whichever format they want and like best. The Transliteration component itself, however, has no reason to use a different format, or the same format as Drupal modules may choose to use.

a) The base \component class is now missing the @ingroup transliteration that it had before, I think that should be put back in.

Each component forms its own group, by design. I'm aware that this is not reflected in API module yet, but we generally do not add @ingroup directives to the components in Drupal\Component\*, since especially the components in Drupal\Component\* cannot really do that, because they are supposed to be decoupled code that is re-usable in other, non-Drupal applications, so any kind of @defgroup or @ingroup directives would have a non-predictable and unintended impact within the API documentation of other applications.

We can keep the @ingroup for the class in Drupal\Core though.

- public function get() {

I double-checked the current code and this seems to be a stray left-over from the earlier code that had a TransliterationFactory. The service container does not need a ->get() method on instantiated service objects, so this can be safely removed.

I think the Core class's documentation should be expanded a bit to mention the specific alter hook

I think that's the wrong place to document such things. No one will reasonably look into the class to find out that there is an alter hook — that's what the hook documentation in language.api.php is for. For the \Core\PHPTransliteration class, I think it is sufficient to state exactly what it states - it invokes a hook, and that's all about it.

RE #19 - I guess I agree with all of that -- at least if the Core class has the @ingroup, people can go there to learn about the hook. And I see that it's in the docs for the overridden method, so that looks good.

So... I like this patch! It is a clean separation of the Core and Component classes, and it makes a lot of sense to me. I also like that you were able to get this to run with Unit instead of Web test base -- must be due to some recent work in UnitTestBase. :) Let's ship it!