See topic title, urgently needed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Nick_vh’s picture

FileSize
36.03 KB

First take, will work more on this when I have time.

Nick_vh’s picture

Status: Active » Needs review
Nick_vh’s picture

FileSize
51.67 KB

Checking if this breaks anything, also adding some more typing in the function definitions.

Status: Needs review » Needs work

The last submitted patch, 1897556-3.patch, failed testing.

Nick_vh’s picture

Status: Needs work » Needs review
FileSize
65.64 KB

Status: Needs review » Needs work

The last submitted patch, 1897556-5.patch, failed testing.

Nick_vh’s picture

Status: Needs work » Needs review
FileSize
77.04 KB

Status: Needs review » Needs work

The last submitted patch, 1897556-7.patch, failed testing.

Nick_vh’s picture

FileSize
83.66 KB

Meh, stupid menu system

Nick_vh’s picture

Status: Needs work » Needs review
Nick_vh’s picture

Status: Needs review » Needs work

Committed this, will do a follow up later on.

Updated :
* apachesolr.drush.inc
* all the facetapi plugins
* Apache_Solr_Document.php
* Drupal_Apache_Solr_Service.php
* Solr_Base_Query.php
* apachesolr.admin.inc
* apachesolr.api.php
* apachesolr.index.inc

We should do all the other ones still.

ianthomas_uk’s picture

Is this change intentional? It's not a documentation change, and has broken a couple of my facets (single value taxonomy fields).

@@ -1973,7 +1982,7 @@ function apachesolr_entity_fields($entity_type = 'node') {
           'facet missing allowed' => FALSE,
           'facet mincount allowed' => FALSE,
           // Field API allows any field to be multi-valued.
-          'multiple' => TRUE,
+          'multiple' => FALSE,
         );
       if ($key !== 'per-field') {
         $mappings[$key] += $defaults;
ianthomas_uk’s picture

I've proposed a fix for the facets this broke at http://drupal.org/node/1979506

Nick_vh’s picture

Status: Needs work » Fixed

Closing this issue as it does not really has a good defined action item. Thanks for finding a little issue here.

pwolanin’s picture

Version: 7.x-1.x-dev » 6.x-3.x-dev
Status: Fixed » Patch (to be ported)

needs to eb backported?

pwolanin’s picture

Version: 6.x-3.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Needs work

setting back to needs work. I think @ianmthomasuk is correct in #12 that the change to the field mappings was a mistake.

ianthomas_uk’s picture

Even if it was committed accidentally, I think the behaviour in 1.2 makes sense and has the benefit that you can sort by the single-value field. The only exception I know about is taxonomy fields, which is addressed in https://drupal.org/node/1979506

pwolanin’s picture

@ianmthomasuk - I thought we were already adding a single value version (the 1st value) for every field when indexing?

ianthomas_uk’s picture

@pwolanin That's not what happens on my site, using 1.1 plus a few patches that shouldn't change that behaviour.

pwolanin’s picture

@ianmthomasuk - check your index. It's possible we are not exposing that as a sort in the UI, but it should be there.

pwolanin’s picture

See apachesolr.index.inc:


    // Also store the first value of the field in a singular index for multi value fields
    if ($field_info['multiple'] && $numeric && !empty($values[0])) {
      $singular_field_info = $field_info;
      $singular_field_info['multiple'] = FALSE;
      $single_key = apachesolr_index_key($singular_field_info);
      $fields[] = array(
        'key' => $single_key,
        'value' => $function($values[0]['value']),
      );
    }
pwolanin’s picture

FileSize
1.07 KB

Here a patch reverting that change, plus a minor cleanup.

pwolanin’s picture

Status: Needs work » Needs review
ianthomas_uk’s picture

Status: Needs review » Reviewed & tested by the community

I've been having a discussion with @pwolanin on IRC about the merits of this patch and whether it's desirable to attempt to index a multi-value Drupal field as a single value Solr field.

Benefits:
* You can change the cardinality of the field without re-indexing
* You can sort on the field

Drawbacks:
* The sort order can be inconsistent, as it depends on the order of the values in the multi-value field (e.g. if you have two nodes with multi-value price fields of (29.99, 19.99) and (24.99, 34.99) and you sorted by price asc then the second node would be listed first, when you probably wanted to sort by minimum price or maximum price).
* Slightly increases index size / indexing time

I'm marking this RTBC as an all-singing all-dancing solution would take quite a lot of work, and this at least resolves the regression for people upgrading from <1.1. It does need a release note to warn users of 1.2 that some of their fields will change back to their old names.

pwolanin’s picture

Status: Reviewed & tested by the community » Fixed

committed

Status: Fixed » Closed (fixed)

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