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.
Comment | File | Size | Author |
---|---|---|---|
#24 | 18427260-followup.patch | 824 bytes | amateescu |
#19 | translit.core_.19.patch | 12.36 KB | sun |
#19 | interdiff.txt | 3.95 KB | sun |
#15 | translit.core_.14.patch | 9.06 KB | sun |
#15 | interdiff.txt | 7.59 KB | sun |
Comments
Comment #1
jhodgdonBasically 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?
Comment #2
Gábor HojtsyIf 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.
Comment #3
amateescu CreditAttribution: amateescu commentedThe only Drupal specific thing in the transliteration class is that drupal_alter() call, so it definitely needs to stay in Component.
Comment #4
jhodgdonRE #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?
Comment #5
sunClarifying issue title.
Attached patch is all we need to do.
Comment #6
amateescu CreditAttribution: amateescu commentedThose 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.
Who says we suggest that? This class is actually very useful outside Drupal.
Comment #7
amateescu CreditAttribution: amateescu commentedCross-post. Yes, this is exactly what we need.
Comment #8
jhodgdonCan we wait on this issue until we resolve #1842710: Transliteration: Make language-specific character overrides use YAML files please?
Comment #9
sunEhhh... 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.
Comment #10
jhodgdonThe 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.
Comment #11
sunFixed letter-casing in PHPTransliteration.
Comment #12
amateescu CreditAttribution: amateescu commentedLooks good to me. Glad that Jennifer is also on board now :)
Comment #13
jhodgdonThere 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:
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:
Current code:
Comment #14
jhodgdonRE (d) in #13 - that explains the test failures.
Comment #15
sunAh, yeah. Thanks for the review!
Comment #17
jhodgdonThose 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?
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:
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. :)
Comment #18
jhodgdonJust 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.
Comment #19
sunI 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.
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 inDrupal\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.
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 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.
Comment #20
jhodgdonRE #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!
Comment #21
sunYup! :) 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)
Comment #22
sunHah! 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! :-)Comment #23
catchLooks 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.
Comment #24
amateescu CreditAttribution: amateescu commentedWe need to explicitly register the transliteration service if we are extending DrupalUnitTestBase.
Comment #25
chx CreditAttribution: chx commentedThat's not the cause #1847828: [HEAD BROKEN] DrupalUnitTestBase and this container and transliteration and kernel and whatnot is.
Comment #26
amateescu CreditAttribution: amateescu commentedSure :)
Comment #27
klausiThis should be fixed here since this issue caused it.
Comment #28
chx CreditAttribution: chx commentedNo, it was not this issue that caused it, klausi read the other damned issue, please.
Comment #30
jhodgdonComponent update for posterity.
Comment #31
klonos...testing issue relationships ;)