Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

I agree with this idea (note that Pathauto predates that module by a few years...). Thanks for submitting it. I do not have time to create a patch for this now but if nobody gets around to it in the next few months then I may do so.

Freso’s picture

Version: 5.x-2.x-dev » 6.x-1.x-dev

Based on the Transliteration project's documentation, it shouldn't be that hard to implement. But. Should support for i18n-ascii.txt go out the plank, or should it be kept as a backup? (I'd definitely be in favour of making it go out the plank, but I'm not either module's maintainer. :))

I'm also thinking that it should probably go against the latest code, with the possibility of being backported.

greggles’s picture

@Freso - great questions/points. I think that if the Transliteration project is used for Pathauto then yes, the i18n-ascii.txt file will be removed and would no longer be supported. Further, if I implement this feature I don't plan on providing an "upgrade path" aside from users manually reconfiguring the Transliteration module. That is to say, if someone has custom rules based on the i18n-ascii.txt file then they will have to figure out how to implement them using the Transliteration project manually and the rules won't be automatically created by any transition code.

Also, definitely this should be done in the 6.x branch of code and, almost definitely in a 6.x-2.x.

Freso’s picture

Status: Active » Needs review
FileSize
10.38 KB

This is way too simple for me to believe one bit that it works as-is. But I don't have time to test this right now. Hm. And I should probably also provide a simple patch that doesn't include INSTALL.txt changes and i18n-ascii.example.txt removal... but not tonight.

Also, we probably want something other than "?" to replace unknown characters in the aliases... should I just make it use "_" hardcoded, or should something like variable_get('pathauto_separator', '-'); be used? (I prefer "_" to "-", but then, it's likely for another purpose as well... but maybe not?)

greggles’s picture

Status: Needs review » Needs work

Indeed the patch needs a little help to separate those extra little changes.

Also, if possible, the transliteration_get() should respect the language that a node is written in as mentioned in #111110: i18n-ascii.txt does not respect section i.e. if I write a node in German then the German transliteration rules should be used to transliterate.

So...who can test? ;)

Freso’s picture

For the language awareness, I've opened #255646: Add a language parameter to transliteration_get() over in the Transliteration queue.

I'll make a double patch later today then, for easier review. And I'll try to test it as well. :)

Freso’s picture

Status: Needs work » Needs review
FileSize
4.42 KB

Actually, I figured just not removing the i18n file in the patch would improve its readability greatly. So here we go.

smk-ka’s picture

Status: Needs review » Needs work

Two small issues:
1. pathauto_cleanstring() should additionally validate the presence of transliteration.module using module_exists(), since Transliteration could've been uninstalled in the meantime.
2. Be nice to translators: when transliteration.module isn't active, the disabled message should be appended to $translation_help instead of duplicating it.

Freso’s picture

  1. Agreed. Will incorporate in next patch.
  2. Two things, 1) in this case I think I agree with you, but there are many similar cases where some languages are forced to making bad translations due to strings being split up in the source code, thus removing power from the translator to actually do a proper translation. Oh, and 2) this won't make it to this patch, but I'll add it to the list in #256340: String clean up. :)
Freso’s picture

Status: Needs work » Needs review
FileSize
4.39 KB

Here's take two, which validates the presence of transliteration.module before calling transliteration_get().

Freso’s picture

Forgot to include a comment update.

Freso’s picture

And another re-roll after a wee batch of code cleaning (not much more left to clean!). :)

Freso’s picture

Making a node (article) with the title "Lill' Mari, / prøve vi / javnlet å tænk' o det høj'," produces "content/lill_mari_prove_vi_javnlet_a_taenk_o_det_hoj" as expected. (Unpatched Transliteration module, so no attempt at language awareness.)

Freso’s picture

Title: Use existing transliteration module » Use existing Transliteration module
Version: 6.x-1.x-dev » 7.x-1.x-dev
FileSize
4.49 KB

Code applied with some offset. Attached is an identical re-roll, just with fixed offset. :)

Also, should module_exists('transliteration') perhaps be replaced with function_exists('transliteration_get')? If either is true, the other should hopefully also be, but... *shrugs* Just thought I'd toss the thought out there.

This also doesn't implement language awareness which is in Transliteration's HEAD. However, this might also be unneeded, as Transliteration will use the current/content language if a language isn't specified, so... we'll have to test that.

Freso’s picture

Alright, it's definitely not language aware. However, this is not a regression, so... could this be checked in and we could then work on implementing the language aware transliteration later.

smk-ka’s picture

FileSize
6.62 KB

New patch adds language awareness. However, I'm not shure if this condition really is enough:

$language = isset($object->language) ? $object->language : NULL;

where $object is the node/user/category/foo object passed to token.

Things that are unclear:
1. Can this lead to unexpected results? I.e., can there be a language property in some object that doesn't denote the object's language (but something completely different)?
2. Will $node->language always be set to a valid, two letter code, or only if the translation module is enabled?

Since this patch needs testing feel free to commit #14 first, will happily re-roll my patch later.

Freso’s picture

Title: Use existing Transliteration module » Use Transliteration module for transliteration
Status: Needs review » Needs work

@smk-ka: Awesome! Thanks for the patch! Since you're on it, I decided to check in the patch in #14, as it will then be easier to review your language changes on their own. So, yeah, please re-roll. :)

smk-ka’s picture

Status: Needs work » Needs review
FileSize
2.97 KB

No problem, here you go.

Freso’s picture

Actually, after realising I was testing with an outdated version of Transliteration (ie., I was testing with Danish transliterations in a branch where the patch with the Danish stuff wasn't in...), I found out that the code I checked in earlier today, actually seems to be language aware. There are a few... "funny" cases, but they seem to be the same with and without your patch from #18.

If anyone could confirm this, I'd be thrilled.

greggles’s picture

I started documentation on this at http://groups.drupal.org/node/12685 - though obviously it could be expanded ;)

Freso’s picture

Status: Needs review » Fixed

Okay, I think this one can be closed now. Transliteration seems to both work and work per-locale. Thank you smk-ka, for your awesome responsiveness and work in fixing the small issues there was! =)

Anonymous’s picture

Status: Fixed » Closed (fixed)

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