Flag node for re-indexing if the 'exclude' setting is changed for one of its fields

For this patch to apply to 6.x-1.x, #551278: CCK mappings don't respect shared fields will have to be ported first.

Just want a sanity check before committing this as I'm less familiar with the CCK integration part of the module.

Enclosed is the patch from #3, ported to the current DRUPAL-6--1 tree as of this morning, and also including the patch from #551278: CCK mappings don't respect shared fields. Again this is for the 6.x-1.x-dev branch, not the 2.x branch.

I'll be testing later and will report back.

Let's go for a combined patch here for 6.x-1.x - so marking #551278: CCK mappings don't respect shared fields closed.

Are we sure that $row->display_settings[NODE_BUILD_SEARCH_INDEX]['exclude'] is always set?

Here's a combined 6.x-1.x patch. Quick test looks good.

Looks at the code - we are in a bit worse shape than I thought.

Working on a patch to use the CCK API rather than do direct table queries.

Also, we don't flag nodes for re-indexing if the 'exclude' setting is changed for one of its CCK fields - seems like that's something we should do?

Adds an implementations of hook_content_fieldapi()

Just a quick review...

+++ apachesolr_search.module
@@ -532,7 +532,7 @@ function apachesolr_search_apachesolr_facets() {
-        'info' => t('Apache Solr Search: Filter by @field', array('@field' => $field['label'])),
+        'info' => t('Apache Solr Search: Filter by @field', array('@field' => $field['widget']['label'])),

This seems like an extra fix, but doesn't seem to hurt =)

Otherwise, code looks fine.

Looks sane (though, I'm not super familiar with the CCK API). Do we not need to load module_load_include('inc', 'content', 'includes/content.crud'); in the hook_content_fieldapi?

re: #11 - since we are taking the data from cck instead of buiding it ourselves, the array keys change in a few palces such as that.

re: #12, that hook is invoked from content.crud.inc, so it will always be loaded afaik.

putting back in James' change to !isset() from is_null()

committed to 6.x-1.x, patch needs to be ported since there are conflicts in 6.x-2.x

"#751004 by jpmckinney, camidoo, rjbrown99, pwolanin: fix use of CCK API, handle NODE_BUILD_SEARCH_INDEX exclusion setting in the UI."

Hmm, interesting possible side-effect - if you are using taxonomy based node access, the terms not accessible to anonymous uers will not get indexed http://api.drupal.org/api/function/taxonomy_node_get_terms/6

This is basically the desired behavior (it leads to consistency) - but leads more urgency, perhaps, to the option of being able to index content as user X.

Well actually, why do we index as anon? This is a administration task, or do i miss something?

So we want all fields not excluded somehow ( not by permissions but the hooks ) going into the index. When users search, we respect permissions. Thats exactly the way to go. Using anons you probably fetch half of the data, because anon cant access some fields / taxonomies and more. The searching user then could access those fields but cant search for them.

Indexing must always be done with uid=1 or "administer content" permissions and that should eb checked beforehand and told to the user, that he must switch permissions.

Anon indexing might work for "public only" sites. But it wont work for private sites or mixed. And as apachesolr already supports nodeaccess it does not make any sense removing that support here.

After removing anon indexing my taxomomy indexing worked fine again

@EugenMayer - most sites run cron as the anonymous user and hence index content that way in Drupal core search.

This change brings consistency - we always know the content is indexed as user 0. This is also the only way we can avoid information disclosure without a more complex scheme as described above. A secheme which will have to be implemented by another module

You mean most sites which dont have any permissions? Is that an argument? And no, taxonomy indexing with an anon user makes absolutely no sence. Did you ever thought about taxonomy_access?

@EugeMayer - yes, I did think about it. The discussion above about CCK fields applies equally to taxonomy.

I also discussed the issue quote a bit with jhodgdon in IRC. Basically - there is no easy of totally satisfying solution. The approach of only indexing (by default) content available to anonymous users is the safest in terms of minimizing the likelihood of information disclosure.

Look at the README here: http://drupalcode.org/viewvc/drupal/contributions/modules/search_by_page...

That module offers a more graduated control by indexing search content as a user with a specific role, but has to jump through hoops like creating fake user accounts. This is not something I'm especially interested in maintaining, but I be happy to consider your patch to add a simplified version of the functionality (e.g. being able to specify a user that is user when indexing content).

Well i cant even thin about why creating fake users.

uring index-time _all_ fields which are _not_ excluded _site wide_ (or not added to the index in the solr config) should be indexed.
Why?: Because if it is hidden by a permission there are users on the page being able to access those fields and they _cannot_ searc for it anymore neither use it as facet. This just makes the whole solr aproach useless.

If you dont like a field to be index:
- Exclude it using the api
- Dont add it in the CCK options in solr
- dont add the taxonomy facet in solr ( for that taxonomy)

This issue def. deals with the wrong problem and thats misconfiguration OR bad access implementations. e.g. i implemented by own version of "apachesolr_access" to respect the rights i have in my Drupal installation. Because everthing should be indexed, but not everthing should be included in the search result. Thats very much possible for cck fields or other things also - but you _have to index them_.

If this patch goes thtough and you talk about "consitency" i ask myself, why apachesolr_nodeaccess ever got implemented. Its against this approach in any way it has been done.

"filter on query" rather then "never index".

To be more clear, iam not for "use the current user for indexing". Iam for use the full-priv user with all rights ( administer content ), so rather uid=1.

Anyway i would be glad if you could add an option to disable anon indexing.

@EugenMayer - patch?

I open a new issue for that when i find time for the implementation and provide the patch.

Here's a 6.2 version.

Note that Drupal 7 core now always runs cron as user 0 with very similar code, so I think it's far to say we are making a reasonable choice, and also we shoulf not need to port this change to Drupal 7.

http://api.drupal.org/api/function/drupal_cron_run/7

#751004 by pwolanin, jpmckinney, rjbrown99, robertDouglass | EugenMayer, camidoo: Fixed Respect NODE_BUILD_SEARCH_INDEX exclusion setting.

Sorry for reopening this one.

Looking at the patch ( pretty huge ) iam not sure what is implemented now. The patch seems to include some quiet other changes also.

What user is used for indexing now? Can i configure this?

@EugenMayer As mentioned earlier, please open a new issue if you want to be able to index as a different user.

Re: #8 "Also, we don't flag nodes for re-indexing if the 'exclude' setting is changed for one of its CCK fields - seems like that's something we should do?"

For 7.x, do we need to implement hook_content_fieldapi as hook_field_update_instance to accomplish the same thing?

From above :

and also we should not need to port this change to Drupal 7.

Marking this as closed, won't fix. I assume this kind of behavior is already available in Drupal 7 branch. If not, please reopen or state clearly what should happen?