As a follow-up to #567832-128: Transliteration in core and #567832-133: Transliteration in core:

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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Basically I think that sun suggested we do (b).

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?

Also, see #1842710: Transliteration: Make language-specific character overrides use YAML files -- if we move the core language overrides into the alter hook, then the class-without-overrides becomes even more useless.

I'm also not sure where to move the Drupal-specific class -- can someone help me navigate to the correct namespace?

Gábor Hojtsy’s picture

Issue tags: +D8MI

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.

amateescu’s picture

The only Drupal specific thing in the transliteration class is that drupal_alter() call, so it definitely needs to stay in Component.

jhodgdon’s picture

RE #3 - well the plan in #1842710: Transliteration: Make language-specific character overrides use YAML files is to put all the language overrides into hooks, so that would mean taking away the ability for any language overrides in the Component class.

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?

sun’s picture

Title: Transliteration: No Drupalisms in \Component » Transliteration component must not contain drupal_alter()
Category: task » bug
Status: Active » Needs review
FileSize
2.41 KB

Clarifying issue title.

Attached patch is all we need to do.

amateescu’s picture

Title: Transliteration component must not contain drupal_alter() » Transliteration: No Drupalisms in \Component
Category: bug » task
Status: Needs review » Active

Those language-specific overrides are not core (Drupal) specific, they are useful for anyone who wants to use our class, so I do not agree with that plan.

and why we want to have one Component class that we suggest no one ever uses

Who says we suggest that? This class is actually very useful outside Drupal.

amateescu’s picture

Title: Transliteration: No Drupalisms in \Component » Transliteration component must not contain drupal_alter()
Category: task » bug
Status: Active » Reviewed & tested by the community

Cross-post. Yes, this is exactly what we need.

jhodgdon’s picture

Status: Reviewed & tested by the community » Postponed
sun’s picture

Status: Postponed » Needs work
+++ b/core/lib/Drupal/Component/Transliteration/PHPTransliteration.php
@@ -204,9 +204,6 @@ protected function readLanguageOverrides($langcode) {
-

+++ b/core/lib/Drupal/Core/Transliteration/PHPTransliteration.php
@@ -0,0 +1,29 @@
+ * Contains \Drupal\Core\Transliteration\PhpTransliteration.

Ehhh... There's a case mismatch between the @file doc and the actual filename, and I copied the class name from the @file doc into the use statement.

PSR-0 is case-sensitive, so this will break... Will attach an adjusted patch in a minute.

jhodgdon’s picture

The classes should be named "PHPTransliteration". Sorry if I got something wrong in the earlier patch! :)

And I agree with going ahead here on this patch, given the comments on the other issue.

sun’s picture

Status: Needs work » Needs review
FileSize
1.78 KB
2.72 KB

Fixed letter-casing in PHPTransliteration.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Glad that Jennifer is also on board now :)

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

There are some problems with this patch actually:

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.

Previous code:

     drupal_alter('transliteration_overrides', $overrides, $langcode);
     $this->languageOverrides[$langcode] = $overrides[$langcode];

Current code:

    drupal_alter('transliteration_overrides', $this->languageOverrides[$langcode], $langcode);
jhodgdon’s picture

RE (d) in #13 - that explains the test failures.

sun’s picture

Status: Needs work » Needs review
FileSize
7.59 KB
9.06 KB

Ah, yeah. Thanks for the review!

Status: Needs review » Needs work

The last submitted patch, translit.core_.14.patch, failed testing.

jhodgdon’s picture

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?

/**
-   * Returns this PHPTransliteration object (for the Drupal Container).
-   */
-  public function get() {
-    return $this;
-  }
-
-  /**

Maybe, maybe not? I am not sure about these container things.

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

jhodgdon’s picture

Status: Needs work » Postponed

Just a note... catch just commented on #1842710: Transliteration: Make language-specific character overrides use YAML files that he meant the core and contrib language-specific overrides should use YAML files, not that both should use hooks.

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.

sun’s picture

Status: Postponed » Needs review
FileSize
3.95 KB
12.36 KB

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.

Fixed the other issues.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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!

sun’s picture

Yup! :) Note that it's not a UnitTestBase but leveraging DrupalUnitTestBase though.

Powered by #1774388: Unit Tests Remastered™, which just happened to land today. :) (and comes with an extensive change notice)

sun’s picture

+++ b/core/lib/Drupal/Component/Transliteration/PHPTransliteration.php

  * class, Copyright © 2004 Brion Vibber <brion@pobox.com>,

Hah! Only now I recognized the © char here — usually that's expressed through @copyright or @author, but of course, that's more than appropriate for a Transliteration class! :-)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks good to me. We can eventually make the drupal_alter() use the Extensions class but for now this at least separates out the Drupal-specific stuff. Committed/pushed to 8.x.

amateescu’s picture

Title: Transliteration component must not contain drupal_alter() » [HEAD BROKEN] Transliteration component must not contain drupal_alter()
Priority: Normal » Critical
Status: Fixed » Needs review
FileSize
824 bytes

We need to explicitly register the transliteration service if we are extending DrupalUnitTestBase.

chx’s picture

amateescu’s picture

Title: [HEAD BROKEN] Transliteration component must not contain drupal_alter() » Transliteration component must not contain drupal_alter()
Priority: Critical » Normal
Status: Needs review » Fixed

Sure :)

klausi’s picture

Title: Transliteration component must not contain drupal_alter() » [HEAD BROKEN] Transliteration component must not contain drupal_alter()
Priority: Normal » Critical
Status: Fixed » Needs work

This should be fixed here since this issue caused it.

chx’s picture

Title: [HEAD BROKEN] Transliteration component must not contain drupal_alter() » Transliteration component must not contain drupal_alter()
Priority: Critical » Normal
Status: Needs work » Fixed

No, it was not this issue that caused it, klausi read the other damned issue, please.

Status: Fixed » Closed (fixed)

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

jhodgdon’s picture

Component: base system » transliteration system

Component update for posterity.

klonos’s picture

Issue summary: View changes
Parent issue: » #567832: Transliteration in core

...testing issue relationships ;)