Show value instead of key in CCK facets

This is my way of subscribing :-D. I did a bunch of stuff to get around this exact problem with this handler. This could me the death of this handler which feels hackish.

http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/apachesolr_...

Related? #551292: Turn facet block functions into an interface with different class implementations

As I can see...

• there are 3 consecutive patches...
• the $mappings array structure is changing: former "callback" became "indexing callback", "display callback" was added
• CCK fields facets are now displayed using content_format() function (from CCK)
• #551278: CCK mappings don't respect shared fields is solving the CCK shared fields conflict

Are there any other things that I should take care?

Anyway... backporting this will not add the capability to alter the facets text from custom modules. This was accomplished by #584378: Add facet text callback & altering hook. Maybe we want this too...

Proposed solution for apachesolr_cck_text_field_callback():

function apachesolr_cck_text_field_callback($facet, $options) {
  if (function_exists('content_format')) {
    $facet_text = content_format($options['delta'], $facet);
  }
  else {
    $facet_text = $facet;
  }
  // Let modules alter $facet_text (passed by reference)
  drupal_alter('apachesolr_cck_text_field', $facet_text, $facet, $options);
  return $facet_text;
}

Backporting this issue to 6.x-1.x & 5.x-2.x I found that the apachesolr_cck_text_field_callback() is not working correctly for CCK fields other than: number, text or any other CCK field that store its value in the value key of the item array.

function apachesolr_cck_text_field_callback($facet, $options) {
  if (function_exists('content_format')) {
    return content_format($options['delta'], array('value' => $facet));
  }
  else {
    return $facet;
  }
}

For example if a nodereference CCK field is passed to this function it will return an empty value. This is because nodereference stores the main value in the nid key rather than "value" key. For the node reference CCK field the content_format() function should be called:

    return content_format($options['delta'], array('nid' => $facet));

And so on... We may found other storage models in the "CCK World"... and we do.

That's why I think that this patch must be improved in this way for 5.x-2.x, 6.x-1.x and 6.x-2.x as well. I'm changing the state of the ticket to reflect this and shortly I will provide a patch for 5.x-2.x.

Before creating a patch I'd like to know what is your opinion about my proposal for apachesolr_cck_text_field_callback() function. Note that it's for 5.x-2.x branch but it shouldn't be different for 6.x.

function apachesolr_cck_text_field_callback($facet, $options) {
  // If CCK module is not present return the plain facet.
  if (!module_exists('content')) {
    return $facet;
  }
  
  // Cached $field_info holds the CCK infos for needed fields
  static $field_info;
  if (!isset($field_info[$options['delta']])) {
    $field_info[$options['delta']] = content_fields($options['delta']);
  }
  
  // Clone the $facet. Do not allow altering.
  $facet_text = $facet;
  
  // Format the native CCK types and open
  // the unknown content-types to custom modules
  
  // CCK Number or Text
  if (($field_info[$options['delta']]['type'] == 'number' && module_exists('number')) || ($field_info[$options['delta']]['type'] == 'text' && module_exists('text'))) {
    return content_format($options['delta'], array('value' => $facet));
  }
  // Nodereference content type
  elseif ($field_info[$options['delta']]['type'] == 'nodereference' && module_exists('nodereference')) {
    // Add also the 'plain' formatter. Otherwise the entire hyperlink will be returned.
    return content_format($options['delta'], array('nid' => $facet), 'plain');
  }
  // Userreference content type
  elseif ($field_info[$options['delta']]['type'] == 'userreference' && module_exists('userreference')) {
    // Add also the 'plain' formatter. Otherwise the entire hyperlink will be returned.
    return content_format($options['delta'], array('uid' => $facet), 'plain');
  }

  // Not a native CCK type... Modules can alter the $facet_text
  $options['field_info'] = $field_info[$options['delta']];
  drupal_alter('apachesolr_cck_text_field', $facet_text, $facet, $options);
   
  return $facet_text;
}

Here's the patch for 5.x-2.x (DRUPAL-5--2). In the patch I used the callback from #10.

Here are both patches, for:

• DRUPAL-5--2 (5.x-2.x)
• DRUPAL-6--1 (6.x-1.x)

I must make an IMPORTANT NOTE: In DUPAL-6--2 patch there is a mistake. In apachesolr.index.inc we have:

        $function = $cck_info['indexing callback'];
        if ($cck_info['indexing callback'] && function_exists($function)) {
           $field = $function($node, $key);
         }

but the key is wrong, it should be indexing_callback and not "indexing callback" (note the underscore instead of " " - space). This is because $cck_info is obtained from apachesolr_cck_fields() and there this field is build first as:

$row->indexing_callback = ...

and then transform into array here...

$fields[$row->field_name] = (array) $row;

In my opinion we must fix this confusion in the future by replacing everywhere "indexing callback" with "indexing_callback". Generaly I'm not a fan of key that contains spaces (such as $op == "update index").

@robertDouglass,

An input on #10 (#9) would be very helpful before committing to 5.x-2.x & 6.x-1.x. And if it's OK then you'll have to port it also in 6.x-2.x.

Thanks!

What's committed right now in 6.x-2.x is this:

function apachesolr_cck_text_field_callback($facet, $options) {
  if (function_exists('content_format')) {
    return content_format($options['delta'], array('value' => $facet));
  }
  else {
    return $facet;
  }
}

This function doesn't work for nodereference and doesn't provide any "open door" to modules. It only let CCK fields that are stores values in the "value" key. No chance for the other ones.

My function is a hybrid

• It centralize, by solving all CCK native field types (text, number, nodereference, userreference).
• It open the door, by allowing modules do their policy through a new hook.

A more "liberal" choice it can be: letting only the hook invoker in this function and write hook implementations only for native CCK field types...

#592594: CCK arguments not integrating properly with facet blocks is blocked until resolution on this issue

Any review on this?

Thanks.
Claudiu

Hey guys,

I have to move forward with this... Everything is so slow here... It's simple: apachesolr_cck_text_field_callback() is NOT working if you have CCK fields like nodereference.

I don't know what to do... I received no reviews from community for the proposed patch... This is blocking everything on this backport.

I will mark this as RTBC and wait another 24h... After that I will:

• Commit the patch to DRUPAL-5--2, DRUPAL-6--1
• Port as patch against DRUPAL-6--2

Thanks.

The 2 patches were committed to CVS (DRUPAL-5--2 & DRUPAL-6--1).

Porting now to 6.x-2.x-dev.

This is fixed now in all branches.

These commits to 6.x will be rolled back for further discussion.

Also, in general leave any fixed issue open for the bot to close after 2 weeks.

This patch to 6.x changed solrconfig.xml, which looks like it was just sloppy pick-up of changes to your working copy. If you need review on a critical issue, please reach out - obviously we've all been very busy recently and not able to follow the issue queue constantly.

here is my 6.x-1.x roll-back patch

committed roll-back for 6.x-1.x.

This is still buggy in 6.x-2.x. I believe the changes are the cause of #550534: Search 404 incompatibility which I also encountered when trying to figure out why the user and taxonomy facet blocks were displaying incorrect labels.

It boils down to the fact that this patch only halfway converted the breadcrumb theme functions to accept a $field array with the '#value' key containing the actual value (notably the hook_theme implementation wasn't updated to use the new argument names). apachesolr_search_nested_facet_items() is still calling apachesolr_breadcrumb_* with $field['#value'] rather than $field. We either need to pass $fields everywhere or the appropriate value everywhere. Or Solr_Base_Query::get_breadcrumb() needs to be smarter about checking if the facet is a CCK field by checking if $breadcrumb_name after drupal_alter('apachesolr_theme_breadcrumb', $breadcrumb_name); gets called but personally I think that's a bad idea.

Okay here's a patch that tries to move all the breadcrumb code to pass in arrays with the value stored in the #value key. It fixes the facet blocks for users and taxonomy terms, and I believe it will address #550534: Search 404 incompatibility.

Just want to be sure -- it looks like this didn't make it into 6.x-1.0-rc5, is that right?

Yeah, this didn't make it in to 6.x-1.0-rc5 and is causing issues with search404 integration.

See #550534: Search 404 incompatibility

case of value (label) =html (particulary i tried to represent image as a facet choice) doesn't work
please advice any another possibilities to realize idea for facets choices to be another than just text (especially images are the most wanted)

so apparently the 5.x code is broken, and we need to discuss the 6.x-1.x code?

We'd prefer not to make significant API changes to 6.x-1.x

This issue seems to have caused an unholy mess. I can't tell what state each branch is in by reading the comments. It seems like only 6.x-1.x was rolled back.

Follow-up patches that would need to be applied to 6.x-1.x if we re-apply this patch there:

#724440: Current search does not display properly the CCK filters
parts of #592722-3: PHP notices

I'm with jpmckinney as to being a bit mixed up as to the current state of this issue. I patched 6.x-1.x-dev with the patch in #30 but with unsuccessful results.

If anyone could post an update as the status of this issue it would be greatly appreciated.

Thanks so much.

Seems like this is fixed for 6.x-2 and HEAD. We are not going to make these changes in 6.x-1.x

Thank you!

I can confirm that this is working with 6.x-2.x-dev.

Had to have this working in 6.x-1.6, did some quick fixes to get the patch #30 working for that version.

Do we have to fix this in 1.x now?

pwolanin said :

We are not going to make these changes in 6.x-1.x

The most important thing is to finish 7.x-1.x and a backport has already started to get this version working with D6. This issue stays closed.

Agree, just needed to have the patch online.