Translated (localized) taxonomy facet blocks

I don't think I made any significant changes to anything related to the taxonomy facets between these versions, so I'm surprised by this report.
Attached is a diff of the changes from beta7 to beta8. A bunch of code was moved to a .inc file, but essentially no meaningful changes occurred for taxonomy-related code.

I have not used any of the translation modules you cite - perhaps you updated one of those at the same time?

We also need localized taxonomy facet blocks. To translate taxonomies we use i18n.

From my point of view the apachesolr module never supported this in the past. So I created a patch that adds i18n support for taxonomy facets.

I made a little mistake in my patch. Here's a fixed version.

Next step:
Use i18n to translate node type facet, too.

Time to leave off work ...

Check for module i18ncontent and not for i18ntaxonomy when translating content types.

We should have a separate issue for at least using the correct timezone for dates.

see #463886: localize apachesolr date formats for apachesolr localization in general and http://drupal.org/node/463886#comment-1664164 for localization of date facet.

Tested in fresh checkout of DRUPAL-6--1 branch. Works!

Need this, sorely =)

I'm not familiar with the i18ntaxonomy module - that's pat of the overall i18n module or stand-alone? I guess this functionality didn't make it into D6 core in any form?

In any case, I'm a little puzzled about how this should work - this patch seems to show the localized taxonomy terms in the search block, but those localized term names would not be available in the search index?

i18ntaxonomy is part of http://drupal.org/project/i18n
I designed the patch to use i18n only if it's available and not to depend on it.
You're right that the patch only translates the terms and vocabulary names when showwing them in facet blocks. For our needs this was enough because we search for terms using term ids and not via full text search.
If terms are also indexed as text than this patch might need some work. But maybe this part is more related to #463886: localize apachesolr date formats.

Terms *are* indexed as text also, so at best this helps in terms of pure faceting for a single-site implementation.

#14 pips1:
I think you described it correctly. My patch here only fixes one use case: The one you originally posted. (We also have two sites live now using this patch to solve the same issue).

I also agree to pwolanin that a perfect solution needs additional work. But from my point of view the context for multi language or multi site searches is much bigger and the current architecture of apachesolr module doesn't fit.

If you have a look at #463886: localize apachesolr date formats you'll see that different languages require different stemming, stopwords, synonyms, acronym handling, concatenating rules, date and number formats, ...

So a multi site / language search might require multiple indexes (one per language) or language specific fields.
(And if you want to take it to extremes it's not enough to distinguish between languages but locales)

I think we should have a BoF at DrupalCon about the larger context of multi language search. Some good brainstorming is in order.

The Doxygen comments don't say what the function parameters are. Typically there would be a one line description following each @param (on the next line).

+ * @param $term
+ * @return object

I despise function names that start with an underscore. _apachesolr_search_localize_taxonomy Can we change them?

+function _apachesolr_search_localize_taxonomy() doesn't sound like a function that returns TRUE or FALSE. Can we find a name more like apache_solr_search_needs_localization() ? (Open to other suggestions).

I don't like packing the module full of if(module_exists('foo')) ... wish there were a better way :[

I'm also interested in such a meeting at DrupalCon. Seems I have to order a ticket ...

@pips - for DC, BoFs could not even be scheduled in advance, not sure what they are doing for PAris, but certainly the BoF schedule is not closed.

@pips1 - great work!

now... back to this patch...

@pwolanin: re #12: Yes, this won't change the index, it just helps navigation. It goes without saying that search.module doesn't index different localized versions of strings; just the default language, so I don't think we should attempt any of that in this patch. Like @mkalkbrenner mentioned, #463886: localize apachesolr date formats is the place for that.

I looked at @robertDouglass's comments in #18 and [hopefully] addressed them in this new patch... except the two module_exists() calls...

In @mkalkbrenner's favor, there *is* this "offending" piece of commenting above theme_apachesolr_search_snippets() :

/**
 * Theme the highlighted snippet text for a search entry.
 *
 * @param object $doc
 * @param array $snippets
 *

And there is also a module_exists() to build the book facet...

=) And no, I didn't kill any kittens by addressing this last bit in the patch =)

Moving to 2.x; new patch attached.

Patch did not apply anymore. New patch.

We'll be discussing this patch Thursday at drupalcon.

Was this discussed? =) If not maybe we could schedule a meeting at DCSF about this.

janusman, mkalkbrenner - can I get an update on this? It'd be great to make sure it's resolved for a 6.2 release. Thanks for your efforts.

My patches for this issue translate the facets when the search result will be shown based on i18n module. I still think this feature should be added to apachesolr because it's a kind of regular user interface translation in drupal.

Regarding indexing the translations as pwolanin pointed out at #12 my plan is to add this feature to Apache Solr Multilingual.

What do you think about this approach?

@Robert: I recognized that you signed up for Drupal Dev Days in Munich in a few weeks. Maybe we could discuss some issues there.

Well, I stand by something like my patch in #25... do you think that could make it in if I re-roll it?

Just looked at #26. I'd give a re-roll a serious look and think that it is most likely the right direction.

I had a look at janusman's patches from #25 and #26, too. They mostly look like updated versions of my initial patches which is fine. So they are still only targeting the UI when the search result will be displayed.
As I pointed out in #30 I agree with robertDouglas #32 to get this into apachesolr.

I opened a separate issue: #769752: Index translated taxonomy terms.
Please give me some feedback about leaving indexing of translated taxonomy terms to Apache Solr Multilingual. Because it would not make sense to implement it there when you plan to target this issue here as well.

mkalkbrenner: you and I are going to sit down in Munich and hash this out. If I don't leave Munich with a i18n friendly ApacheSolr - shame on me!

New patch, my first from Drupalcon =)

subscribing

Are you sure this patch is against the 6.x-2.x branch?

Anyway, I don't like adding three functions that are only called from one other function. If the plan is to re-use these functions elsewhere in the code, I would like that to be part of the patch as well.

Re: #37. The code is in functions because of the way the original code uses variable assignments inside one-liner if statements; this way the logic is spread out. So, I'm guessing you'd like me to rewrite those ifs as well?

E.g.: this one-liner if statment calls my new function

+++ apachesolr_search.module Locally Modified (Based On 1.1.2.6.2.111.2.49)
@@ -841,7 +841,7 @@
-  if (is_object($response->facet_counts->facet_fields->$delta) && ($vocab = taxonomy_vocabulary_load($vid))) {
+  if (is_object($response->facet_counts->facet_fields->$delta) && ($vocab = apachesolr_search_localize_taxonomy_vocabulary(taxonomy_vocabulary_load($vid)))) {

In the meantime, here's a clean version of that patch as it applied but with offset. (Yes, this is for the 2.x branch)

The three functions felt quite logical to me, however the static caching in apachesolr_search_taxonomy_needs_localization() looked a bit redundant, as it only copied data obtained from variable_get(), which is already cached. Removing the caching allowed me to eliminate function apachesolr_search_taxonomy_needs_localization() altogether.

I have also slightly rearranged the conditions in apachesolr_search_get_type() to avoid testing the value of $type twice.

EDIT: Corrected function name.

Good catch @ David Lesieur!

But:
In the new i18n module, the tt() function is obsolete. It was deleted and then reverted.

Readme.txt from i18n:

// $Id: CHANGELOG.txt,v 1.15.4.83 2010/04/30 10:35:55 markuspetrux Exp $

6.x-1.4 to ......
------------------
- Fixed: The "All languages" option can't save back on blocks, #766678
- Deleted obsolete functions tt() and to(). Replaced by i18nstrings() and i18nstrings_update()
- Restore back function tt() to prevent "Fatal error: Call to undefined function tt()" in other modules, by markus_petrux, #358839

Maybe we should use the new functions...

Yes, we should use i18nstrings() instead of tt(). See http://drupal.org/node/783516#comment-2900612

Now using i18nstrings() instead of tt().

Looks fine. Not setting as RTBC since I haven't tested it tho.

The type returned by theme_apachesolr_breadcrumb_type() was not localized. Fixed in this new patch.

For those of us stuck on 6.x-1.0. Here is a re-roll of #26 that applies cleanly to 6.x-1.0.

Why is there a hook_ctools_block_info() in this patch? That wasn't in #26.

Oops, good catch, I'm not sure how that got in there. Use this patch instead.

I think the process is usually to fix the issue in the latest branch, and then backport to older branches. So the patch to review would still be #46.

We are tring to get 6x-1.1 finalized, so I'd actually Stratton with the 6x-1x branch (assuming this won't cause any regressions)

Re-rolled #46 to apply cleanly against latest DRUPAL-6--2.

Removed code related to content type translation, which has been fixed in another issue.

I think the last patch missed a few spots where taxonomy_get_term() was being called directly without i18n support. New patch for review.

Good catch. Looks good to me.

I think I caught another one; CCK field labels. This patch also tries to address that (although it feels a bit roundabout). Please review.

Hmm, I might have overreached, the issue was about taxonomy, not CCK =) It might be best to move #57 onto another issue.

I'm not fond of the way the patch wraps everything in an extra function call - makes it hard to read the code

Ok, how about this?

Actually, nevermind. We *already* have an apachesolr_search_taxonomy_get_term(), and can effectively simplify a lot of code by moving all of those if(function_exists('taxonomy_get_term')) ... inside the wrapper function.

Will work on it.

Spoke too soon. Patch seems fine to me. EDIT: Please review!

Here is a re-roll for the 1.x branch again (patches cleanly against 1.2).

This is not the offical patch for review in this thread. Is there a better place for me to upload these guys? I don't want to distract from your work on 6.x-2.x.

Focusing on 1.x because 2.x is somewhat unsupported. I think I prefer the approach in #60 to that in #57 and earlier patches, i.e. I agree with pwolanin in #59. Not sure about the CCK label stuff that snuck in in #57.

IIUC, we just need a reroll for 6.x-1.x of the patch in #60? If not, please clarify what else "needs work".

Yes, please re-roll. I've only done a superficial review, so I can't promise I won't find something else :)

Ok, new patch for 6.x-1.x. This is just the patch from #62 but with updated offsets.

Needs a re-roll for the Drupal 7 version (or at least check if this is still an issue. )

I just updated from beta8 to beta13, and to rc1 of facet api, and have resolved my taxonomy translation problems in facet blocks for 7.x...

I think with the current D7 version of Apache Solr Search Integration, the localized display of taxonomy terms is Facet API's responsibility. Judging by the post in #68 and the patch to i18n at http://drupal.org/node/1340858#comment-5259368, taxonomy terms should be localized on display without any additional work. The deprecated project by thegreat at http://drupal.org/sandbox/thegreat/1276796 confirms this, so I am marking this issue as fixed.

My D7 still has this issue: a localized taxonomy facet block which doesn't display its translated terms.

That's a facet api problem and was already solved with facet api bonus I think. Closing again

Marking as duplicate. The fix has ben committed to Facet API at #1276812: Multilingual facet items. It is in the dev snapshot, but not yet in the supported release.