When I revert facetapi feature components or clear the cache I get these warnings:

array_search() expects parameter 2 to be array, null given in facetapi.block.inc:44

This problem originates in facetapi_get_delta_map() which does not validate the results it gets back from the cache:

function facetapi_get_delta_map() {
  $map = &drupal_static(__FUNCTION__);
  if (NULL === $map) {
    if ($data = cache_get('facetapi:delta_map')) {
      $map = $data->data;
    }
    else {
      // Build cache data...
    }
  }
  return $map;
}

In my case $data->data is NULL and this empty value is returned, causing the subsequent array_search() warning.

For reference, this is what is currently in my delta_map persistent cache:

[pieter@archlinux docroot (13657 *)]$ drush eval "var_dump(cache_get('facetapi:delta_map'));"
class stdClass#19 (5) {
  public $cid =>
  string(18) "facetapi:delta_map"
  public $data =>
  NULL
  public $created =>
  string(10) "1359378127"
  public $expire =>
  string(2) "-1"
  public $serialized =>
  string(1) "1"
}
CommentFileSizeAuthor
#9 facetapi-1900974-9.patch750 bytespjcdawkins
#1 1900974-1-facetapi-rebuild_delta_map_cache.patch808 bytespfrenssen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Active » Needs review
FileSize
1900974-1-facetapi-rebuild_delta_map_cache.patch808 bytes

Here's a patch, it applies both to 7.x-1.x and 7.x-2.x.

kenorb’s picture

kenorb CreditAttribution: kenorb commented
Status: Needs review » Reviewed & tested by the community
nicksanta’s picture

nicksanta CreditAttribution: nicksanta commented

I have applied this patch and it works perfectly.

gmclelland’s picture

gmclelland CreditAttribution: gmclelland commented

The patch in #1 solved the same problem for me. Thanks!

kenorb’s picture

kenorb CreditAttribution: kenorb commented
Status: Reviewed & tested by the community » Patch (to be ported)
cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Patch (to be ported) » Fixed

Thanks guys! Committed to all versions of Facet API.

pfrenssen, congrats on becoming the 36th code contributor to Facet API!

Thanks,
Chris

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented

Yay! Thanks :-)

Status: Fixed » Closed (fixed)

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

pjcdawkins’s picture

pjcdawkins CreditAttribution: pjcdawkins commented
Status: Closed (fixed) » Needs review
FileSize
facetapi-1900974-9.patch750 bytes

I still get this error, with version 1.3 (after the above patch was committed).

I think we need something like the attached.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Needs review » Needs work

Thanks for the patch, but if possible I'd like to maintain the logic as there is usually a reason why things are coded the way they are. I am definitely open to change it, but there needs to be a reason why.

I think I see the issue, though. When you are assigning a value in a condition with two clauses, you need to wrap the assignment in parens. For example:

if (($data = cache_get('facetapi:delta_map')) && !empty($data->data)) {

Notice the parens around $data = cache_get('facetapi:delta_map'). If possible I'd like to start there and then explore other techniques if necessary.

Thanks for the contributions,
Chris

pjcdawkins’s picture

pjcdawkins CreditAttribution: pjcdawkins commented
Status: Needs work » Needs review

Yes, I can see that the cache_get line is wrong in the current code, because the variable assignment isn't wrapped in (). But that's not relevant.

We just need to be certain that facetapi_get_delta_map() always returns an array.

In the function at the moment:

      // $map is NULL

      foreach (facetapi_get_realm_info() as $realm_name => $realm_info) {

        // [ ... various nested loops]

        $map[facetapi_hash_delta($delta)] = $delta;
      }

      // $map might now be an array, or it might be NULL

If any of the nested loops don't run, then $map never gets initialized as an array.

If we add $map = array(); then $map, and its cached version, will always be an array.

We can remove the && !empty($data->data) because an empty array might be a valid result (and it was done incorrectly anyway).

So I still think my patch in #9 is valid.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

pjcdawkins,

Thanks for the explanation. The reasoning makes sense, and I will take a closer look at the patch.

Thanks,
Chris

Yaron Tal’s picture

Yaron Tal CreditAttribution: Yaron Tal commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Just adding the $map = array(); part would already solve the warning.
Getting the cache logic working is also needed because cache is not working atm.
cache_get() will return a valid cache object or FALSE, so rewriting it as done in #9 should fix this.
Tested and warning is fixed and caching working.

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
CreditAttribution: marcelovani commented

Thats right, the patch #9 works. Thanks.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

The problem is that the hook facetapi_facetapi_realm_info() is coded in facetapi.facetapi.inc file which is only loaded when a search box is present on the page. This hook goes into Drupal cache when cache is cleared.

The problem is that if you clear Drupal cache from a page where this file is loaded, the hook is present and thus saved into cache. If not, it will be absent from the cache. Thus facetapi_get_delta_map() will return nothing.

The true solution is not to prevent a PHP error when that happens, but to prevent the hook from "disappearing" in the first place.

The easy solution is to move all hooks from the include file to the module file. Another solution is to use a module_load_include() in the module to always load the include file when the module is enabled.

You may NOT use this hook in order to load the include file:

hook_hook_info

  • cpliakas committed 3a2287b on 7.x-1.x authored by pjcdawkins 
    Issue #1900974 by pjcdawkins, marcelovani: facetapi_get_block_info()...

  • cpliakas committed 0466e00 on authored by pjcdawkins 
    Issue #1900974 by pjcdawkins, marcelovani: facetapi_get_block_info()...
cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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