CCK facet definitions needs to expand its role and have a 'facets' param.

I don't see that this made it into 7.x either. This does seem pretty useful to have to avoid a confusing admin UI.

Attaching a patch that ignores whitespace to make it easier to review.

We may have missed one in 6.x-2.x

Looking just at 6.x-1.x for the moment. Basically looks good, but possibly we want to make TRUE the default to make things nicer in terms of BC? At the least, we should either set a default or change this:

if ($field['facets']) {

to something like:

if (!empty($field['facets'])) {

to avoid notices about variables that are not set.

+1 to !empty. To make "facet" => "TRUE" the default, wouldn't we have to instead make the key "nofacets" => FALSE? That way empty($field['nofacets']) will succeed on all existing fields.

I meant we could set a default value. See attached.

Yes, and I don't think that works. In 7.x, you simple don't set 'facets' if you don't want it. No 'facets' key is the same as 'facets' => FALSE.

With that patch, you must set 'facets' => FALSE, otherwise it defaults to TRUE, which is not what you expect. No 'facets' key becomes the same as 'facets' => TRUE.

I'd rather have the same behavior in both.

I think it's worth having different behavior in 6.x-1.x otherwise the upgrade will break people's custom cck mappings, so indeed that will have to specifically set 'facets' to FALSE.

We can make it default to TRUE in all branches (or flip the logic to 'nofacets') if you prefer consistency. I don't have a strong feeling as long as for 6.x-1.x an existing implementation would continue to work the same.

Okay, let's go with setting a default in 6.x-1.x. We aren't trying to keep 6.x-1.x and 7.x in sync anyhow.

For 6.1, do we need these lines from the 6.2 patch in #1:

+        if (!isset($mappings[$field_type][$widget_type]) && !isset($mappings['per_field'][$field_name]) && isset($mappings[$field_type]['default'])) {
+          $widget_type = 'default';
+        }

Brought 6.1 patch more in line with patch in #1 (without adding the above, though). Fixed 6.2 and 7.1 patches to use !empty.

Should the comments and readme file about cck field definitions be updated to include this field?

i.e.:

<?php
// A single default mapping for all text fields using optionwidgets.
$mappings['text'] = array(
  'optionwidgets_select' => array('callback' => '', 'index_type' => 'string', 'facets' => TRUE),
  'optionwidgets_buttons' => array('callback' => '', 'index_type' => 'string', 'facets' => TRUE),
  'optionwidgets_onoff' => array('callback' => '', 'index_type' => 'string', 'facets' => TRUE),
);
// A single default mapping for all integer fields using optionwidgets.
$mappings['number_integer'] = array(
  'optionwidgets_select' => array('callback' => '', 'index_type' => 'sint', 'facets' => TRUE),
  'optionwidgets_buttons' => array('callback' => '', 'index_type' => 'sint', 'facets' => TRUE),
  'optionwidgets_onoff' => array('callback' => '', 'index_type' => 'sint', 'facets' => TRUE),
);
// A field-specific mapping would look like:
// $mappings['per-field']['field_model_name'] = array('callback' => '', 'index_type' => 'string', 'facets' => TRUE);
// or
// $mappings['per-field']['field_model_price'] = array('callback' => '', 'index_type' => 'float', 'facets' => TRUE);
// Allow other modules to add or alter mappings.
drupal_alter('apachesolr_cck_fields', $mappings);
?>

and

hook_apachesolr_cck_fields_alter(&$mappings)

  Add or alter index mappings for CCK types.  The default mappings array handles just
  text fields with option widgets:

    $mappings['text'] = array(
      'optionwidgets_select' => array('callback' => '', 'index_type' => 'string', 'facets' => TRUE),
      'optionwidgets_buttons' => array('callback' => '', 'index_type' => 'string', 'facets' => TRUE)
    );

  In your _alter hook implementation you can add additional field types such as:

    $mappings['number_integer']['number'] = array('callback' => '', 'index_type' => 'integer');

  You can allso add a mapping for a specific field.  This will take precedence over any
  mapping for a general field type. A field-specific mapping would look like:

    $mappings['per-field']['field_model_name'] = array('callback' => '', 'index_type' => 'string', 'facets' => TRUE);

  or

    $mappings['per-field']['field_model_price'] = array('callback' => '', 'index_type' => 'float', 'facets' => TRUE);

  If a custom field needs to be searchable but does not need to be faceted you can change the 'facets'
  parameter to FALSE, like:

    $mappings['number_integer']['number'] = array('callback' => '', 'index_type' => 'integer', 'facets' => FALSE);

Other than that the patch looks good to me.

Mind rolling a patch with those changes?

Done.

I haven't tested per-se but visual review looks like these are ready to commit.

committed all - though included another patch together with the 7.x one by accident.

Just to note: #1107502: Merge relevant parts of php client into DrupalApacheSolrService is the referenced patch.

Why was the drush command solr-phpclient removed with this commit?

We no longer require you to download the phpclient.

@scor - see #16 - I committed 2 patches together by mistake.