When running PHP 5.4, I am seeing some errors related to how array_diff_assoc() is handling arrays with nested arrays inside of them. The error I am getting is this: Notice: Array to string conversion in panels_cache_object->cache() (line 182 of modules/panels/includes/plugins.inc).

This seems to be related to the problem referenced in #1525176: Using array_diff_assoc() for multilevel arrays in DrupalDefaultEntityController->cacheGet()

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bgm’s picture

pfrenssen’s picture

Related issue to provide a version of array_diff_assoc() with support for nested arrays in core: #1850798: Add a recursive version of array_diff_assoc().

Volx’s picture

Status: Active » Needs review
FileSize
771 bytes

Here is a patch using the same approach I used for the views patch using array_diff_key instead of array_diff_assoc (first patch in https://drupal.org/node/1511396#comment-7553219). But the comment holds here too, I don't know if that is the intended behavior, but it is the same (or pretty close) to the current behavior due to the behavior of array_diff_assoc.

Colin @ PCMarket’s picture

This patched helped me too when i came across this error in the new Drupal Commons

das-peter’s picture

@Volx I doubt that this approach works. When I debugged it that weren't associative arrays, thus so a key comparison doesn't seem to be sufficient. array_diff_assoc() is about comparing the contents and also checking the index.

I'd say for the next panels release we just depend on drupal 7.23 which provides drupal_array_diff_assoc_recursive().
However, an extended solution would be to provide a replacement function based on #1850798-68: Add a recursive version of array_diff_assoc() in case panels runs with Drupal <7.23 - check second patch.

pfrenssen’s picture

I definitely prefer the first patch. Core releases are more frequent than Panels releases so providing this fallback function for all eternity seems like overkill.

neclimdul’s picture

If you're running 5.4 you'll need the latest versions of core anyways so it doesn't really make sense to provide a backport.

das-peter’s picture

So the dependency is preferred - I'm fine with that :)
While testing with testing our installation after the patch I've found errors like:
Notice: Undefined index: data in panels_cache_object->restore() (line 213 of /var/www/drupal/modules/panels/includes/plugins.inc).

I think this could be a side effect of the patch since the diff seems to work differently (correct?) now.
I've added a small change to ensure that js is recognized whether it is declared implicitly as key or explicitly as "data" key in the js array.
^^ Would be good to know if someone else is experiences this issue!

das-peter’s picture

Hmm, I think this change introduced more trouble than expected.
Because drupal_array_diff_assoc_recursive() returns only the portions of the array that differ, which can be very granular, while we need to ensure to store the whole portion.
My current solution is to iterate over the diff and use drupal_array_merge_deep() to merge the diff into the start js and store it that way in the js cache.

das-peter’s picture

And here we go again. Because I've unified the storage of the js cache for js-code and js-settings the same has to be done for restoring it. It's as simple as use the 'data' key in the array.
If someone wants an interdiff let me know ;)

das-peter’s picture

And another slight change to avoid notices when new scripts are added.

markbannister’s picture

11 worked for me. For some reason the patch would not apply using the patch command a and I had to apply it manually.

das-peter’s picture

I've just tested applying the patch against the latest panels 7.x-3.x and I didn't experience any issues. Please let me know if someone else has trouble applying the patch.

pjcdawkins’s picture

The patch in #11 works for me.

ptmkenny’s picture

I applied the patch in #11.

In response to #8:

After applying the patch, I got several errors, such as:

Notice: Array to string conversion in ctools_page_token_processing() (line 618 of /sites/all/modules/contrib/ctools/ctools.module).
x
Notice: Undefined index: data in panels_cache_object->restore() (line 234 of /sites/all/modules/contrib/panels/includes/plugins.inc).
x
Warning: Invalid argument supplied for foreach() in panels_cache_object->restore() (line 234 of /sites/all/modules/contrib/panels/includes/plugins.inc).

I tried to clear the caches with Drush (cc all) and that failed:

Drush 5.8 does not support Drupal . Use Drush 4 instead. [error]
Drush command terminated abnormally due to an unrecoverable error. [error]
Error: Call to undefined function conf_path() in
/bin/drush/includes/bootstrap.inc, line 784

However, I then logged in to the site and flushed all caches using the admin menu module. Now I don't get the errors anymore and everything appears to be working fine. So, somehow the patch makes it hard to clear the cache with Drush (immediately after applying the patch, I can revert to the dev version with git and then clear the cache with Drush, but if I re-apply the patch, I cannot). That said, the patch does apply and gives the expected result after the cache is cleared.

pjcdawkins’s picture

Status: Needs review » Reviewed & tested by the community

It works for 4 people.

I suggest perhaps it's problematic to depend on Drupal 7.23+ but I don't see any better solution.

mrf’s picture

Status: Reviewed & tested by the community » Needs work

FWIW I noticed the same thing as #12 when applying patch. Doing a patch -p1 worked for me second time through.

Patch should be updated to include the 7.23 as the minimum Drupal version in Panel's info file, that's the only way to guarantee people upgrade Drupal first. Failing to do so and this patch causes fatal errors on any panels page.

el1_1el’s picture

The extended patch in #5 seems to work just fine in core v 7.20, in case anyone needs a quick fix

pjcdawkins’s picture

@mrf the patch in #11 already does alter Panels's info file to specify Drupal 7.23+.

richsky’s picture

#11 = big white screen for me, Fatal error: Call to undefined function drupal_array_diff_assoc_recursive() were is this function supposed to be found?
Oups, still with 7.22 in prod. Nevermind.

lotyrin’s picture

Why not do as Views has done in #1511396:

$array_mapping_func = function_exists('drupal_array_diff_assoc_recursive') ? 'drupal_array_diff_assoc_recursive' : 'array_diff_assoc';
jhedstrom’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.16 KB
2.34 KB

Patch takes into account views fix mentioned above, removing the need for upping the core version in the .info file.

isloera’s picture

How do I install this? I'm new

das-peter’s picture

@isloera Take a look here: https://drupal.org/patch/apply

@jhedstrom Thanks for the update! I'd like to set this to RTBC but unfortunately it still contains a lot of code that I wrote. So I'm not independent enough to do so ;)

IshaDakota’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #22 applies, fixes the problem, and looks good on visual review. Marking RTBC.

Exploratus’s picture

#22 Worked for me as well. Thanks! Commit?

japerry’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

  • Commit baed7ee on 7.x-3.x, 8.x-3.x by japerry:
    Issue #1762290 by jhedstrom, das-peter: Fix php 5.4 array_diff_assoc...