I've seen the variable defaults to a blank string. This seems not to follow the default core breadcrumbs where the first is always "Home". I think the default value for this variable should be an untranslated value of "Home".

What do you think?

CommentFileSizeAuthor
#2 taxonomy_breadcrumb_home_default2.patch2.92 KBhass
#1 taxonomy_breadcrumb_home_default.patch2.97 KBhass
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hass’s picture

hass CreditAttribution: hass commented
Status: Active » Needs review
FileSize
taxonomy_breadcrumb_home_default.patch2.97 KB

I cannot say why, but the value wasn't set to "Home" via hook_install on my box... strange. Why is the string saved translated on install time?

I've seen if it's "Home" it's automatically translated in _taxonomy_breadcrumb_generate_breadcrumb(). This may be the wrong way if it comes to i18n, but the way how it works today. Therefore I've removed the t() in .install.

hass’s picture

hass CreditAttribution: hass commented
FileSize
taxonomy_breadcrumb_home_default2.patch2.92 KB

After more thinking about this - I've removed the variable_set() in hook_install(). We have the fallbacks in variable_get() for this functionality and this makes this line obsolete.

Proper patch attached.

MGN’s picture

MGN CreditAttribution: MGN commented
Status: Needs review » Needs work

Thanks. While looking it over, it seems that the following code is an inappropriate use of t(), since t() is not meant to be used on dynamic strings, and there is no context to aid in the translation. 

  $home_text = variable_get('taxonomy_breadcrumb_home', t('Home')); // Here, t() should be used because 'Home' is static.
  if ($home_text != '') {
     $breadcrumb[] = l(t($home_text), NULL);  // I think t() should just be omitted here. 
 }

I think a similar change is needed in taxonomy_breadcrumb.admin.inc, with the default value being t('Home') instead of 'Home'.

What do you think?

MGN’s picture

MGN CreditAttribution: MGN commented
Status: Needs work » Fixed

I used most of the patch and fixed the inappropriate use of t() issue described in #3. Committed to 6.x-1.x-dev.

hass’s picture

hass CreditAttribution: hass commented

THX

Only as a note - it may be easier, faster and more reliable to use empty() in if ($home_text != '') {

MGN’s picture

MGN CreditAttribution: MGN commented

Thanks for the tip. I agree.

Oops. I forgot that "0" (0 as a string) is also empty. As unlikely as that would be for a home crumb, I expect its better to leave the code as is.

hass’s picture

hass CreditAttribution: hass commented

If you expect someone may enter "0" as home breadcrumb - ok... nothing it's impossible :-)

Status: Fixed » Closed (fixed)

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