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.
Comments
Comment #1
pfrenssenI'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.
Comment #2
dawehnerSo if we really want this now functionality there are sadly some unit-tests missing.
Comment #3
pfrenssenComment #4
damiankloip CreditAttribution: damiankloip commentedI 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 :)
Comment #5
damiankloip CreditAttribution: damiankloip commentedtask as this is also fixing the test coverage.
Comment #6
damiankloip CreditAttribution: damiankloip commented#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.
Comment #7
dawehner1 and '1' is confusing that. What about use 1 and 2?
Comment #8
damiankloip CreditAttribution: damiankloip commentedOh, the point was that they are both == but not ===.
Comment #9
olli CreditAttribution: olli commentedCan we drop the last "Array" to/or keep parameters consistent with mergeDeep/mergeDeepArray?
Comment #10
damiankloip CreditAttribution: damiankloip commentedAlso changed the file docblock to Contains.. as we have to fix that line anyway.
Comment #11
damiankloip CreditAttribution: damiankloip commentedLet's drop the Array, in that case. #10 is with this is without. The choice is yours.
Comment #12
chx CreditAttribution: chx commentedHere'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.
Comment #13
damiankloip CreditAttribution: damiankloip commentedOk, 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.
Comment #14
damiankloip CreditAttribution: damiankloip commentedWorking on this.
Comment #15
pfrenssen@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() ?
Comment #16
damiankloip CreditAttribution: damiankloip commentedYeah, I'm working on just a diffAssoc method to go with the recursive version.
Comment #17
damiankloip CreditAttribution: damiankloip commentedSorry, got sidetracked, how about this? Moved into a diffArray class. Tests included.
Comment #18
chx CreditAttribution: chx commentedThat'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.
Comment #19
damiankloip CreditAttribution: damiankloip commentedAll valid points, it should behave the same as array_diff_assoc. Try this.
Damn, I forgot to add the bananas part....
Comment #20
damiankloip CreditAttribution: damiankloip commentedSorry, also including chx's docblock for the recursive method.
Comment #21
damiankloip CreditAttribution: damiankloip commentedtim.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!
Comment #22
pfrenssenThe documentation has been put above the wrong function :)
Comment #23
pfrenssenUpdated documentation.
Comment #24
damiankloip CreditAttribution: damiankloip commentedHehe, sorry everyone.
Comment #25
pfrenssen#23 and #24 are essentially the same, some minor documentation differences.
Comment #26
damiankloip CreditAttribution: damiankloip commentedWhich one do we want? I guess that's the question :)
Comment #27
tim.plunkettTagging.
Comment #28
damiankloip CreditAttribution: damiankloip commentedJust spotted a small typo.
Comment #29
dawehnerSo I think this patch is ready to roll
There should a follow up to convert all existing calls to array_diff_recursive.
Comment #30
chx CreditAttribution: chx commentedJust a question, wouldn't a @array_diff_assoc work...?
Comment #31
webchickThat's a fair question, which would eliminate a lot of code here. Though we generally try and avoid @ operators because they make debugging frustrating.
Comment #32
damiankloip CreditAttribution: damiankloip commentedYeah, 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.
Comment #33
chx CreditAttribution: chx commentedI think the best is to axe the body of diffAssoc and just do a return @array_diff_assoc . I agree arrayDiffRecursive is useful.
Comment #34
damiankloip CreditAttribution: damiankloip commentedYp, that works for me. Here is a new version with diffAssoc just acting as a wrapper around array_diff_assoc but suppressing the errors.
Comment #35
chx CreditAttribution: chx commentedi need to go back and tag a couple of issue with this tag. anyways, this is good to go.
Comment #36
webchickOk, 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.
Comment #37
SebaSOFT CreditAttribution: SebaSOFT commentedDefinitivelly happening en D7
Comment #38
webchickOk, trying this then.
Comment #39
sigveio CreditAttribution: sigveio commentedI can confirm that this is also an issue with D7 (e.g. on saving panels with simple cache enabled, and the likes). :)
Comment #40
ACF CreditAttribution: ACF commentedNot 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
Comment #41
Adam Clarey CreditAttribution: Adam Clarey commentedHi 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
Comment #42
ACF CreditAttribution: ACF commentedHey,
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.
Comment #43
Adam Clarey CreditAttribution: Adam Clarey commentedIt 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.
Comment #44
pfrenssenVery nice to see this move forward!
The function names used could use some discussion. At the moment these are
drupal_array_diff_assoc()
andarray_diff_assoc_recursive()
(without the "drupal_" prefix). I'm personally OK with these choices.Docblock of the test needs to be updated to something more relevant, this has been copied from utility.inc tests.
Improve the wording a bit, what about:
The first line has one preceding space too many. I also would shorten this description to "Tests drupal_array_diff_assoc().".
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.
Comment #45
ACF CreditAttribution: ACF commentedUpdated with the suggested changes.
Comment #46
pfrenssenLooking good! Just one minor comment, I would indicate the function names by putting parentheses () behind them:
to
Then it is consistent with the docblocks of hook implementations which are all over Drupal :)
Comment #47
ACF CreditAttribution: ACF commentedchange made.
Comment #48
pfrenssenThanks! 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 :)
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
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.
Comment #50
David_Rothstein CreditAttribution: David_Rothstein commentedOf interest: https://bugs.php.net/bug.php?id=60278
Comment #51
sigveio CreditAttribution: sigveio commentedSeconded. 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.
Comment #52
magicmyth CreditAttribution: magicmyth commentedI'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.
Comment #53
damiankloip CreditAttribution: damiankloip commentedHmm, 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?
Comment #54
David_Rothstein CreditAttribution: David_Rothstein commentedWell, 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().
Comment #55
David_Rothstein CreditAttribution: David_Rothstein commentedBetter 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.
Comment #56
dawehnerGreat!
Comment #57
xjm#54: array-diff-recursive-followup-1850798-54.patch queued for re-testing.
Comment #58
xjmComment #59
webchickThanks, David, for your ever-watchful eye. :)
Committed and pushed to 8.x. Trying this on 7.x again.
Comment #60
geerlingguy CreditAttribution: geerlingguy commentedIs one of the earlier patches to be reviewed? Or does just the follow-up need to be ported?
Comment #61
gunwald CreditAttribution: gunwald commentedI tried to commit the patch from #54 and got:
Is there any way to work around that problem besides downgrading PHP?
Comment #62
Jorrit CreditAttribution: Jorrit commentedgunwald: It looks like you're trying to patch Drupal 8. The pach is already in Drupal 8, see #59.
Comment #63
David_Rothstein CreditAttribution: David_Rothstein commentedI think what we need here is a combination of #47 (already written for Drupal 7) and #54 (needs to be backported from Drupal 8).
Comment #64
geerlingguy CreditAttribution: geerlingguy commentedAttached 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.
Comment #66
geerlingguy CreditAttribution: geerlingguy commentedThis gets the failing test working, but is it right?
Comment #67
DeFr CreditAttribution: DeFr commented@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)
Comment #68
geerlingguy CreditAttribution: geerlingguy commentedThis should hopefully get the green light again.
Comment #69
DeFr CreditAttribution: DeFr commentedLooks fine, match what was commited in D8 and the new test pass, so, RTBC :-)
Comment #70
bendev CreditAttribution: bendev commentedtested #68 and works fine thanks
Comment #71
markbannister CreditAttribution: markbannister commentedNew commons install
applied patch 68
still fails on viewing group:
Repeats error 90-100 times on page.
I'm new to patching but the code in the patch appears to be in common.inc
Comment #72
JamesOakleyThis 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.
Comment #73
bendev CreditAttribution: bendev commented#68When 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])) {
Comment #74
DeFr CreditAttribution: DeFr commented@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 &&)Comment #75
bendev CreditAttribution: bendev commentedyes, 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
Comment #76
mvcreplacing tags
Comment #77
geerlingguy CreditAttribution: geerlingguy commented#68: 1850798-base-array_recurse-drupal-68.patch queued for re-testing.
Comment #78
corbin CreditAttribution: corbin commentedI apply #68 to drupal 7 - kickstart 2 : no change
with taxonomy terms - autocompletion widget
Comment #79
mvc@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.
Comment #80
RobLoachAre there any places in Drupal where we could make use of this? Seems silly to add something that isn't being used at all.
Let's move this to something like
Drupal\Component\Utility\DiffAray::assocRecursive()
instead.If we have this in a Component, then we could use PHPUnit instead.
Comment #81
RobLoachOh, this is for Drupal 7. Never mind.
Comment #82
geerlingguy CreditAttribution: geerlingguy commented#68: 1850798-base-array_recurse-drupal-68.patch queued for re-testing.
Comment #83
RobLoachSomeone mentioned utility.inc earlier, so #2002514: Deprecate debug(); remove references to _drupal_debug_message()..
Comment #84
DeFr CreditAttribution: DeFr commented@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.
Comment #85
Elin Yordanov CreditAttribution: Elin Yordanov commented#68: 1850798-base-array_recurse-drupal-68.patch queued for re-testing.
Comment #86
geerlingguy CreditAttribution: geerlingguy commentedStill RTBC, as far as I can tell...
Comment #87
NaX CreditAttribution: NaX commentedFYI: Another issue is waiting for this to be committed
#1525176: Using array_diff_assoc() for multilevel arrays in DrupalDefaultEntityController->cacheGet()
See comment: https://drupal.org/node/1525176#comment-7506515
Comment #88
Anthony Fok CreditAttribution: Anthony Fok commentedSo, 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! :-)
Comment #89
geerlingguy CreditAttribution: geerlingguy commented#68: 1850798-base-array_recurse-drupal-68.patch queued for re-testing.
Comment #90
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted 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.
Comment #91
David_Rothstein CreditAttribution: David_Rothstein commentedAttempting to add the "7.23 release notes" tag again...
Comment #92
XanoFixing tag.