Currently, apachesolr_entity_get_callback accepts an optional $bundle argument, but it is never actually used. Consequently it is not possible to override eg 'result callback' just for a specific bundle. There is a workaround in that you can override 'result callback' for the entity type, but then you get conflicts if multiple modules want to do the same thing.

This patch passes the $bundle argument for:
'document callback'
'status callback'
'result callback'

The following callbacks are still not bundle specific: (it doesn't look like it makes sense for these ones)
'reindex callback'
'bundles changed callback'

This also fixes another instance of http://drupal.org/node/1279164 where bundle is assumed to be present in each result.

Patch is against 7.x git.

Comments

nick_vh’s picture

Status: Active » Needs review

let's see if the testbot dugs it. Sounds like a solid improvement but are you it doesn't need any other adjustments to support all of this?

Status: Needs review » Needs work

The last submitted patch, apachesolr.entity_get_callback.bundles.1.patch, failed testing.

nick_vh’s picture

Status: Needs work » Needs review
levialliance’s picture

Sounds like a solid improvement but are you it doesn't need any other adjustments to support all of this?

When I was stepping through the execution, all the code necessary to support it is already there -- it just hasn't been enabled.

The initial code supporting $bundle was introduced in commit 225cd74527d97c29c508f90a3026bdac01033dd5 / http://drupal.org/node/795912 (the prelim drupal 7 port) although even then it doesn't seem to have been used.

Fortunately all the code to access callbacks is already wrapped with apachesolr_entity_get_callback() so my patch is a simple affair; I created it by grepping for every instance of the word 'callback' so unless there's some part using nonstandard naming that should be enough.

pwolanin’s picture

Status: Needs review » Needs work

minor code style:

+      $bundle = !empty($doc->bundle) ? $doc->bundle : null;
levialliance’s picture

Status: Needs work » Needs review
StatusFileSize
new2.49 KB

Fixed (I assume you were referring to using null instead of NULL)

Repatched against latest 7.x head

nick_vh’s picture

Status: Needs review » Needs work

I think the API docs should be updated before this can be committed?

nick_vh’s picture

leviallance, any update on this?

levialliance’s picture

Attached a patch against latest 7.x-1.x-dev with examples added to hook_apachesolr_entity_info_alter() [I assume this is where they should go, if the documentation should go elsewhere then let me know]

As I was writing it, I realised that the behaviour may not be as expected though. For 'result callback' (which is what I was using it for), the bundle-specific setting overrides the entity-specific setting. This makes sense.

'status callback' and 'document callback' are arrays of callbacks through. Does it make sense for the bundle specific callbacks to completely override the entity settings, or should apachesolr_entity_get_callback() return a merged entity and entity-bundle callback array?

Personally I'd be inclined to the functionality as already implemented in the repository [bundle specific callbacks completely replace the entity callbacks] since it makes the situation when you want to remove a callback for a specific bundle much much easier to implement. I admit though that for something like 'status callback' though it is much less likely that you would want to remove a callback than that you would want to simply add 1 extra check for a given bundle and merging the callbacks would make that situation a bit less code (not to mention that merging callbacks would be less sensitive to multiple modules adding bundle-specific hooks -- again, that's probably a fairly obscure case though and devs can always implement hook_module_implements_alter() if they run into ordering problems)

nick_vh’s picture

Status: Needs work » Needs review

checking testbot first, aside of that it looks good!

I'm not sure about the override question. I think the bundle specific should override where possible but allow the defaults to be in use if you do not override everything.

The status callback should definitively have the same behavior :

$statuscallback[] = 'extra_check'
$statuscallback = array('resetpreviouschecks');

it seems that your patch already implies the same behavior so looks good to go for me

nick_vh’s picture

Status: Needs review » Closed (fixed)

committed to 7.x-1.x and 6.x-3.x