Updated: Comment #143
Problem/Motivation
Sites on PHP 5.4 will see notices when editing nodes with multiple taxonomy terms:
Notice: Array to string conversion in function DrupalDefaultEntityController->cacheGet() (string 368 in file includes/entity.inc).
Steps to reproduce, from redsd in #122:
The error will surface when following these steps:
- Create a new node.
- Add multiple tags
- Save node
- Create a second node
- add multiple tags, but make sure you use atleast one tag that allready exists.
- Save node
- The following error will appear one or multiple times: Notice: Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 364 of \drupal_standard\includes\entity.inc).
Or when
- Choose node with multiple tags
- Delete node
- On confirmpage the error will surface, one error for each term.
Or when
- Choose node with multiple tags
- edit node
- Don't change anything
- Save node
- The following error will appear one or multiple times: Notice: Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 364 of \drupal_standard\includes\entity.inc).
There is no error when only new tags are entered and when the node is actaully deleted (after the confirm).
Proposed resolution
The patch in #143 seems to fix it. Note that this issue only affects 7.x, because the offending code has been removed from 8.x.
Remaining tasks
Tests need review.
Original report by Georgique
PHP 5.4
Got notice when saving node:
Notice: Array to string conversion in function DrupalDefaultEntityController->cacheGet() (string 368 in file /home/george/www/inti.kz.dev/includes/entity.inc).
Looked into code and don't understand why I see it. So can't make patch myself.
Comment | File | Size | Author |
---|---|---|---|
#192 | 1525176.patch | 5.3 KB | klausi |
#188 | 1525176.patch | 4.96 KB | klausi |
#153 | drupal7.entity-system.1525176-153.patch | 4.14 KB | David_Rothstein |
#153 | drupal7.entity-system.1525176-153-TESTS-ONLY.patch | 3.59 KB | David_Rothstein |
#143 | drupal7.entity-system.1525176-143.patch | 1.88 KB | webchick |
Comments
Comment #1
Georgique CreditAttribution: Georgique commentedI found it.
Function array_diff_assoc() converts all elements of input arrays to string. But $entity_values can and contains elements which are arrays too.
So solution is serialize each element of array before comparing. Patch is attached.
Comment #2
bleen CreditAttribution: bleen commentedI havent checked, bu this looks like something that would have to be fixed in D8 first and then backported
Comment #3
BerdirPretty sure this needs a test then... not sure I fully understand it.
Comment #4
tim.plunkettI don't think a notice is a major, bumping down.
Comment #5
barraponto CreditAttribution: barraponto commentedCan we get a wrapper? This is starting to pop up on other modules like Views, Features and even core itself.
Comment #6
Georgique CreditAttribution: Georgique commentedThere is the same problem with using array_diff() also. I've checked. It should be repaired in many places.
Comment #7
rogical CreditAttribution: rogical commentedgot same issue in D7.14
Comment #8
tim.plunkett.
Comment #9
tim.plunkettCNW for tests.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedThe patch from #1 works and is needed in Drupal 7.14.
Please move forward.
Comment #11
tim.plunkettCore patches need tests before being considered finished. See http://drupal.org/core-gates#testing
Comment #12
amateescu CreditAttribution: amateescu commentedActually, I think using
array_diff_assoc()
was wrong in the first place, because that means we don't support array values for $conditions, like we do (by chance) when loading entities from the database.Attaching patches for 8.x and 7.x that should fix this properly. I can provide some tests on Friday if we agree on the approach.
Comment #13
iSoLate CreditAttribution: iSoLate commentedReviewed the patch for Drupal 7, looks great. I tested it and it works fine.
I did not have the time to test for Drupal 8.
Comment #14
tracker2k CreditAttribution: tracker2k commentedPatch #12 works perfectly for me on Drupal 7
Comment #15
XanoThe same patch was successfully applied to Entity.module in #1514764: Array to string conversion error from line 625 of entity/includes/entity.controller.inc.
Comment #16
attiks CreditAttribution: attiks commented@amateescu can you write a test for Drupal 8, so we are sure this works?
Comment #17
Pere OrgaSame problem here on a fresh install of 7.15
Patch #12 seems to solve it.
Comment #18
alecliew CreditAttribution: alecliew commented#12: 1525176-fix_entity_conditions.patch queued for re-testing.
Comment #20
naveko CreditAttribution: naveko commentedThis patch works for Entity 7.x-1.0-rc3. This is the first time I post a patch, so hopefully I haven't done anything wrong.
Comment #21
Georgique CreditAttribution: Georgique commentedComment #23
slippast CreditAttribution: slippast commented#1 and #12 worked for me with Drupal 7.16 (not a fresh install).
Please note that this is a serious issue, more than just a PHP warning. I was unable to actually update my nodes. Following the long string of PHP warnings Drupal had a notification saying that the changes were not saved.
This problem appeared at some point on a MAMP sandbox server running PHP 5.4.4. I'm not sure when exactly because most of the node creation and updating is automated.
Thanks to amateescu.
Comment #24
salomos CreditAttribution: salomos commentedCan anyone tell me what the status of this problem is? I downloaded Crommerce and have the very same issues described above. I only found one download of Commerce 7 that would actually work on my WAMP but it has this error all over the place. If I can't find the solution, I will have to go back to Ubercart. I didn't experience it until I downloaded the Commerce distribution. I am a newbie and not sure how to find out more information or a solution. Thanks everyone.
Comment #25
Pere OrgaRestoring version, category and priority.
Comment #26
Mark_L6n CreditAttribution: Mark_L6n commentedFYI, http://drupal.org/node/1829260#comment-6677906 points out a PHP problem with array_diff_assoc.
I have this issue in 7.16. What is the consensus: is it safe to ignore these warning messages? If not, perhaps this should be changed back to 7.x and 'major'.
Comment #27
attiks CreditAttribution: attiks commented#26 it should be fixed for Drupal 8 first
Comment #28
Mark_L6n CreditAttribution: Mark_L6n commentedI don't understand why it should be fixed for D8 first, if it is causing problems for D7 sites currently in use and is an actual problem (not just faulty warning messages) . Please don't misunderstand--I'm not being confrontational, but as an outsider to the process, it would seem that the current sites being used should have priority over the future dev project for non-trivial bugs.
Comment #29
barraponto CreditAttribution: barraponto commented@mark123 we fix d8 first in order to prevent the error from showing up in the next release. Once it's fixed, we backport the patch so d7 (and d6) get the fix as well.
Comment #30
salomos CreditAttribution: salomos commentedDoes anyone know when this fix might be available? I am just looking for a rough estimate (ballpark figure). I am new to Drupal and I am not really sure how long NORMAL fixes seem to take. Should I look at another solution or hang out for this fix. I am not going live for at least 3 to 4 more weeks. Thanks everyone for helping a newbie!
Comment #31
paladin356 CreditAttribution: paladin356 commentedChanging line from the patch in post #1 in entity.inc worked for me. I do not get those messages anymore. Thanks.
Comment #32
tim.plunkettFixing version.
Comment #33
kenyan CreditAttribution: kenyan commentedSince 7.18 is current, I guess this issue moves to it too.
Comment #34
Peter Bowey CreditAttribution: Peter Bowey commentedPatch #12 works fine on Drupal 7.18
Comment #35
amin0214 CreditAttribution: amin0214 commented#12: 1525176-fix_entity_conditions.patch queued for re-testing.
Comment #36
kclarkson CreditAttribution: kclarkson commentedSo will this patch be added to the next Drupal 7.19 release ?
Comment #37
bleen CreditAttribution: bleen commentedNope
a) no patch gets committed until it passes all the tests (aka turns Green in the issue queue) and is marked as Reviewd & Tested by the Community.
b) bugs are always fixed in the latest branch (in this case 8.x) first so that we dont forget to put the fix in there and thus re-introduce the bug
Neither of those two things have happened yet. If you want to help move this issue along so it can get into D7, then the next step would be to create a patch against Drupal8 that passes all the tests.
Comment #38
Mark_L6n CreditAttribution: Mark_L6n commentedGiven that:
It seems appropriate to raise the priority of this issue to at least 'major', and perhaps 'critical'. I won't make the change myself, but leave it to those more involved with the solution to this issue to decide.
Comment #39
pfrenssenTagging.
Comment #40
pfrenssenMarked #1829260: PHP array_diff* API isn't recommended for multidimensional arrays as a duplicate of this issue.
Comment #41
David_Rothstein CreditAttribution: David_Rothstein commentedIf this is preventing nodes from being saved (and if that is reproducible with Drupal core alone) this should be moved to major or probably critical. Nothing to do with Drupal 7 vs. Drupal 8 most likely; a problem like that would be equally critical for Drupal 8 too :)
However, it's not clear to me from reading this issue whether there are actually a reproducible set of steps (starting from a fresh installation of Drupal core) that result in the node not being saved at all...
Comment #42
guney CreditAttribution: guney commented#1: cache_get.patch queued for re-testing.
Comment #43
Paul B CreditAttribution: Paul B commentedIt looks like the buggy code was removed from Drupal 8 in August (see #1184272).
Comment #44
kclarkson CreditAttribution: kclarkson commenteddoes this mean we can get it for 7 ?
Comment #45
Alan D. CreditAttribution: Alan D. commentedYes, this code was removed in #1184272-81: Remove deprecated $conditions support from entity controller
Needs work as the patch doesn't apply and also from #11 this needs tests
Comment #46
David_Rothstein CreditAttribution: David_Rothstein commentedWonder if #1850798: Add a recursive version of array_diff_assoc() would help here?
Comment #47
David_Rothstein CreditAttribution: David_Rothstein commented.
Comment #48
pmol123 CreditAttribution: pmol123 commentedIs there an estimated time for this fix in D7 (7-21+)?
Is the Dev version of D7 fixed ?
I would gladly move on to D8, but there are not enough modules ported to D8 yet.
Patrick
Comment #49
niudongwei CreditAttribution: niudongwei commentedthis works on 7.21, Thanks!
Comment #50
drupalion CreditAttribution: drupalion commentedThanks. #12 1525176-fix_entity_conditions-D7-do-not-test.patch works on drupal 7.22
Comment #51
pmol123 CreditAttribution: pmol123 commentedOK thank you.
Patch applied to production version of http://www.ExpertWitness.com
I will report back in a few days with feed back.
Patrick
Comment #52
mellamoanton CreditAttribution: mellamoanton commentedHi!
I'm having this issue. It happens since I use "tags". My problem is that I can't apply the #12 1525176-fix_entity_conditions-D7-do-not-test.patch
I'm using Drupal 7.21 on localhost with MAMP.
I attach the screenshot.
Thanks in advance!
Comment #53
barraponto CreditAttribution: barraponto commented@mellamoanton #50 says it works on Drupal 7.22, did you try upgrading then applying?
Comment #54
mellamoanton CreditAttribution: mellamoanton commentedMy apologies, I don't know why I was trying apply patch on Entity module, not Drupal core. Anyway, I could not apply the patch correctly –I don't know why–, so I did it manually –were a few lines–. Now, everything is perfect.
Thanks!
Comment #55
NaX CreditAttribution: NaX commented#12 works for me on 7.22.
Comment #56
Enric Climent CreditAttribution: Enric Climent commented#12 1525176-fix_entity_conditions-D7-do-not-test.patch works for me on Drupal 7.22 with no fresh install (is a blog with a lot of modules preactivated and configured). Thanks.
So, are this patch will be included in next versión of Drupal 7.x (for example 7.23) or in Drupal 7.x-dev?
Sorry my english ;)
Comment #57
rballou CreditAttribution: rballou commentedSo to clarify, what are we waiting on getting this in Drupal 7? I can try to help out if possible.
Comment #58
pmol123 CreditAttribution: pmol123 commentedSeems to be all good here.
I applied the patch #68 at issue:
==> http://drupal.org/node/1850798
this morning on my production drupal site on
==> http://www.ExpertWitness.com
Patrick
Comment #59
BerdirAs said before, we are waiting for a test. Without a test that exposes this bug and proves that the patch fixes it and will ensure that it won't get broken again, this will not get in.
Comment #60
drupov CreditAttribution: drupov commentedFor me the error went away with 1525176-fix_entity_conditions-D7-do-not-test.patch from #12 too.
Comment #61
martink CreditAttribution: martink commentedI tried applying the one-liner patch proposed in #1 (in line 369 on D7.22) and the error has gone away for the moment. I will upgrade to the next version when the issue is fixed definitively!
Comment #62
lucascaro CreditAttribution: lucascaro commentedre-rolling the patch so it get's tested for D7..
edit: saved too quickly, see next comment.
Comment #63
lucascaro CreditAttribution: lucascaro commentedre-rolling the patch so it get's tested for D7..
Comment #64
sokrplare CreditAttribution: sokrplare commented#63 worked great for me!
Comment #65
andrea.cavattoni CreditAttribution: andrea.cavattoni commentedalso replace the "array_diff_assoc" with 'array_diff_assoc_recursive' and copy this function in a module or in the core:
damned php 5.4!
Comment #66
dgeo CreditAttribution: dgeo commentedThis last patch works for us too.
Please commit this to 7.x
Comment #67
delta CreditAttribution: delta commentedWorks for us too, on two fresh project, we have the same problem.
#63 fix it, and i didn't have any regression after the patch.
Comment #68
BerdirComment #69
BerdirLost my comment. This still needs a test, otherwise it can't be commited.
Comment #70
lucascaro CreditAttribution: lucascaro commentedsetting to needs work for the tests
Comment #71
prathK CreditAttribution: prathK commentedSimilar Notices I observed in commons 3.0 while editing events and it got fixed with #63.
Thanks !!!! :)
Comment #72
pfrenssenWouldn't this be solvable with the
drupal_array_diff_assoc_recursive()
function that is being added in #1850798: Add a recursive version of array_diff_assoc()? That issue has tests, has already been committed to D8, and has a backport for D7 in RTBC state.Comment #73
edster1 CreditAttribution: edster1 commented#63 worked great on 7.22 for me!
Comment #74
lucascaro CreditAttribution: lucascaro commented@pfrenssen I think you have a point, as soon as that patch is backported, we need to create a patch here using
drupal_array_diff_assoc_recursive()
instead.Comment #75
Yazzbe CreditAttribution: Yazzbe commented#63 works for me
Comment #76
xcafebabe CreditAttribution: xcafebabe commented#63 worked for me as well, thanks !
Comment #77
David_Rothstein CreditAttribution: David_Rothstein commentedFYI: I just committed #1850798: Add a recursive version of array_diff_assoc() to Drupal 7 (in case that helps move this issue forward, as several of us mentioned above).
Comment #78
andypostis not it a release blocker?
Comment #79
Mark_L6n CreditAttribution: Mark_L6n commentedAdding issue tag 'PHP 5.4'.
Comment #80
alesr CreditAttribution: alesr commentedWhere are tests for this issue?
It would be good if this patch is commited to the next D7 release?
Comment #81
knowledges33ker CreditAttribution: knowledges33ker commentedI also need this. I've patched contrib modules before but never patched drupal core. Can anyone tell me where I put the patch file (in what directory?) to have the patch applied correctly?
Comment #82
bleen CreditAttribution: bleen commented@knowledes33ker .. just like a patch for a contrib module should be applied relative to the root of the contrib module, patches for core should be applied relative to the root of core. There is really good information at http://drupal.org/patch on how to do this. Happy patching
Comment #83
JamesOakley@alesr (#80) - did you clear the PHP 5.4 tag for a reason? Given PHP 5.3 is EOL, and 5.4 is now considered by PHP to be an "old stable" release, ensuring Drupal is fully PHP 5.4-compliant is quite important. I appreciate that tag bloat is a bad thing, but tagging issues that directly relate to PHP 5.4 compliance is either harmless or helpful, IMO.
Comment #84
alesr CreditAttribution: alesr commented@JamesOakley, no, the tag was removed when I replied.
Hope it won't go away this time again.
EDIT: And it did.. Looks like commenting bug with tags?
Comment #85
tstoecklerYeah, that can happen sometimes. Drupal.org strangeness...
Comment #86
handrywahyudi CreditAttribution: handrywahyudi commented#1: cache_get.patch queued for re-testing.
Comment #87
David_Rothstein CreditAttribution: David_Rothstein commentedJust to clarify, I don't think this issue is really related to PHP 5.4 compliance - see discussion in #1850798: Add a recursive version of array_diff_assoc(). If the code is incorrect, it's incorrect for other versions of PHP too (the only difference is that PHP 5.4 is noisier about alerting you to the fact that your code is probably not doing what you think it is).
Comment #88
elgwiedo CreditAttribution: elgwiedo commented#63 works for me as well.
Comment #89
marcofloriano CreditAttribution: marcofloriano commentedGetting this error while trying to update a node through the edit widget at front page. Not sure what i´m suppose to do, apply the patch, turn off warning report or wait for the tests and update.
Comment #90
gowrann CreditAttribution: gowrann commented#63 works for me too on 7.22
Comment #91
apadernoComment #92
maximpodorov CreditAttribution: maximpodorov commentedDrupal includes drupal_array_diff_assoc_recursive() in 7.23, so it's time to fix the issue using this function.
Comment #93
pslcbs CreditAttribution: pslcbs commented#63 worked for me on Drupal 7.23 and php 5.4
Thank you!!
Comment #94
Peter Bowey CreditAttribution: Peter Bowey commentedPatch #63 worked for me via Drupal 7.23 and php 5.5.1
Thank you!!
Comment #95
flocondetoilePatch #63 works fine on Drupal 7.23 and PHP 5.4.17
Thanks
Comment #96
star-szrRegardless of the fix, this still needs tests. Setting to needs work.
Edit: Providing clear steps to reproduce from a clean install of Drupal 7 in the issue summary would help in writing automated tests, see https://drupal.org/contributor-tasks/add-steps-to-reproduce.
Comment #97
star-szrAdding Needs tag for steps to reproduce.
Comment #98
jorgemontoyab CreditAttribution: jorgemontoyab commented#63 works fine. Drupal 7.23, PHP 5.4.9-4.
Comment #99
goldenboy CreditAttribution: goldenboy commented@maximpodorov, @cavax added patch which use drupal_array_diff_assoc_recursive instead of array_diff_assoc.
This patch need Drupal 7.23 at least.
Comment #100
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedis the patch in 99 instead of the one in 63? which supersedes which?
Comment #101
dlu CreditAttribution: dlu commentedSo the testbot can have its fun.
Comment #103
dlu CreditAttribution: dlu commentedIn response to #100, I'd prefer #99 over #63 as it is simpler and reuses existing code. But for older version of D7 (if you can't update for some reason) then #63 could be an option.
Comment #104
dlu CreditAttribution: dlu commentedHere's a reroll of #99. Let see if the bot likes it better.
Comment #105
dlu CreditAttribution: dlu commentedSince the patch passed and we still have issues – steps to reproduce and maybe tests, I'm changing this back to 'needs work.'
From #72, #74, and #77 above it sounds like the 'Needs tests' tag could be removed leaving only the steps to reproduce before RTBC, or do we want tests specific to this case?
Comment #106
star-szrThis bug should be possible to test so I think we should (ref. https://drupal.org/core-gates#testing). Having steps to reproduce the bug would help very much with writing tests and getting this issue resolved.
Comment #107
BerdirThis only fails on 5.4, so we can't yet write a test that visible fails on the testbot but it should be possible to confirm manually and be prepared when we will test on 5.4 as well.
Comment #108
Homotechsual CreditAttribution: Homotechsual commentedSo as we can't test can we go for RTBC and get this into a release?
Makes sense to me that if we cannot test because the testbot doesn't test on PHP 5.4 then we should get the fix committed rather than waiting for testbot to work with PHP 5.4.
Confirmed #104 works on D7.23
Comment #109
BerdirNo, this still needs a test that proves that it fixes the bug on 5.4. That test can be executed manually to confirm it.
Comment #110
dlu CreditAttribution: dlu commentedNo we should still do the tests. I'm going to take a shot at writing them in the next week if I can. We also need steps to reproduce. Any chance you could contribute them?
Comment #111
Yazzbe CreditAttribution: Yazzbe commented#104 worked for me on D7.23
Comment #112
pineiden CreditAttribution: pineiden commentedIt's work to me, i had the same problem and i replace de 'if' line
thank you!
Comment #113
huzookaSame situation here. After patching the notice message is gone.
7.23
Comment #114
flocondetoileMe too. #104 worked on D7.23
Comment #115
GuGuss CreditAttribution: GuGuss commentedPatching entity.inc with #104 fixed the notice on D7.23.
Comment #116
Road Kill CreditAttribution: Road Kill commentedI had this issue when up loading an image and confirm that patching entity.inc with #104 fixed the notice on D7.23. :)
Comment #117
russellb CreditAttribution: russellb commentedHi, we're seeing this notice a lot on a live site, so I'm having a go at steps to reproduce:
PHP 5.4
1) Create new article
2) Add multiple terms to tags field
3) Save node
.. and I'm seeing the notice once if I put 2 terms in the tags field, the notice occurs 3 times if I put 3 terms in the tags field:
Notice: Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 369 of ... includes/entity.inc).
Comment #118
redsd CreditAttribution: redsd commentedGot the same problem when trying to save multiple terms to a node field.
The nodes will save though so it's only the warning with no code breaking results.
I'm in no position to complain, but it's been over a year since a fix have been supplied, how can this still not be resolved?
Comment #119
dlu CreditAttribution: dlu commentedFixes get stalled when nobody steps up to finish the job. In this case we need a test (so that we know that the problem is fixed and stays fixed) along with the steps to reproduce the problem. On top of that the problem (probably) needs to be addressed in D8 as well – at the very least we need to know that the changes in D8 mean that this problem won't be there. When a problem exists in D8 we start fixing it there to ensure that the bug fix is in the most recent version, then we back port to older versions.
It looks like #117 may provide the steps to reproduce, so I've removed the tag. Now if somebody could write the tests we'd be moving.
Comment #120
russellb CreditAttribution: russellb commentedUpdating my proposed steps to reproduce here, I think you need pre-existing terms to get the notice on a first node save. The notices also occur on node delete so this may give us the simplest steps to reproduce:
PHP 5.4
1) Create new article
2) Add multiple terms to tags field
3) Save node
4) Delete node
I've not been able to test this on a fresh Drupal 7 install on PHP 5.4 yet.
Comment #121
russellb CreditAttribution: russellb commentedI didn't mean to switch the issue tag there but maybe is best until steps to reproduce are verified on a clean install.
Comment #122
redsd CreditAttribution: redsd commentedMy test findings are for drupal 7.23 standard install using php 5.4.12:
The error will surface when following these steps:
Or when
Or when
There is no error when only new tags are entered and when the node is actaully deleted (after the confirm).
Comment #123
dlu CreditAttribution: dlu commentedRedsd, thank you for the steps! I'm going to take a shot at writing the tests.
Comment #124
eth01 CreditAttribution: eth01 commentedDrupal 7.23
I'm having the same error Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 364 of ...\includes\entity.inc)
Comment #125
Offlein CreditAttribution: Offlein commentedComment #126
fraweg CreditAttribution: fraweg commentedHello,
I have this issue after upgrading advanced Forum as you can see here:
https://drupal.org/node/2056005#comment-8010695
But #104 did not work for me... Any ideas ?
Best regards
Frank
Comment #127
letapjar CreditAttribution: letapjar commenteddrupal 7.23 - Manually applied the patch from #104 nd it resolved my probelem of:
Array to string conversion in DrupalDefaultEntityController->cacheGet() (line 364
Comment #128
mari3.14 CreditAttribution: mari3.14 commentedWorking on a website using Drupal 7.23 when I encountered the same issue. Patch #104 worked for me.
Comment #129
Sofia-ED CreditAttribution: Sofia-ED commentedReporting the following comment as spam: https://drupal.org/node/1525176#comment-7421712
Hi! Thank you very much for the patch, but I've a question: How I install the patch? x) of the post 105
https://drupal.org/comment/7812787#comment-7812787
Thanks for the help ^^
Comment #130
cutmedia CreditAttribution: cutmedia commented+1 for #104 worked for me on D7.24, PHP 5.4
Comment #131
gowrann CreditAttribution: gowrann commented+1 for #104 worked for me on D7.24, PHP 5.4.17
Comment #132
David_Rothstein CreditAttribution: David_Rothstein commentedThe functional bug here exists on all PHP versions (the only difference with PHP 5.4 is that it's noisier in terms of warning about it). So it should be possible to write a test for this that isn't dependent on PHP 5.4.
Comment #133
MXT+1 #104 worked for me on D7.24, PHP 5.4.22
Thank you and please commit this!
Comment #134
NaX CreditAttribution: NaX commenteddrupal_array_diff_assoc_recursive()
was committed with tests and now is recommend to be used over the built in functionarray_diff_assoc()
#1850798: Add a recursive version of array_diff_assoc()There have been more than a few posts here that show easily and clearly how to reproduce this issue.
Can somebody please explain what is holding up this being committed to D7. This patch is a simple swapping out of
array_diff_assoc()
withdrupal_array_diff_assoc_recursive()
. I don't see why this cant be marked RTBC.Just my 2 cents.
Comment #135
lukio CreditAttribution: lukio commented+1 #134
Comment #136
Offlein CreditAttribution: Offlein commentedSounds like it needs a test, right? No test, no commit?
Comment #137
NaX CreditAttribution: NaX commentedThe parent issue that introduced the new function has a test, cant we assume that test covers us. Are we now suppose to create a duplicate test testing the same thing. I don't really understand what needs to be tested here when there is already a test for the function we are using. Can somebody please explain, I think I am missing something.
drupal_array_diff_assoc_recursive test: http://drupalcode.org/project/drupal.git/blobdiff/3201287ba449a71ea01c21...
Comment #138
pslcbs CreditAttribution: pslcbs commented#104 worked for me on 7.24 and php 5.4.23
Thank you!!
Comment #139
plasmid_stha CreditAttribution: plasmid_stha commentedi am using drupal 7.29... and i m new to drupal...which patch should i use (or it is trial and error practice)......
Comment #140
NaX CreditAttribution: NaX commented@plasmid_stha
As a rule I would always test the last patch submitted related to your version. So in your case patch found at comment #104. But its always good to read some of comments as they might point to a different patch that would be better for you and once you get some background to the issue you will maybe able to assist and provide feedback.
Comment #141
francescosciamanna CreditAttribution: francescosciamanna commented#104 did not work for me on Drupal 7.24 and php 5.2.17 :S Any idea more?
Comment #142
webchickI ran into this issue while working on some Drupal.org stuff, so tagging. I've also updated the issue summary, since this issue is a bit of a mess.
I find it interesting that none of the existing tests seem to catch this, even on the PHP 5.4-specific testbot: https://qa.drupal.org/pifr/test/600308#tabset-tab-4
Comment #143
webchickHere, this test seems to catch it, as well as add something that's probably useful to be tested, given the surrounding tests. Since testbot won't fail a test-only patch, here's a before/after screenshot from my localhost running PHP 5.4.8:
Before the patch:
After the patch:
Comment #144
webchickComment #145
webchickBtw, thanks so much to redsd and russellb for the clear steps to reproduce! Never would've been able to write that test without them. :) Would be great to credit them on commit.
Comment #147
webchickLIES. :)
143: drupal7.entity-system.1525176-143.patch queued for re-testing.
Comment #148
pmichelazzoThe patch #143 works fine for me.
Thanks!
Comment #149
webchickIf that's a "reviewed & tested by the community" then please feel free to mark the issue as such. :) Seems like David is on a commit spree lately, so it'd be a good time to do so. However, someone should really look over those tests I added and make sure they look sane first.
Comment #150
BerdirTests look ok to me, whatever covers this. Wondering if it's worth to add a comment that explains we're also testing this, as they're fairly indirect.
Also not sure if the hardcoded assertText() message is more helpful than the default "$term was found", because usually you want to know *what* it didn't find.
Comment #151
maximpodorov CreditAttribution: maximpodorov commentedThe patch solves the problem. Maybe it's time to commit it and make 7.25 fully PHP 5.4 compatible.
Comment #152
andypostNow #132 addressed so David please include this fix in upcoming "New year release"
Comment #153
David_Rothstein CreditAttribution: David_Rothstein commentedIf the test only fails on PHP 5.4, then it's only testing the PHP notice, not the functional bug.
Although after looking at this for a bit, I found it pretty difficult to come up with a situation where the functional bug is actually triggered. Which makes me confused about #23 (and other reports above that this is a bug with actual consequences) as well as the fact that this issue has 150 followers... PHP notices by themselves aren't really that big of a deal, especially if they aren't triggered frequently.
Anyway, it is possible to test it, although the test is a bit contrived. This one should pass/fail on the testbot as appropriate. I left @webchick's test too, of course, since that looks good in terms of improving the general test coverage (and hers is probably forward-portable to Drupal 8, but mine isn't).
Comment #155
David_Rothstein CreditAttribution: David_Rothstein commentedHm, testbot messages are a bit confusing these days, but you can see above that the patch which was supposed to pass passed, and the patch which was supposed to fail failed.
Comment #156
maximpodorov CreditAttribution: maximpodorov commentedSo we can mark it as RTBC again.
Comment #157
maximpodorov CreditAttribution: maximpodorov commented@David_Rothstein, are there any issues preventing this to be committed?
Comment #158
webchickI think the ire probably stems from the fact that if you find yourself in PHP 5.4 editing a node with multiple terms (for example, almost every single issue on a Drupal.org test site :D) the notices start to become really irritating. It seems like you get 1 notice per tag, so on some issues I was getting a screen-full of notices.
That's a much better/more direct test, thanks. :) Agreed with forward-porting test coverage to 8.x once this is fixed in 7.x.
Comment #159
webchickOh, and I think #23 is a red herring, or at least I wasn't able to reproduce that on either PHP 5.3 nor PHP 5.4 in writing the test that tests that exact scenario. May have been some other module's stuff that conflicted in that case.
Comment #160
mpark CreditAttribution: mpark commentedWhy do not embedded in the new patch?
Comment #161
Damien Tournoud CreditAttribution: Damien Tournoud commentedArray conditions only work by accident, they were never supposed to be supported.
They only work because DBTNG automatically use IN when you pass an array to a condition like:
... will be
column = 'value'
if value is a scalar orcolumn IN ('value1', 'value2')
if value is an array.We discussed removing support for this, but I guess now it's too late :(
So now, this is probably not what we want:
Suppose that
$conditions
containsarray("uid" => array(1, 2, 3))
, thedrupal_array_diff_assoc_recursive()
is not going to do the right thing.The easiest way to test that would be to try to load by id and conditions an entity that is already in the cache, like:
Comment #162
mpark CreditAttribution: mpark commentednice
Comment #163
miromarchi CreditAttribution: miromarchi commentedMinor typo: The patch in #143 (not 142) seems to fix it.
Comment #164
firesidelibrarian CreditAttribution: firesidelibrarian commentedSo, which patch should I use, #143 or #153?
Thanks for working on this!
Comment #165
AlterEgoEast CreditAttribution: AlterEgoEast commentedF1 please
how do I use patch # 143
Thanks in advance!!
Comment #166
AlterEgoEast CreditAttribution: AlterEgoEast commentedF1 please
how do I use patch # 143
Thanks in advance!!
Comment #167
alesr CreditAttribution: alesr commented@firesidelibrarian:
#153 patch has entity CRUD tests added.
That's the difference between patch #153 and #143.
So try with #153 first and if you experience issues, go for #143 and report.
@AlterEgoEast:
Please check this out https://drupal.org/patch/apply
Comment #168
busla CreditAttribution: busla commentedIs there any way to know if this was commited to 7.26?
Comment #169
mikeytown2 CreditAttribution: mikeytown2 commented@busla
Check the release notes: https://drupal.org/drupal-7.26-release-notes
If you want more in depth details check the git shortlog: http://drupalcode.org/project/drupal.git/shortlog/refs/heads/7.x As you can see only 2 issues where committed to 7.26:
#2164179: Add links to Extender documentation in Database API
https://drupal.org/SA-CORE-2014-001
Also there will usually be a message in the thread stating that a patch was committed; looking over this issue I do not see any message.
Given all of these things I can safely say that this issue has not been fixed in 7.26 :)
Comment #170
slipperysky CreditAttribution: slipperysky commentedPHP 5.5.8 (cli)
Installed Drupal 7.26, admin_menu, adminimal_admin_menu, ctools, entity, libraries, panels, token, views...etc.
Saw the error. Found this thread. Ran: git apply....patch #153
No problem. Error stopped.
Edit: Sorry, those files surprized me. Didn't realize that they would all be automatically attached. I've been using Drupal for a while but am new to posting here.
Comment #171
AaronELBorg CreditAttribution: AaronELBorg commentedSlipperysky,
Add a term reference field to a node and make it an autocomplete widget.
Create a new node of that type.
Edit that node and add some terms in the autocomplete field.
Still no error?
Comment #172
slipperysky CreditAttribution: slipperysky commentedAaronELBorg:
OK, I added a second term reference to an article as an autocomplete widget, created a new article, edited article by adding some terms... still no error. Tried a few times.
I noticed the error originally when creating/editing an article and now I don't see it.
So, it appears as though #153 fixed it for me without any problem. I can try running through the whole process again later on a fresh install (before the patch) when I get another chance....
Comment #173
drupov CreditAttribution: drupov commentedHi,
patch from #153 applies and solves the issue for me with drupal-7.26 also.
Comment #174
miromarchi CreditAttribution: miromarchi commentedpatch in #153 works for me as well. I've tested it in two different websites, in two slight different environments.
1) Ubuntu 13.04, drupal 7.23
2) Debian 7.2, drupal 7.26
Both now are smooth!
thanks
Comment #175
MXTFollow up for my #133:
I've tested patch #153 in another environment and all works fine.
(D7.26, PHP 5.4.23)
Please commit this!
Comment #176
klausiI think this test case is incomplete. The last entity_load() fetches the node from the DB, where the nested array condition works. But we need to to test cacheGet() so if you repeat the last entity_load() you will see different results.
Overall I think this notice is a legitimate error report that you have some client code that invokes entity_load() with nested array conditions, which should never be done. Use EntityFieldQuery instead.
We could try to use is_array() on each condition value and then iterate in a loop in cacheGet(), but that feels like re-inventing EFQ.
Are there invocations of entity_load() in core that do this?
Comment #177
ladybug_3777 CreditAttribution: ladybug_3777 commentedI can also confirm on my install of Drupal 7.26 the patch in 153 works
Comment #178
arosboro CreditAttribution: arosboro commentedCan confirm, #153 works with latest Drupal 7.26
Comment #179
steveoriolOk for me too, #153 with D7.26 and PHP5.4.4-14
Notice: Array to string conversion in DrupalDefaultEntityController->cacheGet()Comment #180
jdexter CreditAttribution: jdexter commentedHey @AlterEgoEast .. Been getting chummy with manual live site patching again myself - amazing what you forget after enough of a rest.. :)-
Usually, you can work out how a given patch is trying to apply by looking at the patch for file path its trying to use.
In this case, this is a multi-file patch spanning several subdirectories .. So first thing is to login and go to your site root (ie var/www/public_html/epicdrupisite/) and download the file from the command line:
wget http://drupal.org/files/issues/drupal7.entity-system.1525176-153.patch
If you copied the link - you will likely have to edit 'https' and stick with http as above ..
Saving locally and ftp'ing it in is kinda pointless because you will need ssh to actually apply the patch, but if wget is disabled - have a care to save the patch cut 'n paste in an absolutely known clean plain text. I use textpad - allows a several plain text options.
In either case, you should end up with a .patch file in the root directory at the top of your drup install ..
Next trick is help the patch find the right paths for the files - so - you issue a -p option on the command line - in this case:
patch -p1 < drupal7.entity-system.1525176-153.patch
.. there are several -p options, but in this case - you are telling the patch program that this particular patch file is relatively targeting file locations below the patch file and to strip the leading '/' thus:
.. above is what it should look like after running the command line noted for this patch ..
The primary variations you see are local patches - so no -p .. just
patch < drupal7.some.patch.1525176-153.patch
.. which would patch a file in the same directory as the patch .. and ..
patch -p0 < drupal7.some.system.1525176-153.patch
Which would set the path from drive root for a file like this:
.. Have a care when applying patches to pay attention to the date .. A lot of patches are absolutely unique to a given point in time and if your application is updated after the patch was issued, you may need to knit it in manually ..
NEVER use -f (force) option .. Its almost like they left it in there as some sort of embittered gag on the Mac/MicroGoat users out there ..
Cheers - as always - your mileage may differ ..
Comment #181
Colin @ PCMarket CreditAttribution: Colin @ PCMarket commentedPatch from #153 works for me too on Drupal core 7.26
Comment #182
klausiI had another look at this today. Turns out that the proposed patch by David Rothstein works as is because the cache is simply bypassed with multiple values per property. The drupal_array_diff_assoc_recursive() will weed out all cached entities, because a simple title string "Node 1" will always be different to array('Node 1', Node 2'). In this case ->cacheGet() will always return an empty array, which means the ->load() method will fallback to the database in order to retrieve the entities.
So this is a minor performance problem for people using multiple value conditions with entity loading, because the static entity cache is always bypassed. But that performance problem has existed since Drupal 7.0 and we are merely fixing a bug with the special "Array" value here. So this indeed looks like the least invasive fix for Drupal 7 at this point.
@Damien Tournoud: I could not reproduce a problem with your node UID scenario, could you provide a test case? Otherwise I would say this should go back to RTBC.
Comment #183
mariusz.slonina CreditAttribution: mariusz.slonina commentedPatch #156 works with Drupal7.26 (at a bunch of modules)
Comment #184
Damien Tournoud CreditAttribution: Damien Tournoud commentedPlease see #161 for the complete explanation. What will happen is that we are not returning from the cache entities that we should be returning from the cache.
Comment #185
klausiYes, I read your explanation. Bypassing the cache is not a problem in itself, since the load() method will fallback to loading from the database. Not ideal, but enough to fix the PHP notice and weird behavior with special "Array" values. See #182 for the complete explanation.
Comment #186
Damien Tournoud CreditAttribution: Damien Tournoud commentedI do agree this is not a big problem, but it makes the logic of the patch incorrect, and as a consequence "Needs work".
Comment #187
Nick_vhSorry to spoil the fun but it still fails for me:PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '))' at line 2: SELECT base.tid AS tid, base.vid AS vid, base.name AS name, base.description AS description, base.format AS format, base.weight AS weight, v.machine_name AS vocabulary_machine_name FROM {taxonomy_term_data} base INNER JOIN {taxonomy_vocabulary} v ON base.vid = v.vid WHERE (base.name LIKE :db_condition_placeholder_0 ESCAPE '\\') AND (base.vid IN ()) ; Array ( [:db_condition_placeholder_0] => Normal ) in DrupalDefaultEntityController->load() (line 191 of .../docroot/includes/entity.inc)
To add some context, I first got the bug mentioned in this issue and what follows is an accurate description of what happened here: https://drupal.org/node/1228076. It might be unrelated but stillTo replicate:
Add two fields from the same taxonomy to the content type. Make both of them autotagging and part of the same taxonomy vocabulary. The second one won't be able to fill in the terms from the vocabulary anymore
Update: Removing the field and re-adding the field after encountering the bug seemed to fix it. Ignore this for now as it might be unrelated and a glitch of the matrix
Comment #188
klausiklausi opened a new pull request for this issue.
Comment #189
klausiImplemented proper array condition handling in cache get as proposed by Damien, interdiff is here: https://github.com/klausi/drupal/commit/b8e7cee7efc5dae87e4211206720be84...
Comment #190
Damien Tournoud CreditAttribution: Damien Tournoud commentedFrom visual review, the logic looks sound. I sketched how to properly test this in #161: try loading an entity normally, then with an array-form condition, and assert that you get the same object.
Comment #191
drupov CreditAttribution: drupov commented#188 works for me on drupal 7.26.
Thanks!
Comment #192
klausiAdded a test case to check that the nodes loaded are indeed identical to ones cached before. Also tested it with the original drupal_array_merge_recursive() approach and the test case now fails as expected. Interdiff: https://github.com/klausi/drupal/commit/4ff957c41e28bece06385c329999e45f...
Comment #193
Damien Tournoud CreditAttribution: Damien Tournoud commentedYes. This looks great now, thanks @klausi!
Comment #195
klausi192: 1525176.patch queued for re-testing.
Comment #196
mvwensen CreditAttribution: mvwensen commentedConfirming that patch #192 works on Drupal 7.27
Comment #197
NaX CreditAttribution: NaX commentedComment #198
pslcbs CreditAttribution: pslcbs commented#192 worked for me on Drupal 7.27
Comment #199
arosboro CreditAttribution: arosboro commentedpatch 192 had some issues with patching file modules/simpletest/tests/entity_crud.test but after applying the patch to includes/entity.inc, the error messages disappeared. Drupal 7.27 here.
Comment #200
BoobaaPatch in #192 applies cleanly to D7.27, and solves the problem for me.
Comment #201
Garrett Albright CreditAttribution: Garrett Albright commentedIt worked for me too. Let's get this baby in core!
Comment #202
lomars CreditAttribution: lomars commentedPatch #192 has solved this problem for me with D7.27. No more error message.
But it was not so easy to understand how to apply it (first time) on the differents files manualy.
Is there an automated maner to do that ? Thanks by advance.
It would be nice to have in core !
Comment #203
Garrett Albright CreditAttribution: Garrett Albright commentedlomars, see this page for more information on patches and how to apply them: https://drupal.org/patch/apply
Comment #204
Saoirse1916 CreditAttribution: Saoirse1916 commentedAgree, #192 did the job for me as well on 7.27.
Comment #205
forestgardener CreditAttribution: forestgardener commentedI can also confirm that applying the patch from #192 on drupal 7.27 successfully removed the error messages.
Comment #206
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!
Comment #207
David_Rothstein CreditAttribution: David_Rothstein commentedAlso created #2259501: Forward-port tests for taxonomy terms being retained after node edit to forward-port the taxonomy tests here, as discussed above.
Comment #210
makokis CreditAttribution: makokis commentedIs this issue corrected in the new version 7.28?
thanks
Comment #211
forestgardener CreditAttribution: forestgardener commented@makokis Yes, the changes in the patch are present in the files of 7.28