views 7.x-3.x gives errors doing an array to string conversion with php 5.4: #1511396: php 5.4 and "Notice: Array to string conversion"

in fact, any other module attempting to call array_diff_assoc() on multidimensional arrays will have the same problem. an appropriate solution would be to add a function called array_diff_assoc_recursive() based on comment #12 on that issue for use in views and anywhere else it's needed.

since drupal 7 is supposed to support php 5.4, this will likely need to be backported once accepted into drupal 8.

i wasn't immediately able to find an equivalent function in any library already bundled in drupal 8. my apologies if i've missed it and this is already there.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfrenssen’s picture

Category: bug » feature
Status: Active » Needs review
FileSize
1.63 KB

I'm very much in favor of this. There are real use cases for this, for example Views as mentioned above, but also Features (ref #1588596: Notice: Array to string conversion in features_export_prepare() (line 190 of features.export.inc)), as well as Panels (ref #1762290: PHP 5.4 Issue with array_diff_assoc in plugins.inc).

Here is a patch for 8.x. I've looked around for the right spot to place this, and utility.inc looked like a good candidate.

dawehner’s picture

So if we really want this now functionality there are sadly some unit-tests missing.

pfrenssen’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
damiankloip’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.13 KB

I think this might fit better on the NestedArray helper class? Rather than in utility.inc, that doesn't really have much in.

I have move it to NestedArray and added some tests. I also found out whilst doing this that the NestedArrayUnitTest is never actually tested. As the file has the old ArrayUnitTest name. So we should change that too, otherwise we have no tests :)

damiankloip’s picture

Category: feature » task

task as this is also fixing the test coverage.

damiankloip’s picture

#1511396: php 5.4 and "Notice: Array to string conversion" is now postponed on this issue. I also fixed a quibble from the other issue that NULL values weren't being diffed because isset() was being used. Test should cover that too.

dawehner’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/NestedArrayUnitTest.phpundefined
@@ -143,4 +143,30 @@ function testMergeDeepArray() {
+      'int_diff' => 1,
...
+      'int_diff' => '1',

1 and '1' is confusing that. What about use 1 and 2?

damiankloip’s picture

Oh, the point was that they are both == but not ===.

olli’s picture

+++ b/core/lib/Drupal/Component/Utility/NestedArray.php
@@ -342,4 +342,41 @@ public static function mergeDeepArray(array $arrays, $preserve_integer_keys = FA
+  public static function diffAssocRecursiveArray(array $array1, array $array2) {

Can we drop the last "Array" to/or keep parameters consistent with mergeDeep/mergeDeepArray?

damiankloip’s picture

FileSize
3.13 KB

Also changed the file docblock to Contains.. as we have to fix that line anyway.

damiankloip’s picture

FileSize
3.11 KB

Let's drop the Array, in that case. #10 is with this is without. The choice is yours.

chx’s picture

Here's my comprehension, please fix if I am wrong: Wherever array_diff_assoc was used previously, before 5.4 , if it encountered a multidimensional then it treated the value as 'String' without complaining. Now it complains. To fix the complaint we are not making a similarly ... well not broken, but interesting behaving function. Instead, we create a recursive one. If you were using array_diff_assoc as documented, if you really are just interested in top level differences then.. tough luck? I love PHP. We are basically arguing that unless you know for sure that your array is one dimensional array_diff_assoc is useless and what you really want is recursive.

There was a time when I was this kind of sure. The years in the Drupal core trenches have shattered that. As this function has nothing to do with NestedArray let's move it to a different class and add a nonrecursive version, basically the 5.3 version of array_diff_assoc just to make sure.

damiankloip’s picture

Ok, fair point. In that case I created #1885180: NestedArray tests are never run to fix the nested array tests not running, as this will not have anything to do with that file anymore.

damiankloip’s picture

Assigned: Unassigned » damiankloip

Working on this.

pfrenssen’s picture

@chx There are some instances (in contrib) that are using array_diff_assoc() where they are expecting this to work for multidimensional arrays but were not notified before PHP 5.4 that this is actually not working. This issue just provides the recursive version, it does not assume that all instances of array_diff_assoc() should be replaced with this.

We do need a PHP5.4 compatible version of array_diff_assoc() for that indeed. diffAssocRoot() ?

damiankloip’s picture

Yeah, I'm working on just a diffAssoc method to go with the recursive version.

damiankloip’s picture

FileSize
4.63 KB

Sorry, got sidetracked, how about this? Moved into a diffArray class. Tests included.

chx’s picture

Status: Needs review » Needs work
+   * This is a PHP 5.4 safe version of array_diff_assoc().

That's one very small step above just writing bananas as doxygen but as useful as.

Something like: This function is the same as array_diff_assoc(): compares array1 against array2 and returns the difference. However, while array_diff_assoc considers key => value1 and key => value2 the same if (string) $value1 === (string) $value2 this version does away with the string cast and runs $value1 === $value2.

Reading that it comes clear that's not what we want. The code IMNSHO still should do (string) $value1 === (string) $value2 and then the doxygen should read (yes, effin long, but this is a weirdo):

This function is the same as array_diff_assoc() but multidimensional arrays do not throw notices. Note that as the comparison is made by (string) $value1 === (string) $value2 every array element is considered equal. This is, again, the same as array_diff_assoc() the only reason Drupal provides this function is since PHP 5.4 this casting of arrays to strings throws a notice. If you want to use array_diff_assoc() as it was up to PHP 5.3 then use this function but consider the consequences -- it should be rare to use this function.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
4.7 KB

All valid points, it should behave the same as array_diff_assoc. Try this.

Damn, I forgot to add the bananas part....

damiankloip’s picture

Assigned: damiankloip » Unassigned
FileSize
5.18 KB

Sorry, also including chx's docblock for the recursive method.

damiankloip’s picture

FileSize
5.18 KB

tim.plunkett pointed out this needs to be DiffArray and not diffArray not sure why this was like that to be honest, EVERY class in drupal is Capitalised!

pfrenssen’s picture

Status: Needs review » Needs work

The documentation has been put above the wrong function :)

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
5.02 KB

Updated documentation.

damiankloip’s picture

FileSize
5.02 KB

Hehe, sorry everyone.

pfrenssen’s picture

#23 and #24 are essentially the same, some minor documentation differences.

damiankloip’s picture

Which one do we want? I guess that's the question :)

tim.plunkett’s picture

Component: views.module » base system
Issue tags: +VDC

Tagging.

damiankloip’s picture

FileSize
699 bytes
5.02 KB

Just spotted a small typo.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

So I think this patch is ready to roll

  • The code is proper documented
  • We have tests

There should a follow up to convert all existing calls to array_diff_recursive.

chx’s picture

Just a question, wouldn't a @array_diff_assoc work...?

webchick’s picture

Status: Reviewed & tested by the community » Needs review

That's a fair question, which would eliminate a lot of code here. Though we generally try and avoid @ operators because they make debugging frustrating.

damiankloip’s picture

Yeah, I think I agree. using @ would supress the error and definitely work here, but that is a funny and weird standard to implement.

So I guess that means I stil prefer the verbose way of this patch, which also gives us the arrayDiffRecursive method. As seen in the tests this will provide nested diffs for multi dimensional arrays. So this does what array_diff_assoc cannot anyway. So I guess it's not much more code just to implement the arrayDiff method. A worthwhile trade off, I would say.

chx’s picture

I think the best is to axe the body of diffAssoc and just do a return @array_diff_assoc . I agree arrayDiffRecursive is useful.

damiankloip’s picture

FileSize
1.75 KB
4.6 KB

Yp, that works for me. Here is a new version with diffAssoc just acting as a wrapper around array_diff_assoc but suppressing the errors.

chx’s picture

Status: Needs review » Reviewed & tested by the community

i need to go back and tag a couple of issue with this tag. anyways, this is good to go.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, let's do this.

Committed and pushed to 8.x. Thanks!

Do we need to backport this to D7? I noticed some PHP 5.4 CTools-related issues in the Commons queue the other day.

SebaSOFT’s picture

Definitivelly happening en D7

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
Issue tags: +Needs backport to D7

Ok, trying this then.

sigveio’s picture

I can confirm that this is also an issue with D7 (e.g. on saving panels with simple cache enabled, and the likes). :)

ACF’s picture

Status: Patch (to be ported) » Needs review
FileSize
4.21 KB

Not sure if naming convention is correct or even if i've put stuff in the correct place, but a first go at porting this patch to D7

Adam Clarey’s picture

Hi Alasdair, i've noticed this issue on a project this morning.

I'm getting the error:
Notice: Array to string conversion in module_implements() (line 710 of ~/includes/module.inc).

I have entity_translation installed and the function:

$list = module_list(FALSE, FALSE, $sort);

returns a list where $list['entity_translation'] = 'entity_translation' appears twice and therefore is seen in the loop as an array.

The last patch didn't fix this, do you want me to investigate this use-case further or are you on it?

Adam

ACF’s picture

Hey,

This patch just provides the array_diff_assoc_recursive function that should solve the problem. It doesn't actually replace any instances of array_diff_assoc which are causing a problem in the first place. That will have to be done in a different patch once some solution gets committed.

Adam Clarey’s picture

It turns out my particular issue is traced back to the metatag module adding the 'entity_translation' items to the hook_info array (in module_hook_info). Because metatag and entity_translation are both adding the entity_translation hooks to the array, we get the problem where $hook_info[$hook]['group'] is an array, not a string:

$include_file = isset($hook_info[$hook]['group']) && module_load_include('inc', $module, $module . '.' . $hook_info[$hook]['group']);

In my previous post, i though the issue was coming from:

$list = module_list(FALSE, FALSE, $sort);

But it's not, it's coming from:

$hook_info = module_hook_info();

Perhaps there could be something put in place to trap these events from occurring, but I guess that would be a separate issue to this one.

pfrenssen’s picture

Status: Needs review » Needs work

Very nice to see this move forward!

  • The function names used could use some discussion. At the moment these are drupal_array_diff_assoc() and array_diff_assoc_recursive() (without the "drupal_" prefix). I'm personally OK with these choices.

  • +/**
    + * Test utility functionality in utility.inc.
    + */
    +class ArrayRecursiveUnitTest extends DrupalUnitTestCase {

    Docblock of the test needs to be updated to something more relevant, this has been copied from utility.inc tests.

  • +  public static function getInfo() {
    +    return array(
    +      'name' => 'Array diff assoc tests',
    +      'description' => 'Tests that the array_diff_assoc_recursive function works.',
    +      'group' => 'System',
    +    );
    +  }

    Improve the wording a bit, what about:

    +      'name' => 'Array differences',
    +      'description' => 'Performs tests on drupal_array_diff_assoc() and array_diff_assoc_recursive().',
    
  • +   /**
    +   * Tests function drupal_array_diff_assoc.
    +   */

    The first line has one preceding space too many. I also would shorten this description to "Tests drupal_array_diff_assoc().".

  • +  /**
    +   * Tests function array_diff_assoc_recursive.
    +   */

    Same as above, shorten to "Tests array_diff_assoc_recursive().".

  • An empty line is added at the very end of the test.

Someone else should also best take a look, I can't RTBC this as it still contains most of my patch from #1.

ACF’s picture

Status: Needs work » Needs review
FileSize
4.18 KB

Updated with the suggested changes.

pfrenssen’s picture

Looking good! Just one minor comment, I would indicate the function names by putting parentheses () behind them:

+  /**
+   * Tests drupal_array_diff_assoc.
+   */
...
+  /**
+   * Tests array_diff_assoc_recursive.
+   */

to

+  /**
+   * Tests drupal_array_diff_assoc().
+   */
...
+  /**
+   * Tests array_diff_assoc_recursive().
+   */

Then it is consistent with the docblocks of hook implementations which are all over Drupal :)

ACF’s picture

change made.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! This looks perfect to me.

There is some code from me in the patch but as this was already committed to D8 I suppose it's fine for me to RTBC the backport :)

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work

I don't understand the first half of the patch. The PHP notice is thrown for a good reason; why would we want to suppress it?

The reason is that array_diff_assoc() is not intended for multidimensional arrays and if you try to use it that way the behavior is extremely unexpected:

$ php -r "print_r(array_diff_assoc(array(array('these two arrays')), array(array('are very very different'))));"

Array
(
)

In an ideal world maybe the behavior would be different, but as long as it's not going to be I think it's much better to have a PHP notice to warn you about it than to do the above silently.

If there's ever a situation where someone really does expect the above behavior, they can add the "@" themselves (it's the kind of situation that would warrant a code comment anyway). But I do not see the point of providing a helper function for that, especially since this isn't the only function that behaves that way - e.g., array_diff() does the exact same thing.

David_Rothstein’s picture

sigveio’s picture

If there's ever a situation where someone really does expect the above behavior, they can add the "@" themselves (it's the kind of situation that would warrant a code comment anyway). But I do not see the point of providing a helper function for that, especially since this isn't the only function that behaves that way - e.g., array_diff() does the exact same thing.

Seconded. The drupal_array_diff_assoc() wrapper could be a potential pitfall if people use it without being fully aware of the ramifications (caution is given in the docblock, but still...). It also seems rather redundant (adding extra lines of code without actually simplifying anything) considering that the developer can, as David pointed out, easily add the @ themselves if they knowingly/intentionally wish to suppress the notice.

magicmyth’s picture

I'm probably missing something here but part of this function does not make sense to me. The following condition is contradicting itself.
if (!array_key_exists($key, $array2) && !is_array($array2[$key])) {
The first part of the condition to me states: If $array2 does not have the following $key. So when it checks the next condition (is the value an array?). It will throw a notice because it is basically checking an undeclared variable. To me it seems this should be using an || instead of an &&.

Seeing as this exact same logic has already been reviewed and applied to Drupal 8 I'm guessing my brain is not comprehending this correctly. However I tested this out on Features, which is one of the (many) modules currently miss-using array_diff_assoc on line 208 of features.export.inc and it did indeed throw out the expected notice at that line of code in array_diff_assoc_recursive() because the index key did not exist.

damiankloip’s picture

Hmm, I think you might be right. This should be || and not &&. I guess the tests should be updated too. This might be better material for a follow up?

David_Rothstein’s picture

Well, we can't backport without fixing that bug first anyway. So here's a patch that does both - removes diffAssoc() and fixes the bug in diffAssocRecursive().

For backport, by the way, I'd suggest drupal_array_diff_assoc_recursive() rather than array_diff_assoc_recursive()? Seems more consistent with how we usually do those "we wish PHP provided this" functions, e.g. drupal_array_merge_deep().

David_Rothstein’s picture

Title: need array_diff_assoc_recursive() for php 5.4 » Add a recursive version of array_diff_assoc()

Better title; this doesn't really have anything to do with PHP 5.4 except that PHP 5.4 is noisier about it.

Note that #1525176: Using array_diff_assoc() for multilevel arrays in DrupalDefaultEntityController->cacheGet() is another issue which might be able to use this.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great!

xjm’s picture

xjm’s picture

Issue tags: +Quick fix
webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks, David, for your ever-watchful eye. :)

Committed and pushed to 8.x. Trying this on 7.x again.

geerlingguy’s picture

Is one of the earlier patches to be reviewed? Or does just the follow-up need to be ported?

gunwald’s picture

I tried to commit the patch from #54 and got:

patch -p1  < array-diff-recursive-followup-1850798-54.patch 
patching file core/lib/Drupal/Component/Utility/DiffArray.php
Hunk #1 FAILED at 13.
Hunk #2 FAILED at 54.
2 out of 2 hunks FAILED -- saving rejects to file core/lib/Drupal/Component/Utility/DiffArray.php.rej
patching file core/modules/system/lib/Drupal/system/Tests/Common/DiffArrayUnitTest.php
Hunk #1 FAILED at 47.
Hunk #2 FAILED at 56.
Hunk #3 FAILED at 80.
3 out of 3 hunks FAILED -- saving rejects to file core/modules/system/lib/Drupal/system/Tests/Common/DiffArrayUnitTest.php.rej

Is there any way to work around that problem besides downgrading PHP?

Jorrit’s picture

gunwald: It looks like you're trying to patch Drupal 8. The pach is already in Drupal 8, see #59.

David_Rothstein’s picture

Is one of the earlier patches to be reviewed? Or does just the follow-up need to be ported?

I think what we need here is a combination of #47 (already written for Drupal 7) and #54 (needs to be backported from Drupal 8).

geerlingguy’s picture

Status: Patch (to be ported) » Needs review
FileSize
4.48 KB

Attached patch simply adds in the changes in #54 to the patch in #47, in the appropriate places. Letting the testbot take a whack at it.

Status: Needs review » Needs work

The last submitted patch, 1850798-base-array_recurse-drupal-63.patch, failed testing.

geerlingguy’s picture

Status: Needs work » Needs review
FileSize
4.57 KB

This gets the failing test working, but is it right?

DeFr’s picture

Status: Needs review » Needs work

@geerlingguy: There's a few things in #54 that are not in #66. DiffArray::diffAssoc was removed, which means that both the drupal_array_diff_assoc and its associated test function should be removed too. Removing it, you won't get the fail you had in #64.

(Another point that's mentionned in #54 and that would probably be nice to include in a re-roll is renaming the array_diff_assoc_recursive function introduced in this patch to drupal_array_diff_assoc_recursive, to avoid potential function name collision in case PHP core introduce such a function in a future release)

geerlingguy’s picture

Status: Needs work » Needs review
FileSize
3.48 KB
  • Removed drupal_array_diff_assoc and test (and cleaned up test comments).
  • Renamed array_diff_assoc_recursive() to drupal_array_diff_assoc_recursive() everywhere it occurred.

This should hopefully get the green light again.

DeFr’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine, match what was commited in D8 and the new test pass, so, RTBC :-)

bendev’s picture

tested #68 and works fine thanks

markbannister’s picture

New commons install
applied patch 68
still fails on viewing group:

Notice: Array to string conversion in panels_cache_object->cache() (line 183 of /profiles/commons/modules/contrib/panels/includes/plugins.inc).

Repeats error 90-100 times on page.
I'm new to patching but the code in the patch appears to be in common.inc

JamesOakley’s picture

This is a core patch which needs to fix array handling in core. There are then a number of bugs in contributed modules that can be fixed once this, or something similar, is committed.

I'm guessing you've hit upon a bug in panels that can be fixed once this core issue is resolved, but committing a patch like this to core won't itself fix the panels issue.

bendev’s picture

#68
When connected as anonymous user I am getting theses notices:

Notice : Undefined offset: X in drupal_array_diff_assoc_recursive() (line 6522 dans ...includes/common.inc).

line 6522 is this code : if (!array_key_exists($key, $array2) && !is_array($array2[$key])) {

DeFr’s picture

@bendev: This looks wrong. Are you sure you tested #68 and not something on top of#47 ? With #68 it should be if (!array_key_exists($key, $array2) || !is_array($array2[$key])) (note the || instead of the &&)

bendev’s picture

Issue tags: -Quick fix, -php5.4, -VDC

yes, indeed , I tested the two versions on different sites and I probably forgot to update one of them. THanks for the hint #47 is wrong and #68 works

mvc’s picture

Issue tags: +Quick fix, +php5.4, +VDC

replacing tags

geerlingguy’s picture

corbin’s picture

I apply #68 to drupal 7 - kickstart 2 : no change

Array to string conversion dans DrupalDefaultEntityController->cacheGet() (line 369 in /.../includes/entity.inc).

with taxonomy terms - autocompletion widget

mvc’s picture

@corbin: first this patch needs to be accepted into core, then contrib modules like commerce kickstart need to be patched to use this new function. you could open an issue in that module's queue pointing here for the time being.

RobLoach’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/includes/common.incundefined
@@ -6481,6 +6481,44 @@ function element_set_attributes(array &$element, array $map) {
 /**
+ * Recursively computes the difference of arrays with additional index check.
+ *
+ * This is a version of array_diff_assoc() that supports multidimensional

Are there any places in Drupal where we could make use of this? Seems silly to add something that isn't being used at all.

+++ b/includes/common.incundefined
@@ -6481,6 +6481,44 @@ function element_set_attributes(array &$element, array $map) {
+ * @param array $array2
+ *   The array to compare to.
+ *
+ * @return array
+ *   Returns an array containing all the values from array1 that are not present
+ *   in array2.
+ */
+function drupal_array_diff_assoc_recursive($array1, $array2) {

Let's move this to something like Drupal\Component\Utility\DiffAray::assocRecursive() instead.

+++ b/modules/simpletest/tests/common.testundefined
@@ -2630,3 +2630,75 @@ class FeedIconTest extends DrupalWebTestCase {
 
 }
+
+/**
+ * Test array diff functions.
+ */
+class ArrayDiffUnitTest extends DrupalUnitTestCase {

If we have this in a Component, then we could use PHPUnit instead.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Oh, this is for Drupal 7. Never mind.

geerlingguy’s picture

RobLoach’s picture

DeFr’s picture

@RobLoach: Once this is in, there's one place in D7 core that will be able to use it, DrupalDefaultEntityController::cacheGet.

Keeping this issue as a straight backport of the D8 patch, and having a followup to convert the array_diff_assoc call once this is in seems safer.

Elin Yordanov’s picture

geerlingguy’s picture

Still RTBC, as far as I can tell...

NaX’s picture

Anthony Fok’s picture

So, this patch looks good, is "reviewed & tested by the community".

Hope some kind soul can commit this patch soon so we can start squashing the "Array to string conversion" notices in other modules too! Thanks! :-)

geerlingguy’s picture

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.23 release notes

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/f083e0f

Not sure other modules can really start using this until it comes out in the next core release... but at least getting this in now might help with the other core issue linked to above.

David_Rothstein’s picture

Attempting to add the "7.23 release notes" tag again...

Xano’s picture

Issue tags: -php5.4 +PHP 5.4

Fixing tag.

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